On Mon, Nov 13, 2023 at 07:42:09AM +0100, Vyavahare, Tushar wrote:
>
>
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>
> > Sent: Wednesday, November 8, 2023 8:01 PM
> > To: Vyavahare, Tushar <tushar.vyavahare@xxxxxxxxx>
> > Cc: bpf@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; bjorn@xxxxxxxxxx; Karlsson,
> > Magnus <magnus.karlsson@xxxxxxxxx>; jonathan.lemon@xxxxxxxxx;
> > davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> > ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; Sarkar, Tirthendu
> > <tirthendu.sarkar@xxxxxxxxx>
> > 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@xxxxxxxxx>
> > > Signed-off-by: Tushar Vyavahare <tushar.vyavahare@xxxxxxxxx>
> > > ---
> > > 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
> > >
