[RFC,net-next,v1,2/2] net: remove napi_frag_[un]ref

Commit Message

Mina Almasry March 6, 2024, 11:59 p.m. UTC

With the changes in the last patch, napi_frag_[un]ref() helpers become
reduandant. Remove them, and use __skb_frag_[un]ref() directly.

Signed-off-by: Mina Almasry <[e-mail]>

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

Comments

Dragos Tatulea March 7, 2024, 11:08 a.m. UTC | #1

On Wed, 2024-03-06 at 15:59 -0800, Mina Almasry wrote:
> With the changes in the last patch, napi_frag_[un]ref() helpers become
> reduandant. Remove them, and use __skb_frag_[un]ref() directly.
> 
Typo: reduandant

> Signed-off-by: Mina Almasry <[e-mail]>
> 
> ---
>  drivers/net/ethernet/marvell/sky2.c        |  2 +-
>  drivers/net/ethernet/mellanox/mlx4/en_rx.c |  2 +-
>  include/linux/skbuff.h                     | 45 +++++++++-------------
>  net/core/skbuff.c                          |  4 +-
>  net/tls/tls_device.c                       |  2 +-
>  net/tls/tls_strp.c                         |  2 +-
>  6 files changed, 24 insertions(+), 33 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 51316b0e20bc..9cd04c315592 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -3479,17 +3479,6 @@ static inline struct page *skb_frag_page(const skb_frag_t *frag)
>  
>  bool napi_pp_get_page(struct page *page);
>  
> -static inline void napi_frag_ref(skb_frag_t *frag, bool recycle)
> -{
> -#ifdef CONFIG_PAGE_POOL
> -	struct page *page = skb_frag_page(frag);
> -
> -	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
> @@ -3501,7 +3490,13 @@ static inline void napi_frag_ref(skb_frag_t *frag, bool recycle)
>   */
>  static inline void __skb_frag_ref(skb_frag_t *frag, bool recycle)
>  {
> -	napi_frag_ref(frag, recycle);
> +#ifdef CONFIG_PAGE_POOL
> +	struct page *page = skb_frag_page(frag);
> +
Issue from previous patch propagates here.

> +	if (recycle && napi_pp_get_page(page))
> +		return;
> +#endif
> +	get_page(page);
>  }
>  
>  /**
> @@ -3522,29 +3517,25 @@ int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
>  			 struct bpf_prog *prog);
>  bool napi_pp_put_page(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_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)
I think it would makes sense to have an additional wrapper on top of this
function that takes in a const sk_buff * instead of recycle? This would hide pp
specific details from the caller. It is in line with one of Jakub's comments  in
my patch:
https://lore.kernel.org/netdev/[e-mail]/

Would look good as a 3rd patch maybe. The hard work of coming up with a name
would be all yours :).

>  {
> -	napi_frag_unref(frag, recycle, false);
> +	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);
>  }
>  
>  /**
> @@ -3559,7 +3550,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 6d234faa9d9e..ed7f7e960b78 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -1114,7 +1114,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)
> @@ -4205,7 +4205,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);

I like the cleanup and simplifications in these patches. Thanks Mina! 

After the small issues are addressed you can add on both patches:
Reviewed-by: Dragos Tatulea <[e-mail]>

Patch

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 51316b0e20bc..9cd04c315592 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -3479,17 +3479,6 @@  static inline struct page *skb_frag_page(const skb_frag_t *frag)
 
 bool napi_pp_get_page(struct page *page);
 
-static inline void napi_frag_ref(skb_frag_t *frag, bool recycle)
-{
-#ifdef CONFIG_PAGE_POOL
-	struct page *page = skb_frag_page(frag);
-
-	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
@@ -3501,7 +3490,13 @@  static inline void napi_frag_ref(skb_frag_t *frag, bool recycle)
  */
 static inline void __skb_frag_ref(skb_frag_t *frag, bool recycle)
 {
-	napi_frag_ref(frag, recycle);
+#ifdef CONFIG_PAGE_POOL
+	struct page *page = skb_frag_page(frag);
+
+	if (recycle && napi_pp_get_page(page))
+		return;
+#endif
+	get_page(page);
 }
 
 /**
@@ -3522,29 +3517,25 @@  int skb_cow_data_for_xdp(struct page_pool *pool, struct sk_buff **pskb,
 			 struct bpf_prog *prog);
 bool napi_pp_put_page(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_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);
+	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);
 }
 
 /**
@@ -3559,7 +3550,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 6d234faa9d9e..ed7f7e960b78 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1114,7 +1114,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)
@@ -4205,7 +4205,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);