[PATCH net-next v5 0/2] net: xsk: update tx queue consumer

[PATCH net-next v5 0/2] net: xsk: update tx queue consumer

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

Patch 1 makes sure the consumer is updated at the end of generic xmit.
Patch 2 adds corresponding test.

Jason Xing (2):
  net: xsk: update tx queue consumer immediately after transmission
  selftests/bpf: add a new test to check the consumer update case

 net/xdp/xsk.c                            | 17 ++++----
 tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
 tools/testing/selftests/bpf/xskxceiver.h |  1 +
 3 files changed, 61 insertions(+), 8 deletions(-)

-- 
2.41.3

[PATCH net-next v5 1/2] net: xsk: update tx queue consumer immediately after transmission

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

For afxdp, the return value of sendto() syscall doesn't reflect how many
descs handled in the kernel. One of use cases is that when user-space
application tries to know the number of transmitted skbs and then decides
if it continues to send, say, is it stopped due to max tx budget?

The following formular can be used after sending to learn how many
skbs/descs the kernel takes care of:

  tx_queue.consumers_before - tx_queue.consumers_after

Prior to the current patch, in non-zc mode, the consumer of tx queue is
not immediately updated at the end of each sendto syscall when error
occurs, which leads to the consumer value out-of-dated from the perspective
of user space. So this patch requires store operation to pass the cached
value to the shared value to handle the problem.

More than those explicit errors appearing in the while() loop in
__xsk_generic_xmit(), there are a few possible error cases that might
be neglected in the following call trace:
__xsk_generic_xmit()
    xskq_cons_peek_desc()
        xskq_cons_read_desc()
	    xskq_cons_is_valid_desc()
It will also cause the premature exit in the while() loop even if not
all the descs are consumed.

Based on the above analysis, using @sent_frame could cover all the possible
cases where it might lead to out-of-dated global state of consumer after
finishing __xsk_generic_xmit().

The patch also adds a common helper __xsk_tx_release() to keep align
with the zc mode usage in xsk_tx_release().

Signed-off-by: Jason Xing <kernelxing@tencent.com>
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Acked-by: Stanislav Fomichev <sdf@fomichev.me>
---
v5
Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
1. add acked-by tags

v4
Link: https://lore.kernel.org/all/20250625101014.45066-1-kerneljasonxing@gmail.com/
1. use the common helper
2. keep align with the zc mode usage in xsk_tx_release()

v3
Link: https://lore.kernel.org/all/20250623073129.23290-1-kerneljasonxing@gmail.com/
1. use xskq_has_descs helper.
2. add selftest

V2
Link: https://lore.kernel.org/all/20250619093641.70700-1-kerneljasonxing@gmail.com/
1. filter out those good cases because only those that return error need
updates.
---
 net/xdp/xsk.c | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index 72c000c0ae5f..bd61b0bc9c24 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -300,6 +300,13 @@ static bool xsk_tx_writeable(struct xdp_sock *xs)
 	return true;
 }
 
+static void __xsk_tx_release(struct xdp_sock *xs)
+{
+	__xskq_cons_release(xs->tx);
+	if (xsk_tx_writeable(xs))
+		xs->sk.sk_write_space(&xs->sk);
+}
+
 static bool xsk_is_bound(struct xdp_sock *xs)
 {
 	if (READ_ONCE(xs->state) == XSK_BOUND) {
@@ -407,11 +414,8 @@ void xsk_tx_release(struct xsk_buff_pool *pool)
 	struct xdp_sock *xs;
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list) {
-		__xskq_cons_release(xs->tx);
-		if (xsk_tx_writeable(xs))
-			xs->sk.sk_write_space(&xs->sk);
-	}
+	list_for_each_entry_rcu(xs, &pool->xsk_tx_list, tx_list)
+		__xsk_tx_release(xs);
 	rcu_read_unlock();
 }
 EXPORT_SYMBOL(xsk_tx_release);
@@ -858,8 +862,7 @@ static int __xsk_generic_xmit(struct sock *sk)
 
 out:
 	if (sent_frame)
-		if (xsk_tx_writeable(xs))
-			sk->sk_write_space(sk);
+		__xsk_tx_release(xs);
 
 	mutex_unlock(&xs->mutex);
 	return err;
-- 
2.41.3

[PATCH net-next v5 2/2] selftests/bpf: add a new test to check the consumer update case

[Jason Xing]

From: Jason Xing <kernelxing@tencent.com>

The subtest sends 33 packets at one time on purpose to see if xsk
exitting __xsk_generic_xmit() updates the global consumer of tx queue
when reaching the max loop (max_tx_budget, 32 by default). The number 33
can avoid xskq_cons_peek_desc() updates the consumer when it's about to
quit sending, to accurately check if the issue that the first patch
resolves remains. The new case will not check this issue in zero copy
mode.

Signed-off-by: Jason Xing <kernelxing@tencent.com>
---
v5
Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
1. use the initial approach to add a new testcase
2. add a new flag 'check_consumer' to see if the check is needed
---
 tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
 tools/testing/selftests/bpf/xskxceiver.h |  1 +
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 0ced4026ee44..ed12a55ecf2a 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -109,6 +109,8 @@
 
 #include <network_helpers.h>
 
+#define MAX_TX_BUDGET_DEFAULT 32
+
 static bool opt_verbose;
 static bool opt_print_tests;
 static enum test_mode opt_mode = TEST_MODE_ALL;
@@ -1091,11 +1093,45 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
 	return true;
 }
 
+static u32 load_value(u32 *counter)
+{
+	return __atomic_load_n(counter, __ATOMIC_ACQUIRE);
+}
+
+static bool kick_tx_with_check(struct xsk_socket_info *xsk, int *ret)
+{
+	u32 max_budget = MAX_TX_BUDGET_DEFAULT;
+	u32 cons, ready_to_send;
+	int delta;
+
+	cons = load_value(xsk->tx.consumer);
+	ready_to_send = load_value(xsk->tx.producer) - cons;
+	*ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
+
+	delta = load_value(xsk->tx.consumer) - cons;
+	/* By default, xsk should consume exact @max_budget descs at one
+	 * send in this case where hitting the max budget limit in while
+	 * loop is triggered in __xsk_generic_xmit(). Please make sure that
+	 * the number of descs to be sent is larger than @max_budget, or
+	 * else the tx.consumer will be updated in xskq_cons_peek_desc()
+	 * in time which hides the issue we try to verify.
+	 */
+	if (ready_to_send > max_budget && delta != max_budget)
+		return false;
+
+	return true;
+}
+
 static int kick_tx(struct xsk_socket_info *xsk)
 {
 	int ret;
 
-	ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
+	if (xsk->check_consumer) {
+		if (!kick_tx_with_check(xsk, &ret))
+			return TEST_FAILURE;
+	} else {
+		ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
+	}
 	if (ret >= 0)
 		return TEST_PASS;
 	if (errno == ENOBUFS || errno == EAGAIN || errno == EBUSY || errno == ENETDOWN) {
@@ -2613,6 +2649,18 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
 				   XSK_UMEM__LARGE_FRAME_SIZE * 2);
 }
 
+static int testapp_tx_queue_consumer(struct test_spec *test)
+{
+	int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
+
+	pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
+	test->ifobj_tx->xsk->batch_size = nr_packets;
+	if (!(test->mode & TEST_MODE_ZC))
+		test->ifobj_tx->xsk->check_consumer = true;
+
+	return testapp_validate_traffic(test);
+}
+
 static void run_pkt_test(struct test_spec *test)
 {
 	int ret;
@@ -2723,6 +2771,7 @@ static const struct test_spec tests[] = {
 	{.name = "XDP_ADJUST_TAIL_SHRINK_MULTI_BUFF", .test_func = testapp_adjust_tail_shrink_mb},
 	{.name = "XDP_ADJUST_TAIL_GROW", .test_func = testapp_adjust_tail_grow},
 	{.name = "XDP_ADJUST_TAIL_GROW_MULTI_BUFF", .test_func = testapp_adjust_tail_grow_mb},
+	{.name = "TX_QUEUE_CONSUMER", .test_func = testapp_tx_queue_consumer},
 	};
 
 static void print_tests(void)
diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
index 67fc44b2813b..4df3a5d329ac 100644
--- a/tools/testing/selftests/bpf/xskxceiver.h
+++ b/tools/testing/selftests/bpf/xskxceiver.h
@@ -95,6 +95,7 @@ struct xsk_socket_info {
 	u32 batch_size;
 	u8 dst_mac[ETH_ALEN];
 	u8 src_mac[ETH_ALEN];
+	bool check_consumer;
 };
 
 struct pkt {
-- 
2.41.3

Re: [PATCH net-next v5 2/2] selftests/bpf: add a new test to check the consumer update case

[Stanislav Fomichev]

On 07/02, Jason Xing wrote:
> From: Jason Xing <kernelxing@tencent.com>
> 
> The subtest sends 33 packets at one time on purpose to see if xsk
> exitting __xsk_generic_xmit() updates the global consumer of tx queue
> when reaching the max loop (max_tx_budget, 32 by default). The number 33
> can avoid xskq_cons_peek_desc() updates the consumer when it's about to
> quit sending, to accurately check if the issue that the first patch
> resolves remains. The new case will not check this issue in zero copy
> mode.
> 
> Signed-off-by: Jason Xing <kernelxing@tencent.com>
> ---
> v5
> Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
> 1. use the initial approach to add a new testcase
> 2. add a new flag 'check_consumer' to see if the check is needed
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
>  tools/testing/selftests/bpf/xskxceiver.h |  1 +
>  2 files changed, 51 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 0ced4026ee44..ed12a55ecf2a 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -109,6 +109,8 @@
>  
>  #include <network_helpers.h>
>  
> +#define MAX_TX_BUDGET_DEFAULT 32
> +
>  static bool opt_verbose;
>  static bool opt_print_tests;
>  static enum test_mode opt_mode = TEST_MODE_ALL;
> @@ -1091,11 +1093,45 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
>  	return true;
>  }
>  
> +static u32 load_value(u32 *counter)
> +{
> +	return __atomic_load_n(counter, __ATOMIC_ACQUIRE);
> +}
> +
> +static bool kick_tx_with_check(struct xsk_socket_info *xsk, int *ret)
> +{
> +	u32 max_budget = MAX_TX_BUDGET_DEFAULT;
> +	u32 cons, ready_to_send;
> +	int delta;
> +
> +	cons = load_value(xsk->tx.consumer);
> +	ready_to_send = load_value(xsk->tx.producer) - cons;
> +	*ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> +
> +	delta = load_value(xsk->tx.consumer) - cons;
> +	/* By default, xsk should consume exact @max_budget descs at one
> +	 * send in this case where hitting the max budget limit in while
> +	 * loop is triggered in __xsk_generic_xmit(). Please make sure that
> +	 * the number of descs to be sent is larger than @max_budget, or
> +	 * else the tx.consumer will be updated in xskq_cons_peek_desc()
> +	 * in time which hides the issue we try to verify.
> +	 */
> +	if (ready_to_send > max_budget && delta != max_budget)
> +		return false;
> +
> +	return true;
> +}
> +
>  static int kick_tx(struct xsk_socket_info *xsk)
>  {
>  	int ret;
>  
> -	ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> +	if (xsk->check_consumer) {
> +		if (!kick_tx_with_check(xsk, &ret))
> +			return TEST_FAILURE;
> +	} else {
> +		ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> +	}
>  	if (ret >= 0)
>  		return TEST_PASS;
>  	if (errno == ENOBUFS || errno == EAGAIN || errno == EBUSY || errno == ENETDOWN) {
> @@ -2613,6 +2649,18 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
>  				   XSK_UMEM__LARGE_FRAME_SIZE * 2);
>  }
>  
> +static int testapp_tx_queue_consumer(struct test_spec *test)
> +{
> +	int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
> +
> +	pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
> +	test->ifobj_tx->xsk->batch_size = nr_packets;
> +	if (!(test->mode & TEST_MODE_ZC))
> +		test->ifobj_tx->xsk->check_consumer = true;

The test looks good to me, thank you!

One question here: why not exit/return for TEST_MODE_ZC instead
of conditionally setting check_consumer?

Re: [PATCH net-next v5 2/2] selftests/bpf: add a new test to check the consumer update case

[Jason Xing]

On Thu, Jul 3, 2025 at 12:03 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
>
> On 07/02, Jason Xing wrote:
> > From: Jason Xing <kernelxing@tencent.com>
> >
> > The subtest sends 33 packets at one time on purpose to see if xsk
> > exitting __xsk_generic_xmit() updates the global consumer of tx queue
> > when reaching the max loop (max_tx_budget, 32 by default). The number 33
> > can avoid xskq_cons_peek_desc() updates the consumer when it's about to
> > quit sending, to accurately check if the issue that the first patch
> > resolves remains. The new case will not check this issue in zero copy
> > mode.
> >
> > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > ---
> > v5
> > Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
> > 1. use the initial approach to add a new testcase
> > 2. add a new flag 'check_consumer' to see if the check is needed
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
> >  tools/testing/selftests/bpf/xskxceiver.h |  1 +
> >  2 files changed, 51 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > index 0ced4026ee44..ed12a55ecf2a 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -109,6 +109,8 @@
> >
> >  #include <network_helpers.h>
> >
> > +#define MAX_TX_BUDGET_DEFAULT 32
> > +
> >  static bool opt_verbose;
> >  static bool opt_print_tests;
> >  static enum test_mode opt_mode = TEST_MODE_ALL;
> > @@ -1091,11 +1093,45 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
> >       return true;
> >  }
> >
> > +static u32 load_value(u32 *counter)
> > +{
> > +     return __atomic_load_n(counter, __ATOMIC_ACQUIRE);
> > +}
> > +
> > +static bool kick_tx_with_check(struct xsk_socket_info *xsk, int *ret)
> > +{
> > +     u32 max_budget = MAX_TX_BUDGET_DEFAULT;
> > +     u32 cons, ready_to_send;
> > +     int delta;
> > +
> > +     cons = load_value(xsk->tx.consumer);
> > +     ready_to_send = load_value(xsk->tx.producer) - cons;
> > +     *ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > +
> > +     delta = load_value(xsk->tx.consumer) - cons;
> > +     /* By default, xsk should consume exact @max_budget descs at one
> > +      * send in this case where hitting the max budget limit in while
> > +      * loop is triggered in __xsk_generic_xmit(). Please make sure that
> > +      * the number of descs to be sent is larger than @max_budget, or
> > +      * else the tx.consumer will be updated in xskq_cons_peek_desc()
> > +      * in time which hides the issue we try to verify.
> > +      */
> > +     if (ready_to_send > max_budget && delta != max_budget)
> > +             return false;
> > +
> > +     return true;
> > +}
> > +
> >  static int kick_tx(struct xsk_socket_info *xsk)
> >  {
> >       int ret;
> >
> > -     ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > +     if (xsk->check_consumer) {
> > +             if (!kick_tx_with_check(xsk, &ret))
> > +                     return TEST_FAILURE;
> > +     } else {
> > +             ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > +     }
> >       if (ret >= 0)
> >               return TEST_PASS;
> >       if (errno == ENOBUFS || errno == EAGAIN || errno == EBUSY || errno == ENETDOWN) {
> > @@ -2613,6 +2649,18 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
> >                                  XSK_UMEM__LARGE_FRAME_SIZE * 2);
> >  }
> >
> > +static int testapp_tx_queue_consumer(struct test_spec *test)
> > +{
> > +     int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
> > +
> > +     pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
> > +     test->ifobj_tx->xsk->batch_size = nr_packets;
> > +     if (!(test->mode & TEST_MODE_ZC))
> > +             test->ifobj_tx->xsk->check_consumer = true;
>
> The test looks good to me, thank you!

Thanks.

>
> One question here: why not exit/return for TEST_MODE_ZC instead
> of conditionally setting check_consumer?

As you said, yes, we could skip the zc test for this
testapp_tx_queue_consumer(). It doesn't affect the goal or result of
the subtest. So do you expect me to respin this patch or just leave it
as is?

Thanks,
Jason

Re: [PATCH net-next v5 2/2] selftests/bpf: add a new test to check the consumer update case

[Maciej Fijalkowski]

On Thu, Jul 03, 2025 at 07:09:09AM +0800, Jason Xing wrote:
> On Thu, Jul 3, 2025 at 12:03 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> >
> > On 07/02, Jason Xing wrote:
> > > From: Jason Xing <kernelxing@tencent.com>
> > >
> > > The subtest sends 33 packets at one time on purpose to see if xsk
> > > exitting __xsk_generic_xmit() updates the global consumer of tx queue
> > > when reaching the max loop (max_tx_budget, 32 by default). The number 33
> > > can avoid xskq_cons_peek_desc() updates the consumer when it's about to
> > > quit sending, to accurately check if the issue that the first patch
> > > resolves remains. The new case will not check this issue in zero copy
> > > mode.
> > >
> > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > ---
> > > v5
> > > Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
> > > 1. use the initial approach to add a new testcase
> > > 2. add a new flag 'check_consumer' to see if the check is needed
> > > ---
> > >  tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
> > >  tools/testing/selftests/bpf/xskxceiver.h |  1 +
> > >  2 files changed, 51 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > > index 0ced4026ee44..ed12a55ecf2a 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > @@ -109,6 +109,8 @@
> > >
> > >  #include <network_helpers.h>
> > >
> > > +#define MAX_TX_BUDGET_DEFAULT 32
> > > +
> > >  static bool opt_verbose;
> > >  static bool opt_print_tests;
> > >  static enum test_mode opt_mode = TEST_MODE_ALL;
> > > @@ -1091,11 +1093,45 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
> > >       return true;
> > >  }
> > >
> > > +static u32 load_value(u32 *counter)
> > > +{
> > > +     return __atomic_load_n(counter, __ATOMIC_ACQUIRE);
> > > +}
> > > +
> > > +static bool kick_tx_with_check(struct xsk_socket_info *xsk, int *ret)
> > > +{
> > > +     u32 max_budget = MAX_TX_BUDGET_DEFAULT;
> > > +     u32 cons, ready_to_send;
> > > +     int delta;
> > > +
> > > +     cons = load_value(xsk->tx.consumer);
> > > +     ready_to_send = load_value(xsk->tx.producer) - cons;
> > > +     *ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > +
> > > +     delta = load_value(xsk->tx.consumer) - cons;
> > > +     /* By default, xsk should consume exact @max_budget descs at one
> > > +      * send in this case where hitting the max budget limit in while
> > > +      * loop is triggered in __xsk_generic_xmit(). Please make sure that
> > > +      * the number of descs to be sent is larger than @max_budget, or
> > > +      * else the tx.consumer will be updated in xskq_cons_peek_desc()
> > > +      * in time which hides the issue we try to verify.
> > > +      */
> > > +     if (ready_to_send > max_budget && delta != max_budget)
> > > +             return false;
> > > +
> > > +     return true;
> > > +}
> > > +
> > >  static int kick_tx(struct xsk_socket_info *xsk)
> > >  {
> > >       int ret;
> > >
> > > -     ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > +     if (xsk->check_consumer) {
> > > +             if (!kick_tx_with_check(xsk, &ret))
> > > +                     return TEST_FAILURE;
> > > +     } else {
> > > +             ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > +     }
> > >       if (ret >= 0)
> > >               return TEST_PASS;
> > >       if (errno == ENOBUFS || errno == EAGAIN || errno == EBUSY || errno == ENETDOWN) {
> > > @@ -2613,6 +2649,18 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
> > >                                  XSK_UMEM__LARGE_FRAME_SIZE * 2);
> > >  }
> > >
> > > +static int testapp_tx_queue_consumer(struct test_spec *test)
> > > +{
> > > +     int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
> > > +
> > > +     pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
> > > +     test->ifobj_tx->xsk->batch_size = nr_packets;
> > > +     if (!(test->mode & TEST_MODE_ZC))
> > > +             test->ifobj_tx->xsk->check_consumer = true;
> >
> > The test looks good to me, thank you!
> 
> Thanks.
> 
> >
> > One question here: why not exit/return for TEST_MODE_ZC instead
> > of conditionally setting check_consumer?
> 
> As you said, yes, we could skip the zc test for this
> testapp_tx_queue_consumer(). It doesn't affect the goal or result of
> the subtest. So do you expect me to respin this patch or just leave it
> as is?

Yes I think it would be worth respinning and skipping it for zc. see how
testapp_stats_rx_dropped() does it.

Otherwise we would probably never change it and just keep on running this
test case for zc which is not beneficial at this point.

Besides LGTM!

> 
> Thanks,
> Jason

Re: [PATCH net-next v5 2/2] selftests/bpf: add a new test to check the consumer update case

[Jason Xing]

On Thu, Jul 3, 2025 at 8:37 PM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Thu, Jul 03, 2025 at 07:09:09AM +0800, Jason Xing wrote:
> > On Thu, Jul 3, 2025 at 12:03 AM Stanislav Fomichev <stfomichev@gmail.com> wrote:
> > >
> > > On 07/02, Jason Xing wrote:
> > > > From: Jason Xing <kernelxing@tencent.com>
> > > >
> > > > The subtest sends 33 packets at one time on purpose to see if xsk
> > > > exitting __xsk_generic_xmit() updates the global consumer of tx queue
> > > > when reaching the max loop (max_tx_budget, 32 by default). The number 33
> > > > can avoid xskq_cons_peek_desc() updates the consumer when it's about to
> > > > quit sending, to accurately check if the issue that the first patch
> > > > resolves remains. The new case will not check this issue in zero copy
> > > > mode.
> > > >
> > > > Signed-off-by: Jason Xing <kernelxing@tencent.com>
> > > > ---
> > > > v5
> > > > Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@gmail.com/
> > > > 1. use the initial approach to add a new testcase
> > > > 2. add a new flag 'check_consumer' to see if the check is needed
> > > > ---
> > > >  tools/testing/selftests/bpf/xskxceiver.c | 51 +++++++++++++++++++++++-
> > > >  tools/testing/selftests/bpf/xskxceiver.h |  1 +
> > > >  2 files changed, 51 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > > > index 0ced4026ee44..ed12a55ecf2a 100644
> > > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > > @@ -109,6 +109,8 @@
> > > >
> > > >  #include <network_helpers.h>
> > > >
> > > > +#define MAX_TX_BUDGET_DEFAULT 32
> > > > +
> > > >  static bool opt_verbose;
> > > >  static bool opt_print_tests;
> > > >  static enum test_mode opt_mode = TEST_MODE_ALL;
> > > > @@ -1091,11 +1093,45 @@ static bool is_pkt_valid(struct pkt *pkt, void *buffer, u64 addr, u32 len)
> > > >       return true;
> > > >  }
> > > >
> > > > +static u32 load_value(u32 *counter)
> > > > +{
> > > > +     return __atomic_load_n(counter, __ATOMIC_ACQUIRE);
> > > > +}
> > > > +
> > > > +static bool kick_tx_with_check(struct xsk_socket_info *xsk, int *ret)
> > > > +{
> > > > +     u32 max_budget = MAX_TX_BUDGET_DEFAULT;
> > > > +     u32 cons, ready_to_send;
> > > > +     int delta;
> > > > +
> > > > +     cons = load_value(xsk->tx.consumer);
> > > > +     ready_to_send = load_value(xsk->tx.producer) - cons;
> > > > +     *ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > > +
> > > > +     delta = load_value(xsk->tx.consumer) - cons;
> > > > +     /* By default, xsk should consume exact @max_budget descs at one
> > > > +      * send in this case where hitting the max budget limit in while
> > > > +      * loop is triggered in __xsk_generic_xmit(). Please make sure that
> > > > +      * the number of descs to be sent is larger than @max_budget, or
> > > > +      * else the tx.consumer will be updated in xskq_cons_peek_desc()
> > > > +      * in time which hides the issue we try to verify.
> > > > +      */
> > > > +     if (ready_to_send > max_budget && delta != max_budget)
> > > > +             return false;
> > > > +
> > > > +     return true;
> > > > +}
> > > > +
> > > >  static int kick_tx(struct xsk_socket_info *xsk)
> > > >  {
> > > >       int ret;
> > > >
> > > > -     ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > > +     if (xsk->check_consumer) {
> > > > +             if (!kick_tx_with_check(xsk, &ret))
> > > > +                     return TEST_FAILURE;
> > > > +     } else {
> > > > +             ret = sendto(xsk_socket__fd(xsk->xsk), NULL, 0, MSG_DONTWAIT, NULL, 0);
> > > > +     }
> > > >       if (ret >= 0)
> > > >               return TEST_PASS;
> > > >       if (errno == ENOBUFS || errno == EAGAIN || errno == EBUSY || errno == ENETDOWN) {
> > > > @@ -2613,6 +2649,18 @@ static int testapp_adjust_tail_grow_mb(struct test_spec *test)
> > > >                                  XSK_UMEM__LARGE_FRAME_SIZE * 2);
> > > >  }
> > > >
> > > > +static int testapp_tx_queue_consumer(struct test_spec *test)
> > > > +{
> > > > +     int nr_packets = MAX_TX_BUDGET_DEFAULT + 1;
> > > > +
> > > > +     pkt_stream_replace(test, nr_packets, MIN_PKT_SIZE);
> > > > +     test->ifobj_tx->xsk->batch_size = nr_packets;
> > > > +     if (!(test->mode & TEST_MODE_ZC))
> > > > +             test->ifobj_tx->xsk->check_consumer = true;
> > >
> > > The test looks good to me, thank you!
> >
> > Thanks.
> >
> > >
> > > One question here: why not exit/return for TEST_MODE_ZC instead
> > > of conditionally setting check_consumer?
> >
> > As you said, yes, we could skip the zc test for this
> > testapp_tx_queue_consumer(). It doesn't affect the goal or result of
> > the subtest. So do you expect me to respin this patch or just leave it
> > as is?
>
> Yes I think it would be worth respinning and skipping it for zc. see how
> testapp_stats_rx_dropped() does it.

Got it. I see:
        if (test->mode == TEST_MODE_ZC) {
                ksft_test_result_skip("Can not run RX_DROPPED test for
ZC mode\n");
                return TEST_SKIP;
        }

>
> Otherwise we would probably never change it and just keep on running this
> test case for zc which is not beneficial at this point.
>
> Besides LGTM!

Thanks. Will repost it soon :)

>
> >
> > Thanks,
> > Jason