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

Patch 1: disable dma mapping support for 32-bit arch with 64-bit
         DMA.
Patch 2 & 3: pp page frag tracking support

The small packet drop test show no notiable performance degradation
when page pool is disabled.

V4:
    1. Change error code to EOPNOTSUPP in patch 1.
    2. Drop patch 2.
    3. Use pp_frag_count to indicate if a pp page can be tracked,
       to avoid breaking the mlx5 driver.

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 (3):
  page_pool: disable dma mapping support for 32-bit arch with 64-bit DMA
  page_pool: change BIAS_MAX to support incrementing
  skbuff: keep track of pp page when pp_frag_count is used

 include/linux/mm_types.h | 13 +------------
 include/linux/skbuff.h   | 30 ++++++++++++++++++++----------
 include/net/page_pool.h  | 36 ++++++++++++++++++++++++------------
 net/core/page_pool.c     | 29 +++++++++--------------------
 net/core/skbuff.c        | 10 ++++++++--
 5 files changed, 62 insertions(+), 56 deletions(-)

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

Increment the pp_frag_count of pp page frag in __skb_frag_ref(),
and only use page->pp_magic to indicate a pp page frag in
__skb_frag_unref() to keep track of pp page frag.

Similar handling is done for the head page of a skb too.

As we need the head page of a compound page to decide if it is
from page pool at first, so __page_frag_cache_drain() and
page_ref_inc() is used to avoid unnecessary compound_head()
calling.

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h  | 30 ++++++++++++++++++++----------
 include/net/page_pool.h | 24 +++++++++++++++++++++++-
 net/core/page_pool.c    | 17 ++---------------
 net/core/skbuff.c       | 10 ++++++++--
 4 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 841e2f0f5240..aeee150d4a04 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3073,7 +3073,19 @@ 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);
+
+	page = compound_head(page);
+
+#ifdef CONFIG_PAGE_POOL
+	if (page_pool_is_pp_page(page) &&
+	    page_pool_is_pp_page_frag(page)) {
+		page_pool_atomic_inc_frag_count(page);
+		return;
+	}
+#endif
+
+	page_ref_inc(page);
 }
 
 /**

@@ -3100,11 +3112,16 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
 {
 	struct page *page = skb_frag_page(frag);
 
+	page = compound_head(page);
+
 #ifdef CONFIG_PAGE_POOL
-	if (recycle && page_pool_return_skb_page(page))
+	if (page_pool_is_pp_page(page) &&
+	    (recycle || page_pool_is_pp_page_frag(page))) {
+		page_pool_return_skb_page(page);
 		return;
+	}
 #endif
-	put_page(page);
+	__page_frag_cache_drain(page, 1);
 }
 
 /**

@@ -4718,12 +4735,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)
-{
-	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
-		return false;
-	return page_pool_return_skb_page(virt_to_page(data));
-}
-
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 3855f069627f..740a8ca7f4a6 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,28 @@ 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 bool page_pool_is_pp_page_frag(struct page *page)
+{
+	return !!atomic_long_read(&page->pp_frag_count);
+}
+
 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 2c643b72ce16..d141e00459c9 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -219,6 +219,7 @@ static void page_pool_set_pp_info(struct page_pool *pool,
 {
 	page->pp = pool;
 	page->pp_magic |= PP_SIGNATURE;
+	page_pool_set_frag_count(page, 0);
 }
 
 static void page_pool_clear_pp_info(struct page *page)

@@ -736,22 +737,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 = 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
-	 * 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 +749,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 74601bbc56ac..e3691b025d30 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -646,9 +646,15 @@ 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);
+
+		if (page_pool_is_pp_page(page) &&
+		    (skb->pp_recycle || page_pool_is_pp_page_frag(page))) {
+			page_pool_return_skb_page(page);
 			return;
-		skb_free_frag(head);
+		}
+
+		__page_frag_cache_drain(page, 1);
 	} else {
 		kfree(head);
 	}

-- 
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 when the pp page frag
tracking support is added.

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.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
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..9b60e4301a44 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 -EOPNOTSUPP;
+
 		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 page->pp_frag_count need incrementing for pp page
frag 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 9b60e4301a44..2c643b72ce16 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

On Thu, Sep 30, 2021 at 04:07:47PM +0800, Yunsheng Lin wrote:

There's a BUG_ON we are now ignoring on get_page. 

Same here,  freeing the page is not the only thing put_page does. 

Regardless of the comments above,  providing some numbers on how the
patches affect performance (at least on hns3), would be good to have.

I'll try giving this another look.  I still think having three indicators
to look at before recycling the page is not ideal.


Regards
/Ilias

On 2021/10/4 13:50, Ilias Apalodimas wrote:

On Thu, Sep 30, 2021 at 04:07:47PM +0800, Yunsheng Lin wrote:

quoted

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.

Increment the pp_frag_count of pp page frag in __skb_frag_ref(),
and only use page->pp_magic to indicate a pp page frag in
__skb_frag_unref() to keep track of pp page frag.

Similar handling is done for the head page of a skb too.

As we need the head page of a compound page to decide if it is
from page pool at first, so __page_frag_cache_drain() and
page_ref_inc() is used to avoid unnecessary compound_head()
calling.

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h  | 30 ++++++++++++++++++++----------
 include/net/page_pool.h | 24 +++++++++++++++++++++++-
 net/core/page_pool.c    | 17 ++---------------
 net/core/skbuff.c       | 10 ++++++++--
 4 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 841e2f0f5240..aeee150d4a04 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3073,7 +3073,19 @@ 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);
+
+	page = compound_head(page);
+
+#ifdef CONFIG_PAGE_POOL
+	if (page_pool_is_pp_page(page) &&
+	    page_pool_is_pp_page_frag(page)) {
+		page_pool_atomic_inc_frag_count(page);
+		return;
+	}
+#endif
+
+	page_ref_inc(page);

There's a BUG_ON we are now ignoring on get_page. 

Actually it is a VM_BUG_ON_PAGE(), and it is only turned into a
BUG() while CONFIG_DEBUG_VM is defined.

As there is already tracepoint in page_ref_inc(), I am not sure
VM_BUG_ON_PAGE() is really needed anymore, as there are a few other
place calling page_ref_inc() directly without the VM_BUG_ON_PAGE().

https://elixir.bootlin.com/linux/v5.15-rc4/source/include/linux/page_ref.h#L117

quoted

 }
 
 /**

@@ -3100,11 +3112,16 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
 {
 	struct page *page = skb_frag_page(frag);
 
+	page = compound_head(page);
+
 #ifdef CONFIG_PAGE_POOL
-	if (recycle && page_pool_return_skb_page(page))
+	if (page_pool_is_pp_page(page) &&
+	    (recycle || page_pool_is_pp_page_frag(page))) {
+		page_pool_return_skb_page(page);
 		return;
+	}
 #endif
-	put_page(page);
+	__page_frag_cache_drain(page, 1);

Same here,  freeing the page is not the only thing put_page does. 

I think the __page_frag_cache_drain() has the VM_BUG_ON_PAGE() as
put_page() does.

The one thing I am not sure about it is the devmap managed pages,
which is handled in put_page(), but is not handle in
__page_frag_cache_drain(). Is it possible that devmap managed pages
could be used in the network stack?

quoted

 }
 
 /**

@@ -4718,12 +4735,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)
-{
-	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
-		return false;
-	return page_pool_return_skb_page(virt_to_page(data));
-}
-
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 3855f069627f..740a8ca7f4a6 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,28 @@ 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 bool page_pool_is_pp_page_frag(struct page *page)
+{
+	return !!atomic_long_read(&page->pp_frag_count);
+}
+
 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 2c643b72ce16..d141e00459c9 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -219,6 +219,7 @@ static void page_pool_set_pp_info(struct page_pool *pool,
 {
 	page->pp = pool;
 	page->pp_magic |= PP_SIGNATURE;
+	page_pool_set_frag_count(page, 0);
 }
 
 static void page_pool_clear_pp_info(struct page *page)

@@ -736,22 +737,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 = 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
-	 * 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 +749,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 74601bbc56ac..e3691b025d30 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -646,9 +646,15 @@ 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);
+
+		if (page_pool_is_pp_page(page) &&
+		    (skb->pp_recycle || page_pool_is_pp_page_frag(page))) {
+			page_pool_return_skb_page(page);
 			return;
-		skb_free_frag(head);
+		}
+
+		__page_frag_cache_drain(page, 1);
 	} else {
 		kfree(head);
 	}

-- 
2.33.0

Regardless of the comments above,  providing some numbers on how the
patches affect performance (at least on hns3), would be good to have.

As mentioned in the cover letter:
"The small packet drop test show no notiable performance degradation
when page pool is disabled."

And no notiable performance degradation for the page pool enabled case
with hns3 too.

I'll try giving this another look.  I still think having three indicators
to look at before recycling the page is not ideal.

All three indicators only need to be done when a page has PP_SIGNATURE set,
but do not want to be considered to be a pp page, which seems to be a rare
case?

If the mlx5 driver can change the way of using the page pool as it is now,
we can remove the addtional checking in the future, and just use the pp_magic
to indicate a pp page.

Regards
/Ilias

On 2021/10/4 13:50, Ilias Apalodimas wrote:

On Thu, Sep 30, 2021 at 04:07:47PM +0800, Yunsheng Lin wrote:

quoted

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.

Increment the pp_frag_count of pp page frag in __skb_frag_ref(),
and only use page->pp_magic to indicate a pp page frag in
__skb_frag_unref() to keep track of pp page frag.

Similar handling is done for the head page of a skb too.

As we need the head page of a compound page to decide if it is
from page pool at first, so __page_frag_cache_drain() and
page_ref_inc() is used to avoid unnecessary compound_head()
calling.

Signed-off-by: Yunsheng Lin <redacted>
---
 include/linux/skbuff.h  | 30 ++++++++++++++++++++----------
 include/net/page_pool.h | 24 +++++++++++++++++++++++-
 net/core/page_pool.c    | 17 ++---------------
 net/core/skbuff.c       | 10 ++++++++--
 4 files changed, 53 insertions(+), 28 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 841e2f0f5240..aeee150d4a04 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -3073,7 +3073,19 @@ 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);
+
+	page = compound_head(page);
+
+#ifdef CONFIG_PAGE_POOL
+	if (page_pool_is_pp_page(page) &&
+	    page_pool_is_pp_page_frag(page)) {
+		page_pool_atomic_inc_frag_count(page);
+		return;
+	}
+#endif
+
+	page_ref_inc(page);

There's a BUG_ON we are now ignoring on get_page. 

Actually it is a VM_BUG_ON_PAGE(), and it is only turned into a
BUG() while CONFIG_DEBUG_VM is defined.

As there is already tracepoint in page_ref_inc(), I am not sure
VM_BUG_ON_PAGE() is really needed anymore, as there are a few other
place calling page_ref_inc() directly without the VM_BUG_ON_PAGE().

https://elixir.bootlin.com/linux/v5.15-rc4/source/include/linux/page_ref.h#L117

quoted

 }
 
 /**

@@ -3100,11 +3112,16 @@ static inline void __skb_frag_unref(skb_frag_t *frag, bool recycle)
 {
 	struct page *page = skb_frag_page(frag);
 
+	page = compound_head(page);
+
 #ifdef CONFIG_PAGE_POOL
-	if (recycle && page_pool_return_skb_page(page))
+	if (page_pool_is_pp_page(page) &&
+	    (recycle || page_pool_is_pp_page_frag(page))) {
+		page_pool_return_skb_page(page);
 		return;
+	}
 #endif
-	put_page(page);
+	__page_frag_cache_drain(page, 1);

Same here,  freeing the page is not the only thing put_page does. 

I think the __page_frag_cache_drain() has the VM_BUG_ON_PAGE() as
put_page() does.

The one thing I am not sure about it is the devmap managed pages,
which is handled in put_page(), but is not handle in
__page_frag_cache_drain(). Is it possible that devmap managed pages
could be used in the network stack?

quoted

 }
 
 /**

@@ -4718,12 +4735,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)
-{
-	if (!IS_ENABLED(CONFIG_PAGE_POOL) || !skb->pp_recycle)
-		return false;
-	return page_pool_return_skb_page(virt_to_page(data));
-}
-
 #endif	/* __KERNEL__ */
 #endif	/* _LINUX_SKBUFF_H */

diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 3855f069627f..740a8ca7f4a6 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,28 @@ 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 bool page_pool_is_pp_page_frag(struct page *page)
+{
+	return !!atomic_long_read(&page->pp_frag_count);
+}
+
 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 2c643b72ce16..d141e00459c9 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c

@@ -219,6 +219,7 @@ static void page_pool_set_pp_info(struct page_pool *pool,
 {
 	page->pp = pool;
 	page->pp_magic |= PP_SIGNATURE;
+	page_pool_set_frag_count(page, 0);
 }
 
 static void page_pool_clear_pp_info(struct page *page)

@@ -736,22 +737,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 = 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
-	 * 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 +749,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 74601bbc56ac..e3691b025d30 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -646,9 +646,15 @@ 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);
+
+		if (page_pool_is_pp_page(page) &&
+		    (skb->pp_recycle || page_pool_is_pp_page_frag(page))) {
+			page_pool_return_skb_page(page);
 			return;
-		skb_free_frag(head);
+		}
+
+		__page_frag_cache_drain(page, 1);
 	} else {
 		kfree(head);
 	}

-- 
2.33.0

Regardless of the comments above,  providing some numbers on how the
patches affect performance (at least on hns3), would be good to have.

As mentioned in the cover letter:
"The small packet drop test show no notiable performance degradation
when page pool is disabled."

And no notiable performance degradation for the page pool enabled case
with hns3 too.

I'll try giving this another look.  I still think having three indicators
to look at before recycling the page is not ideal.

All three indicators only need to be done when a page has PP_SIGNATURE set,
but do not want to be considered to be a pp page, which seems to be a rare
case?

If the mlx5 driver can change the way of using the page pool as it is now,
we can remove the addtional checking in the future, and just use the pp_magic
to indicate a pp page.

Regards
/Ilias