Re: [PATCH v2 01/16] netmem: introduce struct netmem_desc struct_group_tagged()'ed on struct net_iov
From: Mina Almasry <hidden>
Date: 2025-05-28 03:39:56
Also in:
bpf, linux-mm, linux-rdma, lkml
On Tue, May 27, 2025 at 7:29 PM Byungchul Park [off-list ref] wrote:
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?
quoted hunk ↗ jump to hunk
--- 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?
+ * 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 hunk ↗ jump to hunk
+ * 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