Re: [PATCH 18/18] mm, netmem: remove the page pool members in struct page

[Byungchul Park <byungchul@sk.com>]

On Fri, May 23, 2025 at 10:55:54AM -0700, Mina Almasry wrote:
> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > Now that all the users of the page pool members in struct page have been
> > gone, the members can be removed from struct page.
> >
> > However, since struct netmem_desc might still use the space in struct
> > page, the size of struct netmem_desc should be checked, until struct
> > netmem_desc has its own instance from slab, to avoid conficting with
> > other members within struct page.
> >
> > Remove the page pool members in struct page and add a static checker for
> > the size.
> >
> > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > ---
> >  include/linux/mm_types.h | 11 -----------
> >  include/net/netmem.h     | 28 +++++-----------------------
> >  2 files changed, 5 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 873e820e1521..5a7864eb9d76 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -119,17 +119,6 @@ struct page {
> >                          */
> >                         unsigned long private;
> >                 };
> > -               struct {        /* page_pool used by netstack */
> > -                       unsigned long _pp_mapping_pad;
> > -                       /**
> > -                        * @pp_magic: magic value to avoid recycling non
> > -                        * page_pool allocated pages.
> > -                        */
> > -                       unsigned long pp_magic;
> > -                       struct page_pool *pp;
> > -                       unsigned long dma_addr;
> > -                       atomic_long_t pp_ref_count;
> > -               };
> >                 struct {        /* Tail pages of compound page */
> >                         unsigned long compound_head;    /* Bit zero is set */
> >                 };
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index c63a7e20f5f3..257c22398d7a 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -77,30 +77,12 @@ struct net_iov_area {
> >         unsigned long base_virtual;
> >  };
> >
> > -/* These fields in struct page are used by the page_pool and net stack:
> > - *
> > - *        struct {
> > - *                unsigned long _pp_mapping_pad;
> > - *                unsigned long pp_magic;
> > - *                struct page_pool *pp;
> > - *                unsigned long dma_addr;
> > - *                atomic_long_t pp_ref_count;
> > - *        };
> > - *
> > - * We mirror the page_pool fields here so the page_pool can access these fields
> > - * without worrying whether the underlying fields belong to a page or net_iov.
> > - *
> > - * The non-net stack fields of struct page are private to the mm stack and must
> > - * never be mirrored to net_iov.
> > +/* XXX: The page pool fields in struct page have been removed but they
> > + * might still use the space in struct page.  Thus, the size of struct
> > + * netmem_desc should be under control until struct netmem_desc has its
> > + * own instance from slab.
> >   */
> > -#define NET_IOV_ASSERT_OFFSET(pg, iov)             \
> > -       static_assert(offsetof(struct page, pg) == \
> > -                     offsetof(struct net_iov, iov))
> > -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> > -NET_IOV_ASSERT_OFFSET(pp, pp);
> > -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> > -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> > -#undef NET_IOV_ASSERT_OFFSET
> > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> >
> 
> Removing these asserts is actually a bit dangerous. Functions like
> netmem_or_pp_magic() rely on the fact that the offsets are the same
> between struct page and struct net_iov to access these fields without

Worth noting this patch removes the page pool fields from struct page.

However, yes, I will keep necessary assertions with some changes applied
so that it can work even after removing the page pool fields like:

NET_IOV_ASSERT_OFFSET(lru, pp_magic);
NET_IOV_ASSERT_OFFSET(mapping, _pp_mapping_pad);

> worrying about the type of the netmem. What we do in these helpers is
> we we clear the least significant bit of the netmem, and then  access
> the field. This works only because we verified at build time that the
> offset is the same.
> 
> I think we have 3 options here:
> 
> 1. Keep the asserts as-is, then in the follow up patch where we remove
> netmem_desc from struct page, we update the asserts to make sure
> struct page and struct net_iov can grab the netmem_desc in a uniform

Ah.  It's worth noting that I'm removing the page pool fields all the
way from strcut page, instead of placing a place-holder that I did in
RFC as Matthew requested.

> way.
> 
> 2. We remove the asserts, but all the helpers that rely on
> __netmem_clear_lsb need to be modified to do custom handling of
> net_iov vs page. Something like:
> 
> static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
> {
>   if (netmem_is_net_iov(netmem)
>      netmem_to_net_iov(netmem)->pp_magic |= pp_magic;
>   else
>     netmem_to_page(netmem)->pp_magic |= pp_magic;

struct page should not have pp_magic field once the page pool fields are
gone.

	Byungchul
> }
> 
> Option #2 requires extra checks, which may affect the performance
> reported by page_pool_bench_simple that I pointed you to before.
> 
> 3. We could swap out all the individual asserts for one assert, if
> both page and net_iov have a netmem_desc subfield. This will also need
> to be reworked when netmem_desc is eventually moved out of struct page
> and is slab allocated:
> 
> NET_IOV_ASSERT_OFFSET(netmem_desc, netmem_desc);
> 
> -- 
> Thanks,
> Mina