| Message ID | 20240814075758.163065-2-liuhangbin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: Fix udpgro failures | expand |
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
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
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
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 }
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(-)