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 10:24, Jinjiang Tu wrote:
> 
> 在 2025/7/3 15:57, David Hildenbrand 写道:
>> 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
> If 2) is executed when do_migrate_range() increment the refcount of the
> folio, the split fails, and retry is meaningless.

kill_procs_now will kill all processes, effectively unmapping the folio 
in that case?

So retrying would later just ... get us an unmapped folio and we can 
make progress?

-- 
Cheers,

David / dhildenb