[PATCH net-next 0/7] some optimization for page pool

Patch 1: disable dma mapping support for 32-bit arch with 64-bit
         DMA.
Patch 2: support non-split page when PP_FLAG_PAGE_FRAG is set.
patch 3: avoid calling compound_head() for skb frag page
Patch 4-7: use pp_magic to identify pp page uniquely.

V3:
    1. add patch 1/4/6/7.
    2. use pp_magic to identify pp page uniquely too.
    3. avoid unnecessary compound_head() calling.

V2: add patch 2, adjust the commit log accroding to the discussion
    in V1, and fix a compiler error reported by kernel test robot.

Yunsheng Lin (7):
  page_pool: disable dma mapping support for 32-bit arch with 64-bit DMA
  page_pool: support non-split page with PP_FLAG_PAGE_FRAG
  pool_pool: avoid calling compound_head() for skb frag page
  page_pool: change BIAS_MAX to support incrementing
  skbuff: keep track of pp page when __skb_frag_ref() is called
  skbuff: only use pp_magic identifier for a skb' head page
  skbuff: remove unused skb->pp_recycle

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  6 ---
 drivers/net/ethernet/marvell/mvneta.c         |  2 -
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  4 +-
 drivers/net/ethernet/marvell/sky2.c           |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |  2 +-
 drivers/net/ethernet/ti/cpsw.c                |  2 -
 drivers/net/ethernet/ti/cpsw_new.c            |  2 -
 include/linux/mm_types.h                      | 13 +-----
 include/linux/skbuff.h                        | 39 ++++++++----------
 include/net/page_pool.h                       | 31 ++++++++------
 net/core/page_pool.c                          | 40 +++++++------------
 net/core/skbuff.c                             | 36 ++++++-----------
 net/tls/tls_device.c                          |  2 +-
 13 files changed, 67 insertions(+), 114 deletions(-)

-- 
2.33.0

As the 32-bit arch with 64-bit DMA seems to rare those days,
and page pool is carrying a lot of code and complexity for
systems that possibly don't exist.

So disable dma mapping support for such systems, if drivers
really want to work on such systems, they have to implement
their own DMA-mapping fallback tracking outside page_pool.

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/mm_types.h | 13 +------------
 include/net/page_pool.h  | 12 +-----------
 net/core/page_pool.c     | 10 ++++++----
 3 files changed, 8 insertions(+), 27 deletions(-)

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 7f8ee09c711f..436e0946d691 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h

@@ -104,18 +104,7 @@ struct page {
 			struct page_pool *pp;
 			unsigned long _pp_mapping_pad;
 			unsigned long dma_addr;
-			union {
-				/**
-				 * dma_addr_upper: might require a 64-bit
-				 * value on 32-bit architectures.
-				 */
-				unsigned long dma_addr_upper;
-				/**
-				 * For frag page support, not supported in
-				 * 32-bit architectures with 64-bit DMA.
-				 */
-				atomic_long_t pp_frag_count;
-			};
+			atomic_long_t pp_frag_count;
 		};
 		struct {	/* slab, slob and slub */
 			union {

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index a4082406a003..3855f069627f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h

@@ -216,24 +216,14 @@ static inline void page_pool_recycle_direct(struct page_pool *pool,
 	page_pool_put_full_page(pool, page, true);
 }
 
-#define PAGE_POOL_DMA_USE_PP_FRAG_COUNT	\
-		(sizeof(dma_addr_t) > sizeof(unsigned long))
-
 static inline dma_addr_t page_pool_get_dma_addr(struct page *page)
 {
-	dma_addr_t ret = page->dma_addr;
-
-	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
-		ret |= (dma_addr_t)page->dma_addr_upper << 16 << 16;
-
-	return ret;
+	return page->dma_addr;
 }
 
 static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
 {
 	page->dma_addr = addr;
-	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
-		page->dma_addr_upper = upper_32_bits(addr);
 }
 
 static inline void page_pool_set_frag_count(struct page *page, long nr)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
 	 * which is the XDP_TX use-case.
 	 */
 	if (pool->p.flags & PP_FLAG_DMA_MAP) {
+		/* DMA-mapping is not supported on 32-bit systems with
+		 * 64-bit DMA mapping.
+		 */
+		if (sizeof(dma_addr_t) > sizeof(unsigned long))
+			return -EINVAL;
+
 		if ((pool->p.dma_dir != DMA_FROM_DEVICE) &&
 		    (pool->p.dma_dir != DMA_BIDIRECTIONAL))
 			return -EINVAL;

@@ -69,10 +75,6 @@ static int page_pool_init(struct page_pool *pool,
 		 */
 	}
 
-	if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT &&
-	    pool->p.flags & PP_FLAG_PAGE_FRAG)
-		return -EINVAL;
-
 	if (ptr_ring_init(&pool->ring, ring_qsize, GFP_KERNEL) < 0)
 		return -ENOMEM;
 

-- 
2.33.0

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 2 +-
 net/core/page_pool.c   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6bdb0db3e825..35eebc2310a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
 	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
 		return false;
-	return page_pool_return_skb_page(virt_to_page(data));
+	return page_pool_return_skb_page(virt_to_head_page(data));
 }
 
 #endif	/* __KERNEL__ */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f7e71dcb6a2e..357fb53343a0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
 {
 	struct page_pool *pp;
 
-	page = compound_head(page);
-
 	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
 	 * in order to preserve any existing bits, such as bit 0 for the
 	 * head page of compound page and bit 1 for pfmemalloc page, so

-- 
2.33.0

As the page->pp_frag_count need incrementing for pp page
tracking support, so change BIAS_MAX to (LONG_MAX / 2) to
avoid overflowing.

Signed-off-by: Yunsheng Lin <redacted>
---
 net/core/page_pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 357fb53343a0..e9516477f9d2 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -24,7 +24,7 @@
 #define DEFER_TIME (msecs_to_jiffies(1000))
 #define DEFER_WARN_INTERVAL (60 * HZ)
 
-#define BIAS_MAX	LONG_MAX
+#define BIAS_MAX	(LONG_MAX / 2)
 
 static int page_pool_init(struct page_pool *pool,
 			  const struct page_pool_params *params)

-- 
2.33.0

As pp_magic is used to identify pp page for a skb's frag page
in previous patch, so do the similar handling for the head
page of a skb too.

And it seems head to frag converting for a skb during GRO and
GSO processing does not need handling when using pp_magic to
identify pp page for a skb' head page and frag page, see
NAPI_GRO_FREE_STOLEN_HEAD for GRO in skb_gro_receive() and
skb_head_frag_to_page_desc() for GSO in skb_segment().

As pp_magic only exist in the head page of a compound page,
and the freeing of a head page for a skb is eventually operated
on the head page of a compound page for both pp and non-pp
page, so use virt_to_head_page() and __page_frag_cache_drain()
in skb_free_head() to avoid unnecessary virt_to_head_page()
calling in page_frag_free().

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 15 ---------------
 net/core/skbuff.c      | 11 +++++++++--
 2 files changed, 9 insertions(+), 17 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a2d3b6fe0c32..b77ee060b64d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4728,20 +4728,5 @@ static inline void skb_mark_for_recycle(struct sk_buff *skb)
 }
 #endif
 
-static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
-{
-	struct page *page;
-
-	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
-		return false;
-
-	page = virt_to_head_page(data);
-	if (!page_pool_is_pp_page(page))
-		return false;
-
-	page_pool_return_skb_page(page);
-	return true;
-}
-
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index db8af3eff255..3718898da499 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -647,9 +647,16 @@ static void skb_free_head(struct sk_buff *skb)
 	unsigned char *head = skb->head;
 
 	if (skb->head_frag) {
-		if (skb_pp_recycle(skb, head))
+		struct page *page = virt_to_head_page(head);
+
+#ifdef CONFIG_PAGE_POOL
+		if (page_pool_is_pp_page(page)) {
+			page_pool_return_skb_page(page);
 			return;
-		skb_free_frag(head);
+		}
+#endif
+
+		__page_frag_cache_drain(page, 1);
 	} else {
 		kfree(head);
 	}

-- 
2.33.0

As we have used pp_magic to identify pp page for the head
and frag page of a skb, the skb->pp_recycle is not used, so
remove it.

Signed-off-by: Yunsheng Lin <redacted>
---
 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  6 ------
 drivers/net/ethernet/marvell/mvneta.c         |  2 --
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  4 +---
 drivers/net/ethernet/ti/cpsw.c                |  2 --
 drivers/net/ethernet/ti/cpsw_new.c            |  2 --
 include/linux/skbuff.h                        | 12 +----------
 net/core/skbuff.c                             | 21 +------------------
 7 files changed, 3 insertions(+), 46 deletions(-)

diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
index 22af3d6ce178..5331e0f2cee4 100644
--- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c

@@ -3864,9 +3864,6 @@ static int hns3_alloc_skb(struct hns3_enet_ring *ring, unsigned int length,
 		return 0;
 	}
 
-	if (ring->page_pool)
-		skb_mark_for_recycle(skb);
-
 	u64_stats_update_begin(&ring->syncp);
 	ring->stats.seg_pkt_cnt++;
 	u64_stats_update_end(&ring->syncp);

@@ -3906,9 +3903,6 @@ static int hns3_add_frag(struct hns3_enet_ring *ring)
 				return -ENXIO;
 			}
 
-			if (ring->page_pool)
-				skb_mark_for_recycle(new_skb);
-
 			ring->frag_num = 0;
 
 			if (ring->tail_skb) {

diff --git a/drivers/net/ethernet/marvell/mvneta.c b/drivers/net/ethernet/marvell/mvneta.c
index 9d460a270601..c852e0dd6d38 100644
--- a/drivers/net/ethernet/marvell/mvneta.c
+++ b/drivers/net/ethernet/marvell/mvneta.c

@@ -2327,8 +2327,6 @@ mvneta_swbm_build_skb(struct mvneta_port *pp, struct page_pool *pool,
 	if (!skb)
 		return ERR_PTR(-ENOMEM);
 
-	skb_mark_for_recycle(skb);
-
 	skb_reserve(skb, xdp->data - xdp->data_hard_start);
 	skb_put(skb, xdp->data_end - xdp->data);
 	skb->ip_summed = mvneta_rx_csum(pp, desc_status);

diff --git a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
index d5c92e43f89e..bacae115c6c6 100644
--- a/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c
+++ b/drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c

@@ -3994,9 +3994,7 @@ static int mvpp2_rx(struct mvpp2_port *port, struct napi_struct *napi,
 			goto err_drop_frame;
 		}
 
-		if (pp)
-			skb_mark_for_recycle(skb);
-		else
+		if (!pp)
 			dma_unmap_single_attrs(dev->dev.parent, dma_addr,
 					       bm_pool->buf_size, DMA_FROM_DEVICE,
 					       DMA_ATTR_SKIP_CPU_SYNC);

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index 66f7ddd9b1f9..2fb5a4545b8b 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c

@@ -430,8 +430,6 @@ static void cpsw_rx_handler(void *token, int len, int status)
 		cpts_rx_timestamp(cpsw->cpts, skb);
 	skb->protocol = eth_type_trans(skb, ndev);
 
-	/* mark skb for recycling */
-	skb_mark_for_recycle(skb);
 	netif_receive_skb(skb);
 
 	ndev->stats.rx_bytes += len;

diff --git a/drivers/net/ethernet/ti/cpsw_new.c b/drivers/net/ethernet/ti/cpsw_new.c
index 7968f24d99c8..1e74d484852d 100644
--- a/drivers/net/ethernet/ti/cpsw_new.c
+++ b/drivers/net/ethernet/ti/cpsw_new.c

@@ -374,8 +374,6 @@ static void cpsw_rx_handler(void *token, int len, int status)
 		cpts_rx_timestamp(cpsw->cpts, skb);
 	skb->protocol = eth_type_trans(skb, ndev);
 
-	/* mark skb for recycling */
-	skb_mark_for_recycle(skb);
 	netif_receive_skb(skb);
 
 	ndev->stats.rx_bytes += len;

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b77ee060b64d..d4bb0e160fef 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -668,8 +668,6 @@ typedef unsigned char *sk_buff_data_t;
  *	@head_frag: skb was allocated from page fragments,
  *		not allocated by kmalloc() or vmalloc().
  *	@pfmemalloc: skbuff was allocated from PFMEMALLOC reserves
- *	@pp_recycle: mark the packet for recycling instead of freeing (implies
- *		page_pool support on driver)
  *	@active_extensions: active extensions (skb_ext_id types)
  *	@ndisc_nodetype: router type (from link layer)
  *	@ooo_okay: allow the mapping of a socket to a queue to be changed

@@ -795,8 +793,7 @@ struct sk_buff {
 				fclone:2,
 				peeked:1,
 				head_frag:1,
-				pfmemalloc:1,
-				pp_recycle:1; /* page_pool recycle indicator */
+				pfmemalloc:1;
 #ifdef CONFIG_SKB_EXTENSIONS
 	__u8			active_extensions;
 #endif

@@ -4721,12 +4718,5 @@ static inline u64 skb_get_kcov_handle(struct sk_buff *skb)
 #endif
 }
 
-#ifdef CONFIG_PAGE_POOL
-static inline void skb_mark_for_recycle(struct sk_buff *skb)
-{
-	skb->pp_recycle = 1;
-}
-#endif
-
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 3718898da499..85ae59f4349a 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -670,7 +670,7 @@ static void skb_release_data(struct sk_buff *skb)
 	if (skb->cloned &&
 	    atomic_sub_return(skb->nohdr ? (1 << SKB_DATAREF_SHIFT) + 1 : 1,
 			      &shinfo->dataref))
-		goto exit;
+		return;
 
 	skb_zcopy_clear(skb, true);
 

@@ -681,17 +681,6 @@ static void skb_release_data(struct sk_buff *skb)
 		kfree_skb_list(shinfo->frag_list);
 
 	skb_free_head(skb);
-exit:
-	/* When we clone an SKB we copy the reycling bit. The pp_recycle
-	 * bit is only set on the head though, so in order to avoid races
-	 * while trying to recycle fragments on __skb_frag_unref() we need
-	 * to make one SKB responsible for triggering the recycle path.
-	 * So disable the recycling bit if an SKB is cloned and we have
-	 * additional references to to the fragmented part of the SKB.
-	 * Eventually the last SKB will have the recycling bit set and it's
-	 * dataref set to 0, which will trigger the recycling
-	 */
-	skb->pp_recycle = 0;
 }
 
 /*

@@ -1073,7 +1062,6 @@ static struct sk_buff *__skb_clone(struct sk_buff *n, struct sk_buff *skb)
 	n->nohdr = 0;
 	n->peeked = 0;
 	C(pfmemalloc);
-	C(pp_recycle);
 	n->destructor = NULL;
 	C(tail);
 	C(end);

@@ -5368,13 +5356,6 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	if (skb_cloned(to))
 		return false;
 
-	/* The page pool signature of struct page will eventually figure out
-	 * which pages can be recycled or not but for now let's prohibit slab
-	 * allocated and page_pool allocated SKBs from being coalesced.
-	 */
-	if (to->pp_recycle != from->pp_recycle)
-		return false;
-
 	if (len <= skb_tailroom(to)) {
 		if (len)
 			BUG_ON(skb_copy_bits(from, 0, skb_put(to, len), len));

-- 
2.33.0

Currently when PP_FLAG_PAGE_FRAG is set, the caller is not
expected to call page_pool_alloc_pages() directly because of
the PP_FLAG_PAGE_FRAG checking in __page_pool_put_page().

The patch removes the above checking to enable non-split page
support when PP_FLAG_PAGE_FRAG is set.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Yunsheng Lin <redacted>
---
 net/core/page_pool.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a65bd7972e37..f7e71dcb6a2e 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -315,11 +315,14 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
 
 	/* Fast-path: Get a page from cache */
 	page = __page_pool_get_cached(pool);
-	if (page)
-		return page;
 
 	/* Slow-path: cache empty, do real allocation */
-	page = __page_pool_alloc_pages_slow(pool, gfp);
+	if (!page)
+		page = __page_pool_alloc_pages_slow(pool, gfp);
+
+	if (likely(page))
+		page_pool_set_frag_count(page, 1);
+
 	return page;
 }
 EXPORT_SYMBOL(page_pool_alloc_pages);

@@ -428,8 +431,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
 		     unsigned int dma_sync_size, bool allow_direct)
 {
 	/* It is not the last user for the page frag case */
-	if (pool->p.flags & PP_FLAG_PAGE_FRAG &&
-	    page_pool_atomic_sub_frag_count_return(page, 1))
+	if (page_pool_atomic_sub_frag_count_return(page, 1))
 		return NULL;
 
 	/* This allocator is optimized for the XDP mode that uses

-- 
2.33.0

As the skb->pp_recycle and page->pp_magic may not be enough
to track if a frag page is from page pool after the calling
of __skb_frag_ref(), mostly because of a data race, see:
commit 2cc3aeb5eccc ("skbuff: Fix a potential race while
recycling page_pool packets").

There may be clone and expand head case that might lose the
track if a frag page is from page pool or not.

And not being able to keep track of pp page may cause problem
for the skb_split() case in tso_fragment() too:
Supposing a skb has 3 frag pages, all coming from a page pool,
and is split to skb1 and skb2:
skb1: first frag page + first half of second frag page
skb2: second half of second frag page + third frag page

How do we set the skb->pp_recycle of skb1 and skb2?
1. If we set both of them to 1, then we may have a similar
   race as the above commit for second frag page.
2. If we set only one of them to 1, then we may have resource
   leaking problem as both first frag page and third frag page
   are indeed from page pool.

So increment the frag count when __skb_frag_ref() is called,
and only use page->pp_magic to indicate if a frag page is from
page pool, to avoid the above data race.

Signed-off-by: Yunsheng Lin <redacted>
---
 drivers/net/ethernet/marvell/sky2.c        |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c |  2 +-
 include/linux/skbuff.h                     | 30 +++++++++++++++++-----
 include/net/page_pool.h                    | 19 +++++++++++++-
 net/core/page_pool.c                       | 14 +---------
 net/core/skbuff.c                          |  4 +--
 net/tls/tls_device.c                       |  2 +-
 7 files changed, 48 insertions(+), 25 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index e9fc74e54b22..91b62f468a26 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c

@@ -2503,7 +2503,7 @@ static void skb_put_frags(struct sk_buff *skb, unsigned int hdr_space,
 
 		if (length == 0) {
 			/* don't need this page */
-			__skb_frag_unref(frag, false);
+			__skb_frag_unref(frag);
 			--skb_shinfo(skb)->nr_frags;
 		} else {
 			size = min(length, (unsigned) PAGE_SIZE);

diff --git a/drivers/net/ethernet/mellanox/mlx4/en_rx.c b/drivers/net/ethernet/mellanox/mlx4/en_rx.c
index 7f6d3b82c29b..ce31b1fd554f 100644
--- a/drivers/net/ethernet/mellanox/mlx4/en_rx.c
+++ b/drivers/net/ethernet/mellanox/mlx4/en_rx.c

@@ -526,7 +526,7 @@ static int mlx4_en_complete_rx_desc(struct mlx4_en_priv *priv,
 fail:
 	while (nr > 0) {
 		nr--;
-		__skb_frag_unref(skb_shinfo(skb)->frags + nr, false);
+		__skb_frag_unref(skb_shinfo(skb)->frags + nr);
 	}
 	return 0;
 }

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 35eebc2310a5..a2d3b6fe0c32 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3073,7 +3073,16 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
  */
 static inline void __skb_frag_ref(skb_frag_t *frag)
 {
-	get_page(skb_frag_page(frag));
+	struct page *page = skb_frag_page(frag);
+
+#ifdef CONFIG_PAGE_POOL
+	if (page_pool_is_pp_page(page)) {
+		page_pool_atomic_inc_frag_count(page);
+		return;
+	}
+#endif
+
+	get_page(page);
 }
 
 /**

@@ -3091,18 +3100,19 @@ static inline void skb_frag_ref(struct sk_buff *skb, int f)
 /**
  * __skb_frag_unref - release a reference on a paged fragment.
  * @frag: the paged fragment
- * @recycle: recycle the page if allocated via page_pool
  *
  * Releases a reference on the paged fragment @frag
  * or recycles the page via the page_pool API.
  */
-static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
+static inline void __skb_frag_unref(skb_frag_t *frag)
 {
 	struct page *page = skb_frag_page(frag);
 
 #ifdef CONFIG_PAGE_POOL
-	if (recycle && page_pool_return_skb_page(page))
+	if (page_pool_is_pp_page(page)) {
+		page_pool_return_skb_page(page);
 		return;
+	}
 #endif
 	put_page(page);
 }

@@ -3116,7 +3126,7 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
  */
 static inline void skb_frag_unref(struct sk_buff *skb, int f)
 {
-	__skb_frag_unref(&skb_shinfo(skb)->frags[f], skb->pp_recycle);
+	__skb_frag_unref(&skb_shinfo(skb)->frags[f]);
 }
 
 /**

@@ -4720,9 +4730,17 @@ static inline void skb_mark_for_recycle(struct sk_buff *skb)
 
 static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
+	struct page *page;
+
 	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
 		return false;
-	return page_pool_return_skb_page(virt_to_head_page(data));
+
+	page = virt_to_head_page(data);
+	if (!page_pool_is_pp_page(page))
+		return false;
+
+	page_pool_return_skb_page(page);
+	return true;
 }
 
 #endif	/* __KERNEL__ */

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 3855f069627f..f9738da37d9f 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h

@@ -164,7 +164,7 @@ inline enum dma_data_direction page_pool_get_dma_dir(struct page_pool *pool)
 	return pool->p.dma_dir;
 }
 
-bool page_pool_return_skb_page(struct page *page);
+void page_pool_return_skb_page(struct page *page);
 
 struct page_pool *page_pool_create(const struct page_pool_params *params);
 

@@ -231,6 +231,23 @@ static inline void page_pool_set_frag_count(struct page *page, long nr)
 	atomic_long_set(&page->pp_frag_count, nr);
 }
 
+static inline void page_pool_atomic_inc_frag_count(struct page *page)
+{
+	atomic_long_inc(&page->pp_frag_count);
+}
+
+static inline bool page_pool_is_pp_page(struct page *page)
+{
+	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
+	 * in order to preserve any existing bits, such as bit 0 for the
+	 * head page of compound page and bit 1 for pfmemalloc page, so
+	 * mask those bits for freeing side when doing below checking,
+	 * and page_is_pfmemalloc() is checked in __page_pool_put_page()
+	 * to avoid recycling the pfmemalloc page.
+	 */
+	return (page->pp_magic & ~0x3UL) == PP_SIGNATURE;
+}
+
 static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
 							  long nr)
 {

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index e9516477f9d2..96a28accce0e 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -738,20 +738,10 @@ void page_pool_update_nid(struct page_pool *pool, int new_nid)
 }
 EXPORT_SYMBOL(page_pool_update_nid);
 
-bool page_pool_return_skb_page(struct page *page)
+void page_pool_return_skb_page(struct page *page)
 {
 	struct page_pool *pp;
 
-	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
-	 * in order to preserve any existing bits, such as bit 0 for the
-	 * head page of compound page and bit 1 for pfmemalloc page, so
-	 * mask those bits for freeing side when doing below checking,
-	 * and page_is_pfmemalloc() is checked in __page_pool_put_page()
-	 * to avoid recycling the pfmemalloc page.
-	 */
-	if (unlikely((page->pp_magic & ~0x3UL) != PP_SIGNATURE))
-		return false;
-
 	pp = page->pp;
 
 	/* Driver set this to memory recycling info. Reset it on recycle.

@@ -760,7 +750,5 @@ bool page_pool_return_skb_page(struct page *page)
 	 * 'flipped' fragment being in use or not.
 	 */
 	page_pool_put_full_page(pp, page, false);
-
-	return true;
 }
 EXPORT_SYMBOL(page_pool_return_skb_page);

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 2170bea2c7de..db8af3eff255 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -668,7 +668,7 @@ static void skb_release_data(struct sk_buff *skb)
 	skb_zcopy_clear(skb, true);
 
 	for (i = 0; i < shinfo->nr_frags; i++)
-		__skb_frag_unref(&shinfo->frags[i], skb->pp_recycle);
+		__skb_frag_unref(&shinfo->frags[i]);
 
 	if (shinfo->frag_list)
 		kfree_skb_list(shinfo->frag_list);

@@ -3563,7 +3563,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 		fragto = &skb_shinfo(tgt)->frags[merge];
 
 		skb_frag_size_add(fragto, skb_frag_size(fragfrom));
-		__skb_frag_unref(fragfrom, skb->pp_recycle);
+		__skb_frag_unref(fragfrom);
 	}
 
 	/* Reposition in the original skb */

diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index b932469ee69c..bd9f1567aa39 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c

@@ -128,7 +128,7 @@ static void destroy_record(struct tls_record_info *record)
 	int i;
 
 	for (i = 0; i < record->num_frags; i++)
-		__skb_frag_unref(&record->frags[i], false);
+		__skb_frag_unref(&record->frags[i]);
 	kfree(record);
 }
 

-- 
2.33.0

Hi Yunsheng, 

On Wed, Sep 22, 2021 at 05:41:24PM +0800, Yunsheng Lin wrote:

Patch 1: disable dma mapping support for 32-bit arch with 64-bit
         DMA.
Patch 2: support non-split page when PP_FLAG_PAGE_FRAG is set.
patch 3: avoid calling compound_head() for skb frag page
Patch 4-7: use pp_magic to identify pp page uniquely.

There's some subtle changes in this patchset that might affect XDP.

What I forgot when I proposed removing the recycling bit,  is that it also
serves as an 'opt-in' mechanism for drivers that want to use page_pool but 
do the recycling internally.  With that removed we need to make sure
nothing bad happens to them.  In theory the page refcnt for mlx5
specifically will be elevated, so we'll just end up unmapping the buffer.
Arguably we could add a similar mechanism internally into page pool,  
which would allow us to enable and disable recycling,  but that's
an extra if per packet allocation and I don't know if we want that on the XDP 
case.
A few numbers pre/post patch for XDP would help, but iirc hns3 doesn't have
XDP support yet?

It's plumbers week so I'll do some testing starting Monday.

Thanks
/Ilias

V3:
    1. add patch 1/4/6/7.
    2. use pp_magic to identify pp page uniquely too.
    3. avoid unnecessary compound_head() calling.

V2: add patch 2, adjust the commit log accroding to the discussion
    in V1, and fix a compiler error reported by kernel test robot.

Yunsheng Lin (7):
  page_pool: disable dma mapping support for 32-bit arch with 64-bit DMA
  page_pool: support non-split page with PP_FLAG_PAGE_FRAG
  pool_pool: avoid calling compound_head() for skb frag page
  page_pool: change BIAS_MAX to support incrementing
  skbuff: keep track of pp page when __skb_frag_ref() is called
  skbuff: only use pp_magic identifier for a skb' head page
  skbuff: remove unused skb->pp_recycle

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  6 ---
 drivers/net/ethernet/marvell/mvneta.c         |  2 -
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  4 +-
 drivers/net/ethernet/marvell/sky2.c           |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |  2 +-
 drivers/net/ethernet/ti/cpsw.c                |  2 -
 drivers/net/ethernet/ti/cpsw_new.c            |  2 -
 include/linux/mm_types.h                      | 13 +-----
 include/linux/skbuff.h                        | 39 ++++++++----------
 include/net/page_pool.h                       | 31 ++++++++------
 net/core/page_pool.c                          | 40 +++++++------------
 net/core/skbuff.c                             | 36 ++++++-----------
 net/tls/tls_device.c                          |  2 +-
 13 files changed, 67 insertions(+), 114 deletions(-)

-- 
2.33.0

On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ? 
None of the current netstack code assumes bv_page is the head page of a 
compound page.  Since our page_pool allocator can will allocate compound
pages for order > 0,  why should we rely on it ?

Thanks
/Ilias

(+cc Matthew) but this looks safe to me.

On Wed, 22 Sept 2021 at 12:43, Yunsheng Lin [off-list ref] wrote:

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

On 22/09/2021 11.41, Yunsheng Lin wrote:

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g. 
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

-Jesper

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

I am fine with both.  In any case though the aforementioned driver can
just remove PP_FLAG_DMA_MAP and do it's own mappings.

Regards
/Ilias

-Jesper

On 2021/9/23 15:07, Ilias Apalodimas wrote:

Hi Yunsheng, 

On Wed, Sep 22, 2021 at 05:41:24PM +0800, Yunsheng Lin wrote:

quoted

Patch 1: disable dma mapping support for 32-bit arch with 64-bit
         DMA.
Patch 2: support non-split page when PP_FLAG_PAGE_FRAG is set.
patch 3: avoid calling compound_head() for skb frag page
Patch 4-7: use pp_magic to identify pp page uniquely.

There's some subtle changes in this patchset that might affect XDP.

What I forgot when I proposed removing the recycling bit,  is that it also
serves as an 'opt-in' mechanism for drivers that want to use page_pool but 
do the recycling internally.  With that removed we need to make sure
nothing bad happens to them.  In theory the page refcnt for mlx5

It seems odd that mlx5 is adding its own page cache on top of page pool,
is it about support both "struct sk_buff" and "struct xdp_buff" for the
same queue?

specifically will be elevated, so we'll just end up unmapping the buffer.
Arguably we could add a similar mechanism internally into page pool,  
which would allow us to enable and disable recycling,  but that's
an extra if per packet allocation and I don't know if we want that on the XDP 
case.

Or we could change mlx5e_rx_cache_get() to check for "page->pp_frag_count
== 1" too, and adjust mlx5e_page_release() accordingly?

A few numbers pre/post patch for XDP would help, but iirc hns3 doesn't have
XDP support yet?

You are right, hns3 doesn't have XDP support yet.

It's plumbers week so I'll do some testing starting Monday.

Thanks
/Ilias

quoted

V3:
    1. add patch 1/4/6/7.
    2. use pp_magic to identify pp page uniquely too.
    3. avoid unnecessary compound_head() calling.

V2: add patch 2, adjust the commit log accroding to the discussion
    in V1, and fix a compiler error reported by kernel test robot.

Yunsheng Lin (7):
  page_pool: disable dma mapping support for 32-bit arch with 64-bit DMA
  page_pool: support non-split page with PP_FLAG_PAGE_FRAG
  pool_pool: avoid calling compound_head() for skb frag page
  page_pool: change BIAS_MAX to support incrementing
  skbuff: keep track of pp page when __skb_frag_ref() is called
  skbuff: only use pp_magic identifier for a skb' head page
  skbuff: remove unused skb->pp_recycle

 .../net/ethernet/hisilicon/hns3/hns3_enet.c   |  6 ---
 drivers/net/ethernet/marvell/mvneta.c         |  2 -
 .../net/ethernet/marvell/mvpp2/mvpp2_main.c   |  4 +-
 drivers/net/ethernet/marvell/sky2.c           |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |  2 +-
 drivers/net/ethernet/ti/cpsw.c                |  2 -
 drivers/net/ethernet/ti/cpsw_new.c            |  2 -
 include/linux/mm_types.h                      | 13 +-----
 include/linux/skbuff.h                        | 39 ++++++++----------
 include/net/page_pool.h                       | 31 ++++++++------
 net/core/page_pool.c                          | 40 +++++++------------
 net/core/skbuff.c                             | 36 ++++++-----------
 net/tls/tls_device.c                          |  2 +-
 13 files changed, 67 insertions(+), 114 deletions(-)

-- 
2.33.0

.

On 2021/9/23 18:02, Ilias Apalodimas wrote:

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

Will change it to EOPNOTSUPP, thanks.

I am fine with both.  In any case though the aforementioned driver can
just remove PP_FLAG_DMA_MAP and do it's own mappings.

Regards
/Ilias

quoted

-Jesper

.

On 2021/9/23 16:33, Ilias Apalodimas wrote:

On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:

quoted

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ? 
None of the current netstack code assumes bv_page is the head page of a 
compound page.  Since our page_pool allocator can will allocate compound
pages for order > 0,  why should we rely on it ?

As the page pool alloc function return 'struct page *' to the caller, which
is the head page of a compound pages for order > 0, so I assume the caller
will pass that to skb_frag_set_page().

For non-pp page, I assume it is ok whether the page is a head page or tail
page, as the pp_magic for both of them are not set with PP_SIGNATURE.

Or should we play safe here, and do the trick as skb_free_head() does in
patch 6?

Thanks
/Ilias

quoted

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 2 +-
 net/core/page_pool.c   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6bdb0db3e825..35eebc2310a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
 	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
 		return false;
-	return page_pool_return_skb_page(virt_to_page(data));
+	return page_pool_return_skb_page(virt_to_head_page(data));
 }
 
 #endif	/* __KERNEL__ */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f7e71dcb6a2e..357fb53343a0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
 {
 	struct page_pool *pp;
 
-	page = compound_head(page);
-
 	/* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
 	 * in order to preserve any existing bits, such as bit 0 for the
 	 * head page of compound page and bit 1 for pfmemalloc page, so

-- 
2.33.0

.

On Thu, 23 Sept 2021 at 14:24, Yunsheng Lin [off-list ref] wrote:

On 2021/9/23 16:33, Ilias Apalodimas wrote:

quoted

On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:

quoted

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ?
None of the current netstack code assumes bv_page is the head page of a
compound page.  Since our page_pool allocator can will allocate compound
pages for order > 0,  why should we rely on it ?

As the page pool alloc function return 'struct page *' to the caller, which
is the head page of a compound pages for order > 0, so I assume the caller
will pass that to skb_frag_set_page().

Yea that's exactly the assumption I was afraid of.
Sure not passing the head page might seem weird atm and the assumption
stands, but the point is we shouldn't blow up the entire network stack
if someone does that eventually.

For non-pp page, I assume it is ok whether the page is a head page or tail
page, as the pp_magic for both of them are not set with PP_SIGNATURE.

Yea that's true, although we removed the checking for coalescing
recyclable and non-recyclable SKBs,   the next patch first checks the
signature before trying to do anything with the skb.

Or should we play safe here, and do the trick as skb_free_head() does in
patch 6?

I don't think the &1 will even be measurable,  so I'd suggest just
dropping this and play safe?

Cheers
/Ilias

quoted

Thanks
/Ilias

quoted

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 2 +-
 net/core/page_pool.c   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6bdb0db3e825..35eebc2310a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
     if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
             return false;
-    return page_pool_return_skb_page(virt_to_page(data));
+    return page_pool_return_skb_page(virt_to_head_page(data));
 }

 #endif      /* __KERNEL__ */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f7e71dcb6a2e..357fb53343a0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
 {
     struct page_pool *pp;

-    page = compound_head(page);
-
     /* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
      * in order to preserve any existing bits, such as bit 0 for the
      * head page of compound page and bit 1 for pfmemalloc page, so
--

2.33.0

.

On 22/09/2021 11.41, Yunsheng Lin wrote:

I really don't like that you add one atomic_long_set operation per page 
alloc call.
This is a fast-path for XDP use-cases, which you are ignoring as you 
drivers doesn't implement XDP.

As I cannot ask you to run XDP benchmarks, I fortunately have some 
page_pool specific microbenchmarks you can run instead.

I will ask you to provide before and after results from running these 
benchmarks [1] and [2].

  [1] 
https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c

  [2] 
https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_cross_cpu.c

How to use these module is documented here[3]:
  [3] 
https://prototype-kernel.readthedocs.io/en/latest/prototype-kernel/build-process.html

This adds an atomic_long_read, even when PP_FLAG_PAGE_FRAG is not set.

  
  	/* This allocator is optimized for the XDP mode that uses

On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:

On 2021/9/23 18:02, Ilias Apalodimas wrote:

quoted

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

Will change it to EOPNOTSUPP, thanks.

Mind sending this one separately (and you can keep my reviewed-by).  It
fits nicely on it's own and since I am not sure about the rest of the
changes yet, it would be nice to get this one in.

Cheers
/Ilias

quoted

I am fine with both.  In any case though the aforementioned driver can
just remove PP_FLAG_DMA_MAP and do it's own mappings.

Regards
/Ilias

quoted

-Jesper

.

On 2021/9/23 21:07, Ilias Apalodimas wrote:

On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:

quoted

On 2021/9/23 18:02, Ilias Apalodimas wrote:

quoted

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

Will change it to EOPNOTSUPP, thanks.

Mind sending this one separately (and you can keep my reviewed-by).  It
fits nicely on it's own and since I am not sure about the rest of the
changes yet, it would be nice to get this one in.

I am not sure sending this one separately really makes sense, as it is
mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
is called" in patch 5 easier.

Cheers
/Ilias

quoted

quoted

I am fine with both.  In any case though the aforementioned driver can
just remove PP_FLAG_DMA_MAP and do it's own mappings.

Regards
/Ilias

quoted

-Jesper

.

.

On 2021/9/23 20:08, Jesper Dangaard Brouer wrote:

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

Currently when PP_FLAG_PAGE_FRAG is set, the caller is not
expected to call page_pool_alloc_pages() directly because of
the PP_FLAG_PAGE_FRAG checking in __page_pool_put_page().

The patch removes the above checking to enable non-split page
support when PP_FLAG_PAGE_FRAG is set.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Yunsheng Lin <redacted>
---
  net/core/page_pool.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a65bd7972e37..f7e71dcb6a2e 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -315,11 +315,14 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
        /* Fast-path: Get a page from cache */
      page = __page_pool_get_cached(pool);
-    if (page)
-        return page;
        /* Slow-path: cache empty, do real allocation */
-    page = __page_pool_alloc_pages_slow(pool, gfp);
+    if (!page)
+        page = __page_pool_alloc_pages_slow(pool, gfp);
+
+    if (likely(page))
+        page_pool_set_frag_count(page, 1);
+

I really don't like that you add one atomic_long_set operation per page alloc call.
This is a fast-path for XDP use-cases, which you are ignoring as you drivers doesn't implement XDP.

As I cannot ask you to run XDP benchmarks, I fortunately have some page_pool specific microbenchmarks you can run instead.

I will ask you to provide before and after results from running these benchmarks [1] and [2].

 [1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c

 [2] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_cross_cpu.c

How to use these module is documented here[3]:
 [3] https://prototype-kernel.readthedocs.io/en/latest/prototype-kernel/build-process.html

Will running these benchmarks to see if any performance overhead noticable here,
thanks for the benchmarks.

quoted

      return page;
  }
  EXPORT_SYMBOL(page_pool_alloc_pages);

@@ -428,8 +431,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
               unsigned int dma_sync_size, bool allow_direct)
  {
      /* It is not the last user for the page frag case */
-    if (pool->p.flags & PP_FLAG_PAGE_FRAG &&
-        page_pool_atomic_sub_frag_count_return(page, 1))
+    if (page_pool_atomic_sub_frag_count_return(page, 1))
          return NULL;

This adds an atomic_long_read, even when PP_FLAG_PAGE_FRAG is not set.

The point here is to have consistent handling for both PP_FLAG_PAGE_FRAG
and non-PP_FLAG_PAGE_FRAG case in the following patch.

As the page->_refcount is accessed in "page_ref_count(page) == 1" checking
in __page_pool_put_page(), and page->pp_frag_count is most likely in the
same cache line as the page->_refcount, So I am not expecting a noticable
overhead here.

Anyway, will use the above benchmarks as an example to verify it.

quoted

        /* This allocator is optimized for the XDP mode that uses

.

On Fri, 24 Sept 2021 at 10:04, Yunsheng Lin [off-list ref] wrote:

On 2021/9/23 21:07, Ilias Apalodimas wrote:

quoted

On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:

quoted

On 2021/9/23 18:02, Ilias Apalodimas wrote:

quoted

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

Will change it to EOPNOTSUPP, thanks.

Mind sending this one separately (and you can keep my reviewed-by).  It
fits nicely on it's own and since I am not sure about the rest of the
changes yet, it would be nice to get this one in.

I am not sure sending this one separately really makes sense, as it is
mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
is called" in patch 5 easier.

It rips out support for devices that are 32bit and have 64bit dma and
make the whole code easier to follow.  I thought we agreed on removing
the support for those devices regardless didn't we?

Regards
/Ilias

On 2021/9/23 19:47, Ilias Apalodimas wrote:

On Thu, 23 Sept 2021 at 14:24, Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/9/23 16:33, Ilias Apalodimas wrote:

quoted

On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:

quoted

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ?
None of the current netstack code assumes bv_page is the head page of a
compound page.  Since our page_pool allocator can will allocate compound
pages for order > 0,  why should we rely on it ?

As the page pool alloc function return 'struct page *' to the caller, which
is the head page of a compound pages for order > 0, so I assume the caller
will pass that to skb_frag_set_page().

Yea that's exactly the assumption I was afraid of.
Sure not passing the head page might seem weird atm and the assumption
stands, but the point is we shouldn't blow up the entire network stack
if someone does that eventually.

quoted

For non-pp page, I assume it is ok whether the page is a head page or tail
page, as the pp_magic for both of them are not set with PP_SIGNATURE.

Yea that's true, although we removed the checking for coalescing
recyclable and non-recyclable SKBs,   the next patch first checks the
signature before trying to do anything with the skb.

quoted

Or should we play safe here, and do the trick as skb_free_head() does in
patch 6?

I don't think the &1 will even be measurable,  so I'd suggest just
dropping this and play safe?

I am not sure what does '&1' mean above.

The one thing I am not sure about the trick done in patch 6 is that
if __page_frag_cache_drain() is right API to use here, I used it because
it is the only API that is expecting a head page.

Cheers
/Ilias

quoted

quoted

Thanks
/Ilias

quoted

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 2 +-
 net/core/page_pool.c   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6bdb0db3e825..35eebc2310a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
     if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
             return false;
-    return page_pool_return_skb_page(virt_to_page(data));
+    return page_pool_return_skb_page(virt_to_head_page(data));
 }

 #endif      /* __KERNEL__ */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f7e71dcb6a2e..357fb53343a0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
 {
     struct page_pool *pp;

-    page = compound_head(page);
-
     /* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
      * in order to preserve any existing bits, such as bit 0 for the
      * head page of compound page and bit 1 for pfmemalloc page, so
--

2.33.0

.

.

On Fri, 24 Sept 2021 at 10:33, Yunsheng Lin [off-list ref] wrote:

On 2021/9/23 19:47, Ilias Apalodimas wrote:

quoted

On Thu, 23 Sept 2021 at 14:24, Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/9/23 16:33, Ilias Apalodimas wrote:

quoted

On Wed, Sep 22, 2021 at 05:41:27PM +0800, Yunsheng Lin wrote:

quoted

As the pp page for a skb frag is always a head page, so make
sure skb_pp_recycle() passes a head page to avoid calling
compound_head() for skb frag page case.

Doesn't that rely on the driver mostly (i.e what's passed in skb_frag_set_page() ?
None of the current netstack code assumes bv_page is the head page of a
compound page.  Since our page_pool allocator can will allocate compound
pages for order > 0,  why should we rely on it ?

As the page pool alloc function return 'struct page *' to the caller, which
is the head page of a compound pages for order > 0, so I assume the caller
will pass that to skb_frag_set_page().

Yea that's exactly the assumption I was afraid of.
Sure not passing the head page might seem weird atm and the assumption
stands, but the point is we shouldn't blow up the entire network stack
if someone does that eventually.

quoted

For non-pp page, I assume it is ok whether the page is a head page or tail
page, as the pp_magic for both of them are not set with PP_SIGNATURE.

Yea that's true, although we removed the checking for coalescing
recyclable and non-recyclable SKBs,   the next patch first checks the
signature before trying to do anything with the skb.

quoted

Or should we play safe here, and do the trick as skb_free_head() does in
patch 6?

I don't think the &1 will even be measurable,  so I'd suggest just
dropping this and play safe?

I am not sure what does '&1' mean above.

I meant the check compound_head() is doing before deciding on the head page.

The one thing I am not sure about the trick done in patch 6 is that
if __page_frag_cache_drain() is right API to use here, I used it because
it is the only API that is expecting a head page.

Yea seemed a bit funny to me in the first place, until I figured out
what exactly it was doing.

Regards
/Ilias

quoted

Cheers
/Ilias

quoted

quoted

Thanks
/Ilias

quoted

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h | 2 +-
 net/core/page_pool.c   | 2 --
 2 files changed, 1 insertion(+), 3 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 6bdb0db3e825..35eebc2310a5 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -4722,7 +4722,7 @@ static inline bool skb_pp_recycle(struct sk_buff *skb, void *data)
 {
     if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
             return false;
-    return page_pool_return_skb_page(virt_to_page(data));
+    return page_pool_return_skb_page(virt_to_head_page(data));
 }

 #endif      /* __KERNEL__ */

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index f7e71dcb6a2e..357fb53343a0 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -742,8 +742,6 @@ bool page_pool_return_skb_page(struct page *page)
 {
     struct page_pool *pp;

-    page = compound_head(page);
-
     /* page->pp_magic is OR'ed with PP_SIGNATURE after the allocation
      * in order to preserve any existing bits, such as bit 0 for the
      * head page of compound page and bit 1 for pfmemalloc page, so
--

2.33.0

.

.

On 2021/9/24 15:25, Ilias Apalodimas wrote:

On Fri, 24 Sept 2021 at 10:04, Yunsheng Lin [off-list ref] wrote:

quoted

On 2021/9/23 21:07, Ilias Apalodimas wrote:

quoted

On Thu, Sep 23, 2021 at 07:13:11PM +0800, Yunsheng Lin wrote:

quoted

On 2021/9/23 18:02, Ilias Apalodimas wrote:

quoted

Hi Jesper,

On Thu, 23 Sept 2021 at 12:33, Jesper Dangaard Brouer
[off-list ref] wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1a6978427d6c..a65bd7972e37 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -49,6 +49,12 @@ static int page_pool_init(struct page_pool *pool,
       * which is the XDP_TX use-case.
       */
      if (pool->p.flags & PP_FLAG_DMA_MAP) {
+             /* DMA-mapping is not supported on 32-bit systems with
+              * 64-bit DMA mapping.
+              */
+             if (sizeof(dma_addr_t) > sizeof(unsigned long))
+                     return -EINVAL;

As I said before, can we please use another error than EINVAL.
We should give drivers a chance/ability to detect this error, and e.g.
fallback to doing DMA mappings inside driver instead.

I suggest using EOPNOTSUPP 95 (Operation not supported).

Will change it to EOPNOTSUPP, thanks.

Mind sending this one separately (and you can keep my reviewed-by).  It
fits nicely on it's own and since I am not sure about the rest of the
changes yet, it would be nice to get this one in.

I am not sure sending this one separately really makes sense, as it is
mainly used to make supporting the "keep track of pp page when __skb_frag_ref()
is called" in patch 5 easier.

It rips out support for devices that are 32bit and have 64bit dma and
make the whole code easier to follow.  I thought we agreed on removing
the support for those devices regardless didn't we?

I am actually not convinced that the code about PAGE_POOL_DMA_USE_PP_FRAG_COUNT
(maybe the name is somewhat confusiong) as it it now, but it is after adding patch
5, and it seems we are not handing the skb_split() case in tso_fragment() for 32bit
arch with 64bit dma too if we still keep PAGE_POOL_DMA_USE_PP_FRAG_COUNT macro.

Regards
/Ilias
.

On 2021/9/24 15:23, Yunsheng Lin wrote:

On 2021/9/23 20:08, Jesper Dangaard Brouer wrote:

quoted

On 22/09/2021 11.41, Yunsheng Lin wrote:

quoted

Currently when PP_FLAG_PAGE_FRAG is set, the caller is not
expected to call page_pool_alloc_pages() directly because of
the PP_FLAG_PAGE_FRAG checking in __page_pool_put_page().

The patch removes the above checking to enable non-split page
support when PP_FLAG_PAGE_FRAG is set.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>
Signed-off-by: Yunsheng Lin <redacted>
---
  net/core/page_pool.c | 12 +++++++-----
  1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index a65bd7972e37..f7e71dcb6a2e 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -315,11 +315,14 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
        /* Fast-path: Get a page from cache */
      page = __page_pool_get_cached(pool);
-    if (page)
-        return page;
        /* Slow-path: cache empty, do real allocation */
-    page = __page_pool_alloc_pages_slow(pool, gfp);
+    if (!page)
+        page = __page_pool_alloc_pages_slow(pool, gfp);
+
+    if (likely(page))
+        page_pool_set_frag_count(page, 1);
+

I really don't like that you add one atomic_long_set operation per page alloc call.
This is a fast-path for XDP use-cases, which you are ignoring as you drivers doesn't implement XDP.

As I cannot ask you to run XDP benchmarks, I fortunately have some page_pool specific microbenchmarks you can run instead.

I will ask you to provide before and after results from running these benchmarks [1] and [2].

 [1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_simple.c

 [2] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/lib/bench_page_pool_cross_cpu.c

How to use these module is documented here[3]:
 [3] https://prototype-kernel.readthedocs.io/en/latest/prototype-kernel/build-process.html

Will running these benchmarks to see if any performance overhead noticable here,
thanks for the benchmarks.

You are right, there is notiable overhead for bench_page_pool_cross_cpu test
case above, possibly due to the cache bouncing caused by page_pool_set_frag_count().

As memntioned by Ilias, mlx5 use page pool and also do the recycling internally,
so handling the page frag tracking consistently for both PP_FLAG_PAGE_FRAG and
non-PP_FLAG_PAGE_FRAG will break the mlx5 driver.

So I will drop this patch for now.

quoted

quoted

      return page;
  }
  EXPORT_SYMBOL(page_pool_alloc_pages);

@@ -428,8 +431,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
               unsigned int dma_sync_size, bool allow_direct)
  {
      /* It is not the last user for the page frag case */
-    if (pool->p.flags & PP_FLAG_PAGE_FRAG &&
-        page_pool_atomic_sub_frag_count_return(page, 1))
+    if (page_pool_atomic_sub_frag_count_return(page, 1))
          return NULL;

This adds an atomic_long_read, even when PP_FLAG_PAGE_FRAG is not set.

The point here is to have consistent handling for both PP_FLAG_PAGE_FRAG
and non-PP_FLAG_PAGE_FRAG case in the following patch.

As the page->_refcount is accessed in "page_ref_count(page) == 1" checking
in __page_pool_put_page(), and page->pp_frag_count is most likely in the
same cache line as the page->_refcount, So I am not expecting a noticable
overhead here.

Anyway, will use the above benchmarks as an example to verify it.

quoted

quoted

        /* This allocator is optimized for the XDP mode that uses

.

_______________________________________________
Linuxarm mailing list -- linuxarm@openeuler.org
To unsubscribe send an email to linuxarm-leave@openeuler.org