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

[Byungchul Park <byungchul@sk.com>]

On Mon, May 26, 2025 at 05:58:10PM +0100, Pavel Begunkov wrote:
> On 5/26/25 02:37, Byungchul Park wrote:
> > 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.
> 
> static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> {
> 	return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> }
> 
> static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem)
> {
> 	return &__netmem_clear_lsb(netmem)->pp_ref_count;
> }
> 
> That's a snippet of code after applying the series. So, let's say we
> take a page, it's casted to netmem, then the netmem (as it was before)
> is casted to net_iov. Before it relied on net_iov and the pp's part of
> the page having the same layout, which was checked by static asserts,
> but now, unless I'm mistaken, it's aligned in the exactly same way but
> points to a seemingly random offset of the page. We should not be doing
> that.

I told it in another thread.  My bad.  I will fix it.

> Just to be clear, I think casting pages to struct net_iov *, as it
> currently is, is quite ugly, but that's something netmem_desc and this
> effort can help with.
> 
> What you likely want to do is:
> 
> Patch 1:
> 
> struct page {
> 	unsigned long flags;
> 	union {
> 		struct_group_tagged(netmem_desc, netmem_desc) {
> 			// same layout as before
> 			...
> 			struct page_pool *pp;
> 			...
> 		};

This part will be gone shortly.  The matters come from absence of this
part.

> 	}
> }
> 
> struct net_iov {
> 	unsigned long flags_padding;
> 	union {
> 		struct {
> 			// same layout as in page + build asserts;
> 			...
> 			struct page_pool *pp;
> 			...
> 		};
> 		struct netmem_desc desc;
> 	};
> };
> 
> struct netmem_desc *page_to_netmem_desc(struct page *page)
> {
> 	return &page->netmem_desc;

page will not have any netmem things in it after this, that matters.

> }
> 
> struct netmem_desc *netmem_to_desc(netmem_t netmem)
> {
> 	if (netmem_is_page(netmem))
> 		return page_to_netmem_desc(netmem_to_page(netmem);
> 	return &netmem_to_niov(netmem)->desc;
> }
> 
> The compiler should be able to optimise the branch in netmem_to_desc(),
> but we might need to help it a bit.
> 
> 
> Then, patch 2 ... N convert page pool and everyone else accessing
> those page fields directly to netmem_to_desc / etc.
> 
> And the final patch replaces the struct group in the page with a
> new field:
> 
> struct netmem_desc {
> 	struct page_pool *pp;
> 	...
> };
> 
> struct page {
> 	unsigned long flags_padding;
> 	union {
> 		struct netmem_desc desc;
		^
		should be gone.

	Byungchul
> 		...
> 	};
> };
> 
> net_iov will drop its union in a later series to avoid conflicts.
> 
> btw, I don't think you need to convert page pool to netmem for this
> to happen, so that can be done in a separate unrelated series. It's
> 18 patches, and netdev usually requires it to be no more than 15.
> 
> -- 
> Pavel Begunkov