Re: [PATCH v4] mm: introduce a new page type for page pool in page type

[Byungchul Park <byungchul@sk.com>]

On Wed, May 13, 2026 at 02:29:46PM +0200, David Hildenbrand (Arm) wrote:
 On 5/13/26 14:18, Byungchul Park wrote:
> > On Wed, May 13, 2026 at 11:00:51AM +0200, Dragos Tatulea wrote:
> >> On 24.02.26 06:13, Byungchul Park wrote:
> >>> Currently, the condition 'page->pp_magic == PP_SIGNATURE' is used to
> >>> determine if a page belongs to a page pool.  However, with the planned
> >>> removal of @pp_magic, we should instead leverage the page_type in struct
> >>> page, such as PGTY_netpp, for this purpose.
> >>>
> >>> Introduce and use the page type APIs e.g. PageNetpp(), __SetPageNetpp(),
> >>> and __ClearPageNetpp() instead, and remove the existing APIs accessing
> >>> @pp_magic e.g. page_pool_page_is_pp(), netmem_or_pp_magic(), and
> >>> netmem_clear_pp_magic().
> >>>
> >>> Plus, add @page_type to struct net_iov at the same offset as struct page
> >>> so as to use the page_type APIs for struct net_iov as well.  While at it,
> >>> reorder @type and @owner in struct net_iov to avoid a hole and
> >>> increasing the struct size.
> >>>
> >>> This work was inspired by the following link:
> >>>
> >>>   https://lore.kernel.org/all/582f41c0-2742-4400-9c81-0d46bf4e8314@gmail.com/
> >>>
> >>> While at it, move the sanity check for page pool to on the free path.
> >>>
> >>> Suggested-by: David Hildenbrand <david@redhat.com>
> >>> Co-developed-by: Pavel Begunkov <asml.silence@gmail.com>
> >>> Signed-off-by: Pavel Begunkov <asml.silence@gmail.com>
> >>> Signed-off-by: Byungchul Park <byungchul@sk.com>
> >>> Acked-by: David Hildenbrand <david@redhat.com>
> >>> Acked-by: Zi Yan <ziy@nvidia.com>
> >>> Acked-by: Vlastimil Babka <vbabka@suse.cz>
> >>> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
> >>> ---
> >>
> >> Seems like this patch broke tcp_mmap because
> >> validate_page_before_insert() returns -EINVAL due
> >> to a page having a type. Here's the full flow:
> >>
> >> getsockopt(TCP_ZEROCOPY_RECEIVE) returns -EINVAL because of the
> >> below flow in the kernel:
> >>
> >> tcp_zerocopy_receive()
> >> -> tcp_zerocopy_vm_insert_batch()
> >>   -> vm_insert_pages()
> >>     -> insert_pages()
> >>       -> insert_page_in_batch_locked()
> >>         -> validate_page_before_insert() returns -EINVAL
> >>            because page_has_type(page) is now true.
> >>
> >> The patch below fixes the issue. But is this a valid fix?
> >
> > Hi,
> >
> > The problem comes from the fact that page_type and _mapcount are
> > union'ed but there is a case where these two information should be kept
> > at the same time.
> >
> > Why don't we allow these two information can be kept in the 4 bytes at
> > the same time until Zi Yan's work on _mapcount and page_type will be
> > done, instead of taking a step back?
> >
> > It can be more optimized but I suggest the approach I just mentioned:
> > ---
> > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > index 64dc44832808..e5ec204866dc 100644
> > --- a/fs/proc/internal.h
> > +++ b/fs/proc/internal.h
> > @@ -185,8 +185,7 @@ static inline int folio_precise_page_mapcount(struct folio *folio,
> >  {
> >       int mapcount = atomic_read(&page->_mapcount) + 1;
> >
> > -     if (page_mapcount_is_type(mapcount))
> > -             mapcount = 0;
> > +     mapcount = page_mapcount_clear_type(mapcount);
> >       if (folio_test_large(folio))
> >               mapcount += folio_entire_mapcount(folio);
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 8260e28205e9..f45064796313 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1865,8 +1865,7 @@ static inline int folio_mapcount(const struct folio *folio)
> >
> >       if (likely(!folio_test_large(folio))) {
> >               mapcount = atomic_read(&folio->_mapcount) + 1;
> > -             if (page_mapcount_is_type(mapcount))
> > -                     mapcount = 0;
> > +             mapcount = page_mapcount_clear_type(mapcount);
> >               return mapcount;
> >       }
> >       return folio_large_mapcount(folio);
> > diff --git a/include/linux/page-flags.h b/include/linux/page-flags.h
> > index 0e03d816e8b9..f3b0d1fa262d 100644
> > --- a/include/linux/page-flags.h
> > +++ b/include/linux/page-flags.h
> > @@ -934,9 +934,9 @@ static inline bool page_type_has_type(int page_type)
> >  }
> >
> >  /* This takes a mapcount which is one more than page->_mapcount */
> > -static inline bool page_mapcount_is_type(unsigned int mapcount)
> > +static inline unsigned int page_mapcount_clear_type(unsigned int mapcount)
> >  {
> > -     return page_type_has_type(mapcount - 1);
> > +     return (unsigned int)(((int)(mapcount << 8)) >> 8);
> >  }
> >
> >  static inline bool page_has_type(const struct page *page)
> > @@ -953,16 +953,20 @@ 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,   \
> > +     VM_BUG_ON_FOLIO(page_type_has_type(data_race(folio->page.page_type)), \
> >                       folio);                                         \
> > -     folio->page.page_type = (unsigned int)PGTY_##lname << 24;       \
> > +     folio->page.page_type &= ~(PGTY_mapcount_underflow << 24);      \
> > +     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)                          \
> > +     int mapcount;                                                   \
> > +                                                                     \
> > +     if (!page_type_has_type(folio->page.page_type))                 \
> >               return;                                                 \
> >       VM_BUG_ON_FOLIO(!folio_test_##fname(folio), folio);             \
> > -     folio->page.page_type = UINT_MAX;                               \
> > +     mapcount = atomic_read(&folio->page._mapcount);                 \
> > +     folio->page.page_type = page_mapcount_clear_type(mapcount);     \
> >  }
> >
> >  #define PAGE_TYPE_OPS(uname, lname, fname)                           \
> > @@ -975,15 +979,20 @@ 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);   \
> > -     page->page_type = (unsigned int)PGTY_##lname << 24;             \
> > +     VM_BUG_ON_PAGE(page_type_has_type(data_race(page->page_type)),  \
> > +                             page);                                  \
> > +     page->page_type &= ~(PGTY_mapcount_underflow << 24);            \
> > +     page->page_type |= (unsigned int)PGTY_##lname << 24;            \
> >  }                                                                    \
> >  static __always_inline void __ClearPage##uname(struct page *page)    \
> >  {                                                                    \
> > -     if (page->page_type == UINT_MAX)                                \
> > +     int mapcount;                                                   \
> > +                                                                     \
> > +     if (!page_type_has_type(page->page_type))                       \
> >               return;                                                 \
> >       VM_BUG_ON_PAGE(!Page##uname(page), page);                       \
> > -     page->page_type = UINT_MAX;                                     \
> > +     mapcount = atomic_read(&page->_mapcount);                       \
> > +     page->page_type = page_mapcount_clear_type(mapcount);           \
> >  }
> >
> >  /*
> > diff --git a/mm/debug.c b/mm/debug.c
> > index 77fa8fe1d641..9a932ded09d4 100644
> > --- a/mm/debug.c
> > +++ b/mm/debug.c
> > @@ -74,8 +74,7 @@ static void __dump_folio(const struct folio *folio, const struct page *page,
> >       int mapcount = atomic_read(&page->_mapcount) + 1;
> >       char *type = "";
> >
> > -     if (page_mapcount_is_type(mapcount))
> > -             mapcount = 0;
> > +     mapcount = page_mapcount_clear_type(mapcount);
> >
> >       pr_warn("page: refcount:%d mapcount:%d mapping:%p index:%#lx pfn:%#lx\n",
> >                       folio_ref_count(folio), mapcount, mapping,
> > ---
> >
> > Thoughts?
> 
> God no.

This is not final patch, but for sharing the rough idea *with code* -
maybe there are more points in code that should be adjusted by the
change.  I just typed the draft patch quick just for sharing idea.

If we should allow pp type pages to be used in mapping as well, then
we should allow a page to keep both its type and mapcount at the same
time.  Am I missing something?

	Byungchul

> --
> Cheers,
> 
> David