Re: [PATCH] mm/memory_hotplug: fix call folio_test_large with tail page in do_migrate_range

[Jinjiang Tu <tujinjiang@huawei.com>]

在 2025/3/26 3:05, David Hildenbrand 写道:
> On 25.03.25 04:02, Jinjiang Tu wrote:
>>
>> 在 2025/3/24 21:44, David Hildenbrand 写道:
>>> On 24.03.25 14:17, Jinjiang Tu wrote:
>>>> We triggered the below BUG:
>>>>
>>>>    page: refcount:0 mapcount:0 mapping:0000000000000000 index:0x2
>>>> pfn:0x240402
>>>>    head: order:9 mapcount:0 entire_mapcount:0 nr_pages_mapped:0
>>>> pincount:0
>>>>    flags: 0x1ffffe0000000040(head|node=1|zone=3|lastcpupid=0x1ffff)
>>>>    page_type: f4(hugetlb)
>>>>    page dumped because: VM_BUG_ON_PAGE(page->compound_head & 1)
>>>>    ------------[ cut here ]------------
>>>>    kernel BUG at ./include/linux/page-flags.h:310!
>>>>    Internal error: Oops - BUG: 00000000f2000800 [#1] PREEMPT SMP
>>>>    Modules linked in:
>>>>    CPU: 7 UID: 0 PID: 166 Comm: sh Not tainted 6.14.0-rc7-dirty #374
>>>>    Hardware name: QEMU QEMU Virtual Machine, BIOS 0.0.0 02/06/2015
>>>>    pstate: 60000005 (nZCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>>    pc : const_folio_flags+0x3c/0x58
>>>>    lr : const_folio_flags+0x3c/0x58
>>>>    Call trace:
>>>>     const_folio_flags+0x3c/0x58 (P)
>>>>     do_migrate_range+0x164/0x720
>>>>     offline_pages+0x63c/0x6fc
>>>>     memory_subsys_offline+0x190/0x1f4
>>>>     device_offline+0xc0/0x13c
>>>>     state_store+0x90/0xd8
>>>>     dev_attr_store+0x18/0x2c
>>>>     sysfs_kf_write+0x44/0x54
>>>>     kernfs_fop_write_iter+0x120/0x1cc
>>>>     vfs_write+0x240/0x378
>>>>     ksys_write+0x70/0x108
>>>>     __arm64_sys_write+0x1c/0x28
>>>>     invoke_syscall+0x48/0x10c
>>>>     el0_svc_common.constprop.0+0x40/0xe0
>>>>
>>>> When allocating a hugetlb folio, between the folio is taken from buddy
>>>> and prep_compound_page() is called, start_isolate_page_range() and
>>>> do_migrate_range() is called. When do_migrate_range() scans the head
>>>> page
>>>> of the hugetlb folio, the compound_head field isn't set, so scans the
>>>> tail page next. And at this time, the compound_head field of tail
>>>> page is
>>>> set, folio_test_large() is called by tail page, thus triggers
>>>> VM_BUG_ON().
>>>>
>>>> To fix it, get folio refcount before calling folio_test_large().
>>>>
>>>> Fixes: 8135d8926c08 ("mm: memory_hotplug: memory hotremove supports
>>>> thp migration")
>>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>>>> ---
>>>>    mm/memory_hotplug.c | 12 +++---------
>>>>    1 file changed, 3 insertions(+), 9 deletions(-)
>>>>
>>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>>> index 16cf9e17077e..f600c26ce5de 100644
>>>> --- a/mm/memory_hotplug.c
>>>> +++ b/mm/memory_hotplug.c
>>>> @@ -1813,21 +1813,15 @@ static void do_migrate_range(unsigned long
>>>> start_pfn, unsigned long end_pfn)
>>>>            page = pfn_to_page(pfn);
>>>>            folio = page_folio(page);
>>>>    -        /*
>>>> -         * No reference or lock is held on the folio, so it might
>>>> -         * be modified concurrently (e.g. split).  As such,
>>>> -         * folio_nr_pages() may read garbage.  This is fine as the
>>>> outer
>>>> -         * loop will revisit the split folio later.
>>>> -         */
>>>> -        if (folio_test_large(folio))
>>>> -            pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>> -
>>>>            if (!folio_try_get(folio))
>>>>                continue;
>>>>              if (unlikely(page_folio(page) != folio))
>>>>                goto put_folio;
>>>>    +        if (folio_test_large(folio))
>>>> +            pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>>>
>>> Moving that will not make it able to skip the large frozen
>>> (refcount==0, e.g., free hugetlb) folio in the continue/put_folio case
>>> above. Hmmmm ..
>> For free hugetlb, pfn is increased by 1 in each loop. This leads to skip
>> free hugetlb slower.
>
> Yes. But now I realize that we have the same issue with free buddy 
> pages already (folio_try_get of each individual page :( ).
>
>>>
>>> We could similarly to dumping folios, snapshot them, so we can read
>>> stable data.
>> extract the code in __dump_page()? But snapshot may lead to
>> do_migrate_range() slower too.
>
> There is a patch series on the list to do that, but it might take a 
> while to clean that up. Ideally, we'd also jump over free buddy pages. 
> In the future we might have better ways to do that.
>
> I don't consider this change here really important, but if all it 
> affects is free hugetlb folios, it's not really worth it to have this 
> code around.
>
> Acked-by: David Hildenbrand <david@redhat.com>
>
> But: I suspect 8135d8926c08 is not the introducing commit. Please 
> re-verify.

commit 8135d8926c08 adds PageTransHuge() call, which may lead to VM_BUG_ON if the page is tail page.
Before it, for hugetlb, PageHuge()、compound_head() and compound_order() are called before getting
page ref,  PageHuge()、compound_head() allows to pass a tail page, compound_order() will not trigger
trigger VM_BUG_ON for tail page, and only lead to reading garbage data, leading to fail offline, but
it will not trigger any VM_BUG_ON. So I think 8135d8926c08 is the introducing commit.

Thanks.

>
> We should not CC stable.
>