Re: [PATCH v2] mm/hugetlb: mm/memory_hotplug: use a folio in scan_movable_pages()

[Sidhartha Kumar <sidhartha.kumar@oracle.com>]

On 5/31/24 6:05 AM, David Hildenbrand wrote:
> On 31.05.24 15:04, David Hildenbrand wrote:
>> On 30.05.24 19:14, Sidhartha Kumar wrote:
>>> By using a folio in scan_movable_pages() we convert the last user of the
>>> page-based hugetlb information macro functions to the folio version.
>>> After this conversion, we can safely remove the page-based definitions
>>> from include/linux/hugetlb.h.
>>>
>>> Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com>
>>> ---
>>>
>>> v1 -> v2:
>>>     simplify pfn skipping logic with pfn |= folio_nr_pages(folio) - 1
>>>     per Matthew
>>>
>>>    include/linux/hugetlb.h |  6 +-----
>>>    mm/memory_hotplug.c     | 11 +++++------
>>>    2 files changed, 6 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
>>> index 15a58f69782c..279aca379b95 100644
>>> --- a/include/linux/hugetlb.h
>>> +++ b/include/linux/hugetlb.h
>>> @@ -616,9 +616,7 @@ static __always_inline                        \
>>>    bool folio_test_hugetlb_##flname(struct folio *folio)        \
>>>        {    void *private = &folio->private;        \
>>>            return test_bit(HPG_##flname, private);        \
>>> -    }                            \
>>> -static inline int HPage##uname(struct page *page)        \
>>> -    { return test_bit(HPG_##flname, &(page->private)); }
>>> +    }
>>>    #define SETHPAGEFLAG(uname, flname)                \
>>>    static __always_inline                        \
>>> @@ -637,8 +635,6 @@ void folio_clear_hugetlb_##flname(struct folio 
>>> *folio)        \
>>>    #define TESTHPAGEFLAG(uname, flname)                \
>>>    static inline bool                        \
>>>    folio_test_hugetlb_##flname(struct folio *folio)        \
>>> -    { return 0; }                        \
>>> -static inline int HPage##uname(struct page *page)        \
>>>        { return 0; }
>>>    #define SETHPAGEFLAG(uname, flname)                \
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index 431b1f6753c0..9c36eb3bbd3b 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -1731,8 +1731,8 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>        unsigned long pfn;
>>>        for (pfn = start; pfn < end; pfn++) {
>>> -        struct page *page, *head;
>>> -        unsigned long skip;
>>> +        struct page *page;
>>> +        struct folio *folio;
>>>            if (!pfn_valid(pfn))
>>>                continue;
>>> @@ -1753,7 +1753,7 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>            if (!PageHuge(page))
>>>                continue;
>>> -        head = compound_head(page);
>>> +        folio = page_folio(page);
>>>            /*
>>>             * This test is racy as we hold no reference or lock.  The
>>>             * hugetlb page could have been free'ed and head is no longer
>>> @@ -1761,10 +1761,9 @@ static int scan_movable_pages(unsigned long start, 
>>> unsigned long end,
>>>             * cases false positives and negatives are possible.  Calling
>>>             * code must deal with these scenarios.
>>>             */
>>> -        if (HPageMigratable(head))
>>> +        if (folio_test_hugetlb_migratable(folio))
>>>                goto found;
>>> -        skip = compound_nr(head) - (pfn - page_to_pfn(head));
>>> -        pfn += skip - 1;
>>> +        pfn |= folio_nr_pages(folio) - 1;
>>
>> Likely not exactly what we want?
>>
>> pfn |= folio_nr_pages(folio);
>>
>> Would make sure that we are "one PFN before the start of the next
>> folio". The pfn++ before the next loop iteration would move us to the
>> next folio.
>>
>> Or am I missing something?
> 
> Okay, I got it wrong.
> 
> "folio_nr_pages(folio) - 1" gives us the bitmask to land one PFN before the end.

ya because folio_nr_pages() will be a power of 2, subtracting 1 turns it into a 
bitmask.

> 
> Acked-by: David Hildenbrand <david@redhat.com>
> 

Thanks for taking a look at this.

>>
>> It might be cleaner if we would handle the "pfn++;" on the "continue;"
>> paths inmstead, and simply here do something like
>>
>>     pfn = ALIGN(pfn + 1, folio_nr_pages(folio));
>>
>> instead.
>>
>