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

On Thu, Jul 10, 2025 at 11:11:51AM -0700, Mina Almasry wrote:

On Thu, Jul 10, 2025 at 1:28 AM Byungchul Park [off-list ref] wrote:

quoted

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.

quoted

+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;
   }

quoted

+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/ (local)

	Byungchul

--
Thanks,
Mina