Message-ID: <IA1PR11MB6514D321A1123B8C280875018FCC2@IA1PR11MB6514.namprd11.prod.outlook.com>
Date: Fri, 28 Feb 2025 09:56:19 +0000
From: "Vyavahare, Tushar" <tushar.vyavahare@...el.com>
To: "Fijalkowski, Maciej" <maciej.fijalkowski@...el.com>
CC: "bpf@...r.kernel.org" <bpf@...r.kernel.org>, "netdev@...r.kernel.org"
	<netdev@...r.kernel.org>, "bjorn@...nel.org" <bjorn@...nel.org>, "Karlsson,
 Magnus" <magnus.karlsson@...el.com>, "jonathan.lemon@...il.com"
	<jonathan.lemon@...il.com>, "davem@...emloft.net" <davem@...emloft.net>,
	"kuba@...nel.org" <kuba@...nel.org>, "pabeni@...hat.com" <pabeni@...hat.com>,
	"ast@...nel.org" <ast@...nel.org>, "daniel@...earbox.net"
	<daniel@...earbox.net>, "Sarkar, Tirthendu" <tirthendu.sarkar@...el.com>
Subject: RE: [PATCH bpf-next v2 2/2] selftests/xsk: Add tail adjustment tests
 and support check



> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> Sent: Thursday, February 27, 2025 11:52 PM
> To: Vyavahare, Tushar <tushar.vyavahare@...el.com>
> Cc: bpf@...r.kernel.org; netdev@...r.kernel.org; bjorn@...nel.org; Karlsson,
> Magnus <magnus.karlsson@...el.com>; jonathan.lemon@...il.com;
> davem@...emloft.net; kuba@...nel.org; pabeni@...hat.com;
> ast@...nel.org; daniel@...earbox.net; Sarkar, Tirthendu
> <tirthendu.sarkar@...el.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@...el.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.

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
> > a/tools/testing/selftests/bpf/xsk_xdp_common.h
> > b/tools/testing/selftests/bpf/xsk_xdp_common.h
> > index 5a6f36f07383..45810ff552da 100644
> > --- a/tools/testing/selftests/bpf/xsk_xdp_common.h
> > +++ b/tools/testing/selftests/bpf/xsk_xdp_common.h
> > @@ -4,6 +4,7 @@
> >  #define XSK_XDP_COMMON_H_
> >
> >  #define MAX_SOCKETS 2
> > +#define PKT_HDR_ALIGN (sizeof(struct ethhdr) + 2) /* Just to align
> > +the data in the packet */
> >
> >  struct xdp_info {
> >  	__u64 count;
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index d60ee6a31c09..ee196b638662 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -524,6 +524,8 @@ static void __test_spec_init(struct test_spec *test,
> struct ifobject *ifobj_tx,
> >  	test->nb_sockets = 1;
> >  	test->fail = false;
> >  	test->set_ring = false;
> > +	test->adjust_tail = false;
> > +	test->adjust_tail_support = false;
> >  	test->mtu = MAX_ETH_PKT_SIZE;
> >  	test->xdp_prog_rx = ifobj_rx->xdp_progs->progs.xsk_def_prog;
> >  	test->xskmap_rx = ifobj_rx->xdp_progs->maps.xsk; @@ -992,6
> +994,31
> > @@ static bool is_metadata_correct(struct pkt *pkt, void *buffer, u64 addr)
> >  	return true;
> >  }
> >
> > +static bool is_adjust_tail_supported(struct xsk_xdp_progs *skel_rx) {
> > +	struct bpf_map *data_map;
> > +	int value = 0;
> > +	int key = 0;
> > +	int ret;
> > +
> > +	data_map = bpf_object__find_map_by_name(skel_rx->obj,
> "xsk_xdp_.bss");
> > +	if (!data_map || !bpf_map__is_internal(data_map)) {
> > +		ksft_print_msg("Error: could not find bss section of XDP
> program\n");
> > +		exit_with_error(errno);
> > +	}
> > +
> > +	ret = bpf_map_lookup_elem(bpf_map__fd(data_map), &key, &value);
> > +	if (ret) {
> > +		ksft_print_msg("Error: bpf_map_lookup_elem failed with
> error %d\n", ret);
> > +		return false;
> > +	}
> > +
> > +	/* Set the 'count' variable to -EOPNOTSUPP in the XDP program if the
> adjust_tail helper is
> > +	 * not supported. Skip the adjust_tail test case in this scenario.
> > +	 */
> > +	return value != -EOPNOTSUPP;
> > +}
> > +
> >  static bool is_frag_valid(struct xsk_umem_info *umem, u64 addr, u32 len,
> u32 expected_pkt_nb,
> >  			  u32 bytes_processed)
> >  {
> > @@ -1768,8 +1795,13 @@ static void *worker_testapp_validate_rx(void
> > *arg)
> >
> >  	if (!err && ifobject->validation_func)
> >  		err = ifobject->validation_func(ifobject);
> > -	if (err)
> > -		report_failure(test);
> > +
> > +	if (err) {
> > +		if (test->adjust_tail && !is_adjust_tail_supported(ifobject-
> >xdp_progs))
> > +			test->adjust_tail_support = false;
> > +		else
> > +			report_failure(test);
> > +	}
> >
> >  	pthread_exit(NULL);
> >  }
> > @@ -2516,6 +2548,84 @@ static int testapp_hw_sw_max_ring_size(struct
> test_spec *test)
> >  	return testapp_validate_traffic(test);  }
> >
> > +static int testapp_xdp_adjust_tail(struct test_spec *test, int count)
> > +{
> > +	struct xsk_xdp_progs *skel_rx = test->ifobj_rx->xdp_progs;
> > +	struct xsk_xdp_progs *skel_tx = test->ifobj_tx->xdp_progs;
> > +	struct bpf_map *data_map;
> > +	int key = 0;
> > +
> > +	test_spec_set_xdp_prog(test, skel_rx->progs.xsk_xdp_adjust_tail,
> > +			       skel_tx->progs.xsk_xdp_adjust_tail,
> > +			       skel_rx->maps.xsk, skel_tx->maps.xsk);
> > +
> > +	data_map = bpf_object__find_map_by_name(skel_rx->obj,
> "xsk_xdp_.bss");
> > +	if (!data_map || !bpf_map__is_internal(data_map)) {
> > +		ksft_print_msg("Error: could not find bss section of XDP
> program\n");
> > +		return TEST_FAILURE;
> > +	}
> > +
> > +	if (bpf_map_update_elem(bpf_map__fd(data_map), &key, &count,
> BPF_ANY)) {
> > +		ksft_print_msg("Error: could not update count element\n");
> > +		return TEST_FAILURE;
> > +	}
> > +
> > +	return testapp_validate_traffic(test); }
> > +
> > +static int testapp_adjust_tail(struct test_spec *test, u32 value, u32
> > +pkt_len) {
> > +	u32 pkt_cnt = DEFAULT_BATCH_SIZE;
> > +	int ret;
> > +
> > +	test->adjust_tail_support = true;
> > +	test->adjust_tail = true;
> > +	test->total_steps = 1;
> > +
> > +	pkt_stream_replace_ifobject(test->ifobj_tx, pkt_cnt, pkt_len);
> > +	pkt_stream_replace_ifobject(test->ifobj_rx, pkt_cnt, pkt_len +
> > +value);
> > +
> > +	ret = testapp_xdp_adjust_tail(test, value);
> > +	if (ret)
> > +		return ret;
> > +
> > +	if (!test->adjust_tail_support) {
> > +		ksft_test_result_skip("%s %sResize pkt with
> bpf_xdp_adjust_tail() not supported\n",
> > +				      mode_string(test), busy_poll_string(test));
> > +	return TEST_SKIP;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int testapp_adjust_tail_common(struct test_spec *test, int
> adjust_value, u32 len,
> > +				      bool set_mtu)
> > +{
> > +	if (set_mtu)
> > +		test->mtu = MAX_ETH_JUMBO_SIZE;
> 
> couldn't we base this on BPF_F_XDP_HAS_FRAGS in some way instead of
> boolean var?
> 

No, it is being set much later. Test framework needs mtu to be set to
enable multi-buffer.

> > +	return testapp_adjust_tail(test, adjust_value, len); }
> > +
> > +static int testapp_adjust_tail_shrink(struct test_spec *test) {
> > +	return testapp_adjust_tail_common(test, -4, MIN_PKT_SIZE, false); }
> > +
> > +static int testapp_adjust_tail_shrink_mb(struct test_spec *test) {
> > +	return testapp_adjust_tail_common(test, -4,
> > +XSK_RING_PROD__DEFAULT_NUM_DESCS * 3, true); }
> > +
> > +static int testapp_adjust_tail_grow(struct test_spec *test) {
> > +	return testapp_adjust_tail_common(test, 4, MIN_PKT_SIZE, false); }
> > +
> > +static int testapp_adjust_tail_grow_mb(struct test_spec *test) {
> > +	return testapp_adjust_tail_common(test, 4,
> > +XSK_RING_PROD__DEFAULT_NUM_DESCS * 3, true); }
> > +
> >  static void run_pkt_test(struct test_spec *test)  {
> >  	int ret;
> > @@ -2622,6 +2732,10 @@ static const struct test_spec tests[] = {
> >  	{.name = "TOO_MANY_FRAGS", .test_func = testapp_too_many_frags},
> >  	{.name = "HW_SW_MIN_RING_SIZE", .test_func =
> testapp_hw_sw_min_ring_size},
> >  	{.name = "HW_SW_MAX_RING_SIZE", .test_func =
> > testapp_hw_sw_max_ring_size},
> > +	{.name = "XDP_ADJUST_TAIL_SHRINK", .test_func =
> testapp_adjust_tail_shrink},
> > +	{.name = "XDP_ADJUST_TAIL_SHRINK_MULTI_BUFF", .test_func =
> testapp_adjust_tail_shrink_mb},
> > +	{.name = "XDP_ADJUST_TAIL_GROW", .test_func =
> testapp_adjust_tail_grow},
> > +	{.name = "XDP_ADJUST_TAIL_GROW_MULTI_BUFF", .test_func =
> > +testapp_adjust_tail_grow_mb},
> >  	};
> >
> >  static void print_tests(void)
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.h
> > b/tools/testing/selftests/bpf/xskxceiver.h
> > index e46e823f6a1a..67fc44b2813b 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.h
> > +++ b/tools/testing/selftests/bpf/xskxceiver.h
> > @@ -173,6 +173,8 @@ struct test_spec {
> >  	u16 nb_sockets;
> >  	bool fail;
> >  	bool set_ring;
> > +	bool adjust_tail;
> > +	bool adjust_tail_support;
> >  	enum test_mode mode;
> >  	char name[MAX_TEST_NAME_SIZE];
> >  };
> > --
> > 2.34.1
> >

Powered by blists - more mailing lists