| Message ID | 20230131130412.432549-1-andrei.gherzan@canonical.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/4] selftests: net: udpgso_bench_rx: Fix 'used uninitialized' compiler warning | expand |
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
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.
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
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?
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.
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--) {
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(-)