On Wed, Jun 25, 2025 at 8:19 PM Maciej Fijalkowski
<maciej.fijalkowski@xxxxxxxxx> wrote:
>
> On Wed, Jun 25, 2025 at 06:10:14PM +0800, 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, to accurately
> > check if the issue that the first patch resolves remains.
> >
> > Speaking of the selftest implementation, it's not possible to use the
> > normal validation_func to check if the issue happens because the whole
> > send packets logic will call the sendto multiple times such that we're
> > unable to detect in time.
> >
> > Signed-off-by: Jason Xing <kernelxing@xxxxxxxxxxx>
> > ---
> > tools/testing/selftests/bpf/xskxceiver.c | 30 ++++++++++++++++++++++--
> > 1 file changed, 28 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> > index 0ced4026ee44..f7aa83706bc7 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
>
> and what if in the future you would increase the generic xmit budget on
> the system? it would be better to wait with test addition when you
> introduce the setsockopt patch.
>
> plus keep in mind that xskxceiver tests ZC drivers as well. so either we
> should have a test that serves all modes or keep it for skb mode only.
>
> > +
> > static bool opt_verbose;
> > static bool opt_print_tests;
> > static enum test_mode opt_mode = TEST_MODE_ALL;
> > @@ -1323,7 +1325,8 @@ static int receive_pkts(struct test_spec *test)
> > return TEST_PASS;
> > }
> >
> > -static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, bool timeout)
> > +static int __send_pkts(struct test_spec *test, struct ifobject *ifobject,
> > + struct xsk_socket_info *xsk, bool timeout)
> > {
> > u32 i, idx = 0, valid_pkts = 0, valid_frags = 0, buffer_len;
> > struct pkt_stream *pkt_stream = xsk->pkt_stream;
> > @@ -1437,9 +1440,21 @@ static int __send_pkts(struct ifobject *ifobject, struct xsk_socket_info *xsk, b
> > }
> >
> > if (!timeout) {
> > + int prev_tx_consumer;
> > +
> > + if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE))
> > + prev_tx_consumer = *xsk->tx.consumer;
> > +
> > if (complete_pkts(xsk, i))
> > return TEST_FAILURE;
> >
> > + if (!strncmp("TX_QUEUE_CONSUMER", test->name, MAX_TEST_NAME_SIZE)) {
> > + int delta = *xsk->tx.consumer - prev_tx_consumer;
>
> hacking the data path logic for single test purpose is rather not good.
> I am also not really sure if this deserves a standalone test case or could
> we just introduce a check in data path in appropriate place.
The big headache is that if we expect to detect such a case, we have
to re-invent a similar send packet logic or hack the data path (a bit
like this patch). I admit it's ugly as I mentioned yesterday.
Sorry, Stanislav, no offense here. If you read this, please don't
blame me. I know you wish me to add one related test case. So here we
are. Since Maciej brought up the similar thought, I keep wondering if
we should give up such a standalone test patch? Honestly it already
involved more time than expected. The primary reason for me is that
the issue doesn't cause much trouble to the application.
Thanks,
Jason
>
> > +
> > + if (delta != MAX_TX_BUDGET_DEFAULT)
> > + return TEST_FAILURE;
> > + }
> > +
> > usleep(10);
> > return TEST_PASS;
> > }
> > @@ -1492,7 +1507,7 @@ static int send_pkts(struct test_spec *test, struct ifobject *ifobject)
> > __set_bit(i, bitmap);
> > continue;
> > }
> > - ret = __send_pkts(ifobject, &ifobject->xsk_arr[i], timeout);
> > + ret = __send_pkts(test, ifobject, &ifobject->xsk_arr[i], timeout);
> > if (ret == TEST_CONTINUE && !test->fail)
> > continue;
> >
> > @@ -2613,6 +2628,16 @@ 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;
> > +
> > + return testapp_validate_traffic(test);
> > +}
> > +
> > static void run_pkt_test(struct test_spec *test)
> > {
> > int ret;
> > @@ -2723,6 +2748,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)
> > --
> > 2.41.3
> >
