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

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

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

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.