> -----Original Message-----
> From: Magnus Karlsson <magnus.karlsson@xxxxxxxxx>
> Sent: Thursday, September 21, 2023 12:31 PM
> To: Vyavahare, Tushar <tushar.vyavahare@xxxxxxxxx>
> Cc: bpf@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; bjorn@xxxxxxxxxx;
> Karlsson, Magnus <magnus.karlsson@xxxxxxxxx>; Fijalkowski, Maciej
> <maciej.fijalkowski@xxxxxxxxx>; jonathan.lemon@xxxxxxxxx;
> davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; Sarkar, Tirthendu
> <tirthendu.sarkar@xxxxxxxxx>
> Subject: Re: [PATCH bpf-next 6/8] selftests/xsk: iterate over all the sockets in
> the send pkts function
>
> On Mon, 18 Sept 2023 at 11:15, Tushar Vyavahare
> <tushar.vyavahare@xxxxxxxxx> wrote:
> >
> > Update send_pkts() to handle multiple sockets for sending packets.
> > Multiple TX sockets are utilized alternately based on the batch size
> > for improve packet transmission.
>
> I do not know if it is "improved" ;-), but it is good to test sending from
> multiple sockets. Please make that clearer.
>
> > Signed-off-by: Tushar Vyavahare <tushar.vyavahare@xxxxxxxxx>
> > ---
> > tools/testing/selftests/bpf/xskxceiver.c | 64
> > +++++++++++++++++-------
> > 1 file changed, 45 insertions(+), 19 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index e67032f04a74..0ef0575c095c 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -1204,13 +1204,13 @@ static int receive_pkts(struct test_spec *test)
> > return TEST_PASS;
> > }
> >
> > -static int __send_pkts(struct ifobject *ifobject, struct pollfd *fds,
> > bool timeout)
> > +static int __send_pkts(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 = ifobject->xsk->pkt_stream;
> > - struct xsk_socket_info *xsk = ifobject->xsk;
> > + struct pkt_stream *pkt_stream = xsk->pkt_stream;
> > struct xsk_umem_info *umem = ifobject->umem;
> > bool use_poll = ifobject->use_poll;
> > + struct pollfd fds = { };
> > int ret;
> >
> > buffer_len = pkt_get_buffer_len(umem,
> > pkt_stream->max_pkt_len); @@ -1222,9 +1222,12 @@ static int
> __send_pkts(struct ifobject *ifobject, struct pollfd *fds, bool timeo
> > return TEST_CONTINUE;
> > }
> >
> > + fds.fd = xsk_socket__fd(xsk->xsk);
> > + fds.events = POLLOUT;
> > +
> > while (xsk_ring_prod__reserve(&xsk->tx, BATCH_SIZE, &idx) <
> BATCH_SIZE) {
> > if (use_poll) {
> > - ret = poll(fds, 1, POLL_TMOUT);
> > + ret = poll(&fds, 1, POLL_TMOUT);
> > if (timeout) {
> > if (ret < 0) {
> > ksft_print_msg("ERROR: [%s]
> > Poll error %d\n", @@ -1303,7 +1306,7 @@ static int __send_pkts(struct
> ifobject *ifobject, struct pollfd *fds, bool timeo
> > xsk->outstanding_tx += valid_frags;
> >
> > if (use_poll) {
> > - ret = poll(fds, 1, POLL_TMOUT);
> > + ret = poll(&fds, 1, POLL_TMOUT);
> > if (ret <= 0) {
> > if (ret == 0 && timeout)
> > return TEST_PASS; @@ -1349,27 +1352,50
> > @@ static int wait_for_tx_completion(struct xsk_socket_info *xsk)
> > return TEST_PASS;
> > }
> >
> > +bool all_packets_sent(struct test_spec *test, unsigned long *bitmap)
> > +{
> > + if (test_bit(test->nb_sockets, bitmap))
> > + return true;
>
> This does not seem to be correct. You are testing one bit here, but are you
> not supposed to test that all bits have been set?
>
Yes, I will fix that.
> > +
> > + return false;
> > +}
> > +
> > static int send_pkts(struct test_spec *test, struct ifobject
> > *ifobject) {
> > - struct pkt_stream *pkt_stream = ifobject->xsk->pkt_stream;
> > bool timeout = !is_umem_valid(test->ifobj_rx);
> > - struct pollfd fds = { };
> > - u32 ret;
> > + u32 i, ret;
> >
> > - fds.fd = xsk_socket__fd(ifobject->xsk->xsk);
> > - fds.events = POLLOUT;
> > + DECLARE_BITMAP(bitmap, MAX_SOCKETS);
>
> Should be with the declarations in RCT order.
>
Yes, I will do.
> >
> > - while (pkt_stream->current_pkt_nb < pkt_stream->nb_pkts) {
> > - ret = __send_pkts(ifobject, &fds, timeout);
> > - if (ret == TEST_CONTINUE && !test->fail)
> > - continue;
> > - if ((ret || test->fail) && !timeout)
> > - return TEST_FAILURE;
> > - if (ret == TEST_PASS && timeout)
> > - return ret;
> > + while (!(all_packets_sent(test, bitmap))) {
> > + for (i = 0; i < test->nb_sockets; i++) {
> > + struct pkt_stream *pkt_stream;
> > +
> > + pkt_stream = ifobject->xsk_arr[i].pkt_stream;
> > + if (!pkt_stream || pkt_stream->current_pkt_nb
> > + >= pkt_stream->nb_pkts) {
>
> Can pkt_stream be NULL?
>
Yes, in the swap_xsk_resources() function, we are setting 'pkt_stream' to NULL. [patch 4 change]
> > + __test_and_set_bit((1 << i), bitmap);
>
> test_and_set? You are not testing anything here so it is enough to just set it.
>
> > + continue;
> > + }
> > + ret = __send_pkts(ifobject, &ifobject->xsk_arr[i], timeout);
> > + if (ret == TEST_CONTINUE && !test->fail)
> > + continue;
> > +
> > + if ((ret || test->fail) && !timeout)
> > + return TEST_FAILURE;
> > +
> > + if (ret == TEST_PASS && timeout)
> > + return ret;
> > +
> > + ret = wait_for_tx_completion(&ifobject->xsk_arr[i]);
> > + if ((ret || test->fail) && !timeout)
> > + return TEST_FAILURE;
> > +
> > + if (ret == TEST_PASS && timeout)
> > + return ret;
>
> Why testing the same things before and after wait_for_tx_completion?
> Should it not be fine to just do it in one place?
>
I will change it.
> > + }
> > }
> >
> > - return wait_for_tx_completion(ifobject->xsk);
> > + return TEST_PASS;
> > }
> >
> > static int get_xsk_stats(struct xsk_socket *xsk, struct
> > xdp_statistics *stats)
> > --
> > 2.34.1
> >
> >
