Re: [PATCH v1 04/29] mm/page_alloc: let page freeing clear any set page type

[Lorenzo Stoakes]

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@xxxxxxxxxx>
> > > Acked-by: Harry Yoo <harry.yoo@xxxxxxxxxx>
> > > Signed-off-by: David Hildenbrand <david@xxxxxxxxxx>
> > > ---
> > >   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
>