[PATCH linux-next 0/3] selftests: net: udpgso_bench_rx: Fix verifty, gsosize, and packet number exceptions

[PATCH linux-next 0/3] selftests: net: udpgso_bench_rx: Fix verifty, gsosize, and packet number exceptions

[yang.yang29]

From: zhang yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>

1.Fix verifty exception
Executing the following command fails:
bash# udpgso_bench_tx -l 4 -4 -D "$DST"
bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
bash# udpgso_bench_rx -4 -G -S 1472 -v
udpgso_bench_rx: data[1472]: len 2944, a(97) != q(113)

This is because the sending buffers are not aligned by 26 bytes, and the
GRO is not merged sequentially, and the receiver does not judge this
situation. We think of the receiver to split the data and then validate
it, just as the application actually uses this feature.

2.Fix gsosize exception
Executing the following command fails:
bash# udpgso_bench_tx -l 4 -4 -D "$DST"
bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
bash# udpgso_bench_rx -4 -G -S 1472
udp rx:     15 MB/s      256 calls/s
udp rx:     30 MB/s      512 calls/s
udpgso_bench_rx: recv: bad gso size, got -1, expected 1472
(-1 == no gso cmsg))

 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 7360
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 1472
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 1472
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 4416
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 11776
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 20608
recv: got one message len:1472, probably not an error.
recv: got one message len:1472, probably not an error.

This is due to network, NAPI, timer, etc., only one message being received.
We believe that this situation should be normal.

3.Fix packet number exception
bash# udpgso_bench_rx -4 -n 100
bash# udpgso_bench_tx -l 1 -4 -D "$DST"
udpgso_bench_rx: wrong packet number! got 0, expected 100

This is because the packets is cleared after print.

Zhang Yunkai (3):
  selftests: net: udpgso_bench_rx: Fix verifty exceptions
  selftests: net: udpgso_bench_rx: Fix gsosize exceptions
  selftests: net: udpgso_bench_rx: Fix packet number exceptions

 tools/testing/selftests/net/udpgso_bench_rx.c | 45 +++++++++++++++++++++------
 1 file changed, 35 insertions(+), 10 deletions(-)

--
2.15.2

[PATCH linux-next 1/3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Yang Yang]

From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>

The verification function of this test case is likely to encounter the
following error, which may confuse users.

Executing the following command fails:
bash# udpgso_bench_tx -l 4 -4 -D "$DST"
bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
bash# udpgso_bench_rx -4 -G -S 1472 -v
udpgso_bench_rx: data[1472]: len 2944, a(97) != q(113)

This is because the sending buffers are not aligned by 26 bytes, and the
GRO is not merged sequentially, and the receiver does not judge this
situation. We do the validation after the data is split at the receiving
end, just as the application actually uses this feature.

Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
Reviewed-by: xu xin (CGEL ZTE) <xu.xin16@zte.com.cn>
Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 38 +++++++++++++++++++++------
 1 file changed, 30 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index f35a924d4a30..a5b7f30659a5 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -189,16 +189,16 @@ static char sanitized_char(char val)
 	return (val >= 'a' && val <= 'z') ? val : '.';
 }
 
-static void do_verify_udp(const char *data, int len)
+static void do_verify_udp(const char *data, int start, int len)
 {
-	char cur = data[0];
+	char cur = data[start];
 	int i;
 
 	/* verify contents */
 	if (cur < 'a' || cur > 'z')
 		error(1, 0, "data initial byte out of range");
 
-	for (i = 1; i < len; i++) {
+	for (i = start + 1; i < start + len; i++) {
 		if (cur == 'z')
 			cur = 'a';
 		else
@@ -212,6 +212,24 @@ static void do_verify_udp(const char *data, int len)
 	}
 }
 
+static void do_verify_udp_gro(const char *data, int len, int gso_size)
+{
+	int remaining = len;
+	int start = 0;
+
+	while (remaining) {
+		if (remaining < 0)
+			break;
+
+		if (remaining > gso_size)
+			do_verify_udp(data, start, gso_size);
+		else
+			do_verify_udp(data, start, remaining);
+		start += gso_size;
+		remaining -= gso_size;
+	}
+}
+
 static int recv_msg(int fd, char *buf, int len, int *gso_size)
 {
 	char control[CMSG_SPACE(sizeof(int))] = {0};
@@ -264,16 +282,20 @@ static void do_flush_udp(int fd)
 		if (cfg_expected_pkt_len && ret != cfg_expected_pkt_len)
 			error(1, 0, "recv: bad packet len, got %d,"
 			      " expected %d\n", ret, cfg_expected_pkt_len);
+		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
+			error(1, 0, "recv: bad gso size, got %d, expected %d %s",
+				gso_size, cfg_expected_gso_size, "(-1 == no gso cmsg))\n");
 		if (len && cfg_verify) {
 			if (ret == 0)
 				error(1, errno, "recv: 0 byte datagram\n");
 
-			do_verify_udp(rbuf, ret);
+			if (!cfg_gro_segment)
+				do_verify_udp(rbuf, 0, ret);
+			else if (gso_size > 0)
+				do_verify_udp_gro(rbuf, ret, gso_size);
+			else
+				do_verify_udp_gro(rbuf, ret, ret);
 		}
-		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
-			error(1, 0, "recv: bad gso size, got %d, expected %d "
-			      "(-1 == no gso cmsg))\n", gso_size,
-			      cfg_expected_gso_size);
 
 		packets++;
 		bytes += ret;
-- 
2.15.2

[PATCH linux-next 2/3] selftests: net: udpgso_bench_rx: Fix gsosize exceptions

[Yang Yang]

From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>

The -S parameter of the receiving terminal will most likely cause the
following error.

Executing the following command fails:
bash# udpgso_bench_tx -l 4 -4 -D "$DST"
bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
bash# udpgso_bench_rx -4 -G -S 1472
udp rx:     15 MB/s      256 calls/s
udp rx:     30 MB/s      512 calls/s
udpgso_bench_rx: recv: bad gso size, got -1, expected 1472
(-1 == no gso cmsg))

The actual message on the receiving end is as follows:
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 7360
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 1472
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 1472
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 4416
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 11776
 IP 192.168.2.199.55238 > 192.168.2.203.8000: UDP, length 20608
recv: got one message len:1472, probably not an error.
recv: got one message len:1472, probably not an error.

This is due to network, NAPI, timer, etc., only one message being received.
We believe that this situation should be normal. We add judgment to filter
out packets that are less than gso_size and do not contain CMSG
information.

Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
Reviewed-by: xu xin (CGEL ZTE) <xu.xin16@zte.com.cn>
Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index a5b7f30659a5..784e88b31f7d 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -282,7 +282,9 @@ static void do_flush_udp(int fd)
 		if (cfg_expected_pkt_len && ret != cfg_expected_pkt_len)
 			error(1, 0, "recv: bad packet len, got %d,"
 			      " expected %d\n", ret, cfg_expected_pkt_len);
-		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
+		/* For some network reasons, ret less than gso_size is not an error */
+		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size &&
+				ret > cfg_expected_gso_size)
 			error(1, 0, "recv: bad gso size, got %d, expected %d %s",
 				gso_size, cfg_expected_gso_size, "(-1 == no gso cmsg))\n");
 		if (len && cfg_verify) {
-- 
2.15.2

[PATCH linux-next 3/3] selftests: net: udpgso_bench_rx: Fix packet number exceptions

[Yang Yang]

From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>

The -n parameter is confusing and seems to only affect the frequency of
determining whether the time reaches 1s. However, the final print of the
program is the number of messages expected to be received, which is always
0.

bash# udpgso_bench_rx -4 -n 100
bash# udpgso_bench_tx -l 1 -4 -D "$DST"
udpgso_bench_rx: wrong packet number! got 0, expected 100

This is because the packets are always cleared after print.

Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
Reviewed-by: xu xin (CGEL ZTE) <xu.xin16@zte.com.cn>
Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
---
 tools/testing/selftests/net/udpgso_bench_rx.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
index 784e88b31f7d..b66bb53af19f 100644
--- a/tools/testing/selftests/net/udpgso_bench_rx.c
+++ b/tools/testing/selftests/net/udpgso_bench_rx.c
@@ -50,7 +50,7 @@ static int  cfg_rcv_timeout_ms;
 static struct sockaddr_storage cfg_bind_addr;
 
 static bool interrupted;
-static unsigned long packets, bytes;
+static unsigned long packets, total_packets, bytes;
 
 static void sigint_handler(int signum)
 {
@@ -405,6 +405,7 @@ static void do_recv(void)
 					"%s rx: %6lu MB/s %8lu calls/s\n",
 					cfg_tcp ? "tcp" : "udp",
 					bytes >> 20, packets);
+			total_packets += packets;
 			bytes = packets = 0;
 			treport = tnow + 1000;
 		}
@@ -415,7 +416,7 @@ static void do_recv(void)
 
 	if (cfg_expected_pkt_nr && (packets != cfg_expected_pkt_nr))
 		error(1, 0, "wrong packet number! got %ld, expected %d\n",
-		      packets, cfg_expected_pkt_nr);
+		      total_packets + packets, cfg_expected_pkt_nr);
 
 	if (close(fd))
 		error(1, errno, "close");
-- 
2.15.2

RE: [PATCH linux-next 1/3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Willem de Bruijn]

Yang Yang wrote:
> From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> 
> The verification function of this test case is likely to encounter the
> following error, which may confuse users.
> 
> Executing the following command fails:
> bash# udpgso_bench_tx -l 4 -4 -D "$DST"
> bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
> bash# udpgso_bench_rx -4 -G -S 1472 -v

Why are you running two senders concurrently? The test is not intended
to handle that case.

> udpgso_bench_rx: data[1472]: len 2944, a(97) != q(113)
> 
> This is because the sending buffers are not aligned by 26 bytes, and the
> GRO is not merged sequentially, and the receiver does not judge this
> situation. We do the validation after the data is split at the receiving
> end, just as the application actually uses this feature.
> 
> Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> Reviewed-by: xu xin (CGEL ZTE) <xu.xin16@zte.com.cn>
> Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
> Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>

RE: [PATCH linux-next 3/3] selftests: net: udpgso_bench_rx: Fix packet number exceptions

[Willem de Bruijn]

Yang Yang wrote:
> From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> 
> The -n parameter is confusing and seems to only affect the frequency of
> determining whether the time reaches 1s.

This statement seems irrelevant to this patch.

Is the point that cfg_expected_pkt_nr is tested in do_flush_udp to
stop reading, but not in the caller of that function, do_recv, to
break out of the loop? That's a fair point and may deserve a fix.

> However, the final print of the
> program is the number of messages expected to be received, which is always
> 0.
> 
> bash# udpgso_bench_rx -4 -n 100
> bash# udpgso_bench_tx -l 1 -4 -D "$DST"
> udpgso_bench_rx: wrong packet number! got 0, expected 100
> 
> This is because the packets are always cleared after print.

This looks good to me. Would be a fix to the commit that introduced
that wrong packet number branch, commit 3327a9c46352
("selftests: add functionals test for UDP GRO").
> 
> Signed-off-by: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> Reviewed-by: xu xin (CGEL ZTE) <xu.xin16@zte.com.cn>
> Reviewed-by: Yang Yang (CGEL ZTE) <yang.yang29@zte.com.cn>
> Cc: Xuexin Jiang (CGEL ZTE) <jiang.xuexin@zte.com.cn>
> ---
>  tools/testing/selftests/net/udpgso_bench_rx.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
> index 784e88b31f7d..b66bb53af19f 100644
> --- a/tools/testing/selftests/net/udpgso_bench_rx.c
> +++ b/tools/testing/selftests/net/udpgso_bench_rx.c
> @@ -50,7 +50,7 @@ static int  cfg_rcv_timeout_ms;
>  static struct sockaddr_storage cfg_bind_addr;
>  
>  static bool interrupted;
> -static unsigned long packets, bytes;
> +static unsigned long packets, total_packets, bytes;
>  
>  static void sigint_handler(int signum)
>  {
> @@ -405,6 +405,7 @@ static void do_recv(void)
>  					"%s rx: %6lu MB/s %8lu calls/s\n",
>  					cfg_tcp ? "tcp" : "udp",
>  					bytes >> 20, packets);
> +			total_packets += packets;
>  			bytes = packets = 0;
>  			treport = tnow + 1000;
>  		}
> @@ -415,7 +416,7 @@ static void do_recv(void)
>  
>  	if (cfg_expected_pkt_nr && (packets != cfg_expected_pkt_nr))
>  		error(1, 0, "wrong packet number! got %ld, expected %d\n",
> -		      packets, cfg_expected_pkt_nr);
> +		      total_packets + packets, cfg_expected_pkt_nr);
>  
>  	if (close(fd))
>  		error(1, errno, "close");
> -- 
> 2.15.2

RE: [PATCH linux-next 1/3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Yang Yang]

> Why are you running two senders concurrently? The test is not intended
> to handle that case.

Sorry for the inaccuracy of the description here, these two commands,
i.e. with or without GSO, cause the problem. The same goes for patch 2/3.
The problem is easily reproducible in the latest kernel, QEMU environment, E1000.

bash# udpgso_bench_tx -l 4 -4 -D "$DST" 
udpgso_bench_tx: write: Connection refused
bash# udpgso_bench_rx -4 -G -S 1472 -v
udpgso_bench_rx: data[1472]: len 17664, a(97) != q(113)

bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
udpgso_bench_tx: sendmsg: Connection refused
bash# udpgso_bench_rx -4 -G -S 1472 -v
udpgso_bench_rx: data[61824]: len 64768, a(97) != w(119)

In one test, the verification data is printed as follows:
abcd...xyz
...
abcd...xyz
abcd...opabcd...xyz

This is because the sender intercepts from the buf at a certain length,
which is not aligned according to 26 bytes, and multiple packets are
merged. The verification of the receiving end needs to be performed
after splitting.

RE: [PATCH linux-next 1/3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Willem de Bruijn]

Yang Yang wrote:
> > Why are you running two senders concurrently? The test is not intended
> > to handle that case.
> 
> Sorry for the inaccuracy of the description here, these two commands,
> i.e. with or without GSO, cause the problem. The same goes for patch 2/3.
> The problem is easily reproducible in the latest kernel, QEMU environment, E1000.
> 
> bash# udpgso_bench_tx -l 4 -4 -D "$DST" 
> udpgso_bench_tx: write: Connection refused
> bash# udpgso_bench_rx -4 -G -S 1472 -v
> udpgso_bench_rx: data[1472]: len 17664, a(97) != q(113)
> 
> bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
> udpgso_bench_tx: sendmsg: Connection refused
> bash# udpgso_bench_rx -4 -G -S 1472 -v
> udpgso_bench_rx: data[61824]: len 64768, a(97) != w(119)

I still don't follow: are you running the tx and rx commands
sequentially or in parallel? On different (virtual) hosts?
 
> In one test, the verification data is printed as follows:
> abcd...xyz
> ...
> abcd...xyz
> abcd...opabcd...xyz
> 
> This is because the sender intercepts from the buf at a certain length,
> which is not aligned according to 26 bytes, and multiple packets are
> merged. The verification of the receiving end needs to be performed
> after splitting.

The 26 bytes is a reference to the pattern printed by the test, which
iterates over A-Z.

Is your point that each individual segment starts at A, so that a test
for pattern {ABC..Z}+ only matches if the segments size is a multiple
of 26, else a the pattern will have discontinuities?

Re: [PATCH linux-next 1/3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

[Paolo Abeni]

On Mon, 2023-04-17 at 20:24 +0800, Yang Yang wrote:
> From: Zhang Yunkai (CGEL ZTE) <zhang.yunkai@zte.com.cn>
> 
> The verification function of this test case is likely to encounter the
> following error, which may confuse users.
> 
> Executing the following command fails:
> bash# udpgso_bench_tx -l 4 -4 -D "$DST"
> bash# udpgso_bench_tx -l 4 -4 -D "$DST" -S 0
> bash# udpgso_bench_rx -4 -G -S 1472 -v
> udpgso_bench_rx: data[1472]: len 2944, a(97) != q(113)


As noted by Willem, both the commit message and the above command
sequence is quite confusing. Please reorder the commands in the exact
sequence you run them, presumably:

udpgso_bench_rx -4 -G -S 1472 -v &
udpgso_bench_tx -l 4 -4 -D "$DST" -S 0


> This is because the sending buffers are not aligned by 26 bytes, and the
> GRO is not merged sequentially, and the receiver does not judge this
> situation. We do the validation after the data is split at the receiving
> end, just as the application actually uses this feature.

The wording from Willem response is much more clear. If applicable,
please use such text.

BTW I could not reproduce the issue with any permutation of the
suggested commands I could think of, so possibly that section need some
extra clarification.

Thanks,

Paolo