Re: [PATCH bpf-next] selftests: xsk: Update poll test cases

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 27, 2022 at 11:49:57AM +0100, Koikkara Reeny, Shibin wrote:
> 
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> > Sent: Tuesday, July 26, 2022 3:10 PM
> > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > netdev@vger.kernel.org; Karlsson, Magnus <magnus.karlsson@intel.com>;
> > bjorn@kernel.org; kuba@kernel.org; andrii@kernel.org; Loftus, Ciara
> > <ciara.loftus@intel.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@intel.com>
> > > > Sent: Friday, July 22, 2022 3:16 PM
> > > > To: Koikkara Reeny, Shibin <shibin.koikkara.reeny@intel.com>
> > > > Cc: bpf@vger.kernel.org; ast@kernel.org; daniel@iogearbox.net;
> > > > netdev@vger.kernel.org; Karlsson, Magnus
> > > > <magnus.karlsson@intel.com>; bjorn@kernel.org; kuba@kernel.org;
> > > > andrii@kernel.org; Loftus, Ciara <ciara.loftus@intel.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@intel.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?

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

> 
> > >
> > > >
> > > > >  		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().

> >