David Hildenbrand
2023-Jun-27 13:14 UTC
[PATCH v1 3/5] mm/memory_hotplug: make offline_and_remove_memory() timeout instead of failing on fatal signals
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