Message-ID: <IA1PR11MB6514B98679051D03FDDED9C78FDE2@IA1PR11MB6514.namprd11.prod.outlook.com>
Date: Tue, 18 Mar 2025 09:22:55 +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 v3 2/2] selftests/xsk: Add tail adjustment tests
 and support check



> -----Original Message-----
> From: Fijalkowski, Maciej <maciej.fijalkowski@...el.com>
> Sent: Wednesday, March 12, 2025 3:41 AM
> 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 v3 2/2] selftests/xsk: Add tail adjustment tests
> and support check
> 
> On Wed, Mar 05, 2025 at 02:18:13PM +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       |  49 ++++++++
> >  tools/testing/selftests/bpf/xsk_xdp_common.h  |   1 +
> >  tools/testing/selftests/bpf/xskxceiver.c      | 107 +++++++++++++++++-
> >  tools/testing/selftests/bpf/xskxceiver.h      |   2 +
> >  4 files changed, 157 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..34c832a5a98c 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 {
> > @@ -14,6 +16,7 @@ struct {
> >  } xsk SEC(".maps");
> >
> >  static unsigned int idx;
> > +int adjust_value = 0;
> >  int count = 0;
> >
> >  SEC("xdp.frags") int xsk_def_prog(struct xdp_md *xdp) @@ -70,4 +73,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, adjust_value);
> > +	if (ret < 0) {
> > +		/* Handle unsupported cases */
> > +		if (ret == -EOPNOTSUPP) {
> > +			/* Set adjust_value to -EOPNOTSUPP to indicate to
> userspace that this case
> > +			 * is unsupported
> > +			 */
> > +			adjust_value = -EOPNOTSUPP;
> > +			return bpf_redirect_map(&xsk, 0, XDP_DROP);
> 
> so in this case you will proceed with running whole traffic? IMHO test should
> be stopped for very first notsupp case, but not sure if it's worth the hassle.
> 

When the expected packet length does not match, the test will fail on the
very first packet in receive_pkts(). After this initial failure, check if
the adjust_tail function is supported, but only for cases where the test
involves adjust_tail and fails.

> > +		}
> > +
> > +		return XDP_DROP;
> > +	}
> > +
> > +	curr_buff_len = bpf_xdp_get_buff_len(xdp);
> > +	if (curr_buff_len != buff_len + adjust_value)
> > +		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 'adjust_value' 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 - adjust_value,
> &seq_num,
> > +sizeof(seq_num));
> 
> what is the purpose of that? test suite expects seq_num to be at the end of
> the packet so you have to double it here?
> 

The test suite expects the seq_num to be at the end of the packet, so it
needs to be doubled to ensure it is placed correctly in the final packet
structure.

> > +	}
> > +
> > +	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..bcc265fc784c 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 adjust_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,
> &adjust_value);
> > +	if (ret) {
> > +		ksft_print_msg("Error: bpf_map_lookup_elem failed with error
> %d\n", ret);
> > +		return false;
> 
> exit_with_error(errno) to be consistent?
> 

Will do.

> > +	}
> > +
> > +	/* Set the 'adjust_value' 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 adjust_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;
> 
> how about setting is_adjust_tail_supported() as validation_func? Would that
> work? Special casing this check specially for tail manipulation tests seems a bit
> odd.
> 

Setting is_adjust_tail_supported() as the validation_func would not work
directly. This function is designed to check if the bpf_xdp_adjust_tail
helper is supported by the XDP program, rather than to validate test
results. It assesses the capability of the XDP program, not the outcome of
the tests. 

> > +		else
> > +			report_failure(test);
> > +	}
> >
> >  	pthread_exit(NULL);
> >  }
> > @@ -2516,6 +2548,73 @@ 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
> > +adjust_value) {
> > +	struct xsk_xdp_progs *skel_rx = test->ifobj_rx->xdp_progs;
> > +	struct xsk_xdp_progs *skel_tx = test->ifobj_tx->xdp_progs;
> > +
> > +	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);
> > +
> > +	skel_rx->bss->adjust_value = adjust_value;
> > +
> > +	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;
> 
> you don't need this variable
> 

will do.

> > +	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;
> 
> missing indent
> 

will do.

> > +	}
> > +
> > +	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;
> 
> could you remove this and instead of bool_set_mtu just pass the value of
> mtu?
> 

will do.

> static int testapp_adjust_tail(struct test_spec *test, u32 value, u32 pkt_len,
> u32 mtu) {
> 	(...)
> 	if (test->mtu != mtu)
> 		test->mtu = mtu;
> 	(...)
> }
> 
> static int testapp_adjust_tail_shrink(struct test_spec *test) {
> 	return testapp_adjust_tail(test, -4, MIN_PKT_SIZE,
> MAX_ETH_PKT_SIZE); }
> 
> static int testapp_adjust_tail_shrink_mb(struct test_spec *test) {
> 	return testapp_adjust_tail(test, -4,
> XSK_RING_PROD__DEFAULT_NUM_DESCS * 3,
> 				   MAX_ETH_JUMBO_SIZE);
> }
> 
> no need for _common func.
> 

will do.

> > +	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);
> 
> Am I reading this right that you are modifying the size by just 4 bytes?
> The bugs that drivers had were for cases when packets got modified by value
> bigger than frag size which caused for example underlying page being freed.
> 
> If that is the case tests do nothing valuable from my perspective.
> 

In the v4 patchset, I have updated the code to modify the packet size by
1024 bytes instead of just 4 bytes.
I will send v4.

> > +}
> > +
> > +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 +2721,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