On Thu, May 29, 2025 at 6:10 PM Byungchul Park [off-list ref] wrote:

On Thu, May 29, 2025 at 09:31:40AM -0700, Mina Almasry wrote:

quoted

On Wed, May 28, 2025 at 8:11 PM Byungchul Park [off-list ref] wrote:

quoted

 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.

quoted

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