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@xxxxxx> 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@xxxxxx>
> > ---
> > 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@xxxxxxxxxxxxx/
Byungchul
> --
> Thanks,
> Mina
