Message-ID: <CAHS8izO0mgDBde57fxuN3ko38906F_C=pxxrSEnFA=_9ECO8oQ@mail.gmail.com>
Date: Thu, 10 Jul 2025 11:11:51 -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, 
	hannes@...xchg.org, ziy@...dia.com, jackmanb@...gle.com
Subject: Re: [PATCH net-next v9 2/8] netmem: introduce utility APIs to use
 struct netmem_desc

On Thu, Jul 10, 2025 at 1:28 AM Byungchul Park <byungchul@...com> wrote:
>
> To eliminate the use of the page pool fields in struct page, the page
> pool code should use netmem descriptor and APIs instead.
>
> However, some code e.g. __netmem_to_page() is still used to access the
> page pool fields e.g. ->pp via struct page, which should be changed so
> as to access them via netmem descriptor, struct netmem_desc instead,
> since the fields no longer will be available in struct page.
>
> Introduce utility APIs to make them easy to use struct netmem_desc as
> descriptor.  The APIs are:
>
>    1. __netmem_to_nmdesc(), to convert netmem_ref to struct netmem_desc,
>       but unsafely without checking if it's net_iov or system memory.
>
>    2. netmem_to_nmdesc(), to convert netmem_ref to struct netmem_desc,
>       safely with checking if it's net_iov or system memory.
>
>    3. nmdesc_to_page(), to convert struct netmem_desc to struct page,
>       assuming struct netmem_desc overlays on struct page.
>
>    4. page_to_nmdesc(), to convert struct page to struct netmem_desc,
>       assuming struct netmem_desc overlays on struct page, allowing only
>       head page to be converted.
>
>    5. nmdesc_adress(), to get its virtual address corresponding to the
>       struct netmem_desc.
>
> Signed-off-by: Byungchul Park <byungchul@...com>
> ---
>  include/net/netmem.h | 41 +++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 41 insertions(+)
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 535cf17b9134..ad9444be229a 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -198,6 +198,32 @@ static inline struct page *netmem_to_page(netmem_ref netmem)
>         return __netmem_to_page(netmem);
>  }
>
> +/**
> + * __netmem_to_nmdesc - unsafely get pointer to the &netmem_desc backing
> + * @netmem
> + * @netmem: netmem reference to convert
> + *
> + * Unsafe version of netmem_to_nmdesc(). When @netmem is always backed
> + * by system memory, performs faster and generates smaller object code
> + * (no check for the LSB, no WARN). When @netmem points to IOV, provokes
> + * undefined behaviour.
> + *
> + * Return: pointer to the &netmem_desc (garbage if @netmem is not backed
> + * by system memory).
> + */
> +static inline struct netmem_desc *__netmem_to_nmdesc(netmem_ref netmem)
> +{
> +       return (__force struct netmem_desc *)netmem;
> +}
> +

Does a netmem_desc represent the pp fields shared between struct page
and struct net_iov, or does netmem_desc represent paged kernel memory?
If the former, I don't think we need a safe and unsafe version of this
helper, since netmem_ref always has netmem_desc fields underneath. If
the latter, then this helper should not exist at all. We should not
allow casting netmem_ref to a netmem_desc without first checking if
it's a net_iov.

To be honest the cover letter should come up with a detailed
explanation of (a) what are the current types (b) what are the new
types (c) what are the relationships between the types, so these
questions stop coming up.

> +static inline struct netmem_desc *netmem_to_nmdesc(netmem_ref netmem)
> +{
> +       if (WARN_ON_ONCE(netmem_is_net_iov(netmem)))
> +               return NULL;
> +
> +       return __netmem_to_nmdesc(netmem);
> +}
> +
>  static inline struct net_iov *netmem_to_net_iov(netmem_ref netmem)
>  {
>         if (netmem_is_net_iov(netmem))
> @@ -314,6 +340,21 @@ static inline netmem_ref netmem_compound_head(netmem_ref netmem)
>         return page_to_netmem(compound_head(netmem_to_page(netmem)));
>  }
>
> +#define nmdesc_to_page(nmdesc)         (_Generic((nmdesc),             \
> +       const struct netmem_desc * :    (const struct page *)(nmdesc),  \
> +       struct netmem_desc * :          (struct page *)(nmdesc)))
> +
> +static inline struct netmem_desc *page_to_nmdesc(struct page *page)
> +{
> +       VM_BUG_ON_PAGE(PageTail(page), page);
> +       return (struct netmem_desc *)page;
> +}
> +

It's not safe to cast a page to netmem_desc, without first checking if
it's a pp page or not, otherwise you may be casting random non-pp
pages to netmem_desc...

> +static inline void *nmdesc_address(struct netmem_desc *nmdesc)
> +{
> +       return page_address(nmdesc_to_page(nmdesc));
> +}
> +
>  /**

Introduce helpers in the same patch that uses them please. Having to
cross reference your series to see if there are any callers to this
(and the callers are correct) is an unnecessary burden to the
reviewers.

-- 
Thanks,
Mina

Powered by blists - more mailing lists