On Thu, Jul 3, 2025 at 8:37 PM Maciej Fijalkowski <maciej.fijalkowski@xxxxxxxxx> 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@xxxxxxxxx> wrote: > > > > > > On 07/02, Jason Xing wrote: > > > > From: Jason Xing <kernelxing@xxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxx> > > > > --- > > > > v5 > > > > Link: https://lore.kernel.org/all/20250627085745.53173-1-kerneljasonxing@xxxxxxxxx/ > > > > 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
