On Tue, Jul 01, 2025 at 10:17:13AM +0200, David Hildenbrand wrote:
> On 30.06.25 17:27, Lorenzo Stoakes wrote:
> > On Mon, Jun 30, 2025 at 02:59:45PM +0200, David Hildenbrand wrote:
> > > Currently, any user of page types must clear that type before freeing
> > > a page back to the buddy, otherwise we'll run into mapcount related
> > > sanity checks (because the page type currently overlays the page
> > > mapcount).
> > >
> > > Let's allow for not clearing the page type by page type users by letting
> > > the buddy handle it instead.
> > >
> > > We'll focus on having a page type set on the first page of a larger
> > > allocation only.
> > >
> > > With this change, we can reliably identify typed folios even though
> > > they might be in the process of getting freed, which will come in handy
> > > in migration code (at least in the transition phase).
> > >
> > > In the future we might want to warn on some page types. Instead of
> > > having an "allow list", let's rather wait until we know about once that
> > > should go on such a "disallow list".
> >
> > Is the idea here to get this to show up on folio dumps or?
>
> As part of the netmem_desc series, there was a discussion about removing the
> mystical PP checks -- page_pool_page_is_pp() in page_alloc.c and replacing
> them by a proper page type check.
>
> In that case, we would probably want to warn in case we get such a netmem
> page unexpectedly freed.
>
> But, that page type does not exist yet in code, so the sanity check must be
> added once introduced.
OK, and I realise that the UINT_MAX thing is a convention for how a reset
page_type looks anyway now.
>
> >
> > >
> > > Reviewed-by: Zi Yan <ziy@nvidia.com>
> > > Acked-by: Harry Yoo <harry.yoo@oracle.com>
> > > Signed-off-by: David Hildenbrand <david@redhat.com>
> > > ---
> > > mm/page_alloc.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 858bc17653af9..44e56d31cfeb1 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -1380,6 +1380,9 @@ __always_inline bool free_pages_prepare(struct page *page,
> > > mod_mthp_stat(order, MTHP_STAT_NR_ANON, -1);
> > > page->mapping = NULL;
> > > }
> > > + if (unlikely(page_has_type(page)))
> > > + page->page_type = UINT_MAX;
> >
> > Feels like this could do with a comment!
>
> /* Reset the page_type -> _mapcount to -1 */
Hm this feels like saying 'the reason we set it to -1 is to set it to -1' :P
I'd be fine with something simple like
/* Set page_type to reset value */
But... Can't we just put a #define somewhere here to make life easy? Like:
include/linux/page-flags.h | 17 ++++++++++-------
1 file changed, 10 insertions(+), 7 deletions(-)
diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
index 4fe5ee67535b..c2abf66ebbce 100644
--- a/include/linux/page-flags.h
+++ b/include/linux/page-flags.h
@@ -197,6 +197,8 @@ enum pageflags {
#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
DECLARE_STATIC_KEY_FALSE(hugetlb_optimize_vmemmap_key);
+#define PAGE_TYPE_RESET (UINT_MAX)
+
/*
* Return the real head page struct iff the @page is a fake head page, otherwise
* return the @page itself. See Documentation/mm/vmemmap_dedup.rst.
@@ -986,16 +988,16 @@ static __always_inline void __folio_set_##fname(struct folio *folio) \
{ \
if (folio_test_##fname(folio)) \
return; \
- VM_BUG_ON_FOLIO(data_race(folio->page.page_type) != UINT_MAX, \
- folio); \
+ VM_WARN_ON_FOLIO(data_race(folio->page.page_type) != \
+ PAGE_TYPE_RESET, folio); \
folio->page.page_type = (unsigned int)PGTY_##lname << 24; \
} \
static __always_inline void __folio_clear_##fname(struct folio *folio) \
{ \
- if (folio->page.page_type == UINT_MAX) \
+ if (folio->page.page_type == PAGE_TYPE_RESET) \
return; \
VM_BUG_ON_FOLIO(!folio_test_##fname(folio), folio); \
- folio->page.page_type = UINT_MAX; \
+ folio->page.page_type = PAGE_TYPE_RESET; \
}
#define PAGE_TYPE_OPS(uname, lname, fname) \
@@ -1008,15 +1010,16 @@ static __always_inline void __SetPage##uname(struct page *page) \
{ \
if (Page##uname(page)) \
return; \
- VM_BUG_ON_PAGE(data_race(page->page_type) != UINT_MAX, page); \
+ VM_BUG_ON_PAGE(data_race(page->page_type) != \
+ PAGE_TYPE_RESET, page); \
page->page_type = (unsigned int)PGTY_##lname << 24; \
} \
static __always_inline void __ClearPage##uname(struct page *page) \
{ \
- if (page->page_type == UINT_MAX) \
+ if (page->page_type == PAGE_TYPE_RESET) \
return; \
VM_BUG_ON_PAGE(!Page##uname(page), page); \
- page->page_type = UINT_MAX; \
+ page->page_type = PAGE_TYPE_RESET; \
}
/*
--
2.50.0
>
> --
> Cheers,
>
> David / dhildenb
>