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

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?