| Message ID | 20240520224407.110062-1-sidhartha.kumar@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/hugetlb: remove {Set,Clear}Hpage macros | expand |
On Mon, May 20, 2024 at 03:44:07PM -0700, Sidhartha Kumar wrote: > All users have been converted to use the folio version of these macros, > we can safely remove the page based interface. Yay! Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org>
On 5/20/24 4:30 PM, Matthew Wilcox wrote: > On Mon, May 20, 2024 at 03:44:07PM -0700, Sidhartha Kumar wrote: >> All users have been converted to use the folio version of these macros, >> we can safely remove the page based interface. > > Yay! > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > There is only one remaining user of page-based Test version of these macros. in mm/memory-hotplug.c: if (!PageHuge(page)) continue; head = compound_head(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 * a hugetlb page before the following check. In such unlikely * cases false positives and negatives are possible. Calling * code must deal with these scenarios. */ if (HPageMigratable(head)) goto found; skip = compound_nr(head) - (pfn - page_to_pfn(head)); I've previously sent a patch to convert this to folios[1] but got feedback that it was unsafe. But I'm not sure why replacing compound_head() with page_folio() and using folio_test_hugetlb_migratable(folio) rather than HPageMigratable(head) changes the existing behavior. With no reference or lock, can't the head pointer also be moved and no longer be a part of page like the comment states. So would the folio conversion just be maintaining this level of existing un-safety that the calling code should handle anyways? [1]: https://lore.kernel.org/lkml/Y89DK23hYiLtgGNk@casper.infradead.org/T/#mb3a339b98386b1cd0b87f94f45163756ebd7feaa
On Mon, May 20, 2024 at 04:44:53PM -0700, Sidhartha Kumar wrote: > On 5/20/24 4:30 PM, Matthew Wilcox wrote: > > On Mon, May 20, 2024 at 03:44:07PM -0700, Sidhartha Kumar wrote: > > > All users have been converted to use the folio version of these macros, > > > we can safely remove the page based interface. > > > > Yay! > > > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> > > There is only one remaining user of page-based Test version of these macros. > > in mm/memory-hotplug.c: > > if (!PageHuge(page)) > continue; > head = compound_head(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 > * a hugetlb page before the following check. In such unlikely > * cases false positives and negatives are possible. Calling > * code must deal with these scenarios. > */ > if (HPageMigratable(head)) > goto found; > skip = compound_nr(head) - (pfn - page_to_pfn(head)); > > > I've previously sent a patch to convert this to folios[1] but got feedback > that it was unsafe. But I'm not sure why replacing compound_head() with > page_folio() and using folio_test_hugetlb_migratable(folio) rather than > HPageMigratable(head) changes the existing behavior. With no reference or > lock, can't the head pointer also be moved and no longer be a part of page > like the comment states. So would the folio conversion just be maintaining > this level of existing un-safety that the calling code should handle > anyways? To be fair, that wasn't the last thing I said about that ... https://lore.kernel.org/lkml/Y89DK23hYiLtgGNk@casper.infradead.org/ and looked like David agreed that this was a case where false postive/negative was fine; we were just looking to be right most of the time.
On 21.05.24 00:44, Sidhartha Kumar wrote: > All users have been converted to use the folio version of these macros, > we can safely remove the page based interface. > > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> > --- Reviewed-by: David Hildenbrand <david@redhat.com>
On Mon, May 20, 2024 at 03:44:07PM -0700, Sidhartha Kumar wrote: > All users have been converted to use the folio version of these macros, > we can safely remove the page based interface. > > Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> Reviewed-by: Oscar Salvador <osalvador@suse.de>
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index 77b30a8c6076b..c99ed9d2192d8 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -657,18 +657,14 @@ static __always_inline \ void folio_set_hugetlb_##flname(struct folio *folio) \ { void *private = &folio->private; \ set_bit(HPG_##flname, private); \ - } \ -static inline void SetHPage##uname(struct page *page) \ - { set_bit(HPG_##flname, &(page->private)); } + } #define CLEARHPAGEFLAG(uname, flname) \ static __always_inline \ void folio_clear_hugetlb_##flname(struct folio *folio) \ { void *private = &folio->private; \ clear_bit(HPG_##flname, private); \ - } \ -static inline void ClearHPage##uname(struct page *page) \ - { clear_bit(HPG_##flname, &(page->private)); } + } #else #define TESTHPAGEFLAG(uname, flname) \ static inline bool \ @@ -680,15 +676,11 @@ static inline int HPage##uname(struct page *page) \ #define SETHPAGEFLAG(uname, flname) \ static inline void \ folio_set_hugetlb_##flname(struct folio *folio) \ - { } \ -static inline void SetHPage##uname(struct page *page) \ { } #define CLEARHPAGEFLAG(uname, flname) \ static inline void \ folio_clear_hugetlb_##flname(struct folio *folio) \ - { } \ -static inline void ClearHPage##uname(struct page *page) \ { } #endif
All users have been converted to use the folio version of these macros, we can safely remove the page based interface. Signed-off-by: Sidhartha Kumar <sidhartha.kumar@oracle.com> --- include/linux/hugetlb.h | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-)