[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <25dd4f6b-5c2c-4d29-b088-e8308e1e300c@oracle.com>
Date: Fri, 31 May 2024 10:39:00 -0700
From: Sidhartha Kumar <sidhartha.kumar@...cle.com>
To: David Hildenbrand <david@...hat.com>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org
Cc: akpm@...ux-foundation.org, vishal.moola@...cle.com, muchun.song@...ux.dev,
osalvador@...e.de, willy@...radead.org
Subject: Re: [PATCH v2] mm/hugetlb: mm/memory_hotplug: use a folio in
scan_movable_pages()
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@...cle.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@...hat.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.
>>
>
Powered by blists - more mailing lists