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

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2025/7/7 20:37, David Hildenbrand 写道:
> On 07.07.25 13:51, Jinjiang Tu wrote:
>>
>> 在 2025/7/3 17:06, David Hildenbrand 写道:
>>> 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?
>>>
>> kill_procs_now()->collect_procs() collects the tasks to kill. But not
>> all tasks that maps the folio
>> will be collected,
>> collect_procs_anon()->task_early_kill()->find_early_kill_thread() 
>> will not
>> select the task (not current) if PF_MCE_PROCESS isn't set and
>> sysctl_memory_failure_early_kill
>> isn't enabled (this is the default behaviour).
>
> I think you're right, that's rather nasty.
>
> We fail to split, but keep the folio mapped into some processes.
>
> And we can't unmap it because unmap_poisoned_folio() does not properly
> support large folios yet.
>
> We really should unmap the folio when splitting fail. :(
unmap_poisoned_folio() doesn't guarantee the folio is unmapped 
successfully, according
to the return val. Although I don't know in which case we will fail to 
unmap.