RE: [PATCH bpf-next 6/8] selftests/xsk: iterate over all the sockets in the send pkts function

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




> -----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
> >
> >




[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux