Message-ID: <672e26ec429be_2a4cd22944c@willemb.c.googlers.com.notmuch>
Date: Fri, 08 Nov 2024 09:57:48 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Joe Damato <jdamato@...tly.com>, 
 netdev@...r.kernel.org
Cc: corbet@....net, 
 hdanton@...a.com, 
 bagasdotme@...il.com, 
 pabeni@...hat.com, 
 namangulati@...gle.com, 
 edumazet@...gle.com, 
 amritha.nambiar@...el.com, 
 sridhar.samudrala@...el.com, 
 sdf@...ichev.me, 
 peter@...eblog.net, 
 m2shafiei@...terloo.ca, 
 bjorn@...osinc.com, 
 hch@...radead.org, 
 willy@...radead.org, 
 willemdebruijn.kernel@...il.com, 
 skhawaja@...gle.com, 
 kuba@...nel.org, 
 Joe Damato <jdamato@...tly.com>, 
 Martin Karsten <mkarsten@...terloo.ca>, 
 "David S. Miller" <davem@...emloft.net>, 
 Simon Horman <horms@...nel.org>, 
 Shuah Khan <shuah@...nel.org>, 
 linux-kernel@...r.kernel.org (open list), 
 linux-kselftest@...r.kernel.org (open list:KERNEL SELFTEST FRAMEWORK)
Subject: Re: [PATCH net-next v8 5/6] selftests: net: Add busy_poll_test

Joe Damato wrote:
> Add an epoll busy poll test using netdevsim.
> 
> This test is comprised of:
>   - busy_poller (via busy_poller.c)
>   - busy_poll_test.sh which loads netdevsim, sets up network namespaces,
>     and runs busy_poller to receive data and socat to send data.
> 
> The selftest tests two different scenarios:
>   - busy poll (the pre-existing version in the kernel)
>   - busy poll with suspend enabled (what this series adds)
> 
> The data transmit is a 1MiB temporary file generated from /dev/urandom
> and the test is considered passing if the md5sum of the input file to
> socat matches the md5sum of the output file from busy_poller.

Nice test.

Busy polling does not affect data integrity. Is the goal of this test
mainly to get coverage, maybe observe if the process would stall
indefinitely?
 
> netdevsim was chosen instead of veth due to netdevsim's support for
> netdev-genl.
> 
> For now, this test uses the functionality that netdevsim provides. In the
> future, perhaps netdevsim can be extended to emulate device IRQs to more
> thoroughly test all pre-existing kernel options (like defer_hard_irqs)
> and suspend.
> 
> Signed-off-by: Joe Damato <jdamato@...tly.com>
> Co-developed-by: Martin Karsten <mkarsten@...terloo.ca>
> Signed-off-by: Martin Karsten <mkarsten@...terloo.ca>
> Acked-by: Stanislav Fomichev <sdf@...ichev.me>
> ---
>  v5:
>    - Updated commit message to replace netcat with socat and fixed
>      misspelling of netdevsim. No functional/code changes.
> 
>  v4:
>    - Updated busy_poll_test.sh:
>      - use socat instead of nc
>      - drop cli.py usage from the script
>      - removed check_ynl
>    - Updated busy_poller.c:
>      - use netlink to configure napi parameters
> 
>  v3:
>    - New in v3
> 
>  tools/testing/selftests/net/.gitignore        |   1 +
>  tools/testing/selftests/net/Makefile          |   3 +-
>  tools/testing/selftests/net/busy_poll_test.sh | 164 +++++++++
>  tools/testing/selftests/net/busy_poller.c     | 328 ++++++++++++++++++
>  4 files changed, 495 insertions(+), 1 deletion(-)
>  create mode 100755 tools/testing/selftests/net/busy_poll_test.sh
>  create mode 100644 tools/testing/selftests/net/busy_poller.c
> 
> diff --git a/tools/testing/selftests/net/busy_poll_test.sh b/tools/testing/selftests/net/busy_poll_test.sh
> new file mode 100755
> index 000000000000..ffc74bc62e5a
> --- /dev/null
> +++ b/tools/testing/selftests/net/busy_poll_test.sh
> @@ -0,0 +1,164 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0-only

Why the different SPDX between the two files?

> +source net_helper.sh
> +
> +NSIM_DEV_1_ID=$((256 + RANDOM % 256))
> +NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
> +NSIM_DEV_2_ID=$((512 + RANDOM % 256))
> +NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_2_ID
> +
> +NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
> +NSIM_DEV_SYS_DEL=/sys/bus/netdevsim/del_device
> +NSIM_DEV_SYS_LINK=/sys/bus/netdevsim/link_device
> +NSIM_DEV_SYS_UNLINK=/sys/bus/netdevsim/unlink_device
> +
> +setup_ns()
> +{
> +	set -e
> +	ip netns add nssv
> +	ip netns add nscl
> +
> +	NSIM_DEV_1_NAME=$(find $NSIM_DEV_1_SYS/net -maxdepth 1 -type d ! \
> +		-path $NSIM_DEV_1_SYS/net -exec basename {} \;)
> +	NSIM_DEV_2_NAME=$(find $NSIM_DEV_2_SYS/net -maxdepth 1 -type d ! \
> +		-path $NSIM_DEV_2_SYS/net -exec basename {} \;)
> +
> +	# ensure the server has 1 queue
> +	ethtool -L $NSIM_DEV_1_NAME combined 1 2>/dev/null
> +
> +	ip link set $NSIM_DEV_1_NAME netns nssv
> +	ip link set $NSIM_DEV_2_NAME netns nscl
> +
> +	ip netns exec nssv ip addr add '192.168.1.1/24' dev $NSIM_DEV_1_NAME
> +	ip netns exec nscl ip addr add '192.168.1.2/24' dev $NSIM_DEV_2_NAME
> +
> +	ip netns exec nssv ip link set dev $NSIM_DEV_1_NAME up
> +	ip netns exec nscl ip link set dev $NSIM_DEV_2_NAME up
> +
> +	set +e
> +}
> +
> +cleanup_ns()
> +{
> +	ip netns del nscl
> +	ip netns del nssv
> +}
> +
> +test_busypoll()
> +{
> +	tmp_file=$(mktemp)
> +	out_file=$(mktemp)
> +
> +	# fill a test file with random data
> +	dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
> +
> +	timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -i${NSIM_DEV_1_IFIDX} -o${out_file}&

nit: consider variables for all repeated constants, including IP addresses and ports
> +
> +	wait_local_port_listen nssv 48675 tcp
> +
> +	ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
> +
> +	wait
> +
> +	tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
> +	out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
> +
> +	if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
> +		res=0
> +	else
> +		echo "md5sum mismatch"
> +		echo "input file md5sum: ${tmp_file_md5sum}";
> +		echo "output file md5sum: ${out_file_md5sum}";
> +		res=1
> +	fi
> +
> +	rm $out_file $tmp_file

Delete these in cleanup()?

> +
> +	return $res
> +}
> +
> +test_busypoll_with_suspend()
> +{

Main feedback: this function is practically a duplicate of the prev.
Consider deduplicating them with one optional argument that enables
suspend mode in busy_poller.

> +	tmp_file=$(mktemp)
> +	out_file=$(mktemp)
> +
> +	# fill a test file with random data
> +	dd if=/dev/urandom of=${tmp_file} bs=1M count=1 2> /dev/null
> +
> +	timeout -k 1s 30s ip netns exec nssv ./busy_poller -p48675 -b192.168.1.1 -m8 -u0 -P1 -g16 -d100 -r50000 -s20000000 -i${NSIM_DEV_1_IFIDX} -o${out_file}&
> +
> +	wait_local_port_listen nssv 48675 tcp
> +
> +	ip netns exec nscl socat -u $tmp_file TCP:192.168.1.1:48675
> +
> +	wait
> +
> +	tmp_file_md5sum=$(md5sum $tmp_file | cut -f1 -d' ')
> +	out_file_md5sum=$(md5sum $out_file | cut -f1 -d' ')
> +
> +	if [ "$tmp_file_md5sum" = "$out_file_md5sum" ]; then
> +		res=0
> +	else
> +		echo "md5sum mismatch"
> +		echo "input file md5sum: ${tmp_file_md5sum}";
> +		echo "output file md5sum: ${out_file_md5sum}";
> +		res=1
> +	fi
> +
> +	rm $out_file $tmp_file
> +
> +	return $res
> +}
> +
> +###
> +### Code start
> +###
> +
> +modprobe netdevsim
> +
> +# linking
> +
> +echo $NSIM_DEV_1_ID > $NSIM_DEV_SYS_NEW
> +echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_NEW
> +udevadm settle

is this generally available on systems under test?

> +
> +setup_ns
> +
> +NSIM_DEV_1_FD=$((256 + RANDOM % 256))

repeated magic constants

> +exec {NSIM_DEV_1_FD}</var/run/netns/nssv
> +NSIM_DEV_1_IFIDX=$(ip netns exec nssv cat /sys/class/net/$NSIM_DEV_1_NAME/ifindex)
> +
> +NSIM_DEV_2_FD=$((256 + RANDOM % 256))
> +exec {NSIM_DEV_2_FD}</var/run/netns/nscl
> +NSIM_DEV_2_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_DEV_2_NAME/ifindex)
> +
> +echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX $NSIM_DEV_2_FD:$NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
> +if [ $? -ne 0 ]; then
> +	echo "linking netdevsim1 with netdevsim2 should succeed"
> +	cleanup_ns
> +	exit 1
> +fi
> +
> +test_busypoll
> +if [ $? -ne 0 ]; then
> +	echo "test_busypoll failed"
> +	cleanup_ns
> +	exit 1
> +fi
> +
> +test_busypoll_with_suspend
> +if [ $? -ne 0 ]; then
> +	echo "test_busypoll_with_suspend failed"
> +	cleanup_ns
> +	exit 1
> +fi
> +
> +echo "$NSIM_DEV_1_FD:$NSIM_DEV_1_IFIDX" > $NSIM_DEV_SYS_UNLINK
> +
> +echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_DEL
> +
> +cleanup_ns
> +
> +modprobe -r netdevsim
> +
> +exit 0
> diff --git a/tools/testing/selftests/net/busy_poller.c b/tools/testing/selftests/net/busy_poller.c
> new file mode 100644
> index 000000000000..8d8aa9e5939a
> --- /dev/null
> +++ b/tools/testing/selftests/net/busy_poller.c
> @@ -0,0 +1,328 @@
> +// SPDX-License-Identifier: GPL-2.0
> +#include <assert.h>
> +#include <errno.h>
> +#include <error.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <limits.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <unistd.h>
> +
> +#include <arpa/inet.h>
> +#include <netinet/in.h>
> +
> +#include <sys/ioctl.h>
> +#include <sys/epoll.h>
> +#include <sys/socket.h>
> +#include <sys/types.h>
> +
> +#include <linux/netlink.h>
> +#include <linux/genetlink.h>
> +#include "netdev-user.h"
> +#include <ynl.h>
> +
> +/* if the headers haven't been updated, we need to define some things */

This should not be needed, as headers are taken from $KERNELSRC/usr after
make headers_install.

Generally discouraged for tests (else every new feature test for a new
features is forced to adds such checks).

> +#if !defined(EPOLL_IOC_TYPE)
> +struct epoll_params {
> +	uint32_t busy_poll_usecs;
> +	uint16_t busy_poll_budget;
> +	uint8_t prefer_busy_poll;
> +
> +	/* pad the struct to a multiple of 64bits */
> +	uint8_t __pad;
> +};
> +
> +#define EPOLL_IOC_TYPE 0x8A
> +#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
> +#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
> +#endif
> +
> +static uint32_t cfg_port = 8000;
> +static struct in_addr cfg_bind_addr = { .s_addr = INADDR_ANY };
> +static char *cfg_outfile;
> +static int cfg_max_events = 8;
> +static int cfg_ifindex;
> +
> +/* busy poll params */
> +static uint32_t cfg_busy_poll_usecs;
> +static uint16_t cfg_busy_poll_budget;
> +static uint8_t cfg_prefer_busy_poll;
> +
> +/* IRQ params */
> +static uint32_t cfg_defer_hard_irqs;
> +static uint64_t cfg_gro_flush_timeout;
> +static uint64_t cfg_irq_suspend_timeout;
> +
> +static void usage(const char *filepath)
> +{
> +	error(1, 0,
> +	      "Usage: %s -p<port> -b<addr> -m<max_events> -u<busy_poll_usecs> -P<prefer_busy_poll> -g<busy_poll_budget> -o<outfile> -d<defer_hard_irqs> -r<gro_flush_timeout> -s<irq_suspend_timeout> -i<ifindex>",
> +	      filepath);
> +}
> +
> +static void parse_opts(int argc, char **argv)
> +{
> +	int ret;
> +	int c;
> +
> +	if (argc <= 1)
> +		usage(argv[0]);
> +
> +	while ((c = getopt(argc, argv, "p:m:b:u:P:g:o:d:r:s:i:")) != -1) {
> +		switch (c) {
> +		case 'u':
> +			cfg_busy_poll_usecs = strtoul(optarg, NULL, 0);
> +			if (cfg_busy_poll_usecs == ULONG_MAX ||
> +			    cfg_busy_poll_usecs > UINT32_MAX)
> +				error(1, ERANGE, "busy_poll_usecs too large");
> +			break;
> +		case 'P':
> +			cfg_prefer_busy_poll = strtoul(optarg, NULL, 0);
> +			if (cfg_prefer_busy_poll == ULONG_MAX ||

Here and elsewhere: not possible due to size of var (here: uint8_t)
I'm surprised that the compiler does not mention this.

> +			    cfg_prefer_busy_poll > 1)
> +				error(1, ERANGE,
> +				      "prefer busy poll should be 0 or 1");
> +			break;
> +		case 'g':
> +			cfg_busy_poll_budget = strtoul(optarg, NULL, 0);
> +			if (cfg_busy_poll_budget == ULONG_MAX ||
> +			    cfg_busy_poll_budget > UINT16_MAX)
> +				error(1, ERANGE,
> +				      "busy poll budget must be [0, UINT16_MAX]");
> +			break;
> +		case 'p':
> +			cfg_port = strtoul(optarg, NULL, 0);
> +			if (cfg_port > UINT16_MAX)
> +				error(1, ERANGE, "port must be <= 65535");
> +			break;
> +		case 'b':
> +			ret = inet_aton(optarg, &cfg_bind_addr);
> +			if (ret == 0)
> +				error(1, errno,
> +				      "bind address %s invalid", optarg);
> +			break;
> +		case 'o':
> +			cfg_outfile = strdup(optarg);
> +			if (!cfg_outfile)
> +				error(1, 0, "outfile invalid");
> +			break;
> +		case 'm':
> +			cfg_max_events = strtol(optarg, NULL, 0);
> +
> +			if (cfg_max_events == LONG_MIN ||
> +			    cfg_max_events == LONG_MAX ||
> +			    cfg_max_events <= 0)
> +				error(1, ERANGE,
> +				      "max events must be > 0 and < LONG_MAX");
> +			break;
> +		case 'd':
> +			cfg_defer_hard_irqs = strtoul(optarg, NULL, 0);
> +
> +			if (cfg_defer_hard_irqs == ULONG_MAX ||
> +			    cfg_defer_hard_irqs > INT32_MAX)
> +				error(1, ERANGE,
> +				      "defer_hard_irqs must be <= INT32_MAX");
> +			break;
> +		case 'r':
> +			cfg_gro_flush_timeout = strtoull(optarg, NULL, 0);
> +
> +			if (cfg_gro_flush_timeout == ULLONG_MAX)
> +				error(1, ERANGE,
> +				      "gro_flush_timeout must be < ULLONG_MAX");
> +			break;
> +		case 's':
> +			cfg_irq_suspend_timeout = strtoull(optarg, NULL, 0);
> +
> +			if (cfg_irq_suspend_timeout == ULLONG_MAX)
> +				error(1, ERANGE,
> +				      "irq_suspend_timeout must be < ULLONG_MAX");
> +			break;
> +		case 'i':
> +			cfg_ifindex = strtoul(optarg, NULL, 0);
> +			if (cfg_ifindex == ULONG_MAX)
> +				error(1, ERANGE,
> +				      "ifindex must be < ULONG_MAX");
> +			break;
> +		}
> +	}
> +
> +	if (!cfg_ifindex)
> +		usage(argv[0]);
> +
> +	if (optind != argc)
> +		usage(argv[0]);
> +}

Powered by blists - more mailing lists