RE: [PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix verifty exceptions

From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: yang.yang29@zte.com.cn, davem@davemloft.net,
	willemdebruijn.kernel@gmail.com
Cc: edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	shuah@kernel.org, netdev@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org,
	zhang.yunkai@zte.com.cn, yang.yang29@zte.com.cn,
	xu.xin16@zte.com.cn
Subject: RE: [PATCH linux-next v3] selftests: net: udpgso_bench_rx: Fix verifty exceptions
Date: Mon, 24 Apr 2023 10:10:54 -0400	[thread overview]
Message-ID: <64468deedd90a_191c8029493@willemb.c.googlers.com.notmuch> (raw)
In-Reply-To: <202304241355464262541@zte.com.cn>

yang.yang29@ 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. The problem is easily
> reproducible in the latest kernel.
> 
> Environment A, the sender:
> bash# udpgso_bench_tx -l 4 -4 -D "$IP_B"
> udpgso_bench_tx: write: Connection refused

This error is not relevant to the bug that is being fixed

> Environment B, the receiver:
> bash# udpgso_bench_rx -4 -G -S 1472 -v
> udpgso_bench_rx: data[1472]: len 17664, a(97) != q(113)
> 
> If the packet is captured, you will see:
> Environment A, the sender:
> bash# tcpdump -i eth0 host "$IP_B" &
> IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
> IP $IP_A.41025 > $IP_B.8000: UDP, length 1472
> IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

Same here

> Environment B, the receiver:
> bash# tcpdump -i eth0 host "$IP_B" &
> IP $IP_A.41025 > $IP_B.8000: UDP, length 7360
> IP $IP_A.41025 > $IP_B.8000: UDP, length 14720
> IP $IP_B > $IP_A: ICMP $IP_B udp port 8000 unreachable, length 556

And here

> In one test, the verification data is printed as follows:
> abcd...xyz           | 1...
> ..                  |
> abcd...xyz           |
> abcd...opabcd...xyz  | ...1472... Not xyzabcd, messages are merged
> ..                  |
> 
> The issue is that the test on receive for expected payload pattern 
> {AB..Z}+ fail for GRO packets if segment payload does not end on a Z.

This is really the only pertinent explanation needed for the fix.

> The issue still exists when using the GRO with -G, but not using the -S
> to obtain gsosize. Therefore, a print has been added to remind users.

So the issue is that -G/cfg_gro_segment enables UDP_GRO, but
-S/cfg_expected_gso_size enables recvmsg cmsg UDP_GRO. We need
gso_size to know whether discontinuities will appear, so cannot
verify payload for -G without -S. There really is no reason to
ever run the test in that configuration, should perhaps fail.

Btw title should start with PATCH net as this is a fix, instead of
PATCH linux-next. And it is verify not verifty. Also needs a Fixes tag:

Fixes: 0a9ac2e954091 ("selftests: add GRO support to udp bench rx program")

> Changes in v3:
> - Simplify description and adjust judgment order.
> 
> Changes in v2:
> - Fix confusing descriptions.
> 
> 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 | 34 +++++++++++++++++++++++----
>  1 file changed, 29 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgso_bench_rx.c b/tools/testing/selftests/net/udpgso_bench_rx.c
> index f35a924d4a30..3ad18cbc570d 100644
> --- a/tools/testing/selftests/net/udpgso_bench_rx.c
> +++ b/tools/testing/selftests/net/udpgso_bench_rx.c
> @@ -189,26 +189,45 @@ 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
>  			cur++;
> 
> -		if (data[i] != cur)
> +		if (data[i] != cur) {
> +			if (cfg_gro_segment && !cfg_expected_gso_size)
> +				error(0, 0, "Use -S to obtain gsosize to guide "
> +					"splitting and verification.");
> +

This is not the place to add a gso_size test. Drop.

>  			error(1, 0, "data[%d]: len %d, %c(%hhu) != %c(%hhu)\n",
>  			      i, len,
>  			      sanitized_char(data[i]), data[i],
>  			      sanitized_char(cur), cur);
> +		}
> +	}
> +}
> +
> +static void do_verify_udp_gro(const char *data, int len, int segment_size)
> +{
> +	int start = 0;
> +
> +	while (len - start > 0) {
> +		if (len - start > segment_size)
> +			do_verify_udp(data, start, segment_size);
> +		else
> +			do_verify_udp(data, start, len - start);

Instead of adding start argument, just pass data + start as first argument.

> +
> +		start += segment_size;
>  	}
>  }
> 
> @@ -268,7 +287,12 @@ static void do_flush_udp(int fd)
>  			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);

This only test a fraction of the payload. The test should always test
the entire payload. It should just be aware of discontinuity at gso_size.
>  		}
>  		if (cfg_expected_gso_size && cfg_expected_gso_size != gso_size)
>  			error(1, 0, "recv: bad gso size, got %d, expected %d "
> -- 
> 2.15.2