Skip to content

xsk: refactors around generic xmit side #9862

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 4 commits into
base: bpf-next_base
Choose a base branch
from
Open

xsk: refactors around generic xmit side #9862

wants to merge 4 commits into from
+3,169 −68

Conversation

kernel-patches-daemon-bpf[bot]
Copy link

Pull request for series with
subject: xsk: refactors around generic xmit side
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: dd948aa
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 5730dac
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-review-bot
Copy link

AI review job summary: https://github.com/kernel-patches/bpf/actions/runs/18018331618

Inline review:

commit 44a8057d9daec9219b38510f080f9ef22aca001d
Author: Maciej Fijalkowski <[email protected]>

xsk: wrap generic metadata handling onto separate function

This commit refactors xsk_build_skb() by extracting metadata handling
into a new xsk_skb_metadata() function. It also adds metadata support
for devices with IFF_TX_SKB_NO_LINEAR flag.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f7e0d254a..7b0c68a70 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> +static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
> +			    struct xdp_desc *desc, struct xsk_buff_pool *pool,
> +			    u32 hr)
> +{
> +	struct xsk_tx_metadata *meta = NULL;
> +
> +	if (unlikely(pool->tx_metadata_len == 0))
> +		return -EINVAL;
> +
> +	meta = buffer - pool->tx_metadata_len;
> +	if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
> +		return -EINVAL;
> +
> +	if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
> +		if (unlikely(meta->request.csum_start +
> +			     meta->request.csum_offset +
> +			     sizeof(__sum16) > desc->len))
> +			return -EINVAL;
> +
> +		skb->csum_start = hr + meta->request.csum_start;
> +		skb->csum_offset = meta->request.csum_offset;
> +		skb->ip_summed = CHECKSUM_PARTIAL;
> +
> +		if (unlikely(pool->tx_sw_csum)) {
> +			int err;
> +
> +			err = skb_checksum_help(skb);
> +			if (err)
> +				return err;
> +		}
> +	}
> +
> +	if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
> +		skb->skb_mstamp_ns = meta->request.launch_time;
> +	xsk_tx_metadata_to_compl(meta, &skb_shinfo(skb)->xsk_meta);
> +
> +	return 0;
> +}
> +
>  static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  					      struct xdp_desc *desc)
>  {
> @@ -669,6 +708,9 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  	int err, i;
>  	u64 addr;
>
> +	addr = desc->addr;
> +	buffer = xsk_buff_raw_get_data(pool, addr);
> +
>  	if (!skb) {
>  		hr = max(NET_SKB_PAD, L1_CACHE_ALIGN(xs->dev->needed_headroom));
>
> @@ -679,6 +721,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
>  		skb_reserve(skb, hr);
>
>  		xsk_skb_init_misc(skb, xs, desc->addr);
> +		if (desc->options & XDP_TX_METADATA) {
> +			err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
> +			if (unlikely(err))
> +				return ERR_PTR(err);

Does this leak the skb allocated by sock_alloc_send_skb()?

When xsk_skb_metadata() fails here, the function returns ERR_PTR(err)
directly without freeing the skb. The caller xsk_build_skb() sets
skb = NULL after getting the error, so the free_err path won't clean
it up either since it checks if (skb && !skb_shinfo(skb)->nr_frags).

> +		}
>  	} else {
>  		xsk_addr = kmem_cache_zalloc(xsk_tx_generic_cache, GFP_KERNEL);
>  		if (!xsk_addr)

[ ... ]

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 9b5c111
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 1193c46
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 105eb5d
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: d43029f
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@mfijalko
xsk: avoid overwriting skb fields for multi-buffer traffic
6fb592b 
We are unnecessarily setting a bunch of skb fields per each processed
descriptor, which is redundant for fragmented frames.

Let us set these respective members for first fragment only. To address
both paths that we have within xsk_build_skb(), move assignments onto
xsk_set_destructor_arg() and rename it to xsk_skb_init_misc().

Signed-off-by: Maciej Fijalkowski <[email protected]>
Acked-by: Stanislav Fomichev <[email protected]>
Reviewed-by: Jason Xing <[email protected]>
@mfijalko
xsk: remove @first_frag from xsk_build_skb()
0bc6b71 
Instead of using auxiliary boolean that tracks if we are at first frag
when gathering all elements of skb, same functionality can be achieved
with checking if skb_shared_info::nr_frags is 0.

Remove @first_frag but be careful around xsk_build_skb_zerocopy() and
NULL the skb pointer when it failed so that common error path does not
incorrectly interpret it during decision whether to call kfree_skb().

Signed-off-by: Maciej Fijalkowski <[email protected]>
Acked-by: Stanislav Fomichev <[email protected]>
Reviewed-by: Jason Xing <[email protected]>
@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 991e555
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1006276
version: 2

@mfijalko
xsk: wrap generic metadata handling onto separate function
5bbcc19 
xsk_build_skb() has gone wild with its size and one of the things we can
do about it is to pull out a branch that takes care of metadata handling
and make it a separate function.

While at it, let us add metadata SW support for devices supporting
IFF_TX_SKB_NO_LINEAR flag, that happen to have separate logic for
building skb in xsk's generic xmit path.

Acked-by: Stanislav Fomichev <[email protected]>
Reviewed-by: Jason Xing <[email protected]>
Signed-off-by: Maciej Fijalkowski <[email protected]>
@kernel-patches-daemon-bpf kernel-patches-daemon-bpf bot force-pushed the bpf-next_base branch 2 times, most recently from ccd38c1 to bc1a71f Compare September 26, 2025 21:06
@kernel-patches-daemon-bpf kernel-patches-daemon-bpf bot force-pushed the bpf-next_base branch 4 times, most recently from c199778 to b0c73f0 Compare September 28, 2025 11:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kernel-patches-bot kernel-patches-bot Awaiting requested review from kernel-patches-bot

Assignees
No one assigned
Labels
ai-review bpf-next new V2-ci-pass V2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@theihor @mfijalko