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.
>
> 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
(...)
