| Message ID | 20240530171427.242018-1-sidhartha.kumar@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] mm/hugetlb: mm/memory_hotplug: use a folio in scan_movable_pages() | expand |
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? 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.
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. Acked-by: David Hildenbrand <david@redhat.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. >
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. >> >
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:
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(-)