[PATCH net 0/2] selftests: Fix udpgro failures

[PATCH net 0/2] selftests: Fix udpgro failures

[Hangbin Liu]

There are 2 issues for the current udpgro test. The first one is the testing
doesn't record all the failures, which may report pass but the test actually
failed. e.g.
https://netdev-3.bots.linux.dev/vmksft-net/results/725661/45-udpgro-sh/stdout

The other one is after commit d7db7775ea2e ("net: veth: do not manipulate
GRO when using XDP"), there is no need to load xdp program to enable GRO
on veth device.

Hangbin Liu (2):
  selftests: udpgro: report error when receive failed
  selftests: udpgro: no need to load xdp for gro

 tools/testing/selftests/net/udpgro.sh | 50 +++++++++++++--------------
 1 file changed, 25 insertions(+), 25 deletions(-)

-- 
2.45.0

[PATCH net 1/2] selftests: udpgro: report error when receive failed

[Hangbin Liu]

Currently, we only check the latest senders's exit code. If the receiver
report failed, it is not recoreded. Fix it by checking the exit code
of all the involved processes.

Before:
  bad GRO lookup                          ok
  multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520

 ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520

 failed
 $ echo $?
 0

After:
  bad GRO lookup                          ok
  multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520

 ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520

 failed
 $ echo $?
 1

Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
Suggested-by: Paolo Abeni <pabeni@redhat.com>
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/udpgro.sh | 41 ++++++++++++++++-----------
 1 file changed, 24 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
index 11a1ebda564f..7e0164247b83 100755
--- a/tools/testing/selftests/net/udpgro.sh
+++ b/tools/testing/selftests/net/udpgro.sh
@@ -49,14 +49,15 @@ run_one() {
 
 	cfg_veth
 
-	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
-		echo "ok" || \
-		echo "failed" &
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} &
+	local PID1=$!
 
 	wait_local_port_listen ${PEER_NS} 8000 udp
 	./udpgso_bench_tx ${tx_args}
-	ret=$?
-	wait $(jobs -p)
+	check_err $?
+	wait ${PID1}
+	check_err $?
+	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"
 	return $ret
 }
 
@@ -93,16 +94,17 @@ run_one_nat() {
 	# ... so that GRO will match the UDP_GRO enabled socket, but packets
 	# 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 -C 1000 -R 10 ${family} -b ${addr2%/*} ${rx_args} && \
-		echo "ok" || \
-		echo "failed"&
+	local PID1=$!
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${family} -b ${addr2%/*} ${rx_args} &
+	local PID2=$!
 
 	wait_local_port_listen "${PEER_NS}" 8000 udp
 	./udpgso_bench_tx ${tx_args}
-	ret=$?
-	kill -INT $pid
-	wait $(jobs -p)
+	check_err $?
+	kill -INT ${PID1}
+	wait ${PID2}
+	check_err $?
+	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"
 	return $ret
 }
 
@@ -115,16 +117,21 @@ run_one_2sock() {
 	cfg_veth
 
 	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" &
+	local PID1=$!
+	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 2000 -R 10 ${rx_args} &
+	local PID2=$!
 
 	wait_local_port_listen "${PEER_NS}" 12345 udp
 	./udpgso_bench_tx ${tx_args} -p 12345
+	check_err $?
 	wait_local_port_listen "${PEER_NS}" 8000 udp
 	./udpgso_bench_tx ${tx_args}
-	ret=$?
-	wait $(jobs -p)
+	check_err $?
+	wait ${PID1}
+	check_err $?
+	wait ${PID2}
+	check_err $?
+	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"
 	return $ret
 }
 
-- 
2.45.0

Re: [PATCH net 1/2] selftests: udpgro: report error when receive failed

[Paolo Abeni]

On 8/14/24 09:57, Hangbin Liu wrote:
> Currently, we only check the latest senders's exit code. If the receiver
> report failed, it is not recoreded. Fix it by checking the exit code
> of all the involved processes.
> 
> Before:
>    bad GRO lookup                          ok
>    multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
> 
>   ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
> 
>   failed
>   $ echo $?
>   0
> 
> After:
>    bad GRO lookup                          ok
>    multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
> 
>   ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
> 
>   failed
>   $ echo $?
>   1
> 
> Fixes: 3327a9c46352 ("selftests: add functionals test for UDP GRO")
> Suggested-by: Paolo Abeni <pabeni@redhat.com>
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
> ---
>   tools/testing/selftests/net/udpgro.sh | 41 ++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
> index 11a1ebda564f..7e0164247b83 100755
> --- a/tools/testing/selftests/net/udpgro.sh
> +++ b/tools/testing/selftests/net/udpgro.sh
> @@ -49,14 +49,15 @@ run_one() {
>   
>   	cfg_veth
>   
> -	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
> -		echo "ok" || \
> -		echo "failed" &
> +	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} &
> +	local PID1=$!
>   
>   	wait_local_port_listen ${PEER_NS} 8000 udp
>   	./udpgso_bench_tx ${tx_args}
> -	ret=$?
> -	wait $(jobs -p)
> +	check_err $?
> +	wait ${PID1}
> +	check_err $?
> +	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"

I think that with the above, in case of a failure, every test after the 
failing one will should fail, regardless of the actual results, am I 
correct?

Thanks,

Paolo

Re: [PATCH net 1/2] selftests: udpgro: report error when receive failed

[Hangbin Liu]

On Wed, Aug 14, 2024 at 12:19:22PM +0200, Paolo Abeni wrote:
> > --- a/tools/testing/selftests/net/udpgro.sh
> > +++ b/tools/testing/selftests/net/udpgro.sh
> > @@ -49,14 +49,15 @@ run_one() {
> >   	cfg_veth
> > -	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
> > -		echo "ok" || \
> > -		echo "failed" &
> > +	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} &
> > +	local PID1=$!
> >   	wait_local_port_listen ${PEER_NS} 8000 udp
> >   	./udpgso_bench_tx ${tx_args}
> > -	ret=$?
> > -	wait $(jobs -p)
> > +	check_err $?
> > +	wait ${PID1}
> > +	check_err $?
> > +	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"
> 
> I think that with the above, in case of a failure, every test after the
> failing one will should fail, regardless of the actual results, am I
> correct?

No, only the failed test echo "failed". The passed tests still
report "ok". The "check_err $?" in run_all function only record none 0
ret as return value.

Thanks
Hangbin

Re: [PATCH net 1/2] selftests: udpgro: report error when receive failed

[Hangbin Liu]

On Wed, Aug 14, 2024 at 03:57:57PM +0800, Hangbin Liu wrote:
> ---
>  tools/testing/selftests/net/udpgro.sh | 41 ++++++++++++++++-----------
>  1 file changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
> index 11a1ebda564f..7e0164247b83 100755
> --- a/tools/testing/selftests/net/udpgro.sh
> +++ b/tools/testing/selftests/net/udpgro.sh
> @@ -49,14 +49,15 @@ run_one() {
>  
>  	cfg_veth
>  
> -	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} && \
> -		echo "ok" || \
> -		echo "failed" &
> +	ip netns exec "${PEER_NS}" ./udpgso_bench_rx -C 1000 -R 10 ${rx_args} &
> +	local PID1=$!
>  
>  	wait_local_port_listen ${PEER_NS} 8000 udp
>  	./udpgso_bench_tx ${tx_args}
> -	ret=$?
> -	wait $(jobs -p)
> +	check_err $?
> +	wait ${PID1}
> +	check_err $?
> +	[ "$ret" -eq 0 ] && echo "ok" || echo "failed"
>  	return $ret
>  }

Self NACK. The ret need to define first in each function, or the check_err
will failed... I forgot to the update the patch before post..

Thanks
Hangbin

[PATCH net 2/2] selftests: udpgro: no need to load xdp for gro

[Hangbin Liu]

After commit d7db7775ea2e ("net: veth: do not manipulate GRO when using
XDP"), there is no need to load XDP program to enable GRO. On the other
hand, the current test is failed due to loading the XDP program. e.g.

 # selftests: net: udpgro.sh
 # ipv4
 #  no GRO                                  ok
 #  no GRO chk cmsg                         ok
 #  GRO                                     ./udpgso_bench_rx: recv: bad packet len, got 1472, expected 14720
 #
 # failed

 [...]

 #  bad GRO lookup                          ok
 #  multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
 #
 # ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
 #
 # failed
 ok 1 selftests: net: udpgro.sh

After fix, all the test passed.

 # ./udpgro.sh
 ipv4
  no GRO                                  ok
  [...]
  multiple GRO socks                      ok

Fixes: d7db7775ea2e ("net: veth: do not manipulate GRO when using XDP")
Reported-by: Yi Chen <yiche@redhat.com>
Closes: https://issues.redhat.com/browse/RHEL-53858
Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>
---
 tools/testing/selftests/net/udpgro.sh | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/udpgro.sh b/tools/testing/selftests/net/udpgro.sh
index 7e0164247b83..180b337c8345 100755
--- a/tools/testing/selftests/net/udpgro.sh
+++ b/tools/testing/selftests/net/udpgro.sh
@@ -7,8 +7,6 @@ source net_helper.sh
 
 readonly PEER_NS="ns-peer-$(mktemp -u XXXXXX)"
 
-BPF_FILE="xdp_dummy.bpf.o"
-
 # set global exit status, but never reset nonzero one.
 check_err()
 {
@@ -38,7 +36,7 @@ cfg_veth() {
 	ip -netns "${PEER_NS}" addr add dev veth1 192.168.1.1/24
 	ip -netns "${PEER_NS}" addr add dev veth1 2001:db8::1/64 nodad
 	ip -netns "${PEER_NS}" link set dev veth1 up
-	ip -n "${PEER_NS}" link set veth1 xdp object ${BPF_FILE} section xdp
+	ip netns exec "${PEER_NS}" ethtool -K veth1 gro on
 }
 
 run_one() {
@@ -203,11 +201,6 @@ run_all() {
 	return $ret
 }
 
-if [ ! -f ${BPF_FILE} ]; then
-	echo "Missing ${BPF_FILE}. Run 'make' first"
-	exit -1
-fi
-
 if [[ $# -eq 0 ]]; then
 	run_all
 elif [[ $1 == "__subprocess" ]]; then
-- 
2.45.0

Re: [PATCH net 2/2] selftests: udpgro: no need to load xdp for gro

[Toke Høiland-Jørgensen]

Hangbin Liu <liuhangbin@gmail.com> writes:

> After commit d7db7775ea2e ("net: veth: do not manipulate GRO when using
> XDP"), there is no need to load XDP program to enable GRO. On the other
> hand, the current test is failed due to loading the XDP program. e.g.
>
>  # selftests: net: udpgro.sh
>  # ipv4
>  #  no GRO                                  ok
>  #  no GRO chk cmsg                         ok
>  #  GRO                                     ./udpgso_bench_rx: recv: bad packet len, got 1472, expected 14720
>  #
>  # failed
>
>  [...]
>
>  #  bad GRO lookup                          ok
>  #  multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
>  #
>  # ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
>  #
>  # failed
>  ok 1 selftests: net: udpgro.sh
>
> After fix, all the test passed.
>
>  # ./udpgro.sh
>  ipv4
>   no GRO                                  ok
>   [...]
>   multiple GRO socks                      ok
>
> Fixes: d7db7775ea2e ("net: veth: do not manipulate GRO when using XDP")
> Reported-by: Yi Chen <yiche@redhat.com>
> Closes: https://issues.redhat.com/browse/RHEL-53858
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.com>

Re: [PATCH net 2/2] selftests: udpgro: no need to load xdp for gro

[Paolo Abeni]

On 8/14/24 09:57, Hangbin Liu wrote:
> After commit d7db7775ea2e ("net: veth: do not manipulate GRO when using
> XDP"), there is no need to load XDP program to enable GRO. On the other
> hand, the current test is failed due to loading the XDP program. e.g.
> 
>   # selftests: net: udpgro.sh
>   # ipv4
>   #  no GRO                                  ok
>   #  no GRO chk cmsg                         ok
>   #  GRO                                     ./udpgso_bench_rx: recv: bad packet len, got 1472, expected 14720
>   #
>   # failed
> 
>   [...]
> 
>   #  bad GRO lookup                          ok
>   #  multiple GRO socks                      ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
>   #
>   # ./udpgso_bench_rx: recv: bad packet len, got 1452, expected 14520
>   #
>   # failed
>   ok 1 selftests: net: udpgro.sh
> 
> After fix, all the test passed.
> 
>   # ./udpgro.sh
>   ipv4
>    no GRO                                  ok
>    [...]
>    multiple GRO socks                      ok
> 
> Fixes: d7db7775ea2e ("net: veth: do not manipulate GRO when using XDP")
> Reported-by: Yi Chen <yiche@redhat.com>
> Closes: https://issues.redhat.com/browse/RHEL-53858
> Signed-off-by: Hangbin Liu <liuhangbin@gmail.com>

LGTM,

Acked-by: Paolo Abeni <pabeni@redhat.com>