Re: [PATCH v3 1/5] mm: memory_hotplug: remove head variable in do_migrate_range()

[Miaohe Lin <linmiaohe@huawei.com>]

On 2024/9/29 10:04, Kefeng Wang wrote:
> 
> 
> On 2024/9/29 9:16, Miaohe Lin wrote:
>> On 2024/9/28 16:39, David Hildenbrand wrote:
>>> On 28.09.24 10:34, David Hildenbrand wrote:
>>>> On 28.09.24 06:55, Matthew Wilcox wrote:
>>>>> On Tue, Aug 27, 2024 at 07:47:24PM +0800, Kefeng Wang wrote:
>>>>>> Directly use a folio for HugeTLB and THP when calculate the next pfn, then
>>>>>> remove unused head variable.
>>>>>
>>>>> I just noticed this got merged.  You're going to hit BUG_ON with it.
>>>>>
>>>>>> -        if (PageHuge(page)) {
>>>>>> -            pfn = page_to_pfn(head) + compound_nr(head) - 1;
>>>>>> -            isolate_hugetlb(folio, &source);
>>>>>> -            continue;
>>>>>> -        } else if (PageTransHuge(page))
>>>>>> -            pfn = page_to_pfn(head) + thp_nr_pages(page) - 1;
>>>>>> +        /*
>>>>>> +         * 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)) {
>>>>>
>>>>> But it's not fine.  Look at the implementation of folio_test_large():
>>>>>
>>>>> static inline bool folio_test_large(const struct folio *folio)
>>>>> {
>>>>>            return folio_test_head(folio);
>>>>> }
>>>>>
>>>>> That's going to be provided by:
>>>>>
>>>>> #define FOLIO_TEST_FLAG(name, page)                                     \
>>>>> static __always_inline bool folio_test_##name(const struct folio *folio) \
>>>>> { return test_bit(PG_##name, const_folio_flags(folio, page)); }
>>>>>
>>>>> and here's the BUG:
>>>>>
>>>>> static const unsigned long *const_folio_flags(const struct folio *folio,
>>>>>                    unsigned n)
>>>>> {
>>>>>            const struct page *page = &folio->page;
>>>>>
>>>>>            VM_BUG_ON_PGFLAGS(PageTail(page), page);
>>>>>            VM_BUG_ON_PGFLAGS(n > 0 && !test_bit(PG_head, &page->flags), page);
>>>>>            return &page[n].flags;
>>>>> }
>>>>>
>>>>> (this page can be transformed from a head page to a tail page because,
>>>>> as the comment notes, we don't hold a reference.
>>>>>
>>>>> Please back this out.
>>>>
>>>> Should we generalize the approach in dump_folio() to locally copy a
>>>> folio, so we can safely perform checks before deciding whether we want
>>>> to try grabbing a reference on the real folio (if it's still a folio :) )?
>>>>
>>>
>>> Oh, and I forgot: isn't the existing code already racy?
>>>
>>> PageTransHuge() -> VM_BUG_ON_PAGE(PageTail(page), page);
> 
> Yes, in v1[1], I asked same question for existing code for PageTransHuge(page),
> 
>   "If the page is a tail page, we will BUG_ON(DEBUG_VM enabled) here,
>    but it seems that we don't guarantee the page won't be a tail page."
> 
> 
> we could delay the calculation after we got a ref, but the traversal of pfn may slow down a little if hint a tail pfn, is it acceptable?
> 
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -1786,15 +1786,6 @@ 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;
> -
>                 /*
>                  * HWPoison pages have elevated reference counts so the migration would
>                  * fail on them. It also doesn't make any sense to migrate them in the
> @@ -1807,6 +1798,8 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>                                 folio_isolate_lru(folio);
>                         if (folio_mapped(folio))
>                                 unmap_poisoned_folio(folio, TTU_IGNORE_MLOCK);
> +                       if (folio_test_large(folio))
> +                               pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>                         continue;
>                 }
> 
> @@ -1823,6 +1816,9 @@ static void do_migrate_range(unsigned long start_pfn, unsigned long end_pfn)
>                                 dump_page(page, "isolation failed");
>                         }
>                 }
> +
> +               if (folio_test_large(folio))
> +                       pfn = folio_pfn(folio) + folio_nr_pages(folio) - 1;
>  put_folio:
>                 folio_put(folio);
>         }
> 
> 
>>
>> do_migrate_range is called after start_isolate_page_range(). So a page might not be able to
>> transform from a head page to a tail page as it's isolated?
> start_isolate_page_range() is only isolate free pages, so maybe irrelevant.

A page transform from a head page to a tail page should through the below steps:
1. The compound page is freed into buddy.
2. It's merged into larger order in buddy.
3. It's allocated as a larger order compound page.

Since it is isolated, I think step 2 or 3 cannot happen. Or am I miss something?

Thanks.