Re: [PATCH v2 01/16] netmem: introduce struct netmem_desc struct_group_tagged()'ed on struct net_iov

On Tue, May 27, 2025 at 08:39:43PM -0700, Mina Almasry wrote:

On Tue, May 27, 2025 at 7:29 PM Byungchul Park [off-list ref] wrote:

quoted

To simplify struct page, the page pool members of struct page should be
moved to other, allowing these members to be removed from struct page.

Introduce a network memory descriptor to store the members, struct
netmem_desc, reusing struct net_iov that already mirrored struct page.

While at it, add a static assert to prevent the size of struct
netmem_desc from getting bigger that might conflict with other members
within struct page.

Signed-off-by: Byungchul Park <byungchul@sk.com>

This patch looks fine to me, but as of this series this change seems
unnecessary, no? You're not using netmem_desc for anything in this
series AFAICT. It may make sense to keep this series as
straightforward renames, and have the series that introduces
netmem_desc be the same one that is removing the page_pool fields from
struct page or does something useful with it?

I didn't document well nor even work quite well for my purpose.  I have
to admit I was also confused because the network code already had a
struct mirroring struct page, net_iov, so I thought it could be the
descriptor along with struct netmeme_desc until the day for page pool.

However, thanks to you, Pavel, and all, I understand better what net_iov
is for.  I posted v3 with all the feedbacks applied, I guess the changes
in v3 are going to meet what you guys asked me.

On the top of v3, I think we can work on organizing struct net_iov to
make it look like:

	struct net_iov {
		struct netmem_desc desc;

		net_iov_field1;
		net_iov_field2;
		net_iov_field3;
		...
	};

In order to make it, we should convert all the references to the
existing fields in struct net_iov, to access them indirectly using desc
field like e.g. 'net_iov->pp' to 'net_iov->desc.pp'.  Once the
conversion is completed, we can make net_iov look better, which is not
urgent and I will not include the work in this series.

Please check v3:

   https://lore.kernel.org/all/20250529031047.7587-1-byungchul@sk.com/ (local)
   https://lore.kernel.org/all/20250529031047.7587-2-byungchul@sk.com/ (local)

Thanks to all.

	Byungchul

quoted

---
 include/net/netmem.h | 41 +++++++++++++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 6 deletions(-)

diff --git a/include/net/netmem.h b/include/net/netmem.h
index 386164fb9c18..a721f9e060a2 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h

@@ -31,12 +31,34 @@ enum net_iov_type {
 };

 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;
+       /*
+        * XXX: Now that struct netmem_desc overlays on struct page,

This starting statement doesn't make sense to me TBH. netmem_desc
doesn't seem to overlay struct page? For example, enum net_iov_type
overlays unsigned long page->flags. Both have very different semantics
and usage, no?

quoted

+        * struct_group_tagged() should cover all of them.  However,
+        * a separate struct netmem_desc should be declared and embedded,
+        * once struct netmem_desc is no longer overlayed but it has its
+        * own instance from slab.  The final form should be:
+        *

Honestly I'm not sure about putting future plans that aren't
completely agreed upon in code comments. May be better to keep these
future plans to the series that actually introduces them?

quoted

+        *    struct netmem_desc {
+        *         unsigned long pp_magic;
+        *         struct page_pool *pp;
+        *         unsigned long dma_addr;
+        *         atomic_long_t pp_ref_count;
+        *    };
+        *
+        *    struct net_iov {
+        *         enum net_iov_type type;
+        *         struct net_iov_area *owner;
+        *         struct netmem_desc;
+        *    };
+        */
+       struct_group_tagged(netmem_desc, desc,
+               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;
+       );
 };

 struct net_iov_area {

@@ -73,6 +95,13 @@ NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
 NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
 #undef NET_IOV_ASSERT_OFFSET

+/*
+ * Since struct netmem_desc uses the space in struct page, the size
+ * should be checked, until struct netmem_desc has its own instance from
+ * slab, to avoid conflicting with other members within struct page.
+ */
+static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
+
 static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
 {
        return niov->owner;
--

2.17.1

-- 
Thanks,
Mina