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 <[email protected]>
---
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;
}
return -ENOENT;
found:
--
2.45.1
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 <[email protected]>
> ---
>
> 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?
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
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 <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>
> 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
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 <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]>
>
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.
>>
>