netdev - Re: [PATCH v2 4/4] selftests: net: udpgso_bench_tx: Cater for pending datagrams zerocopy benchmarking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <d9ca623d01274889913001ce92f686652fa8fea8.camel@redhat.com>
Date:   Tue, 31 Jan 2023 15:51:55 +0100
From:   Paolo Abeni <pabeni@...hat.com>
To:     Andrei Gherzan <andrei.gherzan@...onical.com>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>, Shuah Khan <shuah@...nel.org>
Cc:     netdev@...r.kernel.org, linux-kselftest@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 4/4] selftests: net: udpgso_bench_tx: Cater for
 pending datagrams zerocopy benchmarking

On Tue, 2023-01-31 at 13:04 +0000, Andrei Gherzan wrote:
> The test tool can check that the zerocopy number of completions value is
> valid taking into consideration the number of datagram send calls. This can
> catch the system into a state where the datagrams are still in the system
> (for example in a qdisk, waiting for the network interface to return a
> completion notification, etc).
> 
> This change adds a retry logic of computing the number of completions up to
> a configurable (via CLI) timeout (default: 2 seconds).
> 
> Signed-off-by: Andrei Gherzan <andrei.gherzan@...onical.com>
> ---
>  tools/testing/selftests/net/udpgso_bench_tx.c | 38 +++++++++++++++----
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgso_bench_tx.c b/tools/testing/selftests/net/udpgso_bench_tx.c
> index b47b5c32039f..5a29b5f24023 100644
> --- a/tools/testing/selftests/net/udpgso_bench_tx.c
> +++ b/tools/testing/selftests/net/udpgso_bench_tx.c
> @@ -62,6 +62,7 @@ static int	cfg_payload_len	= (1472 * 42);
>  static int	cfg_port	= 8000;
>  static int	cfg_runtime_ms	= -1;
>  static bool	cfg_poll;
> +static int	cfg_poll_loop_timeout_ms = 2000;
>  static bool	cfg_segment;
>  static bool	cfg_sendmmsg;
>  static bool	cfg_tcp;
> @@ -235,16 +236,17 @@ static void flush_errqueue_recv(int fd)
>  	}
>  }
>  
> -static void flush_errqueue(int fd, const bool do_poll)
> +static void flush_errqueue(int fd, const bool do_poll,
> +		unsigned long poll_timeout, const bool poll_err)
>  {
>  	if (do_poll) {
>  		struct pollfd fds = {0};
>  		int ret;
>  
>  		fds.fd = fd;
> -		ret = poll(&fds, 1, 500);
> +		ret = poll(&fds, 1, poll_timeout);
>  		if (ret == 0) {
> -			if (cfg_verbose)
> +			if ((cfg_verbose) && (poll_err))
>  				fprintf(stderr, "poll timeout\n");
>  		} else if (ret < 0) {
>  			error(1, errno, "poll");
> @@ -254,6 +256,22 @@ static void flush_errqueue(int fd, const bool do_poll)
>  	flush_errqueue_recv(fd);
>  }
>  
> +static void flush_errqueue_retry(int fd, const bool do_poll, unsigned long num_sends)
> +{
> +	unsigned long tnow, tstop;
> +	bool first_try = true;
> +
> +	tnow = gettimeofday_ms();
> +	tstop = tnow + cfg_poll_loop_timeout_ms;
> +	do {
> +		flush_errqueue(fd, do_poll, tstop - tnow, first_try);
> +		first_try = false;
> +		if (!do_poll)
> +			usleep(1000);  // a throttling delay if polling is enabled

Even if the kernel codying style is not very strictly enforced for
self-tests, please avoid c++ style comments.

More importantly, as Willem noded, this function is always called with
do_poll == true. You should drop such argument and the related branch
above.

> +		tnow = gettimeofday_ms();
> +	} while ((stat_zcopies != num_sends) && (tnow < tstop));
> +}
> +
>  static int send_tcp(int fd, char *data)
>  {
>  	int ret, done = 0, count = 0;
> @@ -413,8 +431,9 @@ static int send_udp_segment(int fd, char *data)
>  
>  static void usage(const char *filepath)
>  {
> -	error(1, 0, "Usage: %s [-46acmHPtTuvz] [-C cpu] [-D dst ip] [-l secs] [-M messagenr] [-p port] [-s sendsize] [-S gsosize]",
> -		    filepath);
> +	error(1, 0,
> +			"Usage: %s [-46acmHPtTuvz] [-C cpu] [-D dst ip] [-l secs] [-L secs] [-M messagenr] [-p port] [-s sendsize] [-S gsosize]",
> +			filepath);

Please avoid introducing unnecessary white-space changes (no reason to
move the usage text on a new line)

Cheers,

Paolo