Re: [PATCH net-next v9 2/8] netmem: introduce utility APIs to use struct netmem_desc

[Byungchul Park <byungchul@sk.com>]

On Thu, Jul 10, 2025 at 11:11:51AM -0700, Mina Almasry wrote:
> 
> On Thu, Jul 10, 2025 at 1:28 AM Byungchul Park <byungchul@sk.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@sk.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

Ah, right.  I missed the point.  I will narrow down into a single safe
helper for that purpose.

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

I will.  Thanks for the feedback.

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

Agree, but page_to_nmdesc() will be used in page_pool_page_is_pp() to
check if it's a pp page or not:

   static inline bool page_pool_page_is_pp(struct page *page)
   {
	struct netmem_desc *desc = page_to_nmdesc(page);

	return (desc->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
   }

Hm.. maybe, it'd be better to resore the original code and remove this
page_to_nmdesc() helper.  FYI, the original code was:

   static inline bool page_pool_page_is_pp(struct page *page)
   {
	struct netmem_desc *desc = (struct netmem_desc *)page;

	return (desc->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
   }

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

I adopted how Matthew Wilcox worked on this[1], but sure, I will.

[1] https://lore.kernel.org/linux-mm/20230111042214.907030-3-willy@infradead.org/

	Byungchul

> --
> Thanks,
> Mina