Skip to content

Conversation

@kernel-patches-daemon-bpf
Copy link

Pull request for series with
subject: xsk: introduce pre-allocated memory per xsk CQ
version: 2
url: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033607

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 6f0b824
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033607
version: 2

@kernel-patches-review-bot
Copy link

commit 5bd830fed175ed5eb5cceadda5fbf48077253339
Author: Jason Xing <kernelxing@tencent.com>

xsk: introduce local_cq for each af_xdp socket

This patch introduces infrastructure for a local completion queue in AF_XDP
sockets, adding allocation and cleanup functions that will be used to store
descriptor addresses for later publishing to userspace.

> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index 23e8861e8b25..c53ab2609d8c 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -45,6 +45,12 @@ struct xsk_map {
>  	struct xdp_sock __rcu *xsk_map[];
>  };
>
> +struct local_cq {
> +	u32 prod ____cacheline_aligned_in_smp;
> +	u32 ring_mask ____cacheline_aligned_in_smp;
> +	u64 desc[] ____cacheline_aligned_in_smp;
> +};
> +
>  struct xdp_sock {
>  	/* struct sock must be the first member of struct xdp_sock */
>  	struct sock sk;
> @@ -89,6 +95,8 @@ struct xdp_sock {
>  	struct mutex mutex;
>  	struct xsk_queue *fq_tmp; /* Only as tmp storage before bind */
>  	struct xsk_queue *cq_tmp; /* Only as tmp storage before bind */
> +	/* Maintain addr(s) of descriptors locally */
> +	struct local_cq *lcq;
>  };

[ ... ]

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index f093c3453f64..9b637d5e4528 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -1212,6 +1212,30 @@ static void xsk_delete_from_maps(struct xdp_sock *xs)
>  	}
>  }
>
> +/* Initialize local compeletion queue for each xsk */
> +static int xsk_init_local_cq(struct xdp_sock *xs)
> +{
> +	struct xsk_queue *cq = xs->pool->cq;
> +	size_t size;
> +
> +	if (!cq || !cq->nentries)
> +		return -EINVAL;
> +
> +	size = struct_size_t(struct local_cq, desc, cq->nentries);
> +	xs->lcq = vmalloc(size);
> +	if (!xs->lcq)
> +		return -ENOMEM;
> +	xs->lcq->ring_mask = cq->nentries - 1;
> +	xs->lcq->prod = 0;
> +
> +	return 0;
> +}
> +
> +static void xsk_clear_local_cq(struct xdp_sock *xs)
> +{
> +	vfree(xs->lcq);

Can this lead to a double-free? When xsk_bind() fails after
xsk_init_local_cq() succeeds, xsk_clear_local_cq() is called in the
error path, freeing xs->lcq. However, xs->lcq is not set to NULL after
the vfree(). Later when the socket is closed, xsk_release() calls
xsk_clear_local_cq() again, causing vfree() to be called on the
already-freed pointer.

The path would be: xsk_bind()->xsk_init_local_cq() succeeds, allocating
xs->lcq. Then xp_assign_dev_shared() fails, triggering the error path
that calls xsk_clear_local_cq(). The bind fails and returns to userspace.
User closes the socket, triggering xsk_release()->xsk_clear_local_cq(),
which calls vfree() on the dangling xs->lcq pointer.

Should xs->lcq be set to NULL after the vfree to prevent this double-free?

> +}
> +
>  static int xsk_release(struct socket *sock)
>  {
>  	struct sock *sk = sock->sk;
> @@ -1241,6 +1265,7 @@ static int xsk_release(struct socket *sock)
>  	xskq_destroy(xs->tx);
>  	xskq_destroy(xs->fq_tmp);
>  	xskq_destroy(xs->cq_tmp);
> +	xsk_clear_local_cq(xs);
>
>  	sock_orphan(sk);
>  	sock->sk = NULL;

[ ... ]

> @@ -1360,9 +1385,18 @@ static int xsk_bind(struct socket *sock, struct sockaddr_unsized *addr, int addr
>  				goto out_unlock;
>  			}
>
> +			err = xsk_init_local_cq(xs);
> +			if (err) {
> +				xp_destroy(xs->pool);
> +				xs->pool = NULL;
> +				sockfd_put(sock);
> +				goto out_unlock;
> +			}
> +
>  			err = xp_assign_dev_shared(xs->pool, umem_xs, dev,
>  						   qid);
>  			if (err) {
> +				xsk_clear_local_cq(xs);
>  				xp_destroy(xs->pool);
>  				xs->pool = NULL;
>  				sockfd_put(sock);

[ ... ]



AI reviewed your patch. Please fix the bug or email reply why it's not a bug.
See: https://github.com/kernel-patches/vmtest/blob/master/ci/claude/README.md

In-Reply-To-Subject: xsk: introduce local_cq for each af_xdp socket
CI run summary: https://github.com/kernel-patches/bpf/actions/runs/20257706446

@kernel-patches-daemon-bpf
Copy link
Author

@kernel-patches-daemon-bpf
Copy link
Author

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

@kernel-patches-daemon-bpf
Copy link
Author

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

@kernel-patches-daemon-bpf
Copy link
Author

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

@kernel-patches-daemon-bpf
Copy link
Author

Upstream branch: 3d60306
series: https://patchwork.kernel.org/project/netdevbpf/list/?series=1033607
version: 2

@kernel-patches-daemon-bpf
Copy link
Author

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

This is a prep that will be used to store the addr(s) of descriptors so
that each skb going to the end of life can publish corresponding addr(s)
in its completion queue that can be read by userspace.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
Before the commit 30f241f ("xsk: Fix immature cq descriptor
production"), there is one issue[1] which causes the wrong publish
of descriptors in race condidtion. The above commit fixes the issue
but adds more memory operations in the xmit hot path and interrupt
context, which can cause side effect in performance.

Based on the existing infrastructure, this patch tries to propose
a new solution to fix the problem by using a pre-allocated memory
that is local completion queue to avoid frequently performing memory
functions. The benefit comes from replacing xsk_tx_generic_cache with
local cq.

The core logics are as show below:
1. allocate a new local completion queue when setting the real queue.
2. write the descriptors into the local cq in the xmit path. And
   record the prod as @start_pos that reflects the start position of
   skb in this queue so that later the skb can easily write the desc
   addr(s) from local cq to cq addrs in the destruction phase.
3. initialize the upper 24 bits of destructor_arg to store @start_pos
   in xsk_skb_init_misc().
4. Initialize the lower 8 bits of destructor_arg to store how many
   descriptors the skb owns in xsk_inc_num_desc().
5. write the desc addr(s) from the @start_addr from the local cq
   one by one into the real cq in xsk_destruct_skb(). In turn sync
   the global state of the cq as before.

The format of destructor_arg is designed as:
 ------------------------ --------
|       start_pos        |  num   |
 ------------------------ --------
Using upper 24 bits is enough to keep the temporary descriptors. And
it's also enough to use lower 8 bits to show the number of descriptors
that one skb owns.

[1]: https://lore.kernel.org/all/20250530095957.43248-1-e.kubanski@partner.samsung.com/

Signed-off-by: Jason Xing <kernelxing@tencent.com>
@kernel-patches-daemon-bpf
Copy link
Author

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants