Re: [PATCH v1 3/5] mm/memory_hotplug: make offline_and_remove_memory() timeout instead of failing on fatal signals

[David Hildenbrand]

On 27.06.23 14:40, Michal Hocko wrote:
> On Tue 27-06-23 13:22:18, David Hildenbrand wrote:
> > John Hubbard writes [1]:
> >
> >          Some device drivers add memory to the system via memory hotplug.
> >          When the driver is unloaded, that memory is hot-unplugged.
> >
> >          However, memory hot unplug can fail. And these days, it fails a
> >          little too easily, with respect to the above case. Specifically, if
> >          a signal is pending on the process, hot unplug fails.
> >
> >          [...]
> >
> >          So in this case, other things (unmovable pages, un-splittable huge
> >          pages) can also cause the above problem. However, those are
> >          demonstrably less common than simply having a pending signal. I've
> >          got bug reports from users who can trivially reproduce this by
> >          killing their process with a "kill -9", for example.
>
> This looks like a bug of the said driver no? If the tear down process is
> killed it could very well happen right before offlining so you end up in
> the very same state. Or what am I missing?

IIUC (John can correct me if I am wrong):

1) The process holds the device node open
2) The process gets killed or quits
3) As the process gets torn down, it closes the device node
4) Closing the device node results in the driver removing the device and
   calling offline_and_remove_memory()

So it's not a "tear down process" that triggers that offlining_removal 
somehow explicitly, it's just a side-product of it letting go of the 
device node as the process gets torn down.

>   
> > Especially with ZONE_MOVABLE, offlining is supposed to work in most
> > cases when offlining actually hotplugged (not boot) memory, and only fail
> > in rare corner cases (e.g., some driver holds a reference to a page in
> > ZONE_MOVABLE, turning it unmovable).
> >
> > In these corner cases we really don't want to be stuck forever in
> > offline_and_remove_memory(). But in the general cases, we really want to
> > do our best to make memory offlining succeed -- in a reasonable
> > timeframe.
> >
> > Reliably failing in the described case when there is a fatal signal pending
> > is sub-optimal. The pending signal check is mostly only relevant when user
> > space explicitly triggers offlining of memory using sysfs device attributes
> > ("state" or "online" attribute), but not when coming via
> > offline_and_remove_memory().
> >
> > So let's use a timer instead and ignore fatal signals, because they are
> > not really expressive for offline_and_remove_memory() users. Let's default
> > to 30 seconds if no timeout was specified, and limit the timeout to 120
> > seconds.
>
> I really hate having timeouts back. They just proven to be hard to get
> right and it is essentially a policy implemented in the kernel. They
> simply do not belong to the kernel space IMHO.

As much as I agree with you in terms of offlining triggered from user 
space (e.g., write "state" or "online" attribute) where user-space is 
actually in charge  and can do something reasonable (timeout, retry, 
whatever), in these the offline_and_remove_memory() case it's the driver 
that wants a best-effort memory offlining+removal.

If it times out, virtio-mem will simply try another block or retry 
later. Right now, it could get stuck forever in 
offline_and_remove_memory(), which is obviously "not great". 
Fortunately, for virtio-mem it's configurable and we use the 
alloc_contig_range()-method for now as default.

If it would time out for John's driver, we most certainly don't want to 
be stuck in offline_and_remove_memory(), blocking device/driver 
unloading (and even a reboot IIRC) possibly forever.


I much rather have offline_and_remove_memory() indicate "timeout" to a 
in-kernel user a bit earlier than getting stuck in there forever. The 
timeout parameter allows for giving the in-kernel users a bit of 
flexibility, which I showcases for virtio-mem that unplugs smaller 
blocks and rather wants to fail fast and retry later.


Sure, we could make the timeout configurable to optimize for some corner 
cases, but that's not really what offline_and_remove_memory() users want 
and I doubt anybody would fine-tune that: they want a best-effort 
attempt. And that's IMHO not really a policy, it's an implementation 
detail of these drivers.

For example, the driver from John could simply never call 
offline_and_remove_memory() and always require a reboot when wanting to 
reuse a device. But that's definitely what users want.

virtio-mem could simply never call offline_and_remove_memory() and 
indicate "I don't support unplug of these online memory blocks". But 
that's *definitely* not what users want.


I'm very open for alternatives regarding offline_and_remove_memory(), so 
far this was the only reasonable thing I could come up with that 
actually achieves what we want for these users: not get stuck in there 
forever but rather fail earlier than later.

And most importantly, not somehow try to involve user space that isn't 
even in charge of the offlining operation.

--
Cheers,

David / dhildenb