Re: [PATCH bpf-next] selftests/xsk: fix for SEND_RECEIVE_UNALIGNED test.

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

On Mon, Nov 13, 2023 at 07:42:09AM +0100, Vyavahare, Tushar wrote:
> 
> 
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> > Sent: Wednesday, November 8, 2023 8:01 PM
> > To: Vyavahare, Tushar <tushar.vyavahare@intel.com>
> > Cc: bpf@vger.kernel.org; netdev@vger.kernel.org; bjorn@kernel.org; Karlsson,
> > Magnus <magnus.karlsson@intel.com>; jonathan.lemon@gmail.com;
> > davem@davemloft.net; kuba@kernel.org; pabeni@redhat.com;
> > ast@kernel.org; daniel@iogearbox.net; Sarkar, Tirthendu
> > <tirthendu.sarkar@intel.com>
> > Subject: Re: [PATCH bpf-next] selftests/xsk: fix for SEND_RECEIVE_UNALIGNED
> > test.
> > 
> > On Fri, Nov 03, 2023 at 02:29:36PM +0000, Tushar Vyavahare wrote:
> > > Fix test broken by shared umem test and framework enhancement commit.
> > >
> > > Correct the current implementation of pkt_stream_replace_half() by
> > > ensuring that nb_valid_entries are not set to half, as this is not
> > > true for all the tests.
> > 
> > Please be more specific - so what is the expected value for nb_valid_entries for
> > unaligned mode test then, if not the half?
> > 
> 
> The expected value for nb_valid_entries for the SEND_RECEIVE_UNALIGNED
> test would be equal to the total number of packets sent.
> 
> > >
> > > Create a new function called pkt_modify() that allows for packet
> > > modification to meet specific requirements while ensuring the accurate
> > > maintenance of the valid packet count to prevent inconsistencies in
> > > packet tracking.
> > >
> > > Fixes: 6d198a89c004 ("selftests/xsk: Add a test for shared umem
> > > feature")
> > > Reported-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> > > Signed-off-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> > > ---
> > >  tools/testing/selftests/bpf/xskxceiver.c | 71
> > > ++++++++++++++++--------
> > >  1 file changed, 47 insertions(+), 24 deletions(-)
> > >
> > > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > > b/tools/testing/selftests/bpf/xskxceiver.c
> > > index 591ca9637b23..f7d3a4a9013f 100644
> > > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > > @@ -634,16 +634,35 @@ static u32 pkt_nb_frags(u32 frame_size, struct
> > pkt_stream *pkt_stream, struct pk
> > >  	return nb_frags;
> > >  }
> > >
> > > -static void pkt_set(struct pkt_stream *pkt_stream, struct pkt *pkt,
> > > int offset, u32 len)
> > > +static bool pkt_valid(bool unaligned_mode, int offset, u32 len)
> > 
> > kinda confusing to have is_pkt_valid() and pkt_valid() functions...
> > maybe name this as set_pkt_valid() ? doesn't help much but anyways.
> > 
> 
> will do it.
> 
> > > +{
> > > +	if (len > MAX_ETH_JUMBO_SIZE || (!unaligned_mode && offset < 0))
> > > +		return false;
> > > +
> > > +	return true;
> > > +}
> > > +
> > > +static void pkt_set(struct pkt_stream *pkt_stream, struct xsk_umem_info
> > *umem, struct pkt *pkt,
> > > +		    int offset, u32 len)
> > 
> > How about adding a bool unaligned to pkt_stream instead of passing whole
> > xsk_umem_info to pkt_set - wouldn't this make the diff smaller?
> > 
> 
> We can also do it this way, but in this case, the difference will be
> larger. Wherever we are using "struct pkt_stream *pkt_stream," we must set
> this bool flag again. For example, in places like 
> __pkt_stream_replace_half(), __pkt_stream_generate_custom() , and a few
> more. I believe we should stick with the current approach.

We have a default pkt streams that are restored in run_pkt_test(), so I
believe that setting this unaligned flag could be scoped to each test_func
that is related to unaligned mode tests?

> 
> > >  {
> > >  	pkt->offset = offset;
> > >  	pkt->len = len;
> > > -	if (len > MAX_ETH_JUMBO_SIZE) {
> > > -		pkt->valid = false;
> > > -	} else {
> > > -		pkt->valid = true;
> > > +
> > > +	pkt->valid = pkt_valid(umem->unaligned_mode, offset, len);
> > > +	if (pkt->valid)
> > >  		pkt_stream->nb_valid_entries++;
> > > -	}
> > > +}
> > > +
> > > +static void pkt_modify(struct pkt_stream *pkt_stream, struct
> > xsk_umem_info *umem, struct pkt *pkt,
> > > +		       int offset, u32 len)
> > > +{
> > > +	bool mod_valid;
> > > +
> > > +	pkt->offset = offset;
> > > +	pkt->len = len;
> > > +	mod_valid  = pkt_valid(umem->unaligned_mode, offset, len);
> > 
> > double space
> > 
> 
> will do it.
> 
> > > +	pkt_stream->nb_valid_entries += mod_valid - pkt->valid;
> > > +	pkt->valid = mod_valid;
> > >  }
> > >
> > >  static u32 pkt_get_buffer_len(struct xsk_umem_info *umem, u32 len) @@
> > > -651,7 +670,8 @@ static u32 pkt_get_buffer_len(struct xsk_umem_info
> > *umem, u32 len)
> > >  	return ceil_u32(len, umem->frame_size) * umem->frame_size;  }
> > >
> > > -static struct pkt_stream *__pkt_stream_generate(u32 nb_pkts, u32
> > > pkt_len, u32 nb_start, u32 nb_off)
> > > +static struct pkt_stream *__pkt_stream_generate(struct xsk_umem_info
> > *umem, u32 nb_pkts,
> > > +						u32 pkt_len, u32 nb_start,
> > u32 nb_off)
> > >  {
> > >  	struct pkt_stream *pkt_stream;
> > >  	u32 i;
> > > @@ -665,30 +685,31 @@ static struct pkt_stream
> > *__pkt_stream_generate(u32 nb_pkts, u32 pkt_len, u32 nb
> > >  	for (i = 0; i < nb_pkts; i++) {
> > >  		struct pkt *pkt = &pkt_stream->pkts[i];
> > >
> > > -		pkt_set(pkt_stream, pkt, 0, pkt_len);
> > > +		pkt_set(pkt_stream, umem, pkt, 0, pkt_len);
> > >  		pkt->pkt_nb = nb_start + i * nb_off;
> > >  	}
> > >
> > >  	return pkt_stream;
> > >  }
> > >
> > > -static struct pkt_stream *pkt_stream_generate(u32 nb_pkts, u32
> > > pkt_len)
> > > +static struct pkt_stream *pkt_stream_generate(struct xsk_umem_info
> > > +*umem, u32 nb_pkts, u32 pkt_len)
> > >  {
> > > -	return __pkt_stream_generate(nb_pkts, pkt_len, 0, 1);
> > > +	return __pkt_stream_generate(umem, nb_pkts, pkt_len, 0, 1);
> > >  }
> > >
> > > -static struct pkt_stream *pkt_stream_clone(struct pkt_stream
> > > *pkt_stream)
> > > +static struct pkt_stream *pkt_stream_clone(struct pkt_stream *pkt_stream,
> > > +					   struct xsk_umem_info *umem)
> > >  {
> > > -	return pkt_stream_generate(pkt_stream->nb_pkts, pkt_stream-
> > >pkts[0].len);
> > > +	return pkt_stream_generate(umem, pkt_stream->nb_pkts,
> > > +pkt_stream->pkts[0].len);
> > >  }
> > >
> > >  static void pkt_stream_replace(struct test_spec *test, u32 nb_pkts,
> > > u32 pkt_len)  {
> > >  	struct pkt_stream *pkt_stream;
> > >
> > > -	pkt_stream = pkt_stream_generate(nb_pkts, pkt_len);
> > > +	pkt_stream = pkt_stream_generate(test->ifobj_rx->umem, nb_pkts,
> > > +pkt_len);
> > >  	test->ifobj_tx->xsk->pkt_stream = pkt_stream;
> > > -	pkt_stream = pkt_stream_generate(nb_pkts, pkt_len);
> > > +	pkt_stream = pkt_stream_generate(test->ifobj_tx->umem, nb_pkts,
> > > +pkt_len);
> > >  	test->ifobj_rx->xsk->pkt_stream = pkt_stream;  }
> > >
> > > @@ -698,12 +719,11 @@ static void __pkt_stream_replace_half(struct
> > ifobject *ifobj, u32 pkt_len,
> > >  	struct pkt_stream *pkt_stream;
> > >  	u32 i;
> > >
> > > -	pkt_stream = pkt_stream_clone(ifobj->xsk->pkt_stream);
> > > +	pkt_stream = pkt_stream_clone(ifobj->xsk->pkt_stream, ifobj-
> > >umem);
> > >  	for (i = 1; i < ifobj->xsk->pkt_stream->nb_pkts; i += 2)
> > > -		pkt_set(pkt_stream, &pkt_stream->pkts[i], offset, pkt_len);
> > > +		pkt_modify(pkt_stream, ifobj->umem, &pkt_stream->pkts[i],
> > offset,
> > > +pkt_len);
> > >
> > >  	ifobj->xsk->pkt_stream = pkt_stream;
> > > -	pkt_stream->nb_valid_entries /= 2;
> > >  }
> > >
> > >  static void pkt_stream_replace_half(struct test_spec *test, u32
> > > pkt_len, int offset) @@ -715,9 +735,10 @@ static void
> > > pkt_stream_replace_half(struct test_spec *test, u32 pkt_len, int off
> > > static void pkt_stream_receive_half(struct test_spec *test)  {
> > >  	struct pkt_stream *pkt_stream = test->ifobj_tx->xsk->pkt_stream;
> > > +	struct xsk_umem_info *umem = test->ifobj_rx->umem;
> > >  	u32 i;
> > >
> > > -	test->ifobj_rx->xsk->pkt_stream = pkt_stream_generate(pkt_stream-
> > >nb_pkts,
> > > +	test->ifobj_rx->xsk->pkt_stream = pkt_stream_generate(umem,
> > > +pkt_stream->nb_pkts,
> > >  							      pkt_stream-
> > >pkts[0].len);
> > >  	pkt_stream = test->ifobj_rx->xsk->pkt_stream;
> > >  	for (i = 1; i < pkt_stream->nb_pkts; i += 2) @@ -733,12 +754,12 @@
> > > static void pkt_stream_even_odd_sequence(struct test_spec *test)
> > >
> > >  	for (i = 0; i < test->nb_sockets; i++) {
> > >  		pkt_stream = test->ifobj_tx->xsk_arr[i].pkt_stream;
> > > -		pkt_stream = __pkt_stream_generate(pkt_stream->nb_pkts /
> > 2,
> > > +		pkt_stream = __pkt_stream_generate(test->ifobj_tx->umem,
> > > +pkt_stream->nb_pkts / 2,
> > >  						   pkt_stream->pkts[0].len, i,
> > 2);
> > >  		test->ifobj_tx->xsk_arr[i].pkt_stream = pkt_stream;
> > >
> > >  		pkt_stream = test->ifobj_rx->xsk_arr[i].pkt_stream;
> > > -		pkt_stream = __pkt_stream_generate(pkt_stream->nb_pkts /
> > 2,
> > > +		pkt_stream = __pkt_stream_generate(test->ifobj_rx->umem,
> > > +pkt_stream->nb_pkts / 2,
> > >  						   pkt_stream->pkts[0].len, i,
> > 2);
> > >  		test->ifobj_rx->xsk_arr[i].pkt_stream = pkt_stream;
> > >  	}
> > > @@ -1961,7 +1982,8 @@ static int testapp_stats_tx_invalid_descs(struct
> > > test_spec *test)  static int testapp_stats_rx_full(struct test_spec
> > > *test)  {
> > >  	pkt_stream_replace(test, DEFAULT_UMEM_BUFFERS +
> > DEFAULT_UMEM_BUFFERS / 2, MIN_PKT_SIZE);
> > > -	test->ifobj_rx->xsk->pkt_stream =
> > pkt_stream_generate(DEFAULT_UMEM_BUFFERS, MIN_PKT_SIZE);
> > > +	test->ifobj_rx->xsk->pkt_stream = pkt_stream_generate(test-
> > >ifobj_rx->umem,
> > > +
> > DEFAULT_UMEM_BUFFERS, MIN_PKT_SIZE);
> > >
> > >  	test->ifobj_rx->xsk->rxqsize = DEFAULT_UMEM_BUFFERS;
> > >  	test->ifobj_rx->release_rx = false;
> > > @@ -1972,7 +1994,8 @@ static int testapp_stats_rx_full(struct
> > > test_spec *test)  static int testapp_stats_fill_empty(struct test_spec
> > > *test)  {
> > >  	pkt_stream_replace(test, DEFAULT_UMEM_BUFFERS +
> > DEFAULT_UMEM_BUFFERS / 2, MIN_PKT_SIZE);
> > > -	test->ifobj_rx->xsk->pkt_stream =
> > pkt_stream_generate(DEFAULT_UMEM_BUFFERS, MIN_PKT_SIZE);
> > > +	test->ifobj_rx->xsk->pkt_stream = pkt_stream_generate(test-
> > >ifobj_rx->umem,
> > > +
> > DEFAULT_UMEM_BUFFERS, MIN_PKT_SIZE);
> > >
> > >  	test->ifobj_rx->use_fill_ring = false;
> > >  	test->ifobj_rx->validation_func = validate_fill_empty; @@ -2526,8
> > > +2549,8 @@ int main(int argc, char **argv)
> > >  	init_iface(ifobj_tx, worker_testapp_validate_tx);
> > >
> > >  	test_spec_init(&test, ifobj_tx, ifobj_rx, 0, &tests[0]);
> > > -	tx_pkt_stream_default = pkt_stream_generate(DEFAULT_PKT_CNT,
> > MIN_PKT_SIZE);
> > > -	rx_pkt_stream_default = pkt_stream_generate(DEFAULT_PKT_CNT,
> > MIN_PKT_SIZE);
> > > +	tx_pkt_stream_default = pkt_stream_generate(ifobj_tx->umem,
> > DEFAULT_PKT_CNT, MIN_PKT_SIZE);
> > > +	rx_pkt_stream_default = pkt_stream_generate(ifobj_rx->umem,
> > > +DEFAULT_PKT_CNT, MIN_PKT_SIZE);
> > >  	if (!tx_pkt_stream_default || !rx_pkt_stream_default)
> > >  		exit_with_error(ENOMEM);
> > >  	test.tx_pkt_stream_default = tx_pkt_stream_default;
> > > --
> > > 2.34.1
> > >