Re: [PATCH v2 01/16] netmem: introduce struct netmem_desc struct_group_tagged()'ed on struct net_iov

[Byungchul Park <byungchul@sk.com>]

On Tue, May 27, 2025 at 08:39:43PM -0700, Mina Almasry wrote:
> On Tue, May 27, 2025 at 7:29 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > To simplify struct page, the page pool members of struct page should be
> > moved to other, allowing these members to be removed from struct page.
> >
> > Introduce a network memory descriptor to store the members, struct
> > netmem_desc, reusing struct net_iov that already mirrored struct page.
> >
> > While at it, add a static assert to prevent the size of struct
> > netmem_desc from getting bigger that might conflict with other members
> > within struct page.
> >
> > Signed-off-by: Byungchul Park <byungchul@sk.com>
> 
> This patch looks fine to me, but as of this series this change seems
> unnecessary, no? You're not using netmem_desc for anything in this
> series AFAICT. It may make sense to keep this series as
> straightforward renames, and have the series that introduces
> netmem_desc be the same one that is removing the page_pool fields from
> struct page or does something useful with it?

I didn't document well nor even work quite well for my purpose.  I have
to admit I was also confused because the network code already had a
struct mirroring struct page, net_iov, so I thought it could be the
descriptor along with struct netmeme_desc until the day for page pool.

However, thanks to you, Pavel, and all, I understand better what net_iov
is for.  I posted v3 with all the feedbacks applied, I guess the changes
in v3 are going to meet what you guys asked me.

On the top of v3, I think we can work on organizing struct net_iov to
make it look like:

	struct net_iov {
		struct netmem_desc desc;

		net_iov_field1;
		net_iov_field2;
		net_iov_field3;
		...
	};

In order to make it, we should convert all the references to the
existing fields in struct net_iov, to access them indirectly using desc
field like e.g. 'net_iov->pp' to 'net_iov->desc.pp'.  Once the
conversion is completed, we can make net_iov look better, which is not
urgent and I will not include the work in this series.

Please check v3:

   https://lore.kernel.org/all/20250529031047.7587-1-byungchul@sk.com/
   https://lore.kernel.org/all/20250529031047.7587-2-byungchul@sk.com/

Thanks to all.

	Byungchul

> > ---
> >  include/net/netmem.h | 41 +++++++++++++++++++++++++++++++++++------
> >  1 file changed, 35 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index 386164fb9c18..a721f9e060a2 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -31,12 +31,34 @@ enum net_iov_type {
> >  };
> >
> >  struct net_iov {
> > -       enum net_iov_type type;
> > -       unsigned long pp_magic;
> > -       struct page_pool *pp;
> > -       struct net_iov_area *owner;
> > -       unsigned long dma_addr;
> > -       atomic_long_t pp_ref_count;
> > +       /*
> > +        * XXX: Now that struct netmem_desc overlays on struct page,
> 
> This starting statement doesn't make sense to me TBH. netmem_desc
> doesn't seem to overlay struct page? For example, enum net_iov_type
> overlays unsigned long page->flags. Both have very different semantics
> and usage, no?
> 
> > +        * struct_group_tagged() should cover all of them.  However,
> > +        * a separate struct netmem_desc should be declared and embedded,
> > +        * once struct netmem_desc is no longer overlayed but it has its
> > +        * own instance from slab.  The final form should be:
> > +        *
> 
> Honestly I'm not sure about putting future plans that aren't
> completely agreed upon in code comments. May be better to keep these
> future plans to the series that actually introduces them?
> 
> > +        *    struct netmem_desc {
> > +        *         unsigned long pp_magic;
> > +        *         struct page_pool *pp;
> > +        *         unsigned long dma_addr;
> > +        *         atomic_long_t pp_ref_count;
> > +        *    };
> > +        *
> > +        *    struct net_iov {
> > +        *         enum net_iov_type type;
> > +        *         struct net_iov_area *owner;
> > +        *         struct netmem_desc;
> > +        *    };
> > +        */
> > +       struct_group_tagged(netmem_desc, desc,
> > +               enum net_iov_type type;
> > +               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 {
> > @@ -73,6 +95,13 @@ NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> >  NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> >  #undef NET_IOV_ASSERT_OFFSET
> >
> > +/*
> > + * Since struct netmem_desc uses the space in struct page, the size
> > + * should be checked, until struct netmem_desc has its own instance from
> > + * slab, to avoid conflicting with other members within struct page.
> > + */
> > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> > +
> >  static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
> >  {
> >         return niov->owner;
> > --
> > 2.17.1
> >
> 
> 
> -- 
> Thanks,
> Mina