[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <dec62086-8170-4fea-987a-9cc514cc4b27@redhat.com>
Date: Fri, 31 May 2024 15:05:42 +0200
From: David Hildenbrand <david@...hat.com>
To: Sidhartha Kumar <sidhartha.kumar@...cle.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 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.
Acked-by: David Hildenbrand <david@...hat.com>
>
> 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.
>
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists