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

From: Maciej Fijalkowski <[email-masked]>
Date: Tue, 14 Nov 2023 18:03:23 +0100

On Mon, Nov 13, 2023 at 12:20:46PM +0100, Maciej Fijalkowski wrote:
> On Mon, Nov 13, 2023 at 07:42:09AM +0100, Vyavahare, Tushar wrote:
> > 
> > 
> > > -----Original Message-----
> > > From: Fijalkowski, Maciej <[email-masked]>
> > > Sent: Wednesday, November 8, 2023 8:01 PM
> > > To: Vyavahare, Tushar <[email-masked]>
> > > Cc: [email-masked]; [email-masked]; [email-masked]; Karlsson,
> > > Magnus <[email-masked]>; [email-masked];
> > > [email-masked]; [email-masked]; [email-masked];
> > > [email-masked]; [email-masked]; Sarkar, Tirthendu
> > > <[email-masked]>
> > > 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 <[email-masked]>
> > > > Signed-off-by: Tushar Vyavahare <[email-masked]>
> > > > ---
> > > >  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?

Ok now I see that we are sort of losing context when generating pkt
streams, that's a bit unfortunate in this case. Maybe we can think of some
refactor later on.