| Message ID | 20241107161026.2903044-13-aleksander.lobakin@intel.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | xdp: a fistful of generic changes (+libeth_xdp) | expand |
Alexander Lobakin <aleksander.lobakin@intel.com> writes: > The code which builds an skb from an &xdp_buff keeps multiplying itself > around the drivers with almost no changes. Let's try to stop that by > adding a generic function. > Unlike __xdp_build_skb_from_frame(), always allocate an skbuff head > using napi_build_skb() and make use of the available xdp_rxq pointer to > assign the Rx queue index. In case of PP-backed buffer, mark the skb to > be recycled, as every PP user's been switched to recycle skbs. > > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> > --- > include/net/xdp.h | 1 + > net/core/xdp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 56 insertions(+) > > diff --git a/include/net/xdp.h b/include/net/xdp.h > index 4c19042adf80..b0a25b7060ff 100644 > --- a/include/net/xdp.h > +++ b/include/net/xdp.h > @@ -330,6 +330,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags, > void xdp_warn(const char *msg, const char *func, const int line); > #define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__) > > +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp); > struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp); > struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, > struct sk_buff *skb, > diff --git a/net/core/xdp.c b/net/core/xdp.c > index b1b426a9b146..3a9a3c14b080 100644 > --- a/net/core/xdp.c > +++ b/net/core/xdp.c > @@ -624,6 +624,61 @@ int xdp_alloc_skb_bulk(void **skbs, int n_skb, gfp_t gfp) > } > EXPORT_SYMBOL_GPL(xdp_alloc_skb_bulk); > > +/** > + * xdp_build_skb_from_buff - create an skb from an &xdp_buff > + * @xdp: &xdp_buff to convert to an skb > + * > + * Perform common operations to create a new skb to pass up the stack from > + * an &xdp_buff: allocate an skb head from the NAPI percpu cache, initialize > + * skb data pointers and offsets, set the recycle bit if the buff is PP-backed, > + * Rx queue index, protocol and update frags info. > + * > + * Return: new &sk_buff on success, %NULL on error. > + */ > +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp) > +{ > + const struct xdp_rxq_info *rxq = xdp->rxq; > + const struct skb_shared_info *sinfo; > + struct sk_buff *skb; > + u32 nr_frags = 0; > + int metalen; > + > + if (unlikely(xdp_buff_has_frags(xdp))) { > + sinfo = xdp_get_shared_info_from_buff(xdp); > + nr_frags = sinfo->nr_frags; > + } Why this separate branch at the start of the function? nr_frags is no used until the other branch below, so why not just make that branch on xdp_buff_has_frags() and keep everything frags-related together in one block? -Toke
From: Toke Høiland-Jørgensen <toke@redhat.com> Date: Mon, 11 Nov 2024 17:39:51 +0100 > Alexander Lobakin <aleksander.lobakin@intel.com> writes: > >> The code which builds an skb from an &xdp_buff keeps multiplying itself >> around the drivers with almost no changes. Let's try to stop that by >> adding a generic function. >> Unlike __xdp_build_skb_from_frame(), always allocate an skbuff head >> using napi_build_skb() and make use of the available xdp_rxq pointer to >> assign the Rx queue index. In case of PP-backed buffer, mark the skb to >> be recycled, as every PP user's been switched to recycle skbs. >> >> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> >> --- >> include/net/xdp.h | 1 + >> net/core/xdp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 56 insertions(+) >> >> diff --git a/include/net/xdp.h b/include/net/xdp.h >> index 4c19042adf80..b0a25b7060ff 100644 >> --- a/include/net/xdp.h >> +++ b/include/net/xdp.h >> @@ -330,6 +330,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags, >> void xdp_warn(const char *msg, const char *func, const int line); >> #define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__) >> >> +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp); >> struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp); >> struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, >> struct sk_buff *skb, >> diff --git a/net/core/xdp.c b/net/core/xdp.c >> index b1b426a9b146..3a9a3c14b080 100644 >> --- a/net/core/xdp.c >> +++ b/net/core/xdp.c >> @@ -624,6 +624,61 @@ int xdp_alloc_skb_bulk(void **skbs, int n_skb, gfp_t gfp) >> } >> EXPORT_SYMBOL_GPL(xdp_alloc_skb_bulk); >> >> +/** >> + * xdp_build_skb_from_buff - create an skb from an &xdp_buff >> + * @xdp: &xdp_buff to convert to an skb >> + * >> + * Perform common operations to create a new skb to pass up the stack from >> + * an &xdp_buff: allocate an skb head from the NAPI percpu cache, initialize >> + * skb data pointers and offsets, set the recycle bit if the buff is PP-backed, >> + * Rx queue index, protocol and update frags info. >> + * >> + * Return: new &sk_buff on success, %NULL on error. >> + */ >> +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp) >> +{ >> + const struct xdp_rxq_info *rxq = xdp->rxq; >> + const struct skb_shared_info *sinfo; >> + struct sk_buff *skb; >> + u32 nr_frags = 0; >> + int metalen; >> + >> + if (unlikely(xdp_buff_has_frags(xdp))) { >> + sinfo = xdp_get_shared_info_from_buff(xdp); >> + nr_frags = sinfo->nr_frags; >> + } > > Why this separate branch at the start of the function? nr_frags is no > used until the other branch below, so why not just make that branch on > xdp_buff_has_frags() and keep everything frags-related together in one > block? Because napi_build_skb() will call build_skb_around() which will memset() a piece of shared info including nr_frags. xdp_build_skb_from_frame() has the same logic. I'd be happy to have only one block, but I can't =\ > > -Toke Thanks, Olek
Alexander Lobakin <aleksander.lobakin@intel.com> writes: > From: Toke Høiland-Jørgensen <toke@redhat.com> > Date: Mon, 11 Nov 2024 17:39:51 +0100 > >> Alexander Lobakin <aleksander.lobakin@intel.com> writes: >> >>> The code which builds an skb from an &xdp_buff keeps multiplying itself >>> around the drivers with almost no changes. Let's try to stop that by >>> adding a generic function. >>> Unlike __xdp_build_skb_from_frame(), always allocate an skbuff head >>> using napi_build_skb() and make use of the available xdp_rxq pointer to >>> assign the Rx queue index. In case of PP-backed buffer, mark the skb to >>> be recycled, as every PP user's been switched to recycle skbs. >>> >>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> >>> --- >>> include/net/xdp.h | 1 + >>> net/core/xdp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ >>> 2 files changed, 56 insertions(+) >>> >>> diff --git a/include/net/xdp.h b/include/net/xdp.h >>> index 4c19042adf80..b0a25b7060ff 100644 >>> --- a/include/net/xdp.h >>> +++ b/include/net/xdp.h >>> @@ -330,6 +330,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags, >>> void xdp_warn(const char *msg, const char *func, const int line); >>> #define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__) >>> >>> +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp); >>> struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp); >>> struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, >>> struct sk_buff *skb, >>> diff --git a/net/core/xdp.c b/net/core/xdp.c >>> index b1b426a9b146..3a9a3c14b080 100644 >>> --- a/net/core/xdp.c >>> +++ b/net/core/xdp.c >>> @@ -624,6 +624,61 @@ int xdp_alloc_skb_bulk(void **skbs, int n_skb, gfp_t gfp) >>> } >>> EXPORT_SYMBOL_GPL(xdp_alloc_skb_bulk); >>> >>> +/** >>> + * xdp_build_skb_from_buff - create an skb from an &xdp_buff >>> + * @xdp: &xdp_buff to convert to an skb >>> + * >>> + * Perform common operations to create a new skb to pass up the stack from >>> + * an &xdp_buff: allocate an skb head from the NAPI percpu cache, initialize >>> + * skb data pointers and offsets, set the recycle bit if the buff is PP-backed, >>> + * Rx queue index, protocol and update frags info. >>> + * >>> + * Return: new &sk_buff on success, %NULL on error. >>> + */ >>> +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp) >>> +{ >>> + const struct xdp_rxq_info *rxq = xdp->rxq; >>> + const struct skb_shared_info *sinfo; >>> + struct sk_buff *skb; >>> + u32 nr_frags = 0; >>> + int metalen; >>> + >>> + if (unlikely(xdp_buff_has_frags(xdp))) { >>> + sinfo = xdp_get_shared_info_from_buff(xdp); >>> + nr_frags = sinfo->nr_frags; >>> + } >> >> Why this separate branch at the start of the function? nr_frags is no >> used until the other branch below, so why not just make that branch on >> xdp_buff_has_frags() and keep everything frags-related together in one >> block? > > Because napi_build_skb() will call build_skb_around() which will > memset() a piece of shared info including nr_frags. > xdp_build_skb_from_frame() has the same logic. I'd be happy to have only > one block, but I can't =\ Ah, right. Annoying, but OK :) -Toke
Alexander Lobakin <aleksander.lobakin@intel.com> writes: > The code which builds an skb from an &xdp_buff keeps multiplying itself > around the drivers with almost no changes. Let's try to stop that by > adding a generic function. > Unlike __xdp_build_skb_from_frame(), always allocate an skbuff head > using napi_build_skb() and make use of the available xdp_rxq pointer to > assign the Rx queue index. In case of PP-backed buffer, mark the skb to > be recycled, as every PP user's been switched to recycle skbs. > > Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>
diff --git a/include/net/xdp.h b/include/net/xdp.h index 4c19042adf80..b0a25b7060ff 100644 --- a/include/net/xdp.h +++ b/include/net/xdp.h @@ -330,6 +330,7 @@ xdp_update_skb_shared_info(struct sk_buff *skb, u8 nr_frags, void xdp_warn(const char *msg, const char *func, const int line); #define XDP_WARN(msg) xdp_warn(msg, __func__, __LINE__) +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp); struct xdp_frame *xdp_convert_zc_to_xdp_frame(struct xdp_buff *xdp); struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, struct sk_buff *skb, diff --git a/net/core/xdp.c b/net/core/xdp.c index b1b426a9b146..3a9a3c14b080 100644 --- a/net/core/xdp.c +++ b/net/core/xdp.c @@ -624,6 +624,61 @@ int xdp_alloc_skb_bulk(void **skbs, int n_skb, gfp_t gfp) } EXPORT_SYMBOL_GPL(xdp_alloc_skb_bulk); +/** + * xdp_build_skb_from_buff - create an skb from an &xdp_buff + * @xdp: &xdp_buff to convert to an skb + * + * Perform common operations to create a new skb to pass up the stack from + * an &xdp_buff: allocate an skb head from the NAPI percpu cache, initialize + * skb data pointers and offsets, set the recycle bit if the buff is PP-backed, + * Rx queue index, protocol and update frags info. + * + * Return: new &sk_buff on success, %NULL on error. + */ +struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp) +{ + const struct xdp_rxq_info *rxq = xdp->rxq; + const struct skb_shared_info *sinfo; + struct sk_buff *skb; + u32 nr_frags = 0; + int metalen; + + if (unlikely(xdp_buff_has_frags(xdp))) { + sinfo = xdp_get_shared_info_from_buff(xdp); + nr_frags = sinfo->nr_frags; + } + + skb = napi_build_skb(xdp->data_hard_start, xdp->frame_sz); + if (unlikely(!skb)) + return NULL; + + skb_reserve(skb, xdp->data - xdp->data_hard_start); + __skb_put(skb, xdp->data_end - xdp->data); + + metalen = xdp->data - xdp->data_meta; + if (metalen > 0) + skb_metadata_set(skb, metalen); + + if (is_page_pool_compiled_in() && rxq->mem.type == MEM_TYPE_PAGE_POOL) + skb_mark_for_recycle(skb); + + skb_record_rx_queue(skb, rxq->queue_index); + + if (unlikely(nr_frags)) { + u32 tsize; + + tsize = sinfo->xdp_frags_truesize ? : nr_frags * xdp->frame_sz; + xdp_update_skb_shared_info(skb, nr_frags, + sinfo->xdp_frags_size, tsize, + xdp_buff_is_frag_pfmemalloc(xdp)); + } + + skb->protocol = eth_type_trans(skb, rxq->dev); + + return skb; +} +EXPORT_SYMBOL_GPL(xdp_build_skb_from_buff); + struct sk_buff *__xdp_build_skb_from_frame(struct xdp_frame *xdpf, struct sk_buff *skb, struct net_device *dev)
The code which builds an skb from an &xdp_buff keeps multiplying itself around the drivers with almost no changes. Let's try to stop that by adding a generic function. Unlike __xdp_build_skb_from_frame(), always allocate an skbuff head using napi_build_skb() and make use of the available xdp_rxq pointer to assign the Rx queue index. In case of PP-backed buffer, mark the skb to be recycled, as every PP user's been switched to recycle skbs. Signed-off-by: Alexander Lobakin <aleksander.lobakin@intel.com> --- include/net/xdp.h | 1 + net/core/xdp.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+)