> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>
> Sent: Monday, March 3, 2025 9:45 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 v2 2/2] selftests/xsk: Add tail adjustment tests
> and support check
>
> On Fri, Feb 28, 2025 at 10:56:19AM +0100, Vyavahare, Tushar wrote:
> >
> >
> > > -----Original Message-----
> > > From: Fijalkowski, Maciej <maciej.fijalkowski@xxxxxxxxx>
> > > Sent: Thursday, February 27, 2025 11:52 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 v2 2/2] selftests/xsk: Add tail
> > > adjustment tests and support check
> > >
> > > On Thu, Feb 27, 2025 at 02:27:37PM +0000, Tushar Vyavahare wrote:
> > > > Introduce tail adjustment functionality in xskxceiver using
> > > > bpf_xdp_adjust_tail(). Add `xsk_xdp_adjust_tail` to modify packet
> > > > sizes and drop unmodified packets. Implement
> > > > `is_adjust_tail_supported` to check helper availability. Develop
> > > > packet resizing tests, including shrinking and growing scenarios,
> > > > with functions for both single-buffer and multi-buffer cases.
> > > > Update the test framework to handle various scenarios and adjust MTU
> settings.
> > > > These changes enhance the testing of packet tail adjustments,
> > > > improving
> > > AF_XDP framework reliability.
> > > >
> > > > Signed-off-by: Tushar Vyavahare <tushar.vyavahare@xxxxxxxxx>
> > > > ---
> > > > .../selftests/bpf/progs/xsk_xdp_progs.c | 48 +++++++
> > > > tools/testing/selftests/bpf/xsk_xdp_common.h | 1 +
> > > > tools/testing/selftests/bpf/xskxceiver.c | 118 +++++++++++++++++-
> > > > tools/testing/selftests/bpf/xskxceiver.h | 2 +
> > > > 4 files changed, 167 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/progs/xsk_xdp_progs.c
> > > > b/tools/testing/selftests/bpf/progs/xsk_xdp_progs.c
> > > > index ccde6a4c6319..2e8e2faf17e0 100644
> > > > --- a/tools/testing/selftests/bpf/progs/xsk_xdp_progs.c
> > > > +++ b/tools/testing/selftests/bpf/progs/xsk_xdp_progs.c
> > > > @@ -4,6 +4,8 @@
> > > > #include <linux/bpf.h>
> > > > #include <bpf/bpf_helpers.h>
> > > > #include <linux/if_ether.h>
> > > > +#include <linux/ip.h>
> > > > +#include <linux/errno.h>
> > > > #include "xsk_xdp_common.h"
> > > >
> > > > struct {
> > > > @@ -70,4 +72,50 @@ SEC("xdp") int xsk_xdp_shared_umem(struct
> > > > xdp_md
> > > *xdp)
> > > > return bpf_redirect_map(&xsk, idx, XDP_DROP); }
> > > >
> > > > +SEC("xdp.frags") int xsk_xdp_adjust_tail(struct xdp_md *xdp) {
> > > > + __u32 buff_len, curr_buff_len;
> > > > + int ret;
> > > > +
> > > > + buff_len = bpf_xdp_get_buff_len(xdp);
> > > > + if (buff_len == 0)
> > > > + return XDP_DROP;
> > > > +
> > > > + ret = bpf_xdp_adjust_tail(xdp, count);
> > > > + if (ret < 0) {
> > > > + /* Handle unsupported cases */
> > > > + if (ret == -EOPNOTSUPP) {
> > > > + /* Set count to -EOPNOTSUPP to indicate to userspace
> > > that this case is
> > > > + * unsupported
> > > > + */
> > > > + count = -EOPNOTSUPP;
> > > > + return bpf_redirect_map(&xsk, 0, XDP_DROP);
> > >
> > > is this whole eopnotsupp dance worth the hassle?
> > >
> > > this basically breaks down to underlying driver not supporting xdp
> > > multi- buffer. we already store this state in ifobj->multi_buff_supp.
> > >
> > > could we just check for that and skip the test case instead of using
> > > the count global variable to store the error code which is counter intuitive?
> > >
> >
> > Thanks, Multi-buff is supported it might be that growing is not
> > supported but shrinking is supported. We have difference in result for
> > shrinking and growing tests. We are handling these cases with the existing
> 'count'
> > variable instead of introducing another variable to indicate or access
> > in userspace.
>
> These tests were supposed to exercise bugs against tail adjustment in multi-
> buffer scenarios, hence my comment to base this on this setting.
>
> I won't insist on simplifying it if you decide to keep this but please use
> different variable for communication with user space. We're not short on
> resources and count = -EOPNOTSUPP looks awkward.
>
I'll address the variable naming and include the changes in the v3 patchset.
> >
> > Here's the result matrix:
> > Driver/Mode XDP_ADJUST_TAIL_SHRINK
> XDP_ADJUST_TAIL_SHRINK_MULTI_BUFF XDP_ADJUST_TAIL_GROW
> XDP_ADJUST_TAIL_GROW_MULTI_BUFF
> > virt-eth DRV PASS PASS
> FAIL(EINNVAL) SKIP
> (EOPNOTSUPP)
> > virt-eth SKB PASS PASS
> FAIL(EINNVAL) SKIP
> (EOPNOTSUPP)
> > i40e SKB PASS PASS
> FAIL(EINNVAL) SKIP
> (EOPNOTSUPP)
> > i40e DRV PASS PASS
> PASS PASS
> > i40e ZC PASS PASS
> PASS PASS
> > i40e SKB BUSY-POLL PASS PASS
> FAIL(EINNVAL) SKIP (Not
> supported)
> > i40e DRV BUSY-POLL PASS PASS
> PASS PASS
> > i40e ZC BUSY-POLL PASS PASS
> PASS PASS
> > ice SKB PASS PASS
> FAIL(EINNVAL) SKIP
> (Not supported)
> > ice DRV PASS PASS
> PASS PASS
> > ice ZC PASS PASS
> PASS PASS
> > ice SKB BUSY-POLL PASS PASS
> FAIL(EINNVAL) SKIP (Not
> supported)
> > ice DRV BUSY-POLL PASS PASS
> PASS PASS
> > ice ZC BUSY-POLL PASS PASS
> PASS PASS
> >
> > > > + }
> > > > +
> > > > + return XDP_DROP;
> > > > + }
> > > > +
> > > > + curr_buff_len = bpf_xdp_get_buff_len(xdp);
> > > > + if (curr_buff_len != buff_len + count)
> > > > + return XDP_DROP;
> > > > +
> > > > + if (curr_buff_len > buff_len) {
> > > > + __u32 *pkt_data = (void *)(long)xdp->data;
> > > > + __u32 len, words_to_end, seq_num;
> > > > +
> > > > + len = curr_buff_len - PKT_HDR_ALIGN;
> > > > + words_to_end = len / sizeof(*pkt_data) - 1;
> > > > + seq_num = words_to_end;
> > > > +
> > > > + /* Convert sequence number to network byte order. Store this
> > > in the last 4 bytes of
> > > > + * the packet. Use 'count' to determine the position at the end
> > > of the packet for
> > > > + * storing the sequence number.
> > > > + */
> > > > + seq_num = __constant_htonl(words_to_end);
> > > > + bpf_xdp_store_bytes(xdp, curr_buff_len - count, &seq_num,
> > > sizeof(seq_num));
> > > > + }
> > > > +
> > > > + return bpf_redirect_map(&xsk, 0, XDP_DROP); }
> > > > +
> > > > char _license[] SEC("license") = "GPL"; diff --git
>
> (...)
