Message-ID: <aFyJHm1yRo3o4X2K@mini-arch>
Date: Wed, 25 Jun 2025 16:41:18 -0700
From: Stanislav Fomichev <stfomichev@...il.com>
To: Jason Xing <kerneljasonxing@...il.com>
Cc: Maciej Fijalkowski <maciej.fijalkowski@...el.com>, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	bjorn@...nel.org, magnus.karlsson@...el.com,
	jonathan.lemon@...il.com, sdf@...ichev.me, ast@...nel.org,
	daniel@...earbox.net, hawk@...nel.org, john.fastabend@...il.com,
	joe@...a.to, willemdebruijn.kernel@...il.com, bpf@...r.kernel.org,
	netdev@...r.kernel.org, Jason Xing <kernelxing@...cent.com>
Subject: Re: [PATCH net-next v3 2/2] selftests/bpf: check if the global
 consumer of tx queue updates after send call

On 06/25, Jason Xing wrote:
> On Wed, Jun 25, 2025 at 11:00 PM Stanislav Fomichev
> <stfomichev@...il.com> wrote:
> >
> > On 06/25, Jason Xing wrote:
> > > On Wed, Jun 25, 2025 at 8:19 PM Maciej Fijalkowski
> > > <maciej.fijalkowski@...el.com> wrote:
> > > >
> > > > On Wed, Jun 25, 2025 at 06:10:14PM +0800, Jason Xing wrote:
> > > > > From: Jason Xing <kernelxing@...cent.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, 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@...cent.com>
> > > > > ---
> > > > >  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.
> >
> > We can always update it to follow new budget. The purpose of the test
> > is to document/verify userspace expectations. Sincle even with the
> > setsockopt we are still gonna have the default budget.
> >
> > > > 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.
> >
> > IIUC, Maciej does not suggest to completely drop the test but rather
> > to move this check (unconditionally and only for skb mode) somewhere
> 
> I prefer the former: make it suitable for all the cases. Whether it's
> zero copy mode or non-zc one, the behaviour of the consumer should be

But it does not work the same in zc and non-zc modes :-( There are
a bunch of (uncodumented) quirks here and there. With a test case we
can at least highlight/document them.

Powered by blists - more mailing lists