[PATCH net-next v10 00/12] Split netmem from struct page

[PATCH net-next v10 00/12] Split netmem from struct page

[Byungchul Park]

Hi all,

The MM subsystem is trying to reduce struct page to a single pointer.
See the following link for your information:

   https://kernelnewbies.org/MatthewWilcox/Memdescs/Path

The first step towards that is splitting struct page by its individual
users, as has already been done with folio and slab.  This patchset does
that for page pool.

Matthew Wilcox tried and stopped the same work, you can see in:

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

I focused on removing the page pool members in struct page this time,
not moving the allocation code of page pool from net to mm.  It can be
done later if needed.

The final patch that removes the page pool fields will be posted once
all the conversions are completed.

	Byungchul
---
Changes from v9:
	1. Remove the patch 'page_pool: access ->pp_magic through
	   netmem_desc in page_pool_page_is_pp()' and decide to wait for
	   Pavel's work of PageNetpp() to identify page type for page
	   pool, that doesn't need to access ->pp_magic.
	2. Rename page_to_nmdesc() to pp_page_to_nmdesc() and add
	   DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page)) in it,
	   just in case. (feedbacked by Pavel)
	3. Apply just simple casting from page to netmem_desc for
	   accessing ->pp and ->pp_ref_count, instead of full converting
	   page to netmem_ref for network drivers e.g. mlx4, netdevsim,
	   and mt76.
	4. Expand the support for drivers to access ->pp and
	   ->pp_ref_count to fec, octeontx2-pf, iavf, idpf, mlx5, ti,
	   and xdp.
	5. Squash each helper with its first user. (feedbacked by Mina)

Changes from v8:
	1. Rebase on net-next/main as of Jul 10.
	2. Exclude non-controversial patches that have already been
	   merged to net-next.
	3. Re-add the patches that focus on removing accessing the page
	   pool fields in struct page.
	4. Add utility APIs e.g. casting, to use struct netmem_desc as
	   descriptor, to support __netmem_get_pp() that has started to
	   be used again e.g. by libeth.

Changes from v7 (no actual updates):
	1. Exclude "netmem: introduce struct netmem_desc mirroring
	   struct page" that might be controversial.
	2. Exclude "netmem: introduce a netmem API,
	   virt_to_head_netmem()" since there are no users.

Changes from v6 (no actual updates):
	1. Rebase on net-next/main as of Jun 25.
	2. Supplement a comment describing struct net_iov.
	3. Exclude a controversial patch, "page_pool: access ->pp_magic
	   through struct netmem_desc in page_pool_page_is_pp()".
	4. Exclude "netmem: remove __netmem_get_pp()" since the API
	   started to be used again by libeth.

Changes from v5 (no actual updates):
	1. Rebase on net-next/main as of Jun 20.
	2. Add given 'Reviewed-by's and 'Acked-by's, thanks to all.
	3. Add missing cc's.

Changes from v4:
	1. Add given 'Reviewed-by's, thanks to all.
	2. Exclude potentially controversial patches.

Changes from v3:
	1. Relocates ->owner and ->type of net_iov out of netmem_desc
	   and make them be net_iov specific.
	2. Remove __force when casting struct page to struct netmem_desc.

Changes from v2:
	1. Introduce a netmem API, virt_to_head_netmem(), and use it
	   when it's needed.
	2. Introduce struct netmem_desc as a new struct and union'ed
	   with the existing fields in struct net_iov.
	3. Make page_pool_page_is_pp() access ->pp_magic through struct
	   netmem_desc instead of struct page.
	4. Move netmem alloc APIs from include/net/netmem.h to
	   net/core/netmem_priv.h.
	5. Apply trivial feedbacks, thanks to Mina, Pavel, and Toke.
	6. Add given 'Reviewed-by's, thanks to Mina.

Changes from v1:
	1. Rebase on net-next's main as of May 26.
	2. Check checkpatch.pl, feedbacked by SJ Park.
	3. Add converting of page to netmem in mt76.
	4. Revert 'mlx5: use netmem descriptor and APIs for page pool'
	   since it's on-going by Tariq Toukan.  I will wait for his
	   work to be done.
	5. Revert 'page_pool: use netmem APIs to access page->pp_magic
	   in page_pool_page_is_pp()' since we need more discussion.
	6. Revert 'mm, netmem: remove the page pool members in struct
	   page' since there are some prerequisite works to remove the
	   page pool fields from struct page.  I can submit this patch
	   separatedly later.
	7. Cancel relocating a page pool member in struct page.
	8. Modify static assert for offests and size of struct
	   netmem_desc.

Changes from rfc:
	1. Rebase on net-next's main branch.
	   https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/
	2. Fix a build error reported by kernel test robot.
	   https://lore.kernel.org/all/202505100932.uzAMBW1y-lkp@intel.com/
	3. Add given 'Reviewed-by's, thanks to Mina and Ilias.
	4. Do static_assert() on the size of struct netmem_desc instead
	   of placing place-holder in struct page, feedbacked by
	   Matthew.
	5. Do struct_group_tagged(netmem_desc) on struct net_iov instead
	   of wholly renaming it to strcut netmem_desc, feedbacked by
	   Mina and Pavel.

Byungchul Park (12):
  netmem: introduce struct netmem_desc mirroring struct page
  netmem: use netmem_desc instead of page to access ->pp in
    __netmem_get_pp()
  mlx4: access ->pp_ref_count through netmem_desc instead of page
  netdevsim: access ->pp through netmem_desc instead of page
  mt76: access ->pp through netmem_desc instead of page
  net: fec: access ->pp through netmem_desc instead of page
  octeontx2-pf: access ->pp through netmem_desc instead of page
  iavf: access ->pp through netmem_desc instead of page
  idpf: access ->pp through netmem_desc instead of page
  mlx5: access ->pp through netmem_desc instead of page
  net: ti: icssg-prueth: access ->pp through netmem_desc instead of page
  libeth: xdp: access ->pp through netmem_desc instead of page

 drivers/net/ethernet/freescale/fec_main.c     |  10 +-
 drivers/net/ethernet/intel/iavf/iavf_txrx.c   |   2 +-
 drivers/net/ethernet/intel/idpf/idpf_txrx.c   |   8 +-
 .../marvell/octeontx2/nic/otx2_txrx.c         |   2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |   4 +-
 .../net/ethernet/mellanox/mlx5/core/en/xdp.c  |   3 +-
 .../net/ethernet/ti/icssg/icssg_prueth_sr1.c  |   4 +-
 drivers/net/netdevsim/netdev.c                |   6 +-
 drivers/net/wireless/mediatek/mt76/mt76.h     |   3 +-
 include/net/libeth/xdp.h                      |   2 +-
 include/net/netmem.h                          | 129 +++++++++++++++---
 11 files changed, 135 insertions(+), 38 deletions(-)


base-commit: c65d34296b2252897e37835d6007bbd01b255742
-- 
2.17.1

[PATCH net-next v10 01/12] netmem: introduce struct netmem_desc mirroring struct page

[Byungchul Park]

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, and make it union'ed with the existing fields in struct
net_iov, allowing to organize the fields of struct net_iov.

Signed-off-by: Byungchul Park <byungchul@sk.com>
Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
Reviewed-by: Pavel Begunkov <asml.silence@gmail.com>
Reviewed-by: Mina Almasry <almasrymina@google.com>
Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
Acked-by: Harry Yoo <harry.yoo@oracle.com>
---
 include/net/netmem.h | 116 +++++++++++++++++++++++++++++++++++--------
 1 file changed, 95 insertions(+), 21 deletions(-)

diff --git a/include/net/netmem.h b/include/net/netmem.h
index de1d95f04076..535cf17b9134 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -12,6 +12,50 @@
 #include <linux/mm.h>
 #include <net/net_debug.h>
 
+/* These fields in struct page are used by the page_pool and net stack:
+ *
+ *        struct {
+ *                unsigned long pp_magic;
+ *                struct page_pool *pp;
+ *                unsigned long _pp_mapping_pad;
+ *                unsigned long dma_addr;
+ *                atomic_long_t pp_ref_count;
+ *        };
+ *
+ * We mirror the page_pool fields here so the page_pool can access these
+ * fields without worrying whether the underlying fields belong to a
+ * page or netmem_desc.
+ *
+ * CAUTION: Do not update the fields in netmem_desc without also
+ * updating the anonymous aliasing union in struct net_iov.
+ */
+struct netmem_desc {
+	unsigned long _flags;
+	unsigned long pp_magic;
+	struct page_pool *pp;
+	unsigned long _pp_mapping_pad;
+	unsigned long dma_addr;
+	atomic_long_t pp_ref_count;
+};
+
+#define NETMEM_DESC_ASSERT_OFFSET(pg, desc)        \
+	static_assert(offsetof(struct page, pg) == \
+		      offsetof(struct netmem_desc, desc))
+NETMEM_DESC_ASSERT_OFFSET(flags, _flags);
+NETMEM_DESC_ASSERT_OFFSET(pp_magic, pp_magic);
+NETMEM_DESC_ASSERT_OFFSET(pp, pp);
+NETMEM_DESC_ASSERT_OFFSET(_pp_mapping_pad, _pp_mapping_pad);
+NETMEM_DESC_ASSERT_OFFSET(dma_addr, dma_addr);
+NETMEM_DESC_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
+#undef NETMEM_DESC_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));
+
 /* net_iov */
 
 DECLARE_STATIC_KEY_FALSE(page_pool_mem_providers);
@@ -30,13 +74,48 @@ enum net_iov_type {
 	NET_IOV_MAX = ULONG_MAX
 };
 
+/* A memory descriptor representing abstract networking I/O vectors,
+ * generally for non-pages memory that doesn't have its corresponding
+ * struct page and needs to be explicitly allocated through slab.
+ *
+ * net_iovs are allocated and used by networking code, and the size of
+ * the chunk is PAGE_SIZE.
+ *
+ * This memory can be any form of non-struct paged memory.  Examples
+ * include imported dmabuf memory and imported io_uring memory.  See
+ * net_iov_type for all the supported types.
+ *
+ * @pp_magic:	pp field, similar to the one in struct page/struct
+ *		netmem_desc.
+ * @pp:		the pp this net_iov belongs to, if any.
+ * @dma_addr:	the dma addrs of the net_iov. Needed for the network
+ *		card to send/receive this net_iov.
+ * @pp_ref_count: the pp ref count of this net_iov, exactly the same
+ *		usage as struct page/struct netmem_desc.
+ * @owner:	the net_iov_area this net_iov belongs to, if any.
+ * @type:	the type of the memory.  Different types of net_iovs are
+ *		supported.
+ */
 struct net_iov {
-	enum net_iov_type type;
-	unsigned long pp_magic;
-	struct page_pool *pp;
+	union {
+		struct netmem_desc desc;
+
+		/* XXX: The following part should be removed once all
+		 * the references to them are converted so as to be
+		 * accessed via netmem_desc e.g. niov->desc.pp instead
+		 * of niov->pp.
+		 */
+		struct {
+			unsigned long _flags;
+			unsigned long pp_magic;
+			struct page_pool *pp;
+			unsigned long _pp_mapping_pad;
+			unsigned long dma_addr;
+			atomic_long_t pp_ref_count;
+		};
+	};
 	struct net_iov_area *owner;
-	unsigned long dma_addr;
-	atomic_long_t pp_ref_count;
+	enum net_iov_type type;
 };
 
 struct net_iov_area {
@@ -48,27 +127,22 @@ struct net_iov_area {
 	unsigned long base_virtual;
 };
 
-/* These fields in struct page are used by the page_pool and net stack:
+/* net_iov is union'ed with struct netmem_desc mirroring struct page, so
+ * the page_pool can access these fields without worrying whether the
+ * underlying fields are accessed via netmem_desc or directly via
+ * net_iov, until all the references to them are converted so as to be
+ * accessed via netmem_desc e.g. niov->desc.pp instead of niov->pp.
  *
- *        struct {
- *                unsigned long pp_magic;
- *                struct page_pool *pp;
- *                unsigned long _pp_mapping_pad;
- *                unsigned long dma_addr;
- *                atomic_long_t pp_ref_count;
- *        };
- *
- * We mirror the page_pool fields here so the page_pool can access these fields
- * without worrying whether the underlying fields belong to a page or net_iov.
- *
- * The non-net stack fields of struct page are private to the mm stack and must
- * never be mirrored to net_iov.
+ * The non-net stack fields of struct page are private to the mm stack
+ * and must never be mirrored to net_iov.
  */
-#define NET_IOV_ASSERT_OFFSET(pg, iov)             \
-	static_assert(offsetof(struct page, pg) == \
+#define NET_IOV_ASSERT_OFFSET(desc, iov)                    \
+	static_assert(offsetof(struct netmem_desc, desc) == \
 		      offsetof(struct net_iov, iov))
+NET_IOV_ASSERT_OFFSET(_flags, _flags);
 NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
 NET_IOV_ASSERT_OFFSET(pp, pp);
+NET_IOV_ASSERT_OFFSET(_pp_mapping_pad, _pp_mapping_pad);
 NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
 NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
 #undef NET_IOV_ASSERT_OFFSET
-- 
2.17.1

[PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

To eliminate the use of the page pool fields in struct page, the page
pool code should use netmem descriptor and APIs instead.

However, __netmem_get_pp() still accesses ->pp via struct page.  So
change it to use struct netmem_desc instead, since ->pp no longer will
be available in struct page.

While at it, add a helper, pp_page_to_nmdesc(), that can be used to
extract netmem_desc from page only if it's pp page.  For now that
netmem_desc overlays on page, it can be achieved by just casting.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 include/net/netmem.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/include/net/netmem.h b/include/net/netmem.h
index 535cf17b9134..2b8a7b51ac99 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
 	return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
 }
 
+static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
+{
+	DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
+
+	/* XXX: How to extract netmem_desc from page must be changed,
+	 * once netmem_desc no longer overlays on page and will be
+	 * allocated through slab.
+	 */
+	return (struct netmem_desc *)page;
+}
+
 /**
  * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
  * @netmem: netmem reference to get the pointer from
@@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
  */
 static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
 {
-	return __netmem_to_page(netmem)->pp;
+	return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
 }
 
 static inline struct page_pool *netmem_get_pp(netmem_ref netmem)
-- 
2.17.1

[PATCH net-next v10 03/12] mlx4: access ->pp_ref_count through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make mlx4 access ->pp_ref_count through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/mellanox/mlx4/en_rx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index b33285d755b9..92a16ddb7d86 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
@@ -460,9 +460,11 @@ static int mlx4_en_complete_rx_desc(struct mlx4_en_priv *priv,
 
 		truesize += frag_info->frag_stride;
 		if (frag_info->frag_stride == PAGE_SIZE / 2) {
+			struct netmem_desc *desc = pp_page_to_nmdesc(page);
+
 			frags->page_offset ^= PAGE_SIZE / 2;
 			release = page_count(page) != 1 ||
-				  atomic_long_read(&page->pp_ref_count) != 1 ||
+				  atomic_long_read(&desc->pp_ref_count) != 1 ||
 				  page_is_pfmemalloc(page) ||
 				  page_to_nid(page) != numa_mem_id();
 		} else if (!priv->rx_headroom) {
-- 
2.17.1

[PATCH net-next v10 04/12] netdevsim: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make netdevsim access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/netdevsim/netdev.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index e36d3e846c2d..d6b3a7755f14 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -842,7 +842,8 @@ nsim_pp_hold_write(struct file *file, const char __user *data,
 		if (!ns->page)
 			ret = -ENOMEM;
 	} else {
-		page_pool_put_full_page(ns->page->pp, ns->page, false);
+		page_pool_put_full_page(pp_page_to_nmdesc(ns->page)->pp,
+					ns->page, false);
 		ns->page = NULL;
 	}
 
@@ -1069,7 +1070,8 @@ void nsim_destroy(struct netdevsim *ns)
 
 	/* Put this intentionally late to exercise the orphaning path */
 	if (ns->page) {
-		page_pool_put_full_page(ns->page->pp, ns->page, false);
+		page_pool_put_full_page(pp_page_to_nmdesc(ns->page)->pp,
+					ns->page, false);
 		ns->page = NULL;
 	}
 
-- 
2.17.1

[PATCH net-next v10 05/12] mt76: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make mt76 access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/wireless/mediatek/mt76/mt76.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h b/drivers/net/wireless/mediatek/mt76/mt76.h
index 14927a92f9d1..7e9ddf91b822 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -1798,7 +1798,8 @@ static inline void mt76_put_page_pool_buf(void *buf, bool allow_direct)
 {
 	struct page *page = virt_to_head_page(buf);
 
-	page_pool_put_full_page(page->pp, page, allow_direct);
+	page_pool_put_full_page(pp_page_to_nmdesc(page)->pp, page,
+				allow_direct);
 }
 
 static inline void *
-- 
2.17.1

[PATCH net-next v10 06/12] net: fec: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make fec access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/freescale/fec_main.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index d4eed252ad40..1d0bd6f75368 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1043,7 +1043,9 @@ static void fec_enet_bd_init(struct net_device *dev)
 				struct page *page = txq->tx_buf[i].buf_p;
 
 				if (page)
-					page_pool_put_page(page->pp, page, 0, false);
+					page_pool_put_page(pp_page_to_nmdesc(page)->pp,
+							   page, 0,
+							   false);
 			}
 
 			txq->tx_buf[i].buf_p = NULL;
@@ -1581,7 +1583,8 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
 			xdp_return_frame_rx_napi(xdpf);
 		} else { /* recycle pages of XDP_TX frames */
 			/* The dma_sync_size = 0 as XDP_TX has already synced DMA for_device */
-			page_pool_put_page(page->pp, page, 0, true);
+			page_pool_put_page(pp_page_to_nmdesc(page)->pp, page,
+					   0, true);
 		}
 
 		txq->tx_buf[index].buf_p = NULL;
@@ -3343,7 +3346,8 @@ static void fec_enet_free_buffers(struct net_device *ndev)
 			} else {
 				struct page *page = txq->tx_buf[i].buf_p;
 
-				page_pool_put_page(page->pp, page, 0, false);
+				page_pool_put_page(pp_page_to_nmdesc(page)->pp,
+						   page, 0, false);
 			}
 
 			txq->tx_buf[i].buf_p = NULL;
-- 
2.17.1

[PATCH net-next v10 07/12] octeontx2-pf: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make octeontx2-pf access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index 99ace381cc78..625bb5a05344 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -1571,7 +1571,7 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
 		cq->pool_ptrs++;
 		if (xsk_buff) {
 			xsk_buff_free(xsk_buff);
-		} else if (page->pp) {
+		} else if (pp_page_to_nmdesc(page)->pp) {
 			page_pool_recycle_direct(pool->page_pool, page);
 		} else {
 			otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
-- 
2.17.1

[PATCH net-next v10 08/12] iavf: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make iavf access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/intel/iavf/iavf_txrx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
index aaf70c625655..363c42bf3dcf 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
@@ -1216,7 +1216,7 @@ static struct sk_buff *iavf_build_skb(const struct libeth_fqe *rx_buffer,
 				      unsigned int size)
 {
 	struct page *buf_page = __netmem_to_page(rx_buffer->netmem);
-	u32 hr = buf_page->pp->p.offset;
+	u32 hr = pp_page_to_nmdesc(buf_page)->pp->p.offset;
 	struct sk_buff *skb;
 	void *va;
 
-- 
2.17.1

[PATCH net-next v10 09/12] idpf: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make idpf access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/intel/idpf/idpf_txrx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/intel/idpf/idpf_txrx.c b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
index cef9dfb877e8..6b5f440aede3 100644
--- a/drivers/net/ethernet/intel/idpf/idpf_txrx.c
+++ b/drivers/net/ethernet/intel/idpf/idpf_txrx.c
@@ -3276,8 +3276,10 @@ static u32 idpf_rx_hsplit_wa(const struct libeth_fqe *hdr,
 
 	hdr_page = __netmem_to_page(hdr->netmem);
 	buf_page = __netmem_to_page(buf->netmem);
-	dst = page_address(hdr_page) + hdr->offset + hdr_page->pp->p.offset;
-	src = page_address(buf_page) + buf->offset + buf_page->pp->p.offset;
+	dst = page_address(hdr_page) + hdr->offset +
+		pp_page_to_nmdesc(hdr_page)->pp->p.offset;
+	src = page_address(buf_page) + buf->offset +
+		pp_page_to_nmdesc(buf_page)->pp->p.offset;
 
 	memcpy(dst, src, LARGEST_ALIGN(copy));
 	buf->offset += copy;
@@ -3296,7 +3298,7 @@ static u32 idpf_rx_hsplit_wa(const struct libeth_fqe *hdr,
 struct sk_buff *idpf_rx_build_skb(const struct libeth_fqe *buf, u32 size)
 {
 	struct page *buf_page = __netmem_to_page(buf->netmem);
-	u32 hr = buf_page->pp->p.offset;
+	u32 hr = pp_page_to_nmdesc(buf_page)->pp->p.offset;
 	struct sk_buff *skb;
 	void *va;
 
-- 
2.17.1

[PATCH net-next v10 10/12] mlx5: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make mlx5 access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
index 5ce1b463b7a8..5d51600935a6 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xdp.c
@@ -710,7 +710,8 @@ static void mlx5e_free_xdpsq_desc(struct mlx5e_xdpsq *sq,
 				/* No need to check page_pool_page_is_pp() as we
 				 * know this is a page_pool page.
 				 */
-				page_pool_recycle_direct(page->pp, page);
+				page_pool_recycle_direct(pp_page_to_nmdesc(page)->pp,
+							 page);
 			} while (++n < num);
 
 			break;
-- 
2.17.1

[PATCH net-next v10 11/12] net: ti: icssg-prueth: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make icssg-prueth access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
index ff5f41bf499e..5e225310c9de 100644
--- a/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
+++ b/drivers/net/ethernet/ti/icssg/icssg_prueth_sr1.c
@@ -367,7 +367,7 @@ static irqreturn_t prueth_rx_mgm_ts_thread_sr1(int irq, void *dev_id)
 		return IRQ_NONE;
 
 	prueth_tx_ts_sr1(emac, (void *)page_address(page));
-	page_pool_recycle_direct(page->pp, page);
+	page_pool_recycle_direct(pp_page_to_nmdesc(page)->pp, page);
 
 	return IRQ_HANDLED;
 }
@@ -392,7 +392,7 @@ static irqreturn_t prueth_rx_mgm_rsp_thread(int irq, void *dev_id)
 		complete(&emac->cmd_complete);
 	}
 
-	page_pool_recycle_direct(page->pp, page);
+	page_pool_recycle_direct(pp_page_to_nmdesc(page)->pp, page);
 
 	return IRQ_HANDLED;
 }
-- 
2.17.1

[PATCH net-next v10 12/12] libeth: xdp: access ->pp through netmem_desc instead of page

[Byungchul Park]

To eliminate the use of struct page in page pool, the page pool users
should use netmem descriptor and APIs instead.

Make xdp access ->pp through netmem_desc instead of page.

Signed-off-by: Byungchul Park <byungchul@sk.com>
---
 include/net/libeth/xdp.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/net/libeth/xdp.h b/include/net/libeth/xdp.h
index 6ce6aec6884c..f4880b50e804 100644
--- a/include/net/libeth/xdp.h
+++ b/include/net/libeth/xdp.h
@@ -1292,7 +1292,7 @@ static inline void libeth_xdp_prepare_buff(struct libeth_xdp_buff *xdp,
 	xdp_init_buff(&xdp->base, fqe->truesize, xdp->base.rxq);
 #endif
 	xdp_prepare_buff(&xdp->base, page_address(page) + fqe->offset,
-			 page->pp->p.offset, len, true);
+			 pp_page_to_nmdesc(page)->pp->p.offset, len, true);
 }
 
 /**
-- 
2.17.1

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Mina Almasry]

On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> change it to use struct netmem_desc instead, since ->pp no longer will
> be available in struct page.
>
> While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> extract netmem_desc from page only if it's pp page.  For now that
> netmem_desc overlays on page, it can be achieved by just casting.
>
> Signed-off-by: Byungchul Park <byungchul@sk.com>
> ---
>  include/net/netmem.h | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 535cf17b9134..2b8a7b51ac99 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
>         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
>  }
>
> +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> +{
> +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> +
> +       /* XXX: How to extract netmem_desc from page must be changed,
> +        * once netmem_desc no longer overlays on page and will be
> +        * allocated through slab.
> +        */
> +       return (struct netmem_desc *)page;
> +}
> +

Same thing. Do not create a generic looking pp_page_to_nmdesc helper
which does not check that the page is the correct type. The
DEBUG_NET... is not good enough.

You don't need to add a generic helper here. There is only one call
site. Open code this in the callsite. The one callsite is marked as
unsafe, only called by code that knows that the netmem is specifically
a pp page. Open code this in the unsafe callsite, instead of creating
a generic looking unsafe helper and not even documenting it's unsafe.

>  /**
>   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
>   * @netmem: netmem reference to get the pointer from
> @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
>   */
>  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
>  {
> -       return __netmem_to_page(netmem)->pp;
> +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
>  }

This makes me very sad. Casting from netmem -> page -> nmdesc...

Instead, we should be able to go from netmem directly to nmdesc. I
would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
it return netmem_desc instead of net_iov. Then use it here.

We could have an unsafe version of netmem_to_nmdesc which converts the
netmem to netmem_desc without clearing the lsb and mark it unsafe.

-- 
Thanks,
Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Mina Almasry]

On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
>
> On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > change it to use struct netmem_desc instead, since ->pp no longer will
> > be available in struct page.
> >
> > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > extract netmem_desc from page only if it's pp page.  For now that
> > netmem_desc overlays on page, it can be achieved by just casting.
> >
> > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > ---
> >  include/net/netmem.h | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index 535cf17b9134..2b8a7b51ac99 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> >  }
> >
> > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > +{
> > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > +
> > +       /* XXX: How to extract netmem_desc from page must be changed,
> > +        * once netmem_desc no longer overlays on page and will be
> > +        * allocated through slab.
> > +        */
> > +       return (struct netmem_desc *)page;
> > +}
> > +
>
> Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> which does not check that the page is the correct type. The
> DEBUG_NET... is not good enough.
>
> You don't need to add a generic helper here. There is only one call
> site. Open code this in the callsite. The one callsite is marked as
> unsafe, only called by code that knows that the netmem is specifically
> a pp page. Open code this in the unsafe callsite, instead of creating
> a generic looking unsafe helper and not even documenting it's unsafe.
>

On second read through the series, I actually now think this is a
great idea :-) Adding this helper has simplified the series greatly. I
did not realize you were converting entire drivers to netmem just to
get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
the entire series simpler.

You're also calling it only from code paths like drivers that already
assumed that the page is a pp page and did page->pp deference without
a check, so this should be safe.

Only thing I would change is add a comment explaining that the calling
code needs to check the page is pp page or know it's a pp page (like a
driver that supports pp).


> >  /**
> >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> >   * @netmem: netmem reference to get the pointer from
> > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> >   */
> >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> >  {
> > -       return __netmem_to_page(netmem)->pp;
> > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> >  }
>
> This makes me very sad. Casting from netmem -> page -> nmdesc...
>
> Instead, we should be able to go from netmem directly to nmdesc. I
> would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> it return netmem_desc instead of net_iov. Then use it here.
>
> We could have an unsafe version of netmem_to_nmdesc which converts the
> netmem to netmem_desc without clearing the lsb and mark it unsafe.
>

This, I think, we should address to keep some sanity in the code and
reduce the casts and make it a bit more maintainable.

-- 
Thanks,
Mina

Re: [PATCH net-next v10 12/12] libeth: xdp: access ->pp through netmem_desc instead of page

[kernel test robot]

Hi Byungchul,

kernel test robot noticed the following build warnings:

[auto build test WARNING on c65d34296b2252897e37835d6007bbd01b255742]

url:    https://github.com/intel-lab-lkp/linux/commits/Byungchul-Park/netmem-introduce-struct-netmem_desc-mirroring-struct-page/20250714-200214
base:   c65d34296b2252897e37835d6007bbd01b255742
patch link:    https://lore.kernel.org/r/20250714120047.35901-13-byungchul%40sk.com
patch subject: [PATCH net-next v10 12/12] libeth: xdp: access ->pp through netmem_desc instead of page
config: m68k-allmodconfig (https://download.01.org/0day-ci/archive/20250715/202507150748.9sVeInO8-lkp@intel.com/config)
compiler: m68k-linux-gcc (GCC) 15.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250715/202507150748.9sVeInO8-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507150748.9sVeInO8-lkp@intel.com/

All warnings (new ones prefixed by >>):

   In file included from drivers/net/ethernet/intel/libeth/tx.c:6:
   include/net/libeth/xdp.h: In function 'libeth_xdp_prepare_buff':
>> include/net/libeth/xdp.h:1295:44: warning: passing argument 1 of 'pp_page_to_nmdesc' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
    1295 |                          pp_page_to_nmdesc(page)->pp->p.offset, len, true);
         |                                            ^~~~
   In file included from include/linux/skbuff.h:41,
                    from include/net/net_namespace.h:43,
                    from include/linux/netdevice.h:38,
                    from include/trace/events/xdp.h:8,
                    from include/linux/bpf_trace.h:5,
                    from include/net/libeth/xdp.h:7:
   include/net/netmem.h:270:66: note: expected 'struct page *' but argument is of type 'const struct page *'
     270 | static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
         |                                                     ~~~~~~~~~~~~~^~~~


vim +1295 include/net/libeth/xdp.h

  1263	
  1264	bool libeth_xdp_buff_add_frag(struct libeth_xdp_buff *xdp,
  1265				      const struct libeth_fqe *fqe,
  1266				      u32 len);
  1267	
  1268	/**
  1269	 * libeth_xdp_prepare_buff - fill &libeth_xdp_buff with head FQE data
  1270	 * @xdp: XDP buffer to attach the head to
  1271	 * @fqe: FQE containing the head buffer
  1272	 * @len: buffer len passed from HW
  1273	 *
  1274	 * Internal, use libeth_xdp_process_buff() instead. Initializes XDP buffer
  1275	 * head with the Rx buffer data: data pointer, length, headroom, and
  1276	 * truesize/tailroom. Zeroes the flags.
  1277	 * Uses faster single u64 write instead of per-field access.
  1278	 */
  1279	static inline void libeth_xdp_prepare_buff(struct libeth_xdp_buff *xdp,
  1280						   const struct libeth_fqe *fqe,
  1281						   u32 len)
  1282	{
  1283		const struct page *page = __netmem_to_page(fqe->netmem);
  1284	
  1285	#ifdef __LIBETH_WORD_ACCESS
  1286		static_assert(offsetofend(typeof(xdp->base), flags) -
  1287			      offsetof(typeof(xdp->base), frame_sz) ==
  1288			      sizeof(u64));
  1289	
  1290		*(u64 *)&xdp->base.frame_sz = fqe->truesize;
  1291	#else
  1292		xdp_init_buff(&xdp->base, fqe->truesize, xdp->base.rxq);
  1293	#endif
  1294		xdp_prepare_buff(&xdp->base, page_address(page) + fqe->offset,
> 1295				 pp_page_to_nmdesc(page)->pp->p.offset, len, true);
  1296	}
  1297	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> >
> > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > be available in struct page.
> > >
> > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > extract netmem_desc from page only if it's pp page.  For now that
> > > netmem_desc overlays on page, it can be achieved by just casting.
> > >
> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > ---
> > >  include/net/netmem.h | 13 ++++++++++++-
> > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > index 535cf17b9134..2b8a7b51ac99 100644
> > > --- a/include/net/netmem.h
> > > +++ b/include/net/netmem.h
> > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > >  }
> > >
> > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > +{
> > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > +
> > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > +        * once netmem_desc no longer overlays on page and will be
> > > +        * allocated through slab.
> > > +        */
> > > +       return (struct netmem_desc *)page;
> > > +}
> > > +
> >
> > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > which does not check that the page is the correct type. The
> > DEBUG_NET... is not good enough.
> >
> > You don't need to add a generic helper here. There is only one call
> > site. Open code this in the callsite. The one callsite is marked as
> > unsafe, only called by code that knows that the netmem is specifically
> > a pp page. Open code this in the unsafe callsite, instead of creating
> > a generic looking unsafe helper and not even documenting it's unsafe.
> >
> 
> On second read through the series, I actually now think this is a
> great idea :-) Adding this helper has simplified the series greatly. I
> did not realize you were converting entire drivers to netmem just to
> get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> the entire series simpler.
> 
> You're also calling it only from code paths like drivers that already
> assumed that the page is a pp page and did page->pp deference without
> a check, so this should be safe.
> 
> Only thing I would change is add a comment explaining that the calling
> code needs to check the page is pp page or know it's a pp page (like a
> driver that supports pp).
> 
> 
> > >  /**
> > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > >   * @netmem: netmem reference to get the pointer from
> > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > >   */
> > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > >  {
> > > -       return __netmem_to_page(netmem)->pp;
> > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > >  }
> >
> > This makes me very sad. Casting from netmem -> page -> nmdesc...
> >
> > Instead, we should be able to go from netmem directly to nmdesc. I
> > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > it return netmem_desc instead of net_iov. Then use it here.
> >
> > We could have an unsafe version of netmem_to_nmdesc which converts the
> > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> >
> 
> This, I think, we should address to keep some sanity in the code and
> reduce the casts and make it a bit more maintainable.

I will reflect your suggestions.  To summarize:

   1) The current implementation of pp_page_to_nmdesc() is good enough
      to keep, but add a comment on it like "Check if the page is a pp
      page before calling this function or know it's a pp page.".

   2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
      __netmem_get_pp().

   3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
      netmem_desc, and use it in all users of __netmem_clear_lsb().

Anything else?

	Byungchul
> 
> --
> Thanks,
> Mina

Re: [PATCH net-next v10 12/12] libeth: xdp: access ->pp through netmem_desc instead of page

[kernel test robot]

Hi Byungchul,

kernel test robot noticed the following build errors:

[auto build test ERROR on c65d34296b2252897e37835d6007bbd01b255742]

url:    https://github.com/intel-lab-lkp/linux/commits/Byungchul-Park/netmem-introduce-struct-netmem_desc-mirroring-struct-page/20250714-200214
base:   c65d34296b2252897e37835d6007bbd01b255742
patch link:    https://lore.kernel.org/r/20250714120047.35901-13-byungchul%40sk.com
patch subject: [PATCH net-next v10 12/12] libeth: xdp: access ->pp through netmem_desc instead of page
config: x86_64-rhel-9.4-rust (https://download.01.org/0day-ci/archive/20250715/202507150904.kGZOOZns-lkp@intel.com/config)
compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261)
rustc: rustc 1.88.0 (6b00bc388 2025-06-23)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250715/202507150904.kGZOOZns-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202507150904.kGZOOZns-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from drivers/net/ethernet/intel/libeth/tx.c:6:
>> include/net/libeth/xdp.h:1295:23: error: passing 'const struct page *' to parameter of type 'struct page *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers]
    1295 |                          pp_page_to_nmdesc(page)->pp->p.offset, len, true);
         |                                            ^~~~
   include/net/netmem.h:270:66: note: passing argument to parameter 'page' here
     270 | static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
         |                                                                  ^
   1 error generated.


vim +1295 include/net/libeth/xdp.h

  1263	
  1264	bool libeth_xdp_buff_add_frag(struct libeth_xdp_buff *xdp,
  1265				      const struct libeth_fqe *fqe,
  1266				      u32 len);
  1267	
  1268	/**
  1269	 * libeth_xdp_prepare_buff - fill &libeth_xdp_buff with head FQE data
  1270	 * @xdp: XDP buffer to attach the head to
  1271	 * @fqe: FQE containing the head buffer
  1272	 * @len: buffer len passed from HW
  1273	 *
  1274	 * Internal, use libeth_xdp_process_buff() instead. Initializes XDP buffer
  1275	 * head with the Rx buffer data: data pointer, length, headroom, and
  1276	 * truesize/tailroom. Zeroes the flags.
  1277	 * Uses faster single u64 write instead of per-field access.
  1278	 */
  1279	static inline void libeth_xdp_prepare_buff(struct libeth_xdp_buff *xdp,
  1280						   const struct libeth_fqe *fqe,
  1281						   u32 len)
  1282	{
  1283		const struct page *page = __netmem_to_page(fqe->netmem);
  1284	
  1285	#ifdef __LIBETH_WORD_ACCESS
  1286		static_assert(offsetofend(typeof(xdp->base), flags) -
  1287			      offsetof(typeof(xdp->base), frame_sz) ==
  1288			      sizeof(u64));
  1289	
  1290		*(u64 *)&xdp->base.frame_sz = fqe->truesize;
  1291	#else
  1292		xdp_init_buff(&xdp->base, fqe->truesize, xdp->base.rxq);
  1293	#endif
  1294		xdp_prepare_buff(&xdp->base, page_address(page) + fqe->offset,
> 1295				 pp_page_to_nmdesc(page)->pp->p.offset, len, true);
  1296	}
  1297	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Pavel Begunkov]

On 7/14/25 20:37, Mina Almasry wrote:
> On Mon, Jul 14, 2025 at 5:01 AM Byungchul Park <byungchul@sk.com> wrote:
...>> +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
>> +{
>> +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
>> +
>> +       /* XXX: How to extract netmem_desc from page must be changed,
>> +        * once netmem_desc no longer overlays on page and will be
>> +        * allocated through slab.
>> +        */
>> +       return (struct netmem_desc *)page;
>> +}
>> +
> 
> Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> which does not check that the page is the correct type. The
> DEBUG_NET... is not good enough.
> 
> You don't need to add a generic helper here. There is only one call
> site. Open code this in the callsite. The one callsite is marked as
> unsafe, only called by code that knows that the netmem is specifically
> a pp page. Open code this in the unsafe callsite, instead of creating
> a generic looking unsafe helper and not even documenting it's unsafe.
> 
>>   /**
>>    * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
>>    * @netmem: netmem reference to get the pointer from
>> @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
>>    */
>>   static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
>>   {
>> -       return __netmem_to_page(netmem)->pp;
>> +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
>>   }
> 
> This makes me very sad. Casting from netmem -> page -> nmdesc...

The function is not used, and I don't think the series adds any
new users? It can be killed then. It's a horrible function anyway,
would be much better to have a variant taking struct page * if
necessary.

> Instead, we should be able to go from netmem directly to nmdesc. I
> would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> it return netmem_desc instead of net_iov. Then use it here.

Glad you liked the diff I suggested :) In either case, seems
like it's not strictly necessary for this iteration as
__netmem_get_pp() should be killed, and the rest of patches work
directly with pages.

  
> We could have an unsafe version of netmem_to_nmdesc which converts the
> netmem to netmem_desc without clearing the lsb and mark it unsafe.
> 

-- 
Pavel Begunkov

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Mina Almasry]

On Tue, Jul 15, 2025 at 3:36 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 7/14/25 20:37, Mina Almasry wrote:
> > On Mon, Jul 14, 2025 at 5:01 AM Byungchul Park <byungchul@sk.com> wrote:
> ...>> +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> >> +{
> >> +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> >> +
> >> +       /* XXX: How to extract netmem_desc from page must be changed,
> >> +        * once netmem_desc no longer overlays on page and will be
> >> +        * allocated through slab.
> >> +        */
> >> +       return (struct netmem_desc *)page;
> >> +}
> >> +
> >
> > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > which does not check that the page is the correct type. The
> > DEBUG_NET... is not good enough.
> >
> > You don't need to add a generic helper here. There is only one call
> > site. Open code this in the callsite. The one callsite is marked as
> > unsafe, only called by code that knows that the netmem is specifically
> > a pp page. Open code this in the unsafe callsite, instead of creating
> > a generic looking unsafe helper and not even documenting it's unsafe.
> >
> >>   /**
> >>    * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> >>    * @netmem: netmem reference to get the pointer from
> >> @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> >>    */
> >>   static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> >>   {
> >> -       return __netmem_to_page(netmem)->pp;
> >> +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> >>   }
> >
> > This makes me very sad. Casting from netmem -> page -> nmdesc...
>
> The function is not used, and I don't think the series adds any
> new users? It can be killed then. It's a horrible function anyway,
> would be much better to have a variant taking struct page * if
> necessary.
>
> > Instead, we should be able to go from netmem directly to nmdesc. I
> > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > it return netmem_desc instead of net_iov. Then use it here.
>
> Glad you liked the diff I suggested :) In either case, seems
> like it's not strictly necessary for this iteration as
> __netmem_get_pp() should be killed, and the rest of patches work
> directly with pages.

Good catch, in that case lets just delete __netmem_get_pp and there is
no need to add a netmem_nmdesc unless we find some other call site
that needs it.

-- 
Thanks,
Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Mina Almasry]

On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
>
> On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > >
> > > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > > be available in struct page.
> > > >
> > > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > > extract netmem_desc from page only if it's pp page.  For now that
> > > > netmem_desc overlays on page, it can be achieved by just casting.
> > > >
> > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > ---
> > > >  include/net/netmem.h | 13 ++++++++++++-
> > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > index 535cf17b9134..2b8a7b51ac99 100644
> > > > --- a/include/net/netmem.h
> > > > +++ b/include/net/netmem.h
> > > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > > >  }
> > > >
> > > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > > +{
> > > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > > +
> > > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > > +        * once netmem_desc no longer overlays on page and will be
> > > > +        * allocated through slab.
> > > > +        */
> > > > +       return (struct netmem_desc *)page;
> > > > +}
> > > > +
> > >
> > > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > > which does not check that the page is the correct type. The
> > > DEBUG_NET... is not good enough.
> > >
> > > You don't need to add a generic helper here. There is only one call
> > > site. Open code this in the callsite. The one callsite is marked as
> > > unsafe, only called by code that knows that the netmem is specifically
> > > a pp page. Open code this in the unsafe callsite, instead of creating
> > > a generic looking unsafe helper and not even documenting it's unsafe.
> > >
> >
> > On second read through the series, I actually now think this is a
> > great idea :-) Adding this helper has simplified the series greatly. I
> > did not realize you were converting entire drivers to netmem just to
> > get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> > the entire series simpler.
> >
> > You're also calling it only from code paths like drivers that already
> > assumed that the page is a pp page and did page->pp deference without
> > a check, so this should be safe.
> >
> > Only thing I would change is add a comment explaining that the calling
> > code needs to check the page is pp page or know it's a pp page (like a
> > driver that supports pp).
> >
> >
> > > >  /**
> > > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > > >   * @netmem: netmem reference to get the pointer from
> > > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > >   */
> > > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > > >  {
> > > > -       return __netmem_to_page(netmem)->pp;
> > > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > > >  }
> > >
> > > This makes me very sad. Casting from netmem -> page -> nmdesc...
> > >
> > > Instead, we should be able to go from netmem directly to nmdesc. I
> > > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > > it return netmem_desc instead of net_iov. Then use it here.
> > >
> > > We could have an unsafe version of netmem_to_nmdesc which converts the
> > > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > >
> >
> > This, I think, we should address to keep some sanity in the code and
> > reduce the casts and make it a bit more maintainable.
>
> I will reflect your suggestions.  To summarize:
>
>    1) The current implementation of pp_page_to_nmdesc() is good enough
>       to keep, but add a comment on it like "Check if the page is a pp
>       page before calling this function or know it's a pp page.".
>

Yes please.

>    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
>       __netmem_get_pp().
>

No need following Pavel's feedback. We can just delete
__netmem_get_pp. If we do find a need in the future to extract the
netmem_desc from a netmem_ref, I would rather we do a straight cast
from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
-> netmem_desc.

But that seems unnecessary for this series.


>    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
>       netmem_desc, and use it in all users of __netmem_clear_lsb().
>

Following Pavel's comment, this I think also is not necessary for this
series. Cleaning up the return value of __netmem_clear_lsb is good
work I think, but we're already on v10 of this and I think it would
unnecessary to ask for added cleanups. We can do the cleanup on top.

> Anything else?

Thank you very much :-)

-- 
Thanks,
Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Tue, Jul 15, 2025 at 11:37:45AM +0100, Pavel Begunkov wrote:
> On 7/14/25 20:37, Mina Almasry wrote:
> > On Mon, Jul 14, 2025 at 5:01 AM Byungchul Park <byungchul@sk.com> wrote:
> ...>> +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > +{
> > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > +
> > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > +        * once netmem_desc no longer overlays on page and will be
> > > +        * allocated through slab.
> > > +        */
> > > +       return (struct netmem_desc *)page;
> > > +}
> > > +
> > 
> > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > which does not check that the page is the correct type. The
> > DEBUG_NET... is not good enough.
> > 
> > You don't need to add a generic helper here. There is only one call
> > site. Open code this in the callsite. The one callsite is marked as
> > unsafe, only called by code that knows that the netmem is specifically
> > a pp page. Open code this in the unsafe callsite, instead of creating
> > a generic looking unsafe helper and not even documenting it's unsafe.
> > 
> > >   /**
> > >    * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > >    * @netmem: netmem reference to get the pointer from
> > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > >    */
> > >   static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > >   {
> > > -       return __netmem_to_page(netmem)->pp;
> > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > >   }
> > 
> > This makes me very sad. Casting from netmem -> page -> nmdesc...
> 
> The function is not used, and I don't think the series adds any

That's whay I'd been keeping the patch, 'netmem: remove __netmem_get_pp()'
until v6 [1].

[1] https://lore.kernel.org/all/20250620041224.46646-7-byungchul@sk.com/

However, as the following change log described [2], I excluded the patch
from v7 since __netmem_get_pp() started to be used again by libeth.

[2] https://lore.kernel.org/all/20250625043350.7939-1-byungchul@sk.com/

> new users? It can be killed then. It's a horrible function anyway,
> would be much better to have a variant taking struct page * if
> necessary.
> 
> > Instead, we should be able to go from netmem directly to nmdesc. I
> > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > it return netmem_desc instead of net_iov. Then use it here.
> 
> Glad you liked the diff I suggested :) In either case, seems
> like it's not strictly necessary for this iteration as
> __netmem_get_pp() should be killed, and the rest of patches work
> directly with pages.

Killing __netmem_get_pp() would be the best I think.

	Byungchul
> 
> 
> > We could have an unsafe version of netmem_to_nmdesc which converts the
> > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > 
> 
> --
> Pavel Begunkov

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Tue, Jul 15, 2025 at 12:09:34PM -0700, Mina Almasry wrote:
> On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > > >
> > > > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > > > be available in struct page.
> > > > >
> > > > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > > > extract netmem_desc from page only if it's pp page.  For now that
> > > > > netmem_desc overlays on page, it can be achieved by just casting.
> > > > >
> > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > ---
> > > > >  include/net/netmem.h | 13 ++++++++++++-
> > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > > index 535cf17b9134..2b8a7b51ac99 100644
> > > > > --- a/include/net/netmem.h
> > > > > +++ b/include/net/netmem.h
> > > > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > > > >  }
> > > > >
> > > > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > > > +{
> > > > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > > > +
> > > > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > > > +        * once netmem_desc no longer overlays on page and will be
> > > > > +        * allocated through slab.
> > > > > +        */
> > > > > +       return (struct netmem_desc *)page;
> > > > > +}
> > > > > +
> > > >
> > > > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > > > which does not check that the page is the correct type. The
> > > > DEBUG_NET... is not good enough.
> > > >
> > > > You don't need to add a generic helper here. There is only one call
> > > > site. Open code this in the callsite. The one callsite is marked as
> > > > unsafe, only called by code that knows that the netmem is specifically
> > > > a pp page. Open code this in the unsafe callsite, instead of creating
> > > > a generic looking unsafe helper and not even documenting it's unsafe.
> > > >
> > >
> > > On second read through the series, I actually now think this is a
> > > great idea :-) Adding this helper has simplified the series greatly. I
> > > did not realize you were converting entire drivers to netmem just to
> > > get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> > > the entire series simpler.
> > >
> > > You're also calling it only from code paths like drivers that already
> > > assumed that the page is a pp page and did page->pp deference without
> > > a check, so this should be safe.
> > >
> > > Only thing I would change is add a comment explaining that the calling
> > > code needs to check the page is pp page or know it's a pp page (like a
> > > driver that supports pp).
> > >
> > >
> > > > >  /**
> > > > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > > > >   * @netmem: netmem reference to get the pointer from
> > > > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > >   */
> > > > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > > > >  {
> > > > > -       return __netmem_to_page(netmem)->pp;
> > > > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > > > >  }
> > > >
> > > > This makes me very sad. Casting from netmem -> page -> nmdesc...
> > > >
> > > > Instead, we should be able to go from netmem directly to nmdesc. I
> > > > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > > > it return netmem_desc instead of net_iov. Then use it here.
> > > >
> > > > We could have an unsafe version of netmem_to_nmdesc which converts the
> > > > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > > >
> > >
> > > This, I think, we should address to keep some sanity in the code and
> > > reduce the casts and make it a bit more maintainable.
> >
> > I will reflect your suggestions.  To summarize:
> >
> >    1) The current implementation of pp_page_to_nmdesc() is good enough
> >       to keep, but add a comment on it like "Check if the page is a pp
> >       page before calling this function or know it's a pp page.".
> >
> 
> Yes please.
> 
> >    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
> >       __netmem_get_pp().
> >
> 
> No need following Pavel's feedback. We can just delete
> __netmem_get_pp. If we do find a need in the future to extract the
> netmem_desc from a netmem_ref, I would rather we do a straight cast
> from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
> -> netmem_desc.
> 
> But that seems unnecessary for this series.

No.  The series should remove accessing ->pp through page.

I will kill __netmem_get_pp() as you and I prefer.  However,
__netmem_get_pp() users e.i. libeth_xdp_return_va() and
libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:

as is: __netmem_get_pp(netmem)
to be: __netmem_nmdesc(netmem)->pp

Is it okay with you?

> >    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
> >       netmem_desc, and use it in all users of __netmem_clear_lsb().
> >
> 
> Following Pavel's comment, this I think also is not necessary for this
> series. Cleaning up the return value of __netmem_clear_lsb is good
> work I think, but we're already on v10 of this and I think it would
> unnecessary to ask for added cleanups. We can do the cleanup on top.

However, I still need to include 'introduce __netmem_nmdesc() helper'
in this series since it should be used to remove __netmem_get_pp() as I
described above.  I think I'd better add netmem_nmdesc() too while at it.

I assume __netmem_nmdesc() is an unsafe version not clearing lsb.  The
safe version, netmem_nmdesc() needs an additional operation clearing lsb.
If you are fine with it, I will add these two helpers in the next spin.
Or I could add __netmem_nmdesc() only.  Thoughts?  

> > Anything else?
> 
> Thank you very much :-)

I rather apprecisate your feedbacks.

	Byungchul

> --
> Thanks,
> Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Mina Almasry]

On Tue, Jul 15, 2025 at 9:51 PM Byungchul Park <byungchul@sk.com> wrote:
>
> On Tue, Jul 15, 2025 at 12:09:34PM -0700, Mina Almasry wrote:
> > On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
> > >
> > > On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > > > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > > > >
> > > > > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > > > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > > > > be available in struct page.
> > > > > >
> > > > > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > > > > extract netmem_desc from page only if it's pp page.  For now that
> > > > > > netmem_desc overlays on page, it can be achieved by just casting.
> > > > > >
> > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > ---
> > > > > >  include/net/netmem.h | 13 ++++++++++++-
> > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > >
> > > > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > > > index 535cf17b9134..2b8a7b51ac99 100644
> > > > > > --- a/include/net/netmem.h
> > > > > > +++ b/include/net/netmem.h
> > > > > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > > > > >  }
> > > > > >
> > > > > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > > > > +{
> > > > > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > > > > +
> > > > > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > > > > +        * once netmem_desc no longer overlays on page and will be
> > > > > > +        * allocated through slab.
> > > > > > +        */
> > > > > > +       return (struct netmem_desc *)page;
> > > > > > +}
> > > > > > +
> > > > >
> > > > > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > > > > which does not check that the page is the correct type. The
> > > > > DEBUG_NET... is not good enough.
> > > > >
> > > > > You don't need to add a generic helper here. There is only one call
> > > > > site. Open code this in the callsite. The one callsite is marked as
> > > > > unsafe, only called by code that knows that the netmem is specifically
> > > > > a pp page. Open code this in the unsafe callsite, instead of creating
> > > > > a generic looking unsafe helper and not even documenting it's unsafe.
> > > > >
> > > >
> > > > On second read through the series, I actually now think this is a
> > > > great idea :-) Adding this helper has simplified the series greatly. I
> > > > did not realize you were converting entire drivers to netmem just to
> > > > get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> > > > the entire series simpler.
> > > >
> > > > You're also calling it only from code paths like drivers that already
> > > > assumed that the page is a pp page and did page->pp deference without
> > > > a check, so this should be safe.
> > > >
> > > > Only thing I would change is add a comment explaining that the calling
> > > > code needs to check the page is pp page or know it's a pp page (like a
> > > > driver that supports pp).
> > > >
> > > >
> > > > > >  /**
> > > > > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > > > > >   * @netmem: netmem reference to get the pointer from
> > > > > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > >   */
> > > > > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > > > > >  {
> > > > > > -       return __netmem_to_page(netmem)->pp;
> > > > > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > > > > >  }
> > > > >
> > > > > This makes me very sad. Casting from netmem -> page -> nmdesc...
> > > > >
> > > > > Instead, we should be able to go from netmem directly to nmdesc. I
> > > > > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > > > > it return netmem_desc instead of net_iov. Then use it here.
> > > > >
> > > > > We could have an unsafe version of netmem_to_nmdesc which converts the
> > > > > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > > > >
> > > >
> > > > This, I think, we should address to keep some sanity in the code and
> > > > reduce the casts and make it a bit more maintainable.
> > >
> > > I will reflect your suggestions.  To summarize:
> > >
> > >    1) The current implementation of pp_page_to_nmdesc() is good enough
> > >       to keep, but add a comment on it like "Check if the page is a pp
> > >       page before calling this function or know it's a pp page.".
> > >
> >
> > Yes please.
> >
> > >    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
> > >       __netmem_get_pp().
> > >
> >
> > No need following Pavel's feedback. We can just delete
> > __netmem_get_pp. If we do find a need in the future to extract the
> > netmem_desc from a netmem_ref, I would rather we do a straight cast
> > from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
> > -> netmem_desc.
> >
> > But that seems unnecessary for this series.
>
> No.  The series should remove accessing ->pp through page.
>
> I will kill __netmem_get_pp() as you and I prefer.  However,
> __netmem_get_pp() users e.i. libeth_xdp_return_va() and
> libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
>
> as is: __netmem_get_pp(netmem)
> to be: __netmem_nmdesc(netmem)->pp
>
> Is it okay with you?
>

When Pavel and I were saying 'remove __netmem_get_pp', I think we
meant to remove the entire concept of unsafe netmem -> page
conversions. I think we both don't like them. From this perspective,
__netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).

I think since the unsafe netmem-to-page casts are already in mainline,
lets assume they should stay there until someone feels strongly enough
to remove them. The logic in Olek's patch is sound:

https://lore.kernel.org/all/20241203173733.3181246-8-aleksander.lobakin@intel.com/

Header buffer page pools do always use pages and will likely remain so
for a long time, so I guess lets continue to support them rather than
try to remove them in this series. A followup series could try to
remove them.

> > >    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
> > >       netmem_desc, and use it in all users of __netmem_clear_lsb().
> > >
> >
> > Following Pavel's comment, this I think also is not necessary for this
> > series. Cleaning up the return value of __netmem_clear_lsb is good
> > work I think, but we're already on v10 of this and I think it would
> > unnecessary to ask for added cleanups. We can do the cleanup on top.
>
> However, I still need to include 'introduce __netmem_nmdesc() helper'

Yes.

> in this series since it should be used to remove __netmem_get_pp() as I

lets keep __netmem_get_pp, which does a `return
__netmem_nmdesc(netmem)->pp;` In general we avoid allowing the driver
to do any netmem casts in the driver code, and we do any casting in
core.

> described above.  I think I'd better add netmem_nmdesc() too while at it.
>

Yes. netmem_nmdesc should replace __netmem_clear_lsb.

> I assume __netmem_nmdesc() is an unsafe version not clearing lsb.  The

Yes.

> safe version, netmem_nmdesc() needs an additional operation clearing lsb.

Yes.


--
Thanks,
Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Wed, Jul 16, 2025 at 12:41:04PM -0700, Mina Almasry wrote:
> On Tue, Jul 15, 2025 at 9:51 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > On Tue, Jul 15, 2025 at 12:09:34PM -0700, Mina Almasry wrote:
> > > On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
> > > >
> > > > On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > > > > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > > >    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
> > > >       __netmem_get_pp().
> > > >
> > >
> > > No need following Pavel's feedback. We can just delete
> > > __netmem_get_pp. If we do find a need in the future to extract the
> > > netmem_desc from a netmem_ref, I would rather we do a straight cast
> > > from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
> > > -> netmem_desc.
> > >
> > > But that seems unnecessary for this series.
> >
> > No.  The series should remove accessing ->pp through page.
> >
> > I will kill __netmem_get_pp() as you and I prefer.  However,
> > __netmem_get_pp() users e.i. libeth_xdp_return_va() and
> > libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
> >
> > as is: __netmem_get_pp(netmem)
> > to be: __netmem_nmdesc(netmem)->pp
> >
> > Is it okay with you?
> >
> 
> When Pavel and I were saying 'remove __netmem_get_pp', I think we
> meant to remove the entire concept of unsafe netmem -> page
> conversions. I think we both don't like them. From this perspective,
> __netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).
> 
> I think since the unsafe netmem-to-page casts are already in mainline,
> lets assume they should stay there until someone feels strongly enough
> to remove them. The logic in Olek's patch is sound:
> 
> https://lore.kernel.org/all/20241203173733.3181246-8-aleksander.lobakin@intel.com/
> 
> Header buffer page pools do always use pages and will likely remain so
> for a long time, so I guess lets continue to support them rather than
> try to remove them in this series. A followup series could try to
> remove them.

At the beginning of this work, I was unconfortable to see the network
code keeps the unsafe version maybe for optimization(?).  I decided to
accept it, thinking there must be some reason.

However, it'd be good that a followup series would try to remove them as
you said.

> > > >    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
> > > >       netmem_desc, and use it in all users of __netmem_clear_lsb().
> > > >
> > >
> > > Following Pavel's comment, this I think also is not necessary for this
> > > series. Cleaning up the return value of __netmem_clear_lsb is good
> > > work I think, but we're already on v10 of this and I think it would
> > > unnecessary to ask for added cleanups. We can do the cleanup on top.
> >
> > However, I still need to include 'introduce __netmem_nmdesc() helper'
> 
> Yes.
> 
> > in this series since it should be used to remove __netmem_get_pp() as I
> 
> lets keep __netmem_get_pp, which does a `return

Okay.  I will.

	Byungchul

> __netmem_nmdesc(netmem)->pp;` In general we avoid allowing the driver
> to do any netmem casts in the driver code, and we do any casting in
> core.
> 
> > described above.  I think I'd better add netmem_nmdesc() too while at it.
> >
> 
> Yes. netmem_nmdesc should replace __netmem_clear_lsb.
> 
> > I assume __netmem_nmdesc() is an unsafe version not clearing lsb.  The
> 
> Yes.
> 
> > safe version, netmem_nmdesc() needs an additional operation clearing lsb.
> 
> Yes.
> 
> 
> --
> Thanks,
> Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Wed, Jul 16, 2025 at 12:41:04PM -0700, Mina Almasry wrote:
> On Tue, Jul 15, 2025 at 9:51 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > On Tue, Jul 15, 2025 at 12:09:34PM -0700, Mina Almasry wrote:
> > > On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
> > > >
> > > > On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > > > > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > > > > >
> > > > > > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > > > > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > > > > > be available in struct page.
> > > > > > >
> > > > > > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > > > > > extract netmem_desc from page only if it's pp page.  For now that
> > > > > > > netmem_desc overlays on page, it can be achieved by just casting.
> > > > > > >
> > > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > > ---
> > > > > > >  include/net/netmem.h | 13 ++++++++++++-
> > > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > > > > index 535cf17b9134..2b8a7b51ac99 100644
> > > > > > > --- a/include/net/netmem.h
> > > > > > > +++ b/include/net/netmem.h
> > > > > > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > > > > > >  }
> > > > > > >
> > > > > > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > > > > > +{
> > > > > > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > > > > > +
> > > > > > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > > > > > +        * once netmem_desc no longer overlays on page and will be
> > > > > > > +        * allocated through slab.
> > > > > > > +        */
> > > > > > > +       return (struct netmem_desc *)page;
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > > > > > which does not check that the page is the correct type. The
> > > > > > DEBUG_NET... is not good enough.
> > > > > >
> > > > > > You don't need to add a generic helper here. There is only one call
> > > > > > site. Open code this in the callsite. The one callsite is marked as
> > > > > > unsafe, only called by code that knows that the netmem is specifically
> > > > > > a pp page. Open code this in the unsafe callsite, instead of creating
> > > > > > a generic looking unsafe helper and not even documenting it's unsafe.
> > > > > >
> > > > >
> > > > > On second read through the series, I actually now think this is a
> > > > > great idea :-) Adding this helper has simplified the series greatly. I
> > > > > did not realize you were converting entire drivers to netmem just to
> > > > > get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> > > > > the entire series simpler.
> > > > >
> > > > > You're also calling it only from code paths like drivers that already
> > > > > assumed that the page is a pp page and did page->pp deference without
> > > > > a check, so this should be safe.
> > > > >
> > > > > Only thing I would change is add a comment explaining that the calling
> > > > > code needs to check the page is pp page or know it's a pp page (like a
> > > > > driver that supports pp).
> > > > >
> > > > >
> > > > > > >  /**
> > > > > > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > > > > > >   * @netmem: netmem reference to get the pointer from
> > > > > > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > > >   */
> > > > > > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > > > > > >  {
> > > > > > > -       return __netmem_to_page(netmem)->pp;
> > > > > > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > > > > > >  }
> > > > > >
> > > > > > This makes me very sad. Casting from netmem -> page -> nmdesc...
> > > > > >
> > > > > > Instead, we should be able to go from netmem directly to nmdesc. I
> > > > > > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > > > > > it return netmem_desc instead of net_iov. Then use it here.
> > > > > >
> > > > > > We could have an unsafe version of netmem_to_nmdesc which converts the
> > > > > > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > > > > >
> > > > >
> > > > > This, I think, we should address to keep some sanity in the code and
> > > > > reduce the casts and make it a bit more maintainable.
> > > >
> > > > I will reflect your suggestions.  To summarize:
> > > >
> > > >    1) The current implementation of pp_page_to_nmdesc() is good enough
> > > >       to keep, but add a comment on it like "Check if the page is a pp
> > > >       page before calling this function or know it's a pp page.".
> > > >
> > >
> > > Yes please.
> > >
> > > >    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
> > > >       __netmem_get_pp().
> > > >
> > >
> > > No need following Pavel's feedback. We can just delete
> > > __netmem_get_pp. If we do find a need in the future to extract the
> > > netmem_desc from a netmem_ref, I would rather we do a straight cast
> > > from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
> > > -> netmem_desc.
> > >
> > > But that seems unnecessary for this series.
> >
> > No.  The series should remove accessing ->pp through page.
> >
> > I will kill __netmem_get_pp() as you and I prefer.  However,
> > __netmem_get_pp() users e.i. libeth_xdp_return_va() and
> > libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
> >
> > as is: __netmem_get_pp(netmem)
> > to be: __netmem_nmdesc(netmem)->pp
> >
> > Is it okay with you?
> >
> 
> When Pavel and I were saying 'remove __netmem_get_pp', I think we
> meant to remove the entire concept of unsafe netmem -> page
> conversions. I think we both don't like them. From this perspective,
> __netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).
> 
> I think since the unsafe netmem-to-page casts are already in mainline,
> lets assume they should stay there until someone feels strongly enough
> to remove them. The logic in Olek's patch is sound:
> 
> https://lore.kernel.org/all/20241203173733.3181246-8-aleksander.lobakin@intel.com/
> 
> Header buffer page pools do always use pages and will likely remain so
> for a long time, so I guess lets continue to support them rather than
> try to remove them in this series. A followup series could try to
> remove them.
> 
> > > >    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
> > > >       netmem_desc, and use it in all users of __netmem_clear_lsb().
> > > >
> > >
> > > Following Pavel's comment, this I think also is not necessary for this
> > > series. Cleaning up the return value of __netmem_clear_lsb is good
> > > work I think, but we're already on v10 of this and I think it would
> > > unnecessary to ask for added cleanups. We can do the cleanup on top.
> >
> > However, I still need to include 'introduce __netmem_nmdesc() helper'
> 
> Yes.
> 
> > in this series since it should be used to remove __netmem_get_pp() as I
> 
> lets keep __netmem_get_pp, which does a `return
> __netmem_nmdesc(netmem)->pp;` In general we avoid allowing the driver
> to do any netmem casts in the driver code, and we do any casting in
> core.
> 
> > described above.  I think I'd better add netmem_nmdesc() too while at it.
> >
> 
> Yes. netmem_nmdesc should replace __netmem_clear_lsb.

Trivial concern.  I don't think the contraint that the nmdesc must be
the first field in struct net_iov is __unnecessary__.  Thus, I think
netmem_nmdesc() should be something like:

	if (netmem_is_net_iov(netmem))
		return &(struct net_iov *)((__force unsigned long)netmem &
			~NET_IOV)->desc;
	return __netmem_to_nmdesc(netmem);

Do you want to keep the current contraint so that the just casting to
struct netmem_desc after the clearing, can work in netmem_nmdesc()?

	Byungchul

> > I assume __netmem_nmdesc() is an unsafe version not clearing lsb.  The
> 
> Yes.
> 
> > safe version, netmem_nmdesc() needs an additional operation clearing lsb.
> 
> Yes.
> 
> 
> --
> Thanks,
> Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Byungchul Park]

On Wed, Jul 16, 2025 at 12:41:04PM -0700, Mina Almasry wrote:
> On Tue, Jul 15, 2025 at 9:51 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > On Tue, Jul 15, 2025 at 12:09:34PM -0700, Mina Almasry wrote:
> > > On Mon, Jul 14, 2025 at 6:36 PM Byungchul Park <byungchul@sk.com> wrote:
> > > >
> > > > On Mon, Jul 14, 2025 at 12:58:15PM -0700, Mina Almasry wrote:
> > > > > On Mon, Jul 14, 2025 at 12:37 PM Mina Almasry <almasrymina@google.com> wrote:
> > > > > >
> > > > > > On Mon, Jul 14, 2025 at 5:01 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, __netmem_get_pp() still accesses ->pp via struct page.  So
> > > > > > > change it to use struct netmem_desc instead, since ->pp no longer will
> > > > > > > be available in struct page.
> > > > > > >
> > > > > > > While at it, add a helper, pp_page_to_nmdesc(), that can be used to
> > > > > > > extract netmem_desc from page only if it's pp page.  For now that
> > > > > > > netmem_desc overlays on page, it can be achieved by just casting.
> > > > > > >
> > > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > > ---
> > > > > > >  include/net/netmem.h | 13 ++++++++++++-
> > > > > > >  1 file changed, 12 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > > > > index 535cf17b9134..2b8a7b51ac99 100644
> > > > > > > --- a/include/net/netmem.h
> > > > > > > +++ b/include/net/netmem.h
> > > > > > > @@ -267,6 +267,17 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > > >         return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> > > > > > >  }
> > > > > > >
> > > > > > > +static inline struct netmem_desc *pp_page_to_nmdesc(struct page *page)
> > > > > > > +{
> > > > > > > +       DEBUG_NET_WARN_ON_ONCE(!page_pool_page_is_pp(page));
> > > > > > > +
> > > > > > > +       /* XXX: How to extract netmem_desc from page must be changed,
> > > > > > > +        * once netmem_desc no longer overlays on page and will be
> > > > > > > +        * allocated through slab.
> > > > > > > +        */
> > > > > > > +       return (struct netmem_desc *)page;
> > > > > > > +}
> > > > > > > +
> > > > > >
> > > > > > Same thing. Do not create a generic looking pp_page_to_nmdesc helper
> > > > > > which does not check that the page is the correct type. The
> > > > > > DEBUG_NET... is not good enough.
> > > > > >
> > > > > > You don't need to add a generic helper here. There is only one call
> > > > > > site. Open code this in the callsite. The one callsite is marked as
> > > > > > unsafe, only called by code that knows that the netmem is specifically
> > > > > > a pp page. Open code this in the unsafe callsite, instead of creating
> > > > > > a generic looking unsafe helper and not even documenting it's unsafe.
> > > > > >
> > > > >
> > > > > On second read through the series, I actually now think this is a
> > > > > great idea :-) Adding this helper has simplified the series greatly. I
> > > > > did not realize you were converting entire drivers to netmem just to
> > > > > get rid of page->pp accesses. Adding a pp_page_to_nmdesc helper makes
> > > > > the entire series simpler.
> > > > >
> > > > > You're also calling it only from code paths like drivers that already
> > > > > assumed that the page is a pp page and did page->pp deference without
> > > > > a check, so this should be safe.
> > > > >
> > > > > Only thing I would change is add a comment explaining that the calling
> > > > > code needs to check the page is pp page or know it's a pp page (like a
> > > > > driver that supports pp).
> > > > >
> > > > >
> > > > > > >  /**
> > > > > > >   * __netmem_get_pp - unsafely get pointer to the &page_pool backing @netmem
> > > > > > >   * @netmem: netmem reference to get the pointer from
> > > > > > > @@ -280,7 +291,7 @@ static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> > > > > > >   */
> > > > > > >  static inline struct page_pool *__netmem_get_pp(netmem_ref netmem)
> > > > > > >  {
> > > > > > > -       return __netmem_to_page(netmem)->pp;
> > > > > > > +       return pp_page_to_nmdesc(__netmem_to_page(netmem))->pp;
> > > > > > >  }
> > > > > >
> > > > > > This makes me very sad. Casting from netmem -> page -> nmdesc...
> > > > > >
> > > > > > Instead, we should be able to go from netmem directly to nmdesc. I
> > > > > > would suggest rename __netmem_clear_lsb to netmem_to_nmdesc and have
> > > > > > it return netmem_desc instead of net_iov. Then use it here.
> > > > > >
> > > > > > We could have an unsafe version of netmem_to_nmdesc which converts the
> > > > > > netmem to netmem_desc without clearing the lsb and mark it unsafe.
> > > > > >
> > > > >
> > > > > This, I think, we should address to keep some sanity in the code and
> > > > > reduce the casts and make it a bit more maintainable.
> > > >
> > > > I will reflect your suggestions.  To summarize:
> > > >
> > > >    1) The current implementation of pp_page_to_nmdesc() is good enough
> > > >       to keep, but add a comment on it like "Check if the page is a pp
> > > >       page before calling this function or know it's a pp page.".
> > > >
> > >
> > > Yes please.
> > >
> > > >    2) Introduce the unsafe version, __netmem_to_nmdesc(), and use it in
> > > >       __netmem_get_pp().
> > > >
> > >
> > > No need following Pavel's feedback. We can just delete
> > > __netmem_get_pp. If we do find a need in the future to extract the
> > > netmem_desc from a netmem_ref, I would rather we do a straight cast
> > > from netmem_ref to netmem_desc rather than netmem_ref -> pages/net_iov
> > > -> netmem_desc.
> > >
> > > But that seems unnecessary for this series.
> >
> > No.  The series should remove accessing ->pp through page.
> >
> > I will kill __netmem_get_pp() as you and I prefer.  However,
> > __netmem_get_pp() users e.i. libeth_xdp_return_va() and
> > libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
> >
> > as is: __netmem_get_pp(netmem)
> > to be: __netmem_nmdesc(netmem)->pp
> >
> > Is it okay with you?
> >
> 
> When Pavel and I were saying 'remove __netmem_get_pp', I think we
> meant to remove the entire concept of unsafe netmem -> page
> conversions. I think we both don't like them. From this perspective,
> __netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).
> 
> I think since the unsafe netmem-to-page casts are already in mainline,
> lets assume they should stay there until someone feels strongly enough
> to remove them. The logic in Olek's patch is sound:
> 
> https://lore.kernel.org/all/20241203173733.3181246-8-aleksander.lobakin@intel.com/
> 
> Header buffer page pools do always use pages and will likely remain so
> for a long time, so I guess lets continue to support them rather than
> try to remove them in this series. A followup series could try to
> remove them.
> 
> > > >    3) Rename __netmem_clear_lsb() to netmem_to_nmdesc(), and return
> > > >       netmem_desc, and use it in all users of __netmem_clear_lsb().
> > > >
> > >
> > > Following Pavel's comment, this I think also is not necessary for this
> > > series. Cleaning up the return value of __netmem_clear_lsb is good
> > > work I think, but we're already on v10 of this and I think it would
> > > unnecessary to ask for added cleanups. We can do the cleanup on top.
> >
> > However, I still need to include 'introduce __netmem_nmdesc() helper'
> 
> Yes.
> 
> > in this series since it should be used to remove __netmem_get_pp() as I
> 
> lets keep __netmem_get_pp, which does a `return
> __netmem_nmdesc(netmem)->pp;` In general we avoid allowing the driver
> to do any netmem casts in the driver code, and we do any casting in
> core.
> 
> > described above.  I think I'd better add netmem_nmdesc() too while at it.
> >
> 
> Yes. netmem_nmdesc should replace __netmem_clear_lsb.

Even though the unsafe version is required in this series, on second
though, the safe version, netmem_nmdesc() doesn't have to be a part of
this series.  Let's do adding the safe version on top after.

	Byungchul

> > I assume __netmem_nmdesc() is an unsafe version not clearing lsb.  The
> 
> Yes.
> 
> > safe version, netmem_nmdesc() needs an additional operation clearing lsb.
> 
> Yes.
> 
> 
> --
> Thanks,
> Mina

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Pavel Begunkov]

On 7/16/25 20:41, Mina Almasry wrote:
...>> I will kill __netmem_get_pp() as you and I prefer.  However,
>> __netmem_get_pp() users e.i. libeth_xdp_return_va() and
>> libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
>>
>> as is: __netmem_get_pp(netmem)
>> to be: __netmem_nmdesc(netmem)->pp
>>
>> Is it okay with you?
>>
> 
> When Pavel and I were saying 'remove __netmem_get_pp', I think we
> meant to remove the entire concept of unsafe netmem -> page
> conversions. I think we both don't like them. From this perspective,
> __netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).

Yes. It'd great to have all of them gone. IMHO it's much better
to let the caller do the casting so at least it's explicit and
assumptions are not hidden. E.g. instead of

pp = __netmem_nmdesc(netmem)->pp;

It'd be

struct page *page = __netmem_to_page(netmem);

page_to_nmdesc(page)->pp;
// or page_get_pp(page), or whatever the helper is

That might be easier as well.

-- 
Pavel Begunkov

Re: [PATCH net-next v10 02/12] netmem: use netmem_desc instead of page to access ->pp in __netmem_get_pp()

[Pavel Begunkov]

On 7/17/25 10:17, Pavel Begunkov wrote:
> On 7/16/25 20:41, Mina Almasry wrote:
> ...>> I will kill __netmem_get_pp() as you and I prefer.  However,
>>> __netmem_get_pp() users e.i. libeth_xdp_return_va() and
>>> libeth_xdp_tx_fill_buf() should be altered.  I will modify the code like:
>>>
>>> as is: __netmem_get_pp(netmem)
>>> to be: __netmem_nmdesc(netmem)->pp
>>>
>>> Is it okay with you?
>>>
>>
>> When Pavel and I were saying 'remove __netmem_get_pp', I think we
>> meant to remove the entire concept of unsafe netmem -> page
>> conversions. I think we both don't like them. From this perspective,
>> __netmem_nmdesc(netmem)->pp is just as bad as __netmem_get_pp(netmem).
> 
> Yes. It'd great to have all of them gone. IMHO it's much better
> to let the caller do the casting so at least it's explicit and
> assumptions are not hidden. E.g. instead of
> 
> pp = __netmem_nmdesc(netmem)->pp;
> 
> It'd be
> 
> struct page *page = __netmem_to_page(netmem);
> 
> page_to_nmdesc(page)->pp;
> // or page_get_pp(page), or whatever the helper is
> 
> That might be easier as well.

FWIW, no need to respin a v12 for that

-- 
Pavel Begunkov