[PATCH v2 1/4] selftests: net: udpgso_bench_rx: Fix 'used uninitialized' compiler warning

[PATCH v2 1/4] selftests: net: udpgso_bench_rx: Fix 'used uninitialized' compiler warning

[Andrei Gherzan]

This change fixes the following compiler warning:

/usr/include/x86_64-linux-gnu/bits/error.h:40:5: warning: ‘gso_size’ may
be used uninitialized [-Wmaybe-uninitialized]
   40 |     __error_noreturn (__status, __errnum, __format,
   __va_arg_pack ());
         |
	 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
	 udpgso_bench_rx.c: In function ‘main’:
	 udpgso_bench_rx.c:253:23: note: ‘gso_size’ was declared here
	   253 |         int ret, len, gso_size, budget = 256;

Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index 6a193425c367..d0895bd1933f 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -250,7 +250,7 @@ static int recv_msg(int fd, char *buf, int len, int *gso_size)
 static void do_flush_udp(int fd)
 {
 	static char rbuf[ETH_MAX_MTU];
-	int ret, len, gso_size, budget = 256;
+	int ret, len, gso_size = 0, budget = 256;
 
 	len = cfg_read_all ? sizeof(rbuf) : 0;
 	while (budget--) {
-- 
2.34.1

[PATCH v2 2/4] selftests: net: udpgso_bench_rx/tx: Stop when wrong CLI args are provided

[Andrei Gherzan]

Leaving unrecognized arguments buried in the output, can easily hide a
CLI/script typo. Avoid this by exiting when wrong arguments are provided to
the udpgso_bench test programs.

Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 2 ++
 tools/testing/selftests/net/udpgso_bench_tx.c | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index d0895bd1933f..4058c7451e70 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -336,6 +336,8 @@ static void parse_opts(int argc, char **argv)
 			cfg_verify = true;
 			cfg_read_all = true;
 			break;
+		default:
+			exit(1);
 		}
 	}
 
diff --git a/tools/testing/selftests/net/udpgso_bench_tx.c b/tools/testing/selftests/net/udpgso_bench_tx.c
index f1fdaa270291..b47b5c32039f 100644
--- a/tools/testing/selftests/net/udpgso_bench_tx.c
+++ b/tools/testing/selftests/net/udpgso_bench_tx.c
@@ -490,6 +490,8 @@ static void parse_opts(int argc, char **argv)
 		case 'z':
 			cfg_zerocopy = true;
 			break;
+		default:
+			exit(1);
 		}
 	}
 
-- 
2.34.1

[PATCH v2 3/4] selftests: net: udpgso_bench: Fix racing bug between the rx/tx programs

[Andrei Gherzan]

"udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs
subsequently and while doing so, there is a chance that the rx one is not
ready to accept socket connections. This racing bug could fail the test
with at least one of the following:

./udpgso_bench_tx: connect: Connection refused
./udpgso_bench_tx: sendmsg: Connection refused
./udpgso_bench_tx: write: Connection refused

This change addresses this by making udpgro_bench.sh wait for the rx
program to be ready before firing off the tx one - with an exponential back
off algorithm from 1s to 10s.

Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
---
 tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh
index dc932fd65363..20b5db8fcbde 100755
--- a/tools/testing/selftests/net/udpgso_bench.sh
+++ b/tools/testing/selftests/net/udpgso_bench.sh
@@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m'
 readonly YELLOW='\033[0;33m'
 readonly RED='\033[0;31m'
 readonly NC='\033[0m' # No Color
+readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx
 
 readonly KSFT_PASS=0
 readonly KSFT_FAIL=1
@@ -56,10 +57,27 @@ trap wake_children EXIT
 
 run_one() {
 	local -r args=$@
+	local -r init_delay_s=1
+	local -r max_delay_s=10
+	local delay_s=0
+	local nr_socks=0
 
 	./udpgso_bench_rx &
 	./udpgso_bench_rx -t &
 
+	# Wait for the above test program to get ready to receive connections.
+	delay_s="${init_delay_s}"
+	while [ "$delay_s" -lt "$max_delay_s" ]; do
+		nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")"
+		[ "$nr_socks" -eq 2 ] && break
+		sleep "$delay_s"
+		delay="$((delay*2))"
+	done
+	if [ "$nr_socks" -ne 2 ]; then
+		echo "timed out while waiting for udpgso_bench_rx"
+		exit 1
+	fi
+
 	./udpgso_bench_tx ${args}
 }
 
-- 
2.34.1

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

[Andrei Gherzan]

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@canonical.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
+		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);
 }
 
 static void parse_opts(int argc, char **argv)
@@ -423,7 +442,7 @@ static void parse_opts(int argc, char **argv)
 	int max_len, hdrlen;
 	int c;
 
-	while ((c = getopt(argc, argv, "46acC:D:Hl:mM:p:s:PS:tTuvz")) != -1) {
+	while ((c = getopt(argc, argv, "46acC:D:Hl:L:mM:p:s:PS:tTuvz")) != -1) {
 		switch (c) {
 		case '4':
 			if (cfg_family != PF_UNSPEC)
@@ -452,6 +471,9 @@ static void parse_opts(int argc, char **argv)
 		case 'l':
 			cfg_runtime_ms = strtoul(optarg, NULL, 10) * 1000;
 			break;
+		case 'L':
+			cfg_poll_loop_timeout_ms = strtoul(optarg, NULL, 10) * 1000;
+			break;
 		case 'm':
 			cfg_sendmmsg = true;
 			break;
@@ -679,7 +701,7 @@ int main(int argc, char **argv)
 			num_sends += send_udp(fd, buf[i]);
 		num_msgs++;
 		if ((cfg_zerocopy && ((num_msgs & 0xF) == 0)) || cfg_tx_tstamp)
-			flush_errqueue(fd, cfg_poll);
+			flush_errqueue(fd, cfg_poll, 500, true);
 
 		if (cfg_msg_nr && num_msgs >= cfg_msg_nr)
 			break;
@@ -698,7 +720,7 @@ int main(int argc, char **argv)
 	} while (!interrupted && (cfg_runtime_ms == -1 || tnow < tstop));
 
 	if (cfg_zerocopy || cfg_tx_tstamp)
-		flush_errqueue(fd, true);
+		flush_errqueue_retry(fd, true, num_sends);
 
 	if (close(fd))
 		error(1, errno, "close");
-- 
2.34.1

Re: [PATCH v2 3/4] selftests: net: udpgso_bench: Fix racing bug between the rx/tx programs

[Willem de Bruijn]

On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan
<andrei.gherzan@canonical.com> wrote:
>
> "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs
> subsequently and while doing so, there is a chance that the rx one is not
> ready to accept socket connections. This racing bug could fail the test
> with at least one of the following:
>
> ./udpgso_bench_tx: connect: Connection refused
> ./udpgso_bench_tx: sendmsg: Connection refused
> ./udpgso_bench_tx: write: Connection refused
>
> This change addresses this by making udpgro_bench.sh wait for the rx
> program to be ready before firing off the tx one - with an exponential back
> off algorithm from 1s to 10s.
>
> Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>

please CC: reviewers of previous revisions on new revisions

also for upcoming patches: please clearly mark net or net-next.
> ---
>  tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh
> index dc932fd65363..20b5db8fcbde 100755
> --- a/tools/testing/selftests/net/udpgso_bench.sh
> +++ b/tools/testing/selftests/net/udpgso_bench.sh
> @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m'
>  readonly YELLOW='\033[0;33m'
>  readonly RED='\033[0;31m'
>  readonly NC='\033[0m' # No Color
> +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx

then also pass explicit -p argument to the processes to keep all three
consistent

>
>  readonly KSFT_PASS=0
>  readonly KSFT_FAIL=1
> @@ -56,10 +57,27 @@ trap wake_children EXIT
>
>  run_one() {
>         local -r args=$@
> +       local -r init_delay_s=1
> +       local -r max_delay_s=10
> +       local delay_s=0
> +       local nr_socks=0
>
>         ./udpgso_bench_rx &
>         ./udpgso_bench_rx -t &
>
> +       # Wait for the above test program to get ready to receive connections.
> +       delay_s="${init_delay_s}"
> +       while [ "$delay_s" -lt "$max_delay_s" ]; do
> +               nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")"
> +               [ "$nr_socks" -eq 2 ] && break
> +               sleep "$delay_s"
> +               delay="$((delay*2))"

I don't think we need exponential back-off for something this simple

> +       done
> +       if [ "$nr_socks" -ne 2 ]; then
> +               echo "timed out while waiting for udpgso_bench_rx"
> +               exit 1
> +       fi
> +
>         ./udpgso_bench_tx ${args}
>  }
>
> --
> 2.34.1
>

Re: [PATCH v2 2/4] selftests: net: udpgso_bench_rx/tx: Stop when wrong CLI args are provided

[Willem de Bruijn]

On Tue, Jan 31, 2023 at 8:08 AM Andrei Gherzan
<andrei.gherzan@canonical.com> wrote:
>
> Leaving unrecognized arguments buried in the output, can easily hide a
> CLI/script typo. Avoid this by exiting when wrong arguments are provided to
> the udpgso_bench test programs.
>
> Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>

I'm on the fence on this. Test binaries are not necessarily robust
against bad input. If you insist.

When sending patches to net, please always add a Fixes tag.

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

[Willem de Bruijn]

On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan
<andrei.gherzan@canonical.com> 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@canonical.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

never reached

Re: [PATCH v2 2/4] selftests: net: udpgso_bench_rx/tx: Stop when wrong CLI args are provided

[Andrei Gherzan]

On 23/01/31 08:35AM, Willem de Bruijn wrote:
> On Tue, Jan 31, 2023 at 8:08 AM Andrei Gherzan
> <andrei.gherzan@canonical.com> wrote:
> >
> > Leaving unrecognized arguments buried in the output, can easily hide a
> > CLI/script typo. Avoid this by exiting when wrong arguments are provided to
> > the udpgso_bench test programs.
> >
> > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
> 
> I'm on the fence on this. Test binaries are not necessarily robust
> against bad input. If you insist.

I'll keep it in the set (for next v), but I don't mind if it doesn't end
up applied. It was just something I stumbled into.

> When sending patches to net, please always add a Fixes tag.

I'll keep that in mind.

-- 
Andrei Gherzan

Re: [PATCH v2 3/4] selftests: net: udpgso_bench: Fix racing bug between the rx/tx programs

[Paolo Abeni]

On Tue, 2023-01-31 at 08:33 -0500, Willem de Bruijn wrote:
> On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan
> <andrei.gherzan@canonical.com> wrote:
> > 
> > "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs
> > subsequently and while doing so, there is a chance that the rx one is not
> > ready to accept socket connections. This racing bug could fail the test
> > with at least one of the following:
> > 
> > ./udpgso_bench_tx: connect: Connection refused
> > ./udpgso_bench_tx: sendmsg: Connection refused
> > ./udpgso_bench_tx: write: Connection refused
> > 
> > This change addresses this by making udpgro_bench.sh wait for the rx
> > program to be ready before firing off the tx one - with an exponential back
> > off algorithm from 1s to 10s.
> > 
> > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
> 
> please CC: reviewers of previous revisions on new revisions
> 
> also for upcoming patches: please clearly mark net or net-next.
> > ---
> >  tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> > 
> > diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh
> > index dc932fd65363..20b5db8fcbde 100755
> > --- a/tools/testing/selftests/net/udpgso_bench.sh
> > +++ b/tools/testing/selftests/net/udpgso_bench.sh
> > @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m'
> >  readonly YELLOW='\033[0;33m'
> >  readonly RED='\033[0;31m'
> >  readonly NC='\033[0m' # No Color
> > +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx
> 
> then also pass explicit -p argument to the processes to keep all three
> consistent
> 
> > 
> >  readonly KSFT_PASS=0
> >  readonly KSFT_FAIL=1
> > @@ -56,10 +57,27 @@ trap wake_children EXIT
> > 
> >  run_one() {
> >         local -r args=$@
> > +       local -r init_delay_s=1
> > +       local -r max_delay_s=10
> > +       local delay_s=0
> > +       local nr_socks=0
> > 
> >         ./udpgso_bench_rx &
> >         ./udpgso_bench_rx -t &
> > 
> > +       # Wait for the above test program to get ready to receive connections.
> > +       delay_s="${init_delay_s}"
> > +       while [ "$delay_s" -lt "$max_delay_s" ]; do
> > +               nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")"
> > +               [ "$nr_socks" -eq 2 ] && break
> > +               sleep "$delay_s"
> > +               delay="$((delay*2))"
> 
> I don't think we need exponential back-off for something this simple

Agreed. Additionally you could use constant, sub-second delay (say 0.1)
to keep the runtime delta relatively low.

Cheers,

Paolo

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

[Paolo Abeni]

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@canonical.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

Re: [PATCH v2 3/4] selftests: net: udpgso_bench: Fix racing bug between the rx/tx programs

[Andrei Gherzan]

On 23/01/31 08:33AM, Willem de Bruijn wrote:
> On Tue, Jan 31, 2023 at 8:06 AM Andrei Gherzan
> <andrei.gherzan@canonical.com> wrote:
> >
> > "udpgro_bench.sh" invokes udpgso_bench_rx/udpgso_bench_tx programs
> > subsequently and while doing so, there is a chance that the rx one is not
> > ready to accept socket connections. This racing bug could fail the test
> > with at least one of the following:
> >
> > ./udpgso_bench_tx: connect: Connection refused
> > ./udpgso_bench_tx: sendmsg: Connection refused
> > ./udpgso_bench_tx: write: Connection refused
> >
> > This change addresses this by making udpgro_bench.sh wait for the rx
> > program to be ready before firing off the tx one - with an exponential back
> > off algorithm from 1s to 10s.
> >
> > Signed-off-by: Andrei Gherzan <andrei.gherzan@canonical.com>
> 
> please CC: reviewers of previous revisions on new revisions
> 
> also for upcoming patches: please clearly mark net or net-next.

Ack. I'll do all that in next v.

> > ---
> >  tools/testing/selftests/net/udpgso_bench.sh | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/tools/testing/selftests/net/udpgso_bench.sh b/tools/testing/selftests/net/udpgso_bench.sh
> > index dc932fd65363..20b5db8fcbde 100755
> > --- a/tools/testing/selftests/net/udpgso_bench.sh
> > +++ b/tools/testing/selftests/net/udpgso_bench.sh
> > @@ -7,6 +7,7 @@ readonly GREEN='\033[0;92m'
> >  readonly YELLOW='\033[0;33m'
> >  readonly RED='\033[0;31m'
> >  readonly NC='\033[0m' # No Color
> > +readonly TESTPORT=8000 # Keep this in sync with udpgso_bench_rx/tx
> 
> then also pass explicit -p argument to the processes to keep all three
> consistent

I have no idea how I've missed it. It will be in v4.

> 
> >
> >  readonly KSFT_PASS=0
> >  readonly KSFT_FAIL=1
> > @@ -56,10 +57,27 @@ trap wake_children EXIT
> >
> >  run_one() {
> >         local -r args=$@
> > +       local -r init_delay_s=1
> > +       local -r max_delay_s=10
> > +       local delay_s=0
> > +       local nr_socks=0
> >
> >         ./udpgso_bench_rx &
> >         ./udpgso_bench_rx -t &
> >
> > +       # Wait for the above test program to get ready to receive connections.
> > +       delay_s="${init_delay_s}"
> > +       while [ "$delay_s" -lt "$max_delay_s" ]; do
> > +               nr_socks="$(ss -lnHi | grep -c "\*:${TESTPORT}")"
> > +               [ "$nr_socks" -eq 2 ] && break
> > +               sleep "$delay_s"
> > +               delay="$((delay*2))"
> 
> I don't think we need exponential back-off for something this simple

I'm happy to drop/simplify. I'll go for a simple sleep 1 in a timeout
loop.

> 
> > +       done
> > +       if [ "$nr_socks" -ne 2 ]; then
> > +               echo "timed out while waiting for udpgso_bench_rx"
> > +               exit 1
> > +       fi
> > +
> >         ./udpgso_bench_tx ${args}
> >  }
> >
> > --
> > 2.34.1
> >

-- 
Andrei Gherzan

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

[Andrei Gherzan]

On 23/01/31 03:51PM, Paolo Abeni wrote:
> 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@canonical.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.

Agreed. I will drop.

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

-- 
Andrei Gherzan

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

[Paolo Abeni]

On Tue, 2023-01-31 at 15:08 +0000, Andrei Gherzan wrote:
> On 23/01/31 03:51PM, Paolo Abeni wrote:
> > 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@canonical.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.
> 
> Agreed. I will drop.
> 
> > 
> > > +		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]"

Cheers,

Paolo

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

[Andrei Gherzan]

On 23/01/31 05:22PM, Paolo Abeni wrote:
> On Tue, 2023-01-31 at 15:08 +0000, Andrei Gherzan wrote:
> > On 23/01/31 03:51PM, Paolo Abeni wrote:
> > > 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@canonical.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.
> > 
> > Agreed. I will drop.
> > 
> > > 
> > > > +		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.

-- 
Andrei Gherzan

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

[Paolo Abeni]

On Tue, 2023-01-31 at 16:31 +0000, Andrei Gherzan wrote:
> On 23/01/31 05:22PM, Paolo Abeni wrote:
> > On Tue, 2023-01-31 at 15:08 +0000, Andrei Gherzan wrote:
> > > On 23/01/31 03:51PM, Paolo Abeni wrote:
> > > > 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@canonical.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.
> > > 
> > > Agreed. I will drop.
> > > 
> > > > 
> > > > > +		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.

Cheers,

Paolo

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

[Andrei Gherzan]

On 23/01/31 07:26PM, Paolo Abeni wrote:
> On Tue, 2023-01-31 at 16:31 +0000, Andrei Gherzan wrote:
> > On 23/01/31 05:22PM, Paolo Abeni wrote:
> > > On Tue, 2023-01-31 at 15:08 +0000, Andrei Gherzan wrote:
> > > > On 23/01/31 03:51PM, Paolo Abeni wrote:
> > > > > 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@canonical.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.
> > > > 
> > > > Agreed. I will drop.
> > > > 
> > > > > 
> > > > > > +		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