Re: [PATCH bpf-next v2 2/2] selftests/xsk: Add tail adjustment tests and support check

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

On Fri, Feb 28, 2025 at 10:56:19AM +0100, Vyavahare, Tushar wrote:
> 
> 
> > -----Original Message-----
> > From: Fijalkowski, Maciej <maciej.fijalkowski@intel.com>
> > Sent: Thursday, February 27, 2025 11:52 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 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@intel.com>
> > > ---
> > >  .../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

(...)