Re: [RFC v3 01/18] netmem: introduce struct netmem_desc mirroring struct page

[Byungchul Park <byungchul@sk.com>]

On Thu, May 29, 2025 at 09:31:40AM -0700, Mina Almasry wrote:
> On Wed, May 28, 2025 at 8:11 PM Byungchul Park <byungchul@sk.com> wrote:
> >  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;
> > +       union {
> > +               struct netmem_desc desc;
> > +
> > +               /* XXX: The following part should be removed once all
> > +                * the references to them are converted so as to be
> > +                * accessed via netmem_desc e.g. niov->desc.pp instead
> > +                * of niov->pp.
> > +                *
> > +                * Plus, once struct netmem_desc has it own instance
> > +                * from slab, network's fields of the following can be
> > +                * moved out of struct netmem_desc like:
> > +                *
> > +                *    struct net_iov {
> > +                *       struct netmem_desc desc;
> > +                *       struct net_iov_area *owner;
> > +                *       ...
> > +                *    };
> > +                */
> 
> We do not need to wait until netmem_desc has its own instance from
> slab to move the net_iov-specific fields out of netmem_desc. We can do
> that now, because there are no size restrictions on net_iov.

Got it.  Thanks for explanation.

> So, I recommend change this to:
> 
> struct net_iov {
>   /* Union for anonymous aliasing: */
>   union {
>     struct netmem_desc desc;
>     struct {
>        unsigned long _flags;
>        unsigned long pp_magic;
>        struct page_pool *pp;
>        unsigned long _pp_mapping_pad;
>        unsigned long dma_addr;
>        atomic_long_t pp_ref_count;
>     };
>     struct net_iov_area *owner;
>     enum net_iov_type type;
> };

Do you mean?

  struct net_iov {
    /* Union for anonymous aliasing: */
    union {
      struct netmem_desc desc;
      struct {
         unsigned long _flags;
         unsigned long pp_magic;
         struct page_pool *pp;
         unsigned long _pp_mapping_pad;
         unsigned long dma_addr;
         atomic_long_t pp_ref_count;
      };
    };
    struct net_iov_area *owner;
    enum net_iov_type type;
  };

Right?  If so, I will.

> >  struct net_iov_area {
> > @@ -48,27 +110,22 @@ struct net_iov_area {
> >         unsigned long base_virtual;
> >  };
> >
> > -/* These fields in struct page are used by the page_pool and net stack:
> > +/* net_iov is union'ed with struct netmem_desc mirroring struct page, so
> > + * the page_pool can access these fields without worrying whether the
> > + * underlying fields are accessed via netmem_desc or directly via
> > + * net_iov, until all the references to them are converted so as to be
> > + * accessed via netmem_desc e.g. niov->desc.pp instead of niov->pp.
> >   *
> > - *        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.
> > + * 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) == \
> > +#define NET_IOV_ASSERT_OFFSET(desc, iov)                    \
> > +       static_assert(offsetof(struct netmem_desc, desc) == \
> >                       offsetof(struct net_iov, iov))
> > +NET_IOV_ASSERT_OFFSET(_flags, type);
> 
> Remove this assertion.

I will.

> 
> >  NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> >  NET_IOV_ASSERT_OFFSET(pp, pp);
> > +NET_IOV_ASSERT_OFFSET(_pp_mapping_pad, owner);
> 
> And this one.

I will.

> (_flags, type) and (_pp_mapping_pad, owner) have very different
> semantics and usage, we should not assert they are not the same
> offset. However (pp, pp) and (pp_magic,pp_magic) have the same
> semantics and usage, so we do assert they are at the same offset.
> 
> Code is allowed to access __netmem_clear_lsb(netmem)->pp or
> __netmem_clear_lsb(netmem)->pp_magic without caring what's the
> underlying memory type because both fields have the same semantics and
> usage.
> 
> Code should *not* assume it can access
> __netmem_clear_lsb(netmem)->owner or __netmem_clear_lsb(netmem)->type
> without doing a check whether the underlying memory is
> page/netmem_desc or net_iov. These fields are only usable for net_iov,

Sounds good.  Thanks.

	Byungchul

> so let's explicitly move them to a different place.
> 
> -- 
> Thanks,
> Mina