[PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref
@ 2024-03-27 21:45 Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 1/3] net: make napi_frag_unref reuse skb_page_unref Mina Almasry
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Mina Almasry @ 2024-03-27 21:45 UTC (permalink / raw)
  To: netdev, linux-kernel, linux-rdma
  Cc: Mina Almasry, Ayush Sawal, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

v2:

- Removed RFC tag.
- Rebased on net-next after the merge window opening.
- Added 1 patch at the beginning, "net: make napi_frag_unref reuse
  skb_page_unref" because a recent patch introduced some code
  duplication that can also be improved.
- Addressed feedback from Dragos & Yunsheng.
- Added Dragos's Reviewed-by.

This series is largely motivated by a recent discussion where there was
some confusion on how to properly ref/unref pp pages vs non pp pages:

https://lore.kernel.org/netdev/CAHS8izOoO-EovwMwAm9tLYetwikNPxC0FKyVGu1TPJWSz4bGoA@mail.gmail.com/T/#t

There is some subtely there because pp uses page->pp_ref_count for
refcounting, while non-pp uses get_page()/put_page() for ref counting.
Getting the refcounting pairs wrong can lead to kernel crash.

Additionally currently it may not be obvious to skb users unaware of
page pool internals how to properly acquire a ref on a pp frag. It
requires checking of skb->pp_recycle & is_pp_page() to make the correct
calls and may require some handling at the call site aware of arguable pp
internals.

This series is a minor refactor with a couple of goals:

1. skb users should be able to ref/unref a frag using
   [__]skb_frag_[un]ref() functions without needing to understand pp
   concepts and pp_ref_count vs get/put_page() differences.

2. reference counting functions should have a mirror opposite. I.e. there
   should be a foo_unref() to every foo_ref() with a mirror opposite
   implementation (as much as possible).

This is RFC to collect feedback if this change is desirable, but also so
that I don't race with the fix for the issue Dragos is seeing for his
crash.

https://lore.kernel.org/lkml/CAHS8izN436pn3SndrzsCyhmqvJHLyxgCeDpWXA4r1ANt3RCDLQ@mail.gmail.com/T/

Cc: Dragos Tatulea <dtatulea@nvidia.com>

Mina Almasry (3):
  net: make napi_frag_unref reuse skb_page_unref
  net: mirror skb frag ref/unref helpers
  net: remove napi_frag_unref

 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c |  2 +-
 drivers/net/ethernet/marvell/sky2.c           |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c    |  2 +-
 drivers/net/ethernet/sun/cassini.c            |  4 +-
 include/linux/skbuff.h                        | 44 +++++++-------
 net/core/skbuff.c                             | 58 ++++++-------------
 net/ipv4/esp4.c                               |  2 +-
 net/ipv6/esp6.c                               |  2 +-
 net/tls/tls_device.c                          |  2 +-
 net/tls/tls_strp.c                            |  2 +-
 10 files changed, 52 insertions(+), 68 deletions(-)

-- 
2.44.0.396.g6e790dbe36-goog

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 1/3] net: make napi_frag_unref reuse skb_page_unref
  2024-03-27 21:45 [PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref Mina Almasry
@ 2024-03-27 21:45 ` Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 3/3] net: remove napi_frag_unref Mina Almasry
  2 siblings, 0 replies; 6+ messages in thread
From: Mina Almasry @ 2024-03-27 21:45 UTC (permalink / raw)
  To: netdev, linux-kernel, linux-rdma
  Cc: Mina Almasry, Ayush Sawal, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

The implementations of these 2 functions are almost identical. Remove
the implementation of napi_frag_unref, and make it a call into
skb_page_unref so we don't duplicate the implementation.

Signed-off-by: Mina Almasry <almasrymina@google.com>

---
 include/linux/skbuff.h | 12 +++---------
 net/ipv4/esp4.c        |  2 +-
 net/ipv6/esp6.c        |  2 +-
 3 files changed, 5 insertions(+), 11 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b945af8a6208..bafa5c9ff59a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3524,10 +3524,10 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
 bool napi_pp_put_page(struct page *page, bool napi_safe);
 
 static inline void
-skb_page_unref(const struct sk_buff *skb, struct page *page, bool napi_safe)
+skb_page_unref(struct page *page, bool recycle, bool napi_safe)
 {
 #ifdef CONFIG_PAGE_POOL
-	if (skb->pp_recycle && napi_pp_put_page(page, napi_safe))
+	if (recycle && napi_pp_put_page(page, napi_safe))
 		return;
 #endif
 	put_page(page);
@@ -3536,13 +3536,7 @@ skb_page_unref(const struct sk_buff *skb, struct page *page, bool napi_safe)
 static inline void
 napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
 {
-	struct page *page = skb_frag_page(frag);
-
-#ifdef CONFIG_PAGE_POOL
-	if (recycle && napi_pp_put_page(page, napi_safe))
-		return;
-#endif
-	put_page(page);
+	skb_page_unref(skb_frag_page(frag), recycle, napi_safe);
 }
 
 /**
diff --git a/net/ipv4/esp4.c b/net/ipv4/esp4.c
index d33d12421814..3d2c252c5570 100644
--- a/net/ipv4/esp4.c
+++ b/net/ipv4/esp4.c
@@ -114,7 +114,7 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb)
 	 */
 	if (req->src != req->dst)
 		for (sg = sg_next(req->src); sg; sg = sg_next(sg))
-			skb_page_unref(skb, sg_page(sg), false);
+			skb_page_unref(sg_page(sg), skb->pp_recycle, false);
 }
 
 #ifdef CONFIG_INET_ESPINTCP
diff --git a/net/ipv6/esp6.c b/net/ipv6/esp6.c
index 7371886d4f9f..4fe4f97f5420 100644
--- a/net/ipv6/esp6.c
+++ b/net/ipv6/esp6.c
@@ -131,7 +131,7 @@ static void esp_ssg_unref(struct xfrm_state *x, void *tmp, struct sk_buff *skb)
 	 */
 	if (req->src != req->dst)
 		for (sg = sg_next(req->src); sg; sg = sg_next(sg))
-			skb_page_unref(skb, sg_page(sg), false);
+			skb_page_unref(sg_page(sg), skb->pp_recycle, false);
 }
 
 #ifdef CONFIG_INET6_ESPINTCP
-- 
2.44.0.396.g6e790dbe36-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers
  2024-03-27 21:45 [PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 1/3] net: make napi_frag_unref reuse skb_page_unref Mina Almasry
@ 2024-03-27 21:45 ` Mina Almasry
  2024-03-28 17:07   ` kernel test robot
  2024-03-28 17:18   ` kernel test robot
  2024-03-27 21:45 ` [PATCH net-next v2 3/3] net: remove napi_frag_unref Mina Almasry
  2 siblings, 2 replies; 6+ messages in thread
From: Mina Almasry @ 2024-03-27 21:45 UTC (permalink / raw)
  To: netdev, linux-kernel, linux-rdma
  Cc: Mina Almasry, Ayush Sawal, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

Refactor some of the skb frag ref/unref helpers for improved clarity.

Implement napi_pp_get_page() to be the mirror counterpart of
napi_pp_put_page().

Implement skb_page_ref() to be the mirror of skb_page_unref().

Improve __skb_frag_ref() to become a mirror counterpart of
__skb_frag_unref(). Previously unref could handle pp & non-pp pages,
while the ref could only handle non-pp pages. Now both the ref & unref
helpers can correctly handle both pp & non-pp pages.

Now that __skb_frag_ref() can handle both pp & non-pp pages, remove
skb_pp_frag_ref(), and use __skb_frag_ref() instead.  This lets us
remove pp specific handling from skb_try_coalesce.

Signed-off-by: Mina Almasry <almasrymina@google.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>

---
 .../chelsio/inline_crypto/ch_ktls/chcr_ktls.c |  2 +-
 drivers/net/ethernet/sun/cassini.c            |  4 +-
 include/linux/skbuff.h                        | 22 ++++++--
 net/core/skbuff.c                             | 54 ++++++-------------
 4 files changed, 38 insertions(+), 44 deletions(-)

diff --git a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
index 6482728794dd..f9b0a9533985 100644
--- a/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
+++ b/drivers/net/ethernet/chelsio/inline_crypto/ch_ktls/chcr_ktls.c
@@ -1658,7 +1658,7 @@ static void chcr_ktls_copy_record_in_skb(struct sk_buff *nskb,
 	for (i = 0; i < record->num_frags; i++) {
 		skb_shinfo(nskb)->frags[i] = record->frags[i];
 		/* increase the frag ref count */
-		__skb_frag_ref(&skb_shinfo(nskb)->frags[i]);
+		__skb_frag_ref(&skb_shinfo(nskb)->frags[i], false);
 	}
 
 	skb_shinfo(nskb)->nr_frags = record->num_frags;
diff --git a/drivers/net/ethernet/sun/cassini.c b/drivers/net/ethernet/sun/cassini.c
index bfb903506367..fabba729e1b8 100644
--- a/drivers/net/ethernet/sun/cassini.c
+++ b/drivers/net/ethernet/sun/cassini.c
@@ -1999,7 +1999,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 		skb->len      += hlen - swivel;
 
 		skb_frag_fill_page_desc(frag, page->buffer, off, hlen - swivel);
-		__skb_frag_ref(frag);
+		__skb_frag_ref(frag, false);
 
 		/* any more data? */
 		if ((words[0] & RX_COMP1_SPLIT_PKT) && ((dlen -= hlen) > 0)) {
@@ -2023,7 +2023,7 @@ static int cas_rx_process_pkt(struct cas *cp, struct cas_rx_comp *rxc,
 			frag++;
 
 			skb_frag_fill_page_desc(frag, page->buffer, 0, hlen);
-			__skb_frag_ref(frag);
+			__skb_frag_ref(frag, false);
 			RX_USED_ADD(page, hlen + cp->crc_size);
 		}
 
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index bafa5c9ff59a..058d72a2a250 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3494,15 +3494,29 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
 	return netmem_to_page(frag->netmem);
 }
 
+bool napi_pp_get_page(struct page *page);
+
+static inline void skb_page_ref(struct page *page, bool recycle)
+{
+#ifdef CONFIG_PAGE_POOL
+	if (recycle && napi_pp_get_page(page))
+		return;
+#endif
+	get_page(page);
+}
+
 /**
  * __skb_frag_ref - take an addition reference on a paged fragment.
  * @frag: the paged fragment
+ * @recycle: skb->pp_recycle param of the parent skb. False if no parent skb.
  *
- * Takes an additional reference on the paged fragment @frag.
+ * Takes an additional reference on the paged fragment @frag. Obtains the
+ * correct reference count depending on whether skb->pp_recycle is set and
+ * whether the frag is a page pool frag.
  */
-static inline void __skb_frag_ref(skb_frag_t *frag)
+static inline void __skb_frag_ref(skb_frag_t *frag, bool recycle)
 {
-	get_page(skb_frag_page(frag));
+	skb_page_ref(skb_frag_page(frag), recycle);
 }
 
 /**
@@ -3514,7 +3528,7 @@ static inline void __skb_frag_ref(skb_frag_t *frag)
  */
 static inline void skb_frag_ref(struct sk_buff *skb, int f)
 {
-	__skb_frag_ref(&skb_shinfo(skb)->frags[f]);
+	__skb_frag_ref(&skb_shinfo(skb)->frags[f], skb->pp_recycle);
 }
 
 int skb_pp_cow_data(struct page_pool *pool, struct sk_buff **pskb,
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 17617c29be2d..5c86ecaceb6c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1005,6 +1005,19 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
 EXPORT_SYMBOL(skb_cow_data_for_xdp);
 
 #if IS_ENABLED(CONFIG_PAGE_POOL)
+bool napi_pp_get_page(struct page *page)
+{
+
+	page = compound_head(page);
+
+	if (!is_pp_page(page))
+		return false;
+
+	page_pool_ref_page(head_page);
+	return true;
+}
+EXPORT_SYMBOL(napi_pp_get_page);
+
 bool napi_pp_put_page(struct page *page, bool napi_safe)
 {
 	bool allow_direct = false;
@@ -1057,37 +1070,6 @@ static bool skb_pp_recycle(struct sk_buff *skb, void *data, bool napi_safe)
 	return napi_pp_put_page(virt_to_page(data), napi_safe);
 }
 
-/**
- * skb_pp_frag_ref() - Increase fragment references of a page pool aware skb
- * @skb:	page pool aware skb
- *
- * Increase the fragment reference count (pp_ref_count) of a skb. This is
- * intended to gain fragment references only for page pool aware skbs,
- * i.e. when skb->pp_recycle is true, and not for fragments in a
- * non-pp-recycling skb. It has a fallback to increase references on normal
- * pages, as page pool aware skbs may also have normal page fragments.
- */
-static int skb_pp_frag_ref(struct sk_buff *skb)
-{
-	struct skb_shared_info *shinfo;
-	struct page *head_page;
-	int i;
-
-	if (!skb->pp_recycle)
-		return -EINVAL;
-
-	shinfo = skb_shinfo(skb);
-
-	for (i = 0; i < shinfo->nr_frags; i++) {
-		head_page = compound_head(skb_frag_page(&shinfo->frags[i]));
-		if (likely(is_pp_page(head_page)))
-			page_pool_ref_page(head_page);
-		else
-			page_ref_inc(head_page);
-	}
-	return 0;
-}
-
 static void skb_kfree_head(void *head, unsigned int end_offset)
 {
 	if (end_offset == SKB_SMALL_HEAD_HEADROOM)
@@ -4196,7 +4178,7 @@ int skb_shift(struct sk_buff *tgt, struct sk_buff *skb, int shiftlen)
 			to++;
 
 		} else {
-			__skb_frag_ref(fragfrom);
+			__skb_frag_ref(fragfrom, skb->pp_recycle);
 			skb_frag_page_copy(fragto, fragfrom);
 			skb_frag_off_copy(fragto, fragfrom);
 			skb_frag_size_set(fragto, todo);
@@ -4846,7 +4828,7 @@ struct sk_buff *skb_segment(struct sk_buff *head_skb,
 			}
 
 			*nskb_frag = (i < 0) ? skb_head_frag_to_page_desc(frag_skb) : *frag;
-			__skb_frag_ref(nskb_frag);
+			__skb_frag_ref(nskb_frag, nskb->pp_recycle);
 			size = skb_frag_size(nskb_frag);
 
 			if (pos < offset) {
@@ -5977,10 +5959,8 @@ bool skb_try_coalesce(struct sk_buff *to, struct sk_buff *from,
 	/* if the skb is not cloned this does nothing
 	 * since we set nr_frags to 0.
 	 */
-	if (skb_pp_frag_ref(from)) {
-		for (i = 0; i < from_shinfo->nr_frags; i++)
-			__skb_frag_ref(&from_shinfo->frags[i]);
-	}
+	for (i = 0; i < from_shinfo->nr_frags; i++)
+		__skb_frag_ref(&from_shinfo->frags[i], from->pp_recycle);
 
 	to->truesize += delta;
 	to->len += len;
-- 
2.44.0.396.g6e790dbe36-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers
  2024-03-27 21:45 ` [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers Mina Almasry
@ 2024-03-28 17:07   ` kernel test robot
  2024-03-28 17:18   ` kernel test robot
  1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-03-28 17:07 UTC (permalink / raw)
  To: Mina Almasry, netdev, linux-kernel, linux-rdma
  Cc: llvm, oe-kbuild-all, Mina Almasry, Ayush Sawal, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

Hi Mina,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/net-make-napi_frag_unref-reuse-skb_page_unref/20240328-054816
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240327214523.2182174-3-almasrymina%40google.com
patch subject: [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers
config: x86_64-rhel-8.3-rust (https://download.01.org/0day-ci/archive/20240329/202403290006.WfusvToB-lkp@intel.com/config)
compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240329/202403290006.WfusvToB-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/202403290006.WfusvToB-lkp@intel.com/

All errors (new ones prefixed by >>):

>> net/tls/tls_device_fallback.c:280:22: error: too few arguments to function call, expected 2, have 1
     280 |                 __skb_frag_ref(frag);
         |                 ~~~~~~~~~~~~~~     ^
   include/linux/skbuff.h:3517:20: note: '__skb_frag_ref' declared here
    3517 | static inline void __skb_frag_ref(skb_frag_t *frag, bool recycle)
         |                    ^              ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   1 error generated.


vim +280 net/tls/tls_device_fallback.c

e8f69799810c32 Ilya Lesokhin  2018-04-30  228  
e8f69799810c32 Ilya Lesokhin  2018-04-30  229  /* This function may be called after the user socket is already
e8f69799810c32 Ilya Lesokhin  2018-04-30  230   * closed so make sure we don't use anything freed during
e8f69799810c32 Ilya Lesokhin  2018-04-30  231   * tls_sk_proto_close here
e8f69799810c32 Ilya Lesokhin  2018-04-30  232   */
e8f69799810c32 Ilya Lesokhin  2018-04-30  233  
e8f69799810c32 Ilya Lesokhin  2018-04-30  234  static int fill_sg_in(struct scatterlist *sg_in,
e8f69799810c32 Ilya Lesokhin  2018-04-30  235  		      struct sk_buff *skb,
d80a1b9d186057 Boris Pismenny 2018-07-13  236  		      struct tls_offload_context_tx *ctx,
e8f69799810c32 Ilya Lesokhin  2018-04-30  237  		      u64 *rcd_sn,
e8f69799810c32 Ilya Lesokhin  2018-04-30  238  		      s32 *sync_size,
e8f69799810c32 Ilya Lesokhin  2018-04-30  239  		      int *resync_sgs)
e8f69799810c32 Ilya Lesokhin  2018-04-30  240  {
504148fedb8542 Eric Dumazet   2022-06-30  241  	int tcp_payload_offset = skb_tcp_all_headers(skb);
e8f69799810c32 Ilya Lesokhin  2018-04-30  242  	int payload_len = skb->len - tcp_payload_offset;
e8f69799810c32 Ilya Lesokhin  2018-04-30  243  	u32 tcp_seq = ntohl(tcp_hdr(skb)->seq);
e8f69799810c32 Ilya Lesokhin  2018-04-30  244  	struct tls_record_info *record;
e8f69799810c32 Ilya Lesokhin  2018-04-30  245  	unsigned long flags;
e8f69799810c32 Ilya Lesokhin  2018-04-30  246  	int remaining;
e8f69799810c32 Ilya Lesokhin  2018-04-30  247  	int i;
e8f69799810c32 Ilya Lesokhin  2018-04-30  248  
e8f69799810c32 Ilya Lesokhin  2018-04-30  249  	spin_lock_irqsave(&ctx->lock, flags);
e8f69799810c32 Ilya Lesokhin  2018-04-30  250  	record = tls_get_record(ctx, tcp_seq, rcd_sn);
e8f69799810c32 Ilya Lesokhin  2018-04-30  251  	if (!record) {
e8f69799810c32 Ilya Lesokhin  2018-04-30  252  		spin_unlock_irqrestore(&ctx->lock, flags);
e8f69799810c32 Ilya Lesokhin  2018-04-30  253  		return -EINVAL;
e8f69799810c32 Ilya Lesokhin  2018-04-30  254  	}
e8f69799810c32 Ilya Lesokhin  2018-04-30  255  
e8f69799810c32 Ilya Lesokhin  2018-04-30  256  	*sync_size = tcp_seq - tls_record_start_seq(record);
e8f69799810c32 Ilya Lesokhin  2018-04-30  257  	if (*sync_size < 0) {
e8f69799810c32 Ilya Lesokhin  2018-04-30  258  		int is_start_marker = tls_record_is_start_marker(record);
e8f69799810c32 Ilya Lesokhin  2018-04-30  259  
e8f69799810c32 Ilya Lesokhin  2018-04-30  260  		spin_unlock_irqrestore(&ctx->lock, flags);
e8f69799810c32 Ilya Lesokhin  2018-04-30  261  		/* This should only occur if the relevant record was
e8f69799810c32 Ilya Lesokhin  2018-04-30  262  		 * already acked. In that case it should be ok
e8f69799810c32 Ilya Lesokhin  2018-04-30  263  		 * to drop the packet and avoid retransmission.
e8f69799810c32 Ilya Lesokhin  2018-04-30  264  		 *
e8f69799810c32 Ilya Lesokhin  2018-04-30  265  		 * There is a corner case where the packet contains
e8f69799810c32 Ilya Lesokhin  2018-04-30  266  		 * both an acked and a non-acked record.
e8f69799810c32 Ilya Lesokhin  2018-04-30  267  		 * We currently don't handle that case and rely
a0e128ef88e4a0 Yueh-Shun Li   2023-06-22  268  		 * on TCP to retransmit a packet that doesn't contain
e8f69799810c32 Ilya Lesokhin  2018-04-30  269  		 * already acked payload.
e8f69799810c32 Ilya Lesokhin  2018-04-30  270  		 */
e8f69799810c32 Ilya Lesokhin  2018-04-30  271  		if (!is_start_marker)
e8f69799810c32 Ilya Lesokhin  2018-04-30  272  			*sync_size = 0;
e8f69799810c32 Ilya Lesokhin  2018-04-30  273  		return -EINVAL;
e8f69799810c32 Ilya Lesokhin  2018-04-30  274  	}
e8f69799810c32 Ilya Lesokhin  2018-04-30  275  
e8f69799810c32 Ilya Lesokhin  2018-04-30  276  	remaining = *sync_size;
e8f69799810c32 Ilya Lesokhin  2018-04-30  277  	for (i = 0; remaining > 0; i++) {
e8f69799810c32 Ilya Lesokhin  2018-04-30  278  		skb_frag_t *frag = &record->frags[i];
e8f69799810c32 Ilya Lesokhin  2018-04-30  279  
e8f69799810c32 Ilya Lesokhin  2018-04-30 @280  		__skb_frag_ref(frag);
e8f69799810c32 Ilya Lesokhin  2018-04-30  281  		sg_set_page(sg_in + i, skb_frag_page(frag),
b54c9d5bd6e38e Jonathan Lemon 2019-07-30  282  			    skb_frag_size(frag), skb_frag_off(frag));
e8f69799810c32 Ilya Lesokhin  2018-04-30  283  
e8f69799810c32 Ilya Lesokhin  2018-04-30  284  		remaining -= skb_frag_size(frag);
e8f69799810c32 Ilya Lesokhin  2018-04-30  285  
e8f69799810c32 Ilya Lesokhin  2018-04-30  286  		if (remaining < 0)
e8f69799810c32 Ilya Lesokhin  2018-04-30  287  			sg_in[i].length += remaining;
e8f69799810c32 Ilya Lesokhin  2018-04-30  288  	}
e8f69799810c32 Ilya Lesokhin  2018-04-30  289  	*resync_sgs = i;
e8f69799810c32 Ilya Lesokhin  2018-04-30  290  
e8f69799810c32 Ilya Lesokhin  2018-04-30  291  	spin_unlock_irqrestore(&ctx->lock, flags);
e8f69799810c32 Ilya Lesokhin  2018-04-30  292  	if (skb_to_sgvec(skb, &sg_in[i], tcp_payload_offset, payload_len) < 0)
e8f69799810c32 Ilya Lesokhin  2018-04-30  293  		return -EINVAL;
e8f69799810c32 Ilya Lesokhin  2018-04-30  294  
e8f69799810c32 Ilya Lesokhin  2018-04-30  295  	return 0;
e8f69799810c32 Ilya Lesokhin  2018-04-30  296  }
e8f69799810c32 Ilya Lesokhin  2018-04-30  297  

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers
  2024-03-27 21:45 ` [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers Mina Almasry
  2024-03-28 17:07   ` kernel test robot
@ 2024-03-28 17:18   ` kernel test robot
  1 sibling, 0 replies; 6+ messages in thread
From: kernel test robot @ 2024-03-28 17:18 UTC (permalink / raw)
  To: Mina Almasry, netdev, linux-kernel, linux-rdma
  Cc: llvm, oe-kbuild-all, Mina Almasry, Ayush Sawal, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

Hi Mina,

kernel test robot noticed the following build errors:

[auto build test ERROR on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Mina-Almasry/net-make-napi_frag_unref-reuse-skb_page_unref/20240328-054816
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240327214523.2182174-3-almasrymina%40google.com
patch subject: [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers
config: riscv-defconfig (https://download.01.org/0day-ci/archive/20240329/202403290103.BHENUIYW-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project 23de3862dce582ce91c1aa914467d982cb1a73b4)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240329/202403290103.BHENUIYW-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/202403290103.BHENUIYW-lkp@intel.com/

All errors (new ones prefixed by >>):

   In file included from net/core/skbuff.c:40:
   In file included from include/linux/mm.h:2208:
   include/linux/vmstat.h:522:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
     522 |         return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
         |                               ~~~~~~~~~~~ ^ ~~~
>> net/core/skbuff.c:1016:21: error: use of undeclared identifier 'head_page'
    1016 |         page_pool_ref_page(head_page);
         |                            ^
   1 warning and 1 error generated.


vim +/head_page +1016 net/core/skbuff.c

  1010	
  1011		page = compound_head(page);
  1012	
  1013		if (!is_pp_page(page))
  1014			return false;
  1015	
> 1016		page_pool_ref_page(head_page);
  1017		return true;
  1018	}
  1019	EXPORT_SYMBOL(napi_pp_get_page);
  1020	

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH net-next v2 3/3] net: remove napi_frag_unref
  2024-03-27 21:45 [PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 1/3] net: make napi_frag_unref reuse skb_page_unref Mina Almasry
  2024-03-27 21:45 ` [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers Mina Almasry
@ 2024-03-27 21:45 ` Mina Almasry
  2 siblings, 0 replies; 6+ messages in thread
From: Mina Almasry @ 2024-03-27 21:45 UTC (permalink / raw)
  To: netdev, linux-kernel, linux-rdma
  Cc: Mina Almasry, Ayush Sawal, David S. Miller, Eric Dumazet,
	Jakub Kicinski, Paolo Abeni, Mirko Lindner, Stephen Hemminger,
	Tariq Toukan, Steffen Klassert, Herbert Xu, David Ahern,
	Boris Pismenny, John Fastabend, Dragos Tatulea

With the changes in the last patches, napi_frag_unref() is now
reduandant. Remove it and use skb_page_unref directly.

Signed-off-by: Mina Almasry <almasrymina@google.com>
Reviewed-by: Dragos Tatulea <dtatulea@nvidia.com>

---
 drivers/net/ethernet/marvell/sky2.c        |  2 +-
 drivers/net/ethernet/mellanox/mlx4/en_rx.c |  2 +-
 include/linux/skbuff.h                     | 14 +++++---------
 net/core/skbuff.c                          |  4 ++--
 net/tls/tls_device.c                       |  2 +-
 net/tls/tls_strp.c                         |  2 +-
 6 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index 07720841a8d7..8e00a5856856 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -2501,7 +2501,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, false, false);
 			--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 eac49657bd07..4dbf29b46979 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, false, false);
 	}
 	return 0;
 }
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 058d72a2a250..c3edb4a3450a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3547,23 +3547,19 @@ skb_page_unref(struct page *page, bool recycle, bool napi_safe)
 	put_page(page);
 }
 
-static inline void
-napi_frag_unref(skb_frag_t *frag, bool recycle, bool napi_safe)
-{
-	skb_page_unref(skb_frag_page(frag), recycle, napi_safe);
-}
-
 /**
  * __skb_frag_unref - release a reference on a paged fragment.
  * @frag: the paged fragment
  * @recycle: recycle the page if allocated via page_pool
+ * @napi_safe: set to true if running in the same napi context as where the
+ * consumer would run.
  *
  * 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, bool recycle, bool napi_safe)
 {
-	napi_frag_unref(frag, recycle, false);
+	skb_page_unref(skb_frag_page(frag), recycle, napi_safe);
 }
 
 /**
@@ -3578,7 +3574,7 @@ static inline void skb_frag_unref(struct sk_buff *skb, int f)
 	struct skb_shared_info *shinfo = skb_shinfo(skb);
 
 	if (!skb_zcopy_managed(skb))
-		__skb_frag_unref(&shinfo->frags[f], skb->pp_recycle);
+		__skb_frag_unref(&shinfo->frags[f], skb->pp_recycle, false);
 }
 
 /**
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5c86ecaceb6c..a6dbba56e047 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1109,7 +1109,7 @@ static void skb_release_data(struct sk_buff *skb, enum skb_drop_reason reason,
 	}
 
 	for (i = 0; i < shinfo->nr_frags; i++)
-		napi_frag_unref(&shinfo->frags[i], skb->pp_recycle, napi_safe);
+		__skb_frag_unref(&shinfo->frags[i], skb->pp_recycle, napi_safe);
 
 free_head:
 	if (shinfo->frag_list)
@@ -4200,7 +4200,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, skb->pp_recycle, false);
 	}
 
 	/* Reposition in the original skb */
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index bf8ed36b1ad6..5dc6381f34fb 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -140,7 +140,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], false, false);
 	kfree(record);
 }
 
diff --git a/net/tls/tls_strp.c b/net/tls/tls_strp.c
index ca1e0e198ceb..85b41f226978 100644
--- a/net/tls/tls_strp.c
+++ b/net/tls/tls_strp.c
@@ -196,7 +196,7 @@ static void tls_strp_flush_anchor_copy(struct tls_strparser *strp)
 	DEBUG_NET_WARN_ON_ONCE(atomic_read(&shinfo->dataref) != 1);
 
 	for (i = 0; i < shinfo->nr_frags; i++)
-		__skb_frag_unref(&shinfo->frags[i], false);
+		__skb_frag_unref(&shinfo->frags[i], false, false);
 	shinfo->nr_frags = 0;
 	if (strp->copy_mode) {
 		kfree_skb_list(shinfo->frag_list);
-- 
2.44.0.396.g6e790dbe36-goog


^ permalink raw reply related	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2024-03-28 17:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-27 21:45 [PATCH net-next v2 0/3] Minor cleanups to skb frag ref/unref Mina Almasry
2024-03-27 21:45 ` [PATCH net-next v2 1/3] net: make napi_frag_unref reuse skb_page_unref Mina Almasry
2024-03-27 21:45 ` [PATCH net-next v2 2/3] net: mirror skb frag ref/unref helpers Mina Almasry
2024-03-28 17:07   ` kernel test robot
2024-03-28 17:18   ` kernel test robot
2024-03-27 21:45 ` [PATCH net-next v2 3/3] net: remove napi_frag_unref Mina Almasry

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.