Message-ID: <CAL+tcoCSkXTJMPA7NQ7yEObmd2+HZ7mmppknq+yUUk=H4qYNow@mail.gmail.com>
Date: Wed, 25 Jun 2025 23:35:24 +0800
From: Jason Xing <kerneljasonxing@...il.com>
To: Stanislav Fomichev <stfomichev@...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 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
the same: update when finishing sending packets. I will give it a try
again tomorrow since it's too late for me right now. Sorry.

Thanks,
Jason

Powered by blists - more mailing lists