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@xxxxxx> 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
