Re: [PATCH v2 2/2] mm/memory_hotplug: fix hwpoisoned large folio handling in do_migrate_range

[David Hildenbrand <david@redhat.com>]

On 03.07.25 09:46, Jinjiang Tu wrote:
> 
> 在 2025/7/1 22:21, Oscar Salvador 写道:
>> On Fri, Jun 27, 2025 at 08:57:47PM +0800, Jinjiang Tu wrote:
>>> In do_migrate_range(), the hwpoisoned folio may be large folio, which
>>> can't be handled by unmap_poisoned_folio().
>>>
>>> I can reproduce this issue in qemu after adding delay in memory_failure()
>>>
>>> BUG: kernel NULL pointer dereference, address: 0000000000000000
>>> Workqueue: kacpi_hotplug acpi_hotplug_work_fn
>>> RIP: 0010:try_to_unmap_one+0x16a/0xfc0
>>>    <TASK>
>>>    rmap_walk_anon+0xda/0x1f0
>>>    try_to_unmap+0x78/0x80
>>>    ? __pfx_try_to_unmap_one+0x10/0x10
>>>    ? __pfx_folio_not_mapped+0x10/0x10
>>>    ? __pfx_folio_lock_anon_vma_read+0x10/0x10
>>>    unmap_poisoned_folio+0x60/0x140
>>>    do_migrate_range+0x4d1/0x600
>>>    ? slab_memory_callback+0x6a/0x190
>>>    ? notifier_call_chain+0x56/0xb0
>>>    offline_pages+0x3e6/0x460
>>>    memory_subsys_offline+0x130/0x1f0
>>>    device_offline+0xba/0x110
>>>    acpi_bus_offline+0xb7/0x130
>>>    acpi_scan_hot_remove+0x77/0x290
>>>    acpi_device_hotplug+0x1e0/0x240
>>>    acpi_hotplug_work_fn+0x1a/0x30
>>>    process_one_work+0x186/0x340
>>>
>>> In this case, just make offline_pages() fail.
>>>
>>> Besides, do_migrate_range() may be called between memory_failure set
>>> hwposion flag and ioslate the folio from lru, so remove WARN_ON(). In other
>>> places, unmap_poisoned_folio() is called when the folio is isolated, obey
>>> it in do_migrate_range() too.
>>>
>>> Fixes: b15c87263a69 ("hwpoison, memory_hotplug: allow hwpoisoned pages to be offlined")
>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> ...
>>> @@ -2041,11 +2048,9 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages,
>>>    
>>>    			ret = scan_movable_pages(pfn, end_pfn, &pfn);
>>>    			if (!ret) {
>>> -				/*
>>> -				 * TODO: fatal migration failures should bail
>>> -				 * out
>>> -				 */
>>> -				do_migrate_range(pfn, end_pfn);
>>> +				ret = do_migrate_range(pfn, end_pfn);
>>> +				if (ret)
>>> +					break;
>> I am not really sure about this one.
>> I get the reason you're adding it, but note that migrate_pages() can also return
>> "fatal" errors and we don't propagate that.
>>
>> The moto has always been to migrate as much as possible, and this changes this
>> behaviour.
>>    
> If we just skip to next pfn, offline_pages() will deadloop meaningless
> util received signal.

Yeah, that's also not good,

> It seems there is no document to guarantee memory offline have to
> migrate as much as possible.

We should try offlining as good as possible. But if there is something 
we just cannot possibly migrate, there is no sense in retrying.

Now, could we run into this case here because we are racing with other 
code, and actually retrying again could make it work?

Remind me again: how exactly do we arrive at this point of having a 
large folio that is hwpoisoned but still mapped?

In memory_failure(), we do on a  large folio

1) folio_set_has_hwpoisoned
2) try_to_split_thp_page
3) if splitting fails, kill_procs_now

So given that, couldn't we just retry the migration until the race is 
over and we are good?

-- 
Cheers,

David / dhildenb