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

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.

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.

quoted

 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.

quoted

 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