On 23/01/31 07:26PM, Paolo Abeni wrote:

On Tue, 2023-01-31 at 16:31 +0000, Andrei Gherzan wrote:

quoted

On 23/01/31 05:22PM, Paolo Abeni wrote:

quoted

On Tue, 2023-01-31 at 15:08 +0000, Andrei Gherzan wrote:

quoted

On 23/01/31 03:51PM, Paolo Abeni wrote:

quoted

On Tue, 2023-01-31 at 13:04 +0000, Andrei Gherzan wrote:

quoted

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 <redacted>
---
 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.

Agreed. I will drop.

quoted

quoted

+		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)

The only reason why I've done this was to make scripts/checkpatch.pl
happy:

WARNING: line length of 141 exceeds 100 columns
#83: FILE: tools/testing/selftests/net/udpgso_bench_tx.c:432:

I can drop and ignore the warning, or maybe it would have been better to
just mention this in git message. What do you prefer?

Long lines are allowed for (kernel) messages, to make them easily grep-
able.

In this specific case you can either append the new text to the message
without introducing that strange indentation or even better break the
usage string alike:

	"Usage: %s [-46acmHPtTuvz] [-C cpu] [-D dst ip] [-l secs] [-L secs]"
	" [-L secs] [-M messagenr] [-p port] [-s sendsize] [-S gsosize]"

Funny I went through this too but it also fails with:

WARNING: quoted string split across lines
#84: FILE: tools/testing/selftests/net/udpgso_bench_tx.c:433

This is how I usually do it but it seems like it's flagged too.

I'm all for ignoring this warning in this specific context. Among other
things it will be consistent with other existing self-tests.

Eventually the checkpatch script could be tuned (with an unrelated
patch) to discriminate between kernel and self-tests code.

In that case I will use quoted strings split across lines in the next
version

-- 
Andrei Gherzan