diff mbox series

[net] selftests: fixes for UDP GRO

Message ID 2b1f4d9fe21346bc9f85c096e1b0922e4ff3c19d.1551173111.git.pabeni@redhat.com
State Accepted
Delegated to: David Miller
Headers show
Series [net] selftests: fixes for UDP GRO | expand

Commit Message

Paolo Abeni Feb. 26, 2019, 2:27 p.m. UTC
The current implementation for UDP GRO tests is racy: the receiver
may flush the RX queue while the sending is still transmitting and
incorrectly report RX errors, with a wrong number of packet received.

Add explicit timeouts to the receiver for both connection activation
(first packet received for UDP) and reception completion, so that
in the above critical scenario the receiver will wait for the
transfer completion.

Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 tools/testing/selftests/net/udpgro.sh         |  8 ++--
 tools/testing/selftests/net/udpgso_bench_rx.c | 42 +++++++++++++------
 2 files changed, 33 insertions(+), 17 deletions(-)

Comments

Willem de Bruijn Feb. 26, 2019, 6:38 p.m. UTC | #1
On Tue, Feb 26, 2019 at 9:28 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> The current implementation for UDP GRO tests is racy: the receiver
> may flush the RX queue while the sending is still transmitting and
> incorrectly report RX errors, with a wrong number of packet received.
>
> Add explicit timeouts to the receiver for both connection activation
> (first packet received for UDP) and reception completion, so that
> in the above critical scenario the receiver will wait for the
> transfer completion.
>
> Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Acked-by: Willem de Bruijn <willemb@google.com>

---

This is because of that "force termination after the second poll()"?

Could perhaps also just extend do_recv to wait while (!interrupted &&
tnow < tstart + 2000) and avoid the explicit arguments.
Paolo Abeni Feb. 27, 2019, 9:26 a.m. UTC | #2
Hi,

On Tue, 2019-02-26 at 13:38 -0500, Willem de Bruijn wrote:
> On Tue, Feb 26, 2019 at 9:28 AM Paolo Abeni <pabeni@redhat.com> wrote:
> > The current implementation for UDP GRO tests is racy: the receiver
> > may flush the RX queue while the sending is still transmitting and
> > incorrectly report RX errors, with a wrong number of packet received.
> > 
> > Add explicit timeouts to the receiver for both connection activation
> > (first packet received for UDP) and reception completion, so that
> > in the above critical scenario the receiver will wait for the
> > transfer completion.
> > 
> > Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Acked-by: Willem de Bruijn <willemb@google.com>

Thanks for reviewing.

> ---
> 
> This is because of that "force termination after the second poll()"?

Yes, exactly.

> Could perhaps also just extend do_recv to wait while (!interrupted &&
> tnow < tstart + 2000) and avoid the explicit arguments.

I thought about that, but then UDP GRO self-tests would take a bit more
and the binary helper would be less re-usable for future test cases.

Cheers,

Paolo
David Miller March 1, 2019, 7:24 p.m. UTC | #3
From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 26 Feb 2019 15:27:43 +0100

> The current implementation for UDP GRO tests is racy: the receiver
> may flush the RX queue while the sending is still transmitting and
> incorrectly report RX errors, with a wrong number of packet received.
> 
> Add explicit timeouts to the receiver for both connection activation
> (first packet received for UDP) and reception completion, so that
> in the above critical scenario the receiver will wait for the
> transfer completion.
> 
> Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Applied, thanks Paolo.
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
index aeac53a99aeb..ac2a30be9b32 100755
--- a/tools/testing/selftests/net/udpgro.sh
+++ b/tools/testing/selftests/net/udpgro.sh
@@ -37,7 +37,7 @@  run_one() {
 
 	cfg_veth
 
-	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} && \
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
 		echo "ok" || \
 		echo "failed" &
 
@@ -81,7 +81,7 @@  run_one_nat() {
 	# will land on the 'plain' one
 	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -G ${family} -b ${addr1} -n 0 &
 	pid=$!
-	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${family} -b ${addr2%/*} ${rx_args} && \
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${family} -b ${addr2%/*} ${rx_args} && \
 		echo "ok" || \
 		echo "failed"&
 
@@ -99,8 +99,8 @@  run_one_2sock() {
 
 	cfg_veth
 
-	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} -p 12345 &
-	ip netns exec "${PEER_NS}" ./udpgso_bench_rx ${rx_args} && \
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} -p 12345 &
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 2000 -R 10 ${rx_args} && \
 		echo "ok" || \
 		echo "failed" &
 
diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index 0c960f673324..db3d4a8b5a4c 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -45,6 +45,8 @@  static int  cfg_alen 		= sizeof(struct sockaddr_in6);
 static int  cfg_expected_pkt_nr;
 static int  cfg_expected_pkt_len;
 static int  cfg_expected_gso_size;
+static int  cfg_connect_timeout_ms;
+static int  cfg_rcv_timeout_ms;
 static struct sockaddr_storage cfg_bind_addr;
 
 static bool interrupted;
@@ -87,7 +89,7 @@  static unsigned long gettimeofday_ms(void)
 	return (tv.tv_sec * 1000) + (tv.tv_usec / 1000);
 }
 
-static void do_poll(int fd)
+static void do_poll(int fd, int timeout_ms)
 {
 	struct pollfd pfd;
 	int ret;
@@ -102,8 +104,16 @@  static void do_poll(int fd)
 			break;
 		if (ret == -1)
 			error(1, errno, "poll");
-		if (ret == 0)
-			continue;
+		if (ret == 0) {
+			if (!timeout_ms)
+				continue;
+
+			timeout_ms -= 10;
+			if (timeout_ms <= 0) {
+				interrupted = true;
+				break;
+			}
+		}
 		if (pfd.revents != POLLIN)
 			error(1, errno, "poll: 0x%x expected 0x%x\n",
 					pfd.revents, POLLIN);
@@ -134,7 +144,7 @@  static int do_socket(bool do_tcp)
 		if (listen(accept_fd, 1))
 			error(1, errno, "listen");
 
-		do_poll(accept_fd);
+		do_poll(accept_fd, cfg_connect_timeout_ms);
 		if (interrupted)
 			exit(0);
 
@@ -273,7 +283,9 @@  static void do_flush_udp(int fd)
 
 static void usage(const char *filepath)
 {
-	error(1, 0, "Usage: %s [-Grtv] [-b addr] [-p port] [-l pktlen] [-n packetnr] [-S gsosize]", filepath);
+	error(1, 0, "Usage: %s [-C connect_timeout] [-Grtv] [-b addr] [-p port]"
+	      " [-l pktlen] [-n packetnr] [-R rcv_timeout] [-S gsosize]",
+	      filepath);
 }
 
 static void parse_opts(int argc, char **argv)
@@ -282,7 +294,7 @@  static void parse_opts(int argc, char **argv)
 
 	/* bind to any by default */
 	setup_sockaddr(PF_INET6, "::", &cfg_bind_addr);
-	while ((c = getopt(argc, argv, "4b:Gl:n:p:rS:tv")) != -1) {
+	while ((c = getopt(argc, argv, "4b:C:Gl:n:p:rR:S:tv")) != -1) {
 		switch (c) {
 		case '4':
 			cfg_family = PF_INET;
@@ -292,6 +304,9 @@  static void parse_opts(int argc, char **argv)
 		case 'b':
 			setup_sockaddr(cfg_family, optarg, &cfg_bind_addr);
 			break;
+		case 'C':
+			cfg_connect_timeout_ms = strtoul(optarg, NULL, 0);
+			break;
 		case 'G':
 			cfg_gro_segment = true;
 			break;
@@ -307,6 +322,9 @@  static void parse_opts(int argc, char **argv)
 		case 'r':
 			cfg_read_all = true;
 			break;
+		case 'R':
+			cfg_rcv_timeout_ms = strtoul(optarg, NULL, 0);
+			break;
 		case 'S':
 			cfg_expected_gso_size = strtol(optarg, NULL, 0);
 			break;
@@ -329,8 +347,9 @@  static void parse_opts(int argc, char **argv)
 
 static void do_recv(void)
 {
+	int timeout_ms = cfg_tcp ? cfg_rcv_timeout_ms : cfg_connect_timeout_ms;
 	unsigned long tnow, treport;
-	int fd, loop = 0;
+	int fd;
 
 	fd = do_socket(cfg_tcp);
 
@@ -342,12 +361,7 @@  static void do_recv(void)
 
 	treport = gettimeofday_ms() + 1000;
 	do {
-		/* force termination after the second poll(); this cope both
-		 * with sender slower than receiver and missing packet errors
-		 */
-		if (cfg_expected_pkt_nr && loop++)
-			interrupted = true;
-		do_poll(fd);
+		do_poll(fd, timeout_ms);
 
 		if (cfg_tcp)
 			do_flush_tcp(fd);
@@ -365,6 +379,8 @@  static void do_recv(void)
 			treport = tnow + 1000;
 		}
 
+		timeout_ms = cfg_rcv_timeout_ms;
+
 	} while (!interrupted);
 
 	if (cfg_expected_pkt_nr && (packets != cfg_expected_pkt_nr))