On Thu, May 29, 2025 at 6:10 PM Byungchul Park <byungchul@xxxxxx> wrote:
>
> 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.
>
Yes, sounds good.
Also, maybe having a union with the same fields for anonymous aliasing
can be error prone if someone updates netmem_desc and forgets to
update the mirror in struct net_iov. If you can think of a way to deal
with that, great, if not lets maybe put a comment on top of struct
netmem_desc:
/* Do not update the fields in netmem_desc without also updating the
anonymous aliasing union in struct net_iov */.
Or something like that.
--
Thanks,
Mina
