Subject: Re: [PATCH bpf-next v3 03/10] selftests/xsk: add option to only run tests in a single mode

From: Maciej Fijalkowski <[email-masked]>
Date: Wed, 13 Sep 2023 19:12:02 +0200

On Wed, Sep 13, 2023 at 01:02:25PM +0200, Magnus Karlsson wrote:
> From: Magnus Karlsson <[email-masked]>
> 
> Add an option -m on the command line that allows the user to run the
> tests in a single mode instead of all of them. Valid modes are skb,
> drv, and zc (zero-copy). An example:
> 
> To run test suite in drv mode only:
> 
> ./test_xsk.sh -m drv
> 
> Signed-off-by: Magnus Karlsson <[email-masked]>
> ---
>  tools/testing/selftests/bpf/test_xsk.sh  | 10 ++++++-
>  tools/testing/selftests/bpf/xskxceiver.c | 34 +++++++++++++++++++++---
>  tools/testing/selftests/bpf/xskxceiver.h |  4 +--
>  3 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/bpf/test_xsk.sh b/tools/testing/selftests/bpf/test_xsk.sh
> index 2aa5a3445056..85e7a7e843f7 100755
> --- a/tools/testing/selftests/bpf/test_xsk.sh
> +++ b/tools/testing/selftests/bpf/test_xsk.sh
> @@ -73,17 +73,21 @@
>  #
>  # Run test suite for physical device in loopback mode
>  #   sudo ./test_xsk.sh -i IFACE
> +#
> +# Run test suite in a specific mode only [skb,drv,zc]
> +#   sudo ./test_xsk.sh -m MODE
>  
>  . xsk_prereqs.sh
>  
>  ETH=""
>  
> -while getopts "vi:d" flag
> +while getopts "vi:dm:" flag
>  do
>  	case "${flag}" in
>  		v) verbose=1;;
>  		d) debug=1;;
>  		i) ETH=${OPTARG};;
> +		m) MODE=${OPTARG};;
>  	esac
>  done
>  
> @@ -153,6 +157,10 @@ if [[ $verbose -eq 1 ]]; then
>  	ARGS+="-v "
>  fi
>  
> +if [ -n "$MODE" ]; then
> +	ARGS+="-m ${MODE} "
> +fi
> +
>  retval=$?
>  test_status $retval "${TEST_NAME}"
>  
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 514fe994e02b..9f79c2b6aa97 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -107,6 +107,9 @@
>  static const char *MAC1 = "\x00\x0A\x56\x9E\xEE\x62";
>  static const char *MAC2 = "\x00\x0A\x56\x9E\xEE\x61";
>  
> +static bool opt_verbose;
> +static enum test_mode opt_mode = TEST_MODE_ALL;
> +
>  static void __exit_with_error(int error, const char *file, const char *func, int line)
>  {
>  	ksft_test_result_fail("[%s:%s:%i]: ERROR: %d/\"%s\"\n", file, func, line, error,
> @@ -310,17 +313,19 @@ static struct option long_options[] = {
>  	{"interface", required_argument, 0, 'i'},
>  	{"busy-poll", no_argument, 0, 'b'},
>  	{"verbose", no_argument, 0, 'v'},
> +	{"mode", required_argument, 0, 'm'},
>  	{0, 0, 0, 0}
>  };
>  
>  static void usage(const char *prog)
>  {
>  	const char *str =
> -		"  Usage: %s [OPTIONS]\n"
> +		"  Usage: xskxceiver [OPTIONS]\n"
>  		"  Options:\n"
>  		"  -i, --interface      Use interface\n"
>  		"  -v, --verbose        Verbose output\n"
> -		"  -b, --busy-poll      Enable busy poll\n";
> +		"  -b, --busy-poll      Enable busy poll\n"
> +		"  -m, --mode           Run only mode skb, drv, or zc\n";
>  
>  	ksft_print_msg(str, prog);
>  }
> @@ -342,7 +347,7 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
>  	opterr = 0;
>  
>  	for (;;) {
> -		c = getopt_long(argc, argv, "i:vb", long_options, &option_index);
> +		c = getopt_long(argc, argv, "i:vbm:", long_options, &option_index);
>  		if (c == -1)
>  			break;
>  
> @@ -371,6 +376,21 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
>  			ifobj_tx->busy_poll = true;
>  			ifobj_rx->busy_poll = true;
>  			break;
> +		case 'm':
> +			if (!strncmp("skb", optarg, min_t(size_t, strlen(optarg),
> +							  strlen("skb")))) {
> +				opt_mode = TEST_MODE_SKB;
> +			} else if (!strncmp("drv", optarg, min_t(size_t, strlen(optarg),
> +								 strlen("drv")))) {
> +				opt_mode = TEST_MODE_DRV;
> +			} else if (!strncmp("zc", optarg, min_t(size_t, strlen(optarg),
> +								strlen("zc")))) {

with that logic i can type -m zcafxdprocks and this will still fly.
Sorry for being such a PITA over this but couldn't we simplify this to

			if (!strncmp("skb", optarg, strlen(optarg)))
				opt_mode = TEST_MODE_SKB;
			else if (!strncmp("drv", optarg, strlen(optarg)))
				opt_mode = TEST_MODE_DRV;
			else if (!strncmp("zc", optarg, strlen(optarg)))
				opt_mode = TEST_MODE_ZC;

as one of the next patches moves ksft_exit_xfail() to print_usage which
makes the braces in branch statements redundant. Using len of optarg
solves the -m zcafxdprocks problem.

> +				opt_mode = TEST_MODE_ZC;
> +			} else {
> +				usage(basename(argv[0]));
> +				ksft_exit_xfail();
> +			}
> +			break;
>  		default:
>  			usage(basename(argv[0]));
>  			ksft_exit_xfail();
> @@ -2365,9 +2385,15 @@ int main(int argc, char **argv)
>  	test.tx_pkt_stream_default = tx_pkt_stream_default;
>  	test.rx_pkt_stream_default = rx_pkt_stream_default;
>  
> -	ksft_set_plan(modes * TEST_TYPE_MAX);
> +	if (opt_mode == TEST_MODE_ALL)
> +		ksft_set_plan(modes * TEST_TYPE_MAX);
> +	else
> +		ksft_set_plan(TEST_TYPE_MAX);
>  
>  	for (i = 0; i < modes; i++) {
> +		if (opt_mode != TEST_MODE_ALL && i != opt_mode)
> +			continue;
> +
>  		for (j = 0; j < TEST_TYPE_MAX; j++) {
>  			test_spec_init(&test, ifobj_tx, ifobj_rx, i);
>  			run_pkt_test(&test, i, j);
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 233b66cef64a..1412492e9618 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -63,7 +63,7 @@ enum test_mode {
>  	TEST_MODE_SKB,
>  	TEST_MODE_DRV,
>  	TEST_MODE_ZC,
> -	TEST_MODE_MAX
> +	TEST_MODE_ALL
>  };
>  
>  enum test_type {
> @@ -98,8 +98,6 @@ enum test_type {
>  	TEST_TYPE_MAX
>  };
>  
> -static bool opt_verbose;
> -
>  struct xsk_umem_info {
>  	struct xsk_ring_prod fq;
>  	struct xsk_ring_cons cq;
> -- 
> 2.42.0
>