linux-kernel - Re: [PATCH v2 01/16] netmem: introduce struct netmem_desc struct_group_tagged()'ed on struct net

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHS8izPTBnSL_zrW-u0mKBXC=iP9=WZcS9etCRpufCkpCwYoAg@mail.gmail.com>
Date: Tue, 27 May 2025 20:39:43 -0700
From: Mina Almasry <almasrymina@...gle.com>
To: Byungchul Park <byungchul@...com>
Cc: willy@...radead.org, netdev@...r.kernel.org, linux-kernel@...r.kernel.org, 
	linux-mm@...ck.org, kernel_team@...ynix.com, kuba@...nel.org, 
	ilias.apalodimas@...aro.org, harry.yoo@...cle.com, hawk@...nel.org, 
	akpm@...ux-foundation.org, davem@...emloft.net, john.fastabend@...il.com, 
	andrew+netdev@...n.ch, asml.silence@...il.com, toke@...hat.com, 
	tariqt@...dia.com, edumazet@...gle.com, pabeni@...hat.com, saeedm@...dia.com, 
	leon@...nel.org, ast@...nel.org, daniel@...earbox.net, david@...hat.com, 
	lorenzo.stoakes@...cle.com, Liam.Howlett@...cle.com, vbabka@...e.cz, 
	rppt@...nel.org, surenb@...gle.com, mhocko@...e.com, horms@...nel.org, 
	linux-rdma@...r.kernel.org, bpf@...r.kernel.org, vishal.moola@...il.com
Subject: Re: [PATCH v2 01/16] netmem: introduce struct netmem_desc
 struct_group_tagged()'ed on struct net_iov

On Tue, May 27, 2025 at 7:29 PM Byungchul Park <byungchul@...com> 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@...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?

> ---
>  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?

> +        *    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