| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <aGZ5dq5P0G8e8A/J@boxer>
Date: Thu, 3 Jul 2025 14:37:10 +0200
From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
To: Jason Xing <kerneljasonxing@...il.com>
CC: Stanislav Fomichev <stfomichev@...il.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 v5 2/2] selftests/bpf: add a new test to check
the consumer update case
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@...il.com> wrote:
> >
> > On 07/02, 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 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@...cent.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
Powered by blists - more mailing lists