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.
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.
This change is also valuable for virtio-mem in BBM (Big Block Mode) with
"bbm_safe_unplug=off", to avoid endless loops when stuck forever in
offline_and_remove_memory().
While at it, drop the "extern" from offline_and_remove_memory() to make
it fit into a single line.
[1] https://lkml.kernel.org/r/[email protected]
Signed-off-by: David Hildenbrand <[email protected]>
---
drivers/virtio/virtio_mem.c | 2 +-
include/linux/memory_hotplug.h | 2 +-
mm/memory_hotplug.c | 50 ++++++++++++++++++++++++++++++++--
3 files changed, 50 insertions(+), 4 deletions(-)
diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
index cb8bc6f6aa90..f8792223f1db 100644
--- a/drivers/virtio/virtio_mem.c
+++ b/drivers/virtio/virtio_mem.c
@@ -738,7 +738,7 @@ static int virtio_mem_offline_and_remove_memory(struct virtio_mem *vm,
"offlining and removing memory: 0x%llx - 0x%llx\n", addr,
addr + size - 1);
- rc = offline_and_remove_memory(addr, size);
+ rc = offline_and_remove_memory(addr, size, 0);
if (!rc) {
atomic64_sub(size, &vm->offline_size);
/*
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 9fcbf5706595..d5f9e8b5a4a4 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -307,7 +307,7 @@ extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
struct zone *zone, struct memory_group *group);
extern int remove_memory(u64 start, u64 size);
extern void __remove_memory(u64 start, u64 size);
-extern int offline_and_remove_memory(u64 start, u64 size);
+int offline_and_remove_memory(u64 start, u64 size, unsigned int timeout_ms);
#else
static inline void try_offline_node(int nid) {}
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 0d2151df4ee1..ca635121644a 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -152,6 +152,22 @@ void put_online_mems(void)
bool movable_node_enabled = false;
+/*
+ * Protected by the device hotplug lock: offline_and_remove_memory()
+ * will activate a timer such that offlining cannot be stuck forever.
+ *
+ * With an active timer, fatal signals will be ignored, because they can be
+ * counter-productive when dying user space triggers device unplug/driver
+ * unloading that ends up offlining+removing device memory.
+ */
+static bool mhp_offlining_timer_active;
+static atomic_t mhp_offlining_timer_expired;
+
+static void mhp_offline_timer_fn(struct timer_list *unused)
+{
+ atomic_set(&mhp_offlining_timer_expired, 1);
+}
+
#ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
int mhp_default_online_type = MMOP_OFFLINE;
#else
@@ -1879,7 +1895,18 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
do {
pfn = start_pfn;
do {
- if (fatal_signal_pending(current)) {
+ /*
+ * If a timer is active, we're coming via
+ * offline_and_remove_memory() and want to ignore even
+ * fatal signals.
+ */
+ if (mhp_offlining_timer_active) {
+ if (atomic_read(&mhp_offlining_timer_expired)) {
+ ret = -ETIMEDOUT;
+ reason = "timeout";
+ goto failed_removal_isolated;
+ }
+ } else if (fatal_signal_pending(current)) {
ret = -EINTR;
reason = "signal backoff";
goto failed_removal_isolated;
@@ -2232,11 +2259,17 @@ static int try_reonline_memory_block(struct memory_block *mem, void *arg)
* memory is still in use. Primarily useful for memory devices that logically
* unplugged all memory (so it's no longer in use) and want to offline + remove
* that memory.
+ *
+ * offline_and_remove_memory() will not fail on fatal signals. Instead, it will
+ * fail once the timeout has been reached and offlining was not completed. If
+ * no timeout was specified, it will timeout after 30 seconds. The timeout is
+ * limited to 120 seconds.
*/
-int offline_and_remove_memory(u64 start, u64 size)
+int offline_and_remove_memory(u64 start, u64 size, unsigned int timeout_ms)
{
const unsigned long mb_count = size / memory_block_size_bytes();
uint8_t *online_types, *tmp;
+ struct timer_list timer;
int rc;
if (!IS_ALIGNED(start, memory_block_size_bytes()) ||
@@ -2261,9 +2294,22 @@ int offline_and_remove_memory(u64 start, u64 size)
lock_device_hotplug();
+ if (!timeout_ms)
+ timeout_ms = 30 * MSEC_PER_SEC;
+ timeout_ms = min_t(unsigned int, timeout_ms, 120 * MSEC_PER_SEC);
+
+ timer_setup_on_stack(&timer, mhp_offline_timer_fn, 0);
+ mod_timer(&timer, jiffies + msecs_to_jiffies(timeout_ms));
+ mhp_offlining_timer_active = true;
+
tmp = online_types;
rc = walk_memory_blocks(start, size, &tmp, try_offline_memory_block);
+ timer_delete_sync(&timer);
+ atomic_set(&mhp_offlining_timer_expired, 0);
+ mhp_offlining_timer_active = false;
+ destroy_timer_on_stack(&timer);
+
/*
* In case we succeeded to offline all memory, remove it.
* This cannot fail as it cannot get onlined in the meantime.
--
2.40.1
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?
> 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.
> This change is also valuable for virtio-mem in BBM (Big Block Mode) with
> "bbm_safe_unplug=off", to avoid endless loops when stuck forever in
> offline_and_remove_memory().
>
> While at it, drop the "extern" from offline_and_remove_memory() to make
> it fit into a single line.
>
> [1] https://lkml.kernel.org/r/[email protected]
>
> Signed-off-by: David Hildenbrand <[email protected]>
> ---
> drivers/virtio/virtio_mem.c | 2 +-
> include/linux/memory_hotplug.h | 2 +-
> mm/memory_hotplug.c | 50 ++++++++++++++++++++++++++++++++--
> 3 files changed, 50 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/virtio/virtio_mem.c b/drivers/virtio/virtio_mem.c
> index cb8bc6f6aa90..f8792223f1db 100644
> --- a/drivers/virtio/virtio_mem.c
> +++ b/drivers/virtio/virtio_mem.c
> @@ -738,7 +738,7 @@ static int virtio_mem_offline_and_remove_memory(struct virtio_mem *vm,
> "offlining and removing memory: 0x%llx - 0x%llx\n", addr,
> addr + size - 1);
>
> - rc = offline_and_remove_memory(addr, size);
> + rc = offline_and_remove_memory(addr, size, 0);
> if (!rc) {
> atomic64_sub(size, &vm->offline_size);
> /*
> diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
> index 9fcbf5706595..d5f9e8b5a4a4 100644
> --- a/include/linux/memory_hotplug.h
> +++ b/include/linux/memory_hotplug.h
> @@ -307,7 +307,7 @@ extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> struct zone *zone, struct memory_group *group);
> extern int remove_memory(u64 start, u64 size);
> extern void __remove_memory(u64 start, u64 size);
> -extern int offline_and_remove_memory(u64 start, u64 size);
> +int offline_and_remove_memory(u64 start, u64 size, unsigned int timeout_ms);
>
> #else
> static inline void try_offline_node(int nid) {}
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index 0d2151df4ee1..ca635121644a 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -152,6 +152,22 @@ void put_online_mems(void)
>
> bool movable_node_enabled = false;
>
> +/*
> + * Protected by the device hotplug lock: offline_and_remove_memory()
> + * will activate a timer such that offlining cannot be stuck forever.
> + *
> + * With an active timer, fatal signals will be ignored, because they can be
> + * counter-productive when dying user space triggers device unplug/driver
> + * unloading that ends up offlining+removing device memory.
> + */
> +static bool mhp_offlining_timer_active;
> +static atomic_t mhp_offlining_timer_expired;
> +
> +static void mhp_offline_timer_fn(struct timer_list *unused)
> +{
> + atomic_set(&mhp_offlining_timer_expired, 1);
> +}
> +
> #ifndef CONFIG_MEMORY_HOTPLUG_DEFAULT_ONLINE
> int mhp_default_online_type = MMOP_OFFLINE;
> #else
> @@ -1879,7 +1895,18 @@ int __ref offline_pages(unsigned long start_pfn, unsigned long nr_pages,
> do {
> pfn = start_pfn;
> do {
> - if (fatal_signal_pending(current)) {
> + /*
> + * If a timer is active, we're coming via
> + * offline_and_remove_memory() and want to ignore even
> + * fatal signals.
> + */
> + if (mhp_offlining_timer_active) {
> + if (atomic_read(&mhp_offlining_timer_expired)) {
> + ret = -ETIMEDOUT;
> + reason = "timeout";
> + goto failed_removal_isolated;
> + }
> + } else if (fatal_signal_pending(current)) {
> ret = -EINTR;
> reason = "signal backoff";
> goto failed_removal_isolated;
> @@ -2232,11 +2259,17 @@ static int try_reonline_memory_block(struct memory_block *mem, void *arg)
> * memory is still in use. Primarily useful for memory devices that logically
> * unplugged all memory (so it's no longer in use) and want to offline + remove
> * that memory.
> + *
> + * offline_and_remove_memory() will not fail on fatal signals. Instead, it will
> + * fail once the timeout has been reached and offlining was not completed. If
> + * no timeout was specified, it will timeout after 30 seconds. The timeout is
> + * limited to 120 seconds.
> */
> -int offline_and_remove_memory(u64 start, u64 size)
> +int offline_and_remove_memory(u64 start, u64 size, unsigned int timeout_ms)
> {
> const unsigned long mb_count = size / memory_block_size_bytes();
> uint8_t *online_types, *tmp;
> + struct timer_list timer;
> int rc;
>
> if (!IS_ALIGNED(start, memory_block_size_bytes()) ||
> @@ -2261,9 +2294,22 @@ int offline_and_remove_memory(u64 start, u64 size)
>
> lock_device_hotplug();
>
> + if (!timeout_ms)
> + timeout_ms = 30 * MSEC_PER_SEC;
> + timeout_ms = min_t(unsigned int, timeout_ms, 120 * MSEC_PER_SEC);
> +
> + timer_setup_on_stack(&timer, mhp_offline_timer_fn, 0);
> + mod_timer(&timer, jiffies + msecs_to_jiffies(timeout_ms));
> + mhp_offlining_timer_active = true;
> +
> tmp = online_types;
> rc = walk_memory_blocks(start, size, &tmp, try_offline_memory_block);
>
> + timer_delete_sync(&timer);
> + atomic_set(&mhp_offlining_timer_expired, 0);
> + mhp_offlining_timer_active = false;
> + destroy_timer_on_stack(&timer);
> +
> /*
> * In case we succeeded to offline all memory, remove it.
> * This cannot fail as it cannot get onlined in the meantime.
> --
> 2.40.1
--
Michal Hocko
SUSE Labs
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
On Tue 27-06-23 15:14:11, David Hildenbrand wrote:
> 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.
Isn't that just fragile? The operation might fail for other reasons. Why
cannot there be a hold on the resource to control the tear down
explicitly?
> > > 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.
It seems that offline_and_remove_memory is using a wrong operation then.
If it wants an opportunistic offlining with some sort of policy. Timeout
might be just one policy to use but failure mode or a retry count might
be a better fit for some users. So rather than (ab)using offline_pages,
would be make more sense to extract basic offlining steps and allow
drivers like virtio-mem to reuse them and define their own policy?
> 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.
Now I am confused. John driver wants to tear down the device now? There
is no way to release that memory otherwise AFAIU from the initial
problem description.
--
Michal Hocko
SUSE Labs
On 27.06.23 16:17, Michal Hocko wrote:
> On Tue 27-06-23 15:14:11, David Hildenbrand wrote:
>> 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.
>
> Isn't that just fragile? The operation might fail for other reasons. Why
> cannot there be a hold on the resource to control the tear down
> explicitly?
I'll let John comment on that. But from what I understood, in most
setups where ZONE_MOVABLE gets used for hotplugged memory
offline_and_remove_memory() succeeds and allows for reusing the device
later without a reboot.
For the cases where it doesn't work, a reboot is required.
>
>>>> 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.
>
> It seems that offline_and_remove_memory is using a wrong operation then.
> If it wants an opportunistic offlining with some sort of policy. Timeout
> might be just one policy to use but failure mode or a retry count might
> be a better fit for some users. So rather than (ab)using offline_pages,
> would be make more sense to extract basic offlining steps and allow
> drivers like virtio-mem to reuse them and define their own policy?
virtio-mem, in default operation, does that: use alloc_contig_range() to
logically unplug ("fake offline") that memory and then just trigger
offline_and_remove_memory() to make it "officially offline".
In that mode, offline_and_remove_memory() cannot really timeout and is
almost always going to succeed (except memory notifiers and some hugetlb
dissolving).
Right now we also allow the admin to configure ordinary offlining
directly (without prior fake offlining) when bigger memory blocks are
used: offline_pages() is more reliable than alloc_contig_range(), for
example, because it disables the PCP and the LRU cache, and retries more
often (well, unfortunately then also forever). It has a higher chance of
succeeding especially when bigger blocks of memory are offlined+removed.
Maybe we should make the alloc_contig_range()-based mechanism more
configurable and make it the only mode in virtio-mem, such that we don't
have to mess with offline_and_remove_memory() endless loops -- at least
for virtio-mem.
>
>> 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.
>
> Now I am confused. John driver wants to tear down the device now? There
> is no way to release that memory otherwise AFAIU from the initial
> problem description.
From what I understood if offline_and_remove_memory() succeeded, the
device can be reinitialized and used again. Otherwise, a reboot is
required because that memory is still added to the system.
Thanks Michal!
--
Cheers,
David / dhildenb
On Tue 27-06-23 16:57:53, David Hildenbrand wrote:
> On 27.06.23 16:17, Michal Hocko wrote:
> > On Tue 27-06-23 15:14:11, David Hildenbrand wrote:
> > > 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.
> >
> > Isn't that just fragile? The operation might fail for other reasons. Why
> > cannot there be a hold on the resource to control the tear down
> > explicitly?
>
> I'll let John comment on that. But from what I understood, in most setups
> where ZONE_MOVABLE gets used for hotplugged memory
> offline_and_remove_memory() succeeds and allows for reusing the device later
> without a reboot.
>
> For the cases where it doesn't work, a reboot is required.
Then the solution should be really robust and means to handle the
failure - e.g. by retrying or alerting the admin.
> > > > > 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.
> >
> > It seems that offline_and_remove_memory is using a wrong operation then.
> > If it wants an opportunistic offlining with some sort of policy. Timeout
> > might be just one policy to use but failure mode or a retry count might
> > be a better fit for some users. So rather than (ab)using offline_pages,
> > would be make more sense to extract basic offlining steps and allow
> > drivers like virtio-mem to reuse them and define their own policy?
>
> virtio-mem, in default operation, does that: use alloc_contig_range() to
> logically unplug ("fake offline") that memory and then just trigger
> offline_and_remove_memory() to make it "officially offline".
>
> In that mode, offline_and_remove_memory() cannot really timeout and is
> almost always going to succeed (except memory notifiers and some hugetlb
> dissolving).
>
> Right now we also allow the admin to configure ordinary offlining directly
> (without prior fake offlining) when bigger memory blocks are used:
> offline_pages() is more reliable than alloc_contig_range(), for example,
> because it disables the PCP and the LRU cache, and retries more often (well,
> unfortunately then also forever). It has a higher chance of succeeding
> especially when bigger blocks of memory are offlined+removed.
>
> Maybe we should make the alloc_contig_range()-based mechanism more
> configurable and make it the only mode in virtio-mem, such that we don't
> have to mess with offline_and_remove_memory() endless loops -- at least for
> virtio-mem.
Yes, that sounds better than hooking up into offline_pages the way this
patch is doing.
--
Michal Hocko
SUSE Labs
On 6/27/23 08:14, Michal Hocko wrote:
> On Tue 27-06-23 16:57:53, David Hildenbrand wrote:
...
>>>> 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.
>>>
>>> Isn't that just fragile? The operation might fail for other reasons. Why
>>> cannot there be a hold on the resource to control the tear down
>>> explicitly?
>>
>> I'll let John comment on that. But from what I understood, in most setups
>> where ZONE_MOVABLE gets used for hotplugged memory
>> offline_and_remove_memory() succeeds and allows for reusing the device later
>> without a reboot.
>>
>> For the cases where it doesn't work, a reboot is required.
That is exactly correct. That's what we ran into.
And there are workarounds (for example: kthreads don't have any signals
pending...), but I did want to follow through here and make -mm aware of the
problem. And see if there is a better way.
...
>>> It seems that offline_and_remove_memory is using a wrong operation then.
>>> If it wants an opportunistic offlining with some sort of policy. Timeout
>>> might be just one policy to use but failure mode or a retry count might
>>> be a better fit for some users. So rather than (ab)using offline_pages,
>>> would be make more sense to extract basic offlining steps and allow
>>> drivers like virtio-mem to reuse them and define their own policy?
...like this, perhaps. Sounds promising!
thanks,
--
John Hubbard
NVIDIA
Hi David,
kernel test robot noticed the following build warnings:
[auto build test WARNING on 6995e2de6891c724bfeb2db33d7b87775f913ad1]
url: https://github.com/intel-lab-lkp/linux/commits/David-Hildenbrand/mm-memory_hotplug-check-for-fatal-signals-only-in-offline_pages/20230627-192444
base: 6995e2de6891c724bfeb2db33d7b87775f913ad1
patch link: https://lore.kernel.org/r/20230627112220.229240-4-david%40redhat.com
patch subject: [PATCH v1 3/5] mm/memory_hotplug: make offline_and_remove_memory() timeout instead of failing on fatal signals
config: x86_64-randconfig-x006-20230627 (https://download.01.org/0day-ci/archive/20230628/[email protected]/config)
compiler: clang version 15.0.7 (https://github.com/llvm/llvm-project.git 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a)
reproduce: (https://download.01.org/0day-ci/archive/20230628/[email protected]/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <[email protected]>
| Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
All warnings (new ones prefixed by >>):
>> mm/memory_hotplug.c:163:13: warning: unused variable 'mhp_offlining_timer_active' [-Wunused-variable]
static bool mhp_offlining_timer_active;
^
mm/memory_hotplug.c:166:13: warning: unused function 'mhp_offline_timer_fn' [-Wunused-function]
static void mhp_offline_timer_fn(struct timer_list *unused)
^
2 warnings generated.
vim +/mhp_offlining_timer_active +163 mm/memory_hotplug.c
154
155 /*
156 * Protected by the device hotplug lock: offline_and_remove_memory()
157 * will activate a timer such that offlining cannot be stuck forever.
158 *
159 * With an active timer, fatal signals will be ignored, because they can be
160 * counter-productive when dying user space triggers device unplug/driver
161 * unloading that ends up offlining+removing device memory.
162 */
> 163 static bool mhp_offlining_timer_active;
164 static atomic_t mhp_offlining_timer_expired;
165
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki