Re: [RFC 19/19] mm, netmem: remove the page pool members in struct page

[Byungchul Park <byungchul@sk.com>]

On Fri, May 09, 2025 at 07:02:30PM +0100, Matthew Wilcox wrote:
> On Fri, May 09, 2025 at 08:51:26PM +0900, Byungchul Park wrote:
> > +++ b/include/linux/mm_types.h
> > @@ -20,6 +20,7 @@
> >  #include <linux/seqlock.h>
> >  #include <linux/percpu_counter.h>
> >  #include <linux/types.h>
> > +#include <net/netmem_type.h> /* for page pool */
> >  
> >  #include <asm/mmu.h>
> >  
> > @@ -118,17 +119,7 @@ struct page {
> >  			 */
> >  			unsigned long private;
> >  		};
> > -		struct {	/* page_pool used by netstack */
> > -			/**
> > -			 * @pp_magic: magic value to avoid recycling non
> > -			 * page_pool allocated pages.
> > -			 */
> > -			unsigned long pp_magic;
> > -			struct page_pool *pp;
> > -			unsigned long _pp_mapping_pad;
> > -			unsigned long dma_addr;
> > -			atomic_long_t pp_ref_count;
> > -		};
> > +		struct __netmem_desc place_holder_1; /* for page pool */
> >  		struct {	/* Tail pages of compound page */
> >  			unsigned long compound_head;	/* Bit zero is set */
> >  		};
> 
> The include and the place holder aren't needed.

Or netmem_desc overlaying struct page might be conflict with other
fields of sturct page e.g. _mapcount, _refcount and the like, once the
layout of struct page *extremly changes* in the future before
netmem_desc has its own instance.

So placing a place holder like this is the safest way, IMO, to prevent
the unextected result.  Am I missing something?

> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index 00064e766b889..c414de6c6ab0d 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -10,6 +10,7 @@
> >  
> >  #include <linux/mm.h>
> >  #include <net/net_debug.h>
> > +#include <net/netmem_type.h>
> 
> ... which I think means you don't need the separate header file.

Agree if I don't use the place holder in mm_types.h.

> >  /* net_iov */
> >  
> > @@ -20,15 +21,6 @@ DECLARE_STATIC_KEY_FALSE(page_pool_mem_providers);
> >   */
> >  #define NET_IOV 0x01UL
> >  
> > -struct netmem_desc {
> > -	unsigned long __unused_padding;
> > -	unsigned long pp_magic;
> > -	struct page_pool *pp;
> > -	struct net_iov_area *owner;
> > -	unsigned long dma_addr;
> > -	atomic_long_t pp_ref_count;
> > -};
> > -
> >  struct net_iov_area {
> >  	/* Array of net_iovs for this area. */
> >  	struct netmem_desc *niovs;
> > @@ -38,31 +30,6 @@ 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_magic;
> > - *                struct page_pool *pp;
> > - *                unsigned long _pp_mapping_pad;
> > - *                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.
> > - */
> > -#define NET_IOV_ASSERT_OFFSET(pg, iov)             \
> > -	static_assert(offsetof(struct page, pg) == \
> > -		      offsetof(struct netmem_desc, 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
> 
> ... but you do want to keep asserting that netmem_desc and
> net_iov have the same offsets.  And you want to assert that struct page
> is big enough to hold everything in netmem_desc, like we do for slab:
> 
> static_assert(sizeof(struct slab) <= sizeof(struct page));

I will.  However, as I mentioned above, the total size doesn't matter
but the layout change of struct page might matter, I think.

	Byungchul