| Message ID | 20240306235922.282781-3-almasrymina@google.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Minor cleanups to skb frag ref/unref | expand |
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 <almasrymina@google.com> > > --- > 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/20240306195359.1afb26d2@kernel.org/ 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 <dtatulea@nvidia.com>
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);
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 <almasrymina@google.com> --- 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(-)