Date:   Fri, 29 Jul 2022 13:29:03 +0000
From:   "Koikkara Reeny, Shibin" <shibin.koikkara.reeny@...el.com>
To:     "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
CC:     "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "ast@...nel.org" <ast@...nel.org>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        "bjorn@...nel.org" <bjorn@...nel.org>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "andrii@...nel.org" <andrii@...nel.org>,
        "Loftus, Ciara" <ciara.loftus@...el.com>
Subject: RE: [PATCH bpf-next] selftests: xsk: Update poll test cases



> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> Sent: Wednesday, July 27, 2022 4:26 PM
> To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@...el.com>
> Cc: bpf@...r.kernel.org; ast@...nel.org; daniel@...earbox.net;
> netdev@...r.kernel.org; Karlsson, Magnus <magnus.karlsson@...el.com>;
> bjorn@...nel.org; kuba@...nel.org; andrii@...nel.org; Loftus, Ciara
> <ciara.loftus@...el.com>
> Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> 
> On Wed, Jul 27, 2022 at 11:49:57AM +0100, Koikkara Reeny, Shibin wrote:
> >
> > > -----Original Message-----
> > > From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> > > Sent: Tuesday, July 26, 2022 3:10 PM
> > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@...el.com>
> > > Cc: bpf@...r.kernel.org; ast@...nel.org; daniel@...earbox.net;
> > > netdev@...r.kernel.org; Karlsson, Magnus
> > > <magnus.karlsson@...el.com>; bjorn@...nel.org; kuba@...nel.org;
> > > andrii@...nel.org; Loftus, Ciara <ciara.loftus@...el.com>
> > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test cases
> > >
> > > On Tue, Jul 26, 2022 at 10:43:36AM +0100, Koikkara Reeny, Shibin wrote:
> > > > > -----Original Message-----
> > > > > From: Maciej Fijalkowski <maciej.fijalkowski@...el.com>
> > > > > Sent: Friday, July 22, 2022 3:16 PM
> > > > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@...el.com>
> > > > > Cc: bpf@...r.kernel.org; ast@...nel.org; daniel@...earbox.net;
> > > > > netdev@...r.kernel.org; Karlsson, Magnus
> > > > > <magnus.karlsson@...el.com>; bjorn@...nel.org; kuba@...nel.org;
> > > > > andrii@...nel.org; Loftus, Ciara <ciara.loftus@...el.com>
> > > > > Subject: Re: [PATCH bpf-next] selftests: xsk: Update poll test
> > > > > cases
> > > > >
> > > > > On Mon, Jul 18, 2022 at 09:57:12AM +0000, Shibin Koikkara Reeny
> wrote:
> > > > > > Poll test case was not testing all the functionality of the
> > > > > > poll feature in the testsuite. This patch update the poll test
> > > > > > case with 2 more testcases to check the timeout features.
> > > > > >
> > > > > > Poll test case have 4 sub test cases:
> > > > >
> > > > > Hi Shibin,
> > > > >
> > > > > Kinda not clear with count of added test cases, at first you say
> > > > > you add 2 more but then you mention something about 4 sub test
> cases.
> > > > >
> > > > > To me these are separate test cases.
> > > > >
> > > > Hi Maciej,
> > > >
> > > > Will update it in V2
> > > >
> > > > > >
> > > > > > 1. TEST_TYPE_RX_POLL:
> > > > > > Check if POLLIN function work as expect.
> > > > > >
> > > > > > 2. TEST_TYPE_TX_POLL:
> > > > > > Check if POLLOUT function work as expect.
> > > > >
> > > > > From run_pkt_test, I don't see any difference between 1 and 2.
> > > > > Why split then?
> > > > >
> > > >
> > > >
> > > > It was done to show which case exactly broke. If RX poll event or
> > > > TX poll event
> > > >
> > > > > >
> > > > > > 3. TEST_TYPE_POLL_RXQ_EMPTY:
> > > > >
> > > > > 3 and 4 don't match with the code here
> > > > > (TEST_TYPE_POLL_{R,T}XQ_TMOUT)
> > > > >
> > > > > > call poll function with parameter POLLIN on empty rx queue
> > > > > > will cause timeout.If return timeout then test case is pass.
> > > > > >
> > > >
> > > >
> > > > True but  It was change to RXQ_EMPTY and TXQ_FULL from _TMOUT to
> > > make
> > > > it more clearer to what exactly is happening to cause timeout.
> > > >
> > > > > > 4. TEST_TYPE_POLL_TXQ_FULL:
> > > > > > When txq is filled and packets are not cleaned by the kernel
> > > > > > then if we invoke the poll function with POLLOUT then it
> > > > > > should trigger timeout.If return timeout then test case is pass.
> > > > > >
> > > > > > Signed-off-by: Shibin Koikkara Reeny
> > > > > > <shibin.koikkara.reeny@...el.com>
> > > > > > ---
> > > > > >  tools/testing/selftests/bpf/xskxceiver.c | 173
> > > > > > +++++++++++++++++------
> > > > > > +++++++++++++++++tools/testing/selftests/bpf/xskxceiver.h
> > > > > > +++++++++++++++++|
> > > > > > 10 +-
> > > > > >  2 files changed, 139 insertions(+), 44 deletions(-)
> > > > > >
> > > > > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > index 74d56d971baf..8ecab3a47c9e 100644
> > > > > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > > > > @@ -424,6 +424,8 @@ static void __test_spec_init(struct
> > > > > > test_spec *test, struct ifobject *ifobj_tx,
> > > > > >
> > > > > >  		ifobj->xsk = &ifobj->xsk_arr[0];
> > > > > >  		ifobj->use_poll = false;
> > > > > > +		ifobj->skip_rx = false;
> > > > > > +		ifobj->skip_tx = false;
> > > > >
> > > > > Any chances of trying to avoid these booleans? Not that it's a
> > > > > hard nack, but the less booleans we spread around in this code the
> better.
> > > >
> > > >
> > > > Not sure if it is possible but using any other logic will make the
> > > > code more complex and less readable.
> > >
> > > How did you come with such judgement? You didn't even try the idea
> > > that I gave to you about having a testapp_validate_traffic()
> > > equivalent with a single thread.
> > >
> >
> > Hi Maciej,
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index 4394788829bf..0b58e026f2a2 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -1317,6 +1317,24 @@ static void *worker_testapp_validate_rx(void
> *arg)
> >         pthread_exit(NULL);
> >  }
> >
> > +static int testapp_validate_traffic_txq_tmout(struct test_spec *test)
> > +{
> > +       struct ifobject *ifobj_tx = test->ifobj_tx;
> > +       pthread_t t0;
> > +
> > +       if (pthread_barrier_init(&barr, NULL, 2))
> > +               exit_with_error(errno);
> > +
> > +       test->current_step++;
> > +       pkt_stream_reset(ifobj_rx->pkt_stream);
> > +
> > +       pthread_create(&t0, NULL, ifobj_tx->func_ptr, test);
> > +       pthread_join(t0, NULL);
> > +
> > +       return !!test->fail;
> > +}
> > +
> >
> > This is what you are suggesting do ?
> >
> > My point is ifobj_tx->func_ptr calls worker_testapp_validate_tx() ==>
> send_pkts() ==> __send_pkts().
> >
> > Normal case when poll timeout happen send_pkts() return TEST_FAILURE
> which is expected.
> > Test Case like TEST_TYPE_POLL_TXQ_TMOUT and
> TEST_TYPE_POLL_RXQ_TMOUT
> > when poll timeout happen it should return TEST_PASS rather than
> > TEST_FAILURE. How should I let the send_pkts() to know what timeout
> type of test is running without a new variable or flag?
> > Then boolean skip_rx and skip_tx are both used in the send_pkts() and
> receive_pkts().
> >
> > This is why I thought it might be complex but if you have new suggestion I
> open to try it.
> 
> In such case you could just lookup if test->ifobj_rx has valid umem pointer or
> xsk socket. If you didn't spawn rx thread then you know that resources for Rx
> are not initialized, thread_common_ops() was not called there. And you
> have a pointer to test_spec which allows you to dig up other ifobject. There
> are also {r,t}x_on booleans which probably could be used for your purposes,
> couldn't they? You are not testing the bidirectional traffic, you're rather
> working on a single thread in one direction.
> 
> Do you see what I'm trying to explain?
> 

Sure. I have updated it in the Patch v3.

> And sorry if I sounded mean or something in previous message exchanges,
> just some harsh stuff on my side that is happening. We're only humans, after
> all ;)

No worries... 😉

> 
> >
> > > >
> > > > >
> > > > > >  		ifobj->use_fill_ring = true;
> > > > > >  		ifobj->release_rx = true;
> > > > > >  		ifobj->pkt_stream = test->pkt_stream_default; @@
> -589,6
> > > > > +591,19 @@
> > > > > > static struct pkt_stream *pkt_stream_clone(struct
> > > > > > xsk_umem_info
> > > > > *umem,
> > > > > >  	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > > > > pkt_stream->pkts[0].len);  }
> > > > > >
> > > > > > +static void pkt_stream_invalid(struct test_spec *test, u32
> > > > > > +nb_pkts,
> > > > > > +u32 pkt_len) {
> > > > > > +	struct pkt_stream *pkt_stream;
> > > > > > +	u32 i;
> > > > > > +
> > > > > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem,
> > > > > nb_pkts, pkt_len);
> > > > > > +	for (i = 0; i < nb_pkts; i++)
> > > > > > +		pkt_stream->pkts[i].valid = false;
> > > > > > +
> > > > > > +	test->ifobj_tx->pkt_stream = pkt_stream;
> > > > > > +	test->ifobj_rx->pkt_stream = pkt_stream; }
> > > > >
> > > > > Please explain how this work, e.g. why you need to have to have
> > > > > invalid pkt stream + avoiding launching rx thread and why one of
> > > > > them is
> > > not enough.
> > > > >
> > > > > Personally I think this is not needed. When calling
> > > > > pkt_stream_generate(), validity of pkt is set based on length of
> > > > > packet vs
> > > frame size:
> > > > >
> > > > > 		if (pkt_len > umem->frame_size)
> > > > > 			pkt_stream->pkts[i].valid = false;
> > > > >
> > > > > so couldn't you use 2k frame size and bigger length of a packet?
> > > > >
> > > > This function was introduced for TEST_TYPE_POLL_TXQ_FULL keep the
> > > > TX full and stop nofying the kernel that there is packet to cleanup.
> > > > So we are manually setting the packets to invalid. This help to
> > > > keep the __send_pkts() more generic and reduce the if conditions.
> > > > ex: xsk_ring_prod__submit() is not needed to be added inside if
> condition.
> > >
> > > I understand the intend behind it but what I was saying was that you
> > > have everything ready to be used without a need for introducing new
> functions.
> > > You could also try out what I suggested just to see if this makes
> > > things simpler.
> > >
> >
> > Are you suggesting to do this ?
> 
> Yes
> 
> >                 test->ifobj_tx->use_poll = true;
> > -               pkt_stream_invalid(test, 2 * DEFAULT_PKT_CNT, PKT_SIZE);
> > +               test->ifobj_tx->umem->frame_size = 2048;
> > +               pkt_stream_replace(test, 2 * DEFAULT_PKT_CNT, 2048);
> >                 testapp_validate_traffic(test);
> >
> >
> > > >
> > > > You are right we don't need rx stream but thought it will be good
> > > > to keep as can be used for other features in future and will be more
> generic.
> 
> Yeah if you really want I'll be ok with pkt_stream_invalid().

Let's see how it looks. I have updated it in the patch v3.

https://lore.kernel.org/bpf/20220729132337.211443-1-shibin.koikkara.reeny@intel.com/

> 
> > >

Powered by blists - more mailing lists