[PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Pablo Martin Medrano]

After debugging the following output for big_tcp.sh on a board:

CLI GSO | GW GRO | GW GSO | SER GRO
on        on       on       on      : [PASS]
on        off      on       off     : [PASS]
off       on       on       on      : [FAIL_on_link1]
on        on       off      on      : [FAIL_on_link1]

Davide Caratti found that by default the test duration 1s is too short
in slow systems to reach the correct cwd size necessary for tcp/ip to
generate at least one packet bigger than 65536 (matching the iptables
match on length rule the test evaluates)

This skips (with xfail) the aforementioned failing combinations when
KSFT_MACHINE_SLOW is set. For that the test has been modified to use
facilities from net/lib.sh.

The new output for the test will look like this (example with a forced
XFAIL)

Testing for BIG TCP:
      CLI GSO | GW GRO | GW GSO | SER GRO
TEST: on        on       on       on                    [ OK ]
TEST: on        off      on       off                   [ OK ]
TEST: off       on       on       on                    [XFAIL]

Changes in v2:
- Don't break the loop and use lib.sh facilities (thanks Peter Machata)
- Rephrased the subject from "longer netperf session on slow machines"
  as the patch is not configuring a longer session but skipping
- Added tags and SOB and the Fixes: hash (thank you Davide Caratti)
- Link to v1: https://lore.kernel.org/all/b800a71479a24a4142542051636e980c3b547434.1739794830.git.pablmart@redhat.com/

Fixes: a19747c3b9b ("selftests: net: let big_tcp test cope with slow env")
Suggested-by: Davide Caratti <dcaratti@redhat.com>
Suggested-by: Petr Machata <petrm@nvidia.com>
Signed-off-by: Pablo Martin Medrano <pablmart@redhat.com>
---
 tools/testing/selftests/net/big_tcp.sh | 21 ++++++++++-----------
 1 file changed, 10 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/big_tcp.sh b/tools/testing/selftests/net/big_tcp.sh
index 2db9d15cd45f..dc2ecfd58961 100755
--- a/tools/testing/selftests/net/big_tcp.sh
+++ b/tools/testing/selftests/net/big_tcp.sh
@@ -21,8 +21,7 @@ CLIENT_GW6="2001:db8:1::2"
 MAX_SIZE=128000
 CHK_SIZE=65535
 
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
+source lib.sh
 
 setup() {
 	ip netns add $CLIENT_NS
@@ -143,21 +142,20 @@ do_test() {
 	start_counter link3 $SERVER_NS
 	do_netperf $CLIENT_NS
 
-	if check_counter link1 $ROUTER_NS; then
-		check_counter link3 $SERVER_NS || ret="FAIL_on_link3"
-	else
-		ret="FAIL_on_link1"
-	fi
+	check_counter link1 $ROUTER_NS
+	check_err $? "fail on link1"
+	check_counter link3 $SERVER_NS
+	check_err $? "fail on link3"
 
 	stop_counter link1 $ROUTER_NS
 	stop_counter link3 $SERVER_NS
-	printf "%-9s %-8s %-8s %-8s: [%s]\n" \
-		$cli_tso $gw_gro $gw_tso $ser_gro $ret
+	log_test "$(printf "%-9s %-8s %-8s %-8s" \
+			$cli_tso $gw_gro $gw_tso $ser_gro)"
 	test $ret = "PASS"
 }
 
 testup() {
-	echo "CLI GSO | GW GRO | GW GSO | SER GRO" && \
+	echo "      CLI GSO | GW GRO | GW GSO | SER GRO" && \
 	do_test "on"  "on"  "on"  "on"  && \
 	do_test "on"  "off" "on"  "off" && \
 	do_test "off" "on"  "on"  "on"  && \
@@ -176,7 +174,8 @@ if ! ip link help 2>&1 | grep gso_ipv4_max_size &> /dev/null; then
 fi
 
 trap cleanup EXIT
+xfail_on_slow
 setup && echo "Testing for BIG TCP:" && \
 NF=4 testup && echo "***v4 Tests Done***" && \
 NF=6 testup && echo "***v6 Tests Done***"
-exit $?
+exit $EXIT_STATUS
-- 
2.48.1

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Simon Horman]

On Wed, Feb 19, 2025 at 06:07:58PM +0100, Pablo Martin Medrano wrote:
> After debugging the following output for big_tcp.sh on a board:
> 
> CLI GSO | GW GRO | GW GSO | SER GRO
> on        on       on       on      : [PASS]
> on        off      on       off     : [PASS]
> off       on       on       on      : [FAIL_on_link1]
> on        on       off      on      : [FAIL_on_link1]
> 
> Davide Caratti found that by default the test duration 1s is too short
> in slow systems to reach the correct cwd size necessary for tcp/ip to
> generate at least one packet bigger than 65536 (matching the iptables
> match on length rule the test evaluates)
> 
> This skips (with xfail) the aforementioned failing combinations when
> KSFT_MACHINE_SLOW is set. For that the test has been modified to use
> facilities from net/lib.sh.
> 
> The new output for the test will look like this (example with a forced
> XFAIL)
> 
> Testing for BIG TCP:
>       CLI GSO | GW GRO | GW GSO | SER GRO
> TEST: on        on       on       on                    [ OK ]
> TEST: on        off      on       off                   [ OK ]
> TEST: off       on       on       on                    [XFAIL]
> 
> Changes in v2:
> - Don't break the loop and use lib.sh facilities (thanks Peter Machata)
> - Rephrased the subject from "longer netperf session on slow machines"
>   as the patch is not configuring a longer session but skipping
> - Added tags and SOB and the Fixes: hash (thank you Davide Caratti)
> - Link to v1: https://lore.kernel.org/all/b800a71479a24a4142542051636e980c3b547434.1739794830.git.pablmart@redhat.com/

Hi Pablo,

FWIIW, I think this can be moved to below the scissors ("---").

> Fixes: a19747c3b9b ("selftests: net: let big_tcp test cope with slow env")

Checkpatch complains that fixes tags should have 12 or more characters of
sha1 hash.

Fixes: a19747c3b9bf ("selftests: net: let big_tcp test cope with slow env")

Lastly, and most importantly, it seems that there is new feedback on a
predecessor of this patch, which probably needs to be addressed.

- Re: [PATCH net] selftests/net: big_tcp: longer netperf session on slow machines
  https://lore.kernel.org/all/20250221144408.784cc642@kernel.org/

> Suggested-by: Davide Caratti <dcaratti@redhat.com>
> Suggested-by: Petr Machata <petrm@nvidia.com>
> Signed-off-by: Pablo Martin Medrano <pablmart@redhat.com>
> ---
>  tools/testing/selftests/net/big_tcp.sh | 21 ++++++++++-----------
>  1 file changed, 10 insertions(+), 11 deletions(-)

...

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Pablo Martin Medrano]

On Mon, 24 Feb 2025, Simon Horman wrote:

> Hi Pablo,
>
> FWIIW, I think this can be moved to below the scissors ("---").
>
> Checkpatch complains that fixes tags should have 12 or more characters of
> sha1 hash.
>
> Fixes: a19747c3b9bf ("selftests: net: let big_tcp test cope with slow env")

Thank you Simon! I will modify this for the next version. Looks like I am
falling in all the stones possible for my first patch :) Receiving such a
friendly support is great.

>
> Lastly, and most importantly, it seems that there is new feedback on a
> predecessor of this patch, which probably needs to be addressed.

Addressing it. Thank you for pointing it out!

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Simon Horman]

On Mon, Feb 24, 2025 at 06:34:55PM +0100, Pablo Martin Medrano wrote:
> 
> 
> On Mon, 24 Feb 2025, Simon Horman wrote:
> 
> > Hi Pablo,
> >
> > FWIIW, I think this can be moved to below the scissors ("---").
> >
> > Checkpatch complains that fixes tags should have 12 or more characters of
> > sha1 hash.
> >
> > Fixes: a19747c3b9bf ("selftests: net: let big_tcp test cope with slow env")
> 
> Thank you Simon! I will modify this for the next version. Looks like I am
> falling in all the stones possible for my first patch :) Receiving such a
> friendly support is great.

Perhaps there are too many stones :)

> > Lastly, and most importantly, it seems that there is new feedback on a
> > predecessor of this patch, which probably needs to be addressed.
> 
> Addressing it. Thank you for pointing it out!

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Petr Machata]

It is customary to Cc people who have expressed interest in past
versions of your patches. I had opinions on v1, so please Cc me
on v2+ as well.

One more of them stones :)

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Petr Machata]

Pablo Martin Medrano <pablmart@redhat.com> writes:

> diff --git a/tools/testing/selftests/net/big_tcp.sh b/tools/testing/selftests/net/big_tcp.sh
> index 2db9d15cd45f..dc2ecfd58961 100755
> --- a/tools/testing/selftests/net/big_tcp.sh
> +++ b/tools/testing/selftests/net/big_tcp.sh
> @@ -21,8 +21,7 @@ CLIENT_GW6="2001:db8:1::2"
>  MAX_SIZE=128000
>  CHK_SIZE=65535
>  
> -# Kselftest framework requirement - SKIP code is 4.
> -ksft_skip=4
> +source lib.sh
>  
>  setup() {
>  	ip netns add $CLIENT_NS
> @@ -143,21 +142,20 @@ do_test() {

Just a small comment.

In general it is a good hygiene to have a RET=0 for each log_test(). A
test that neglects to set it will get a spurious failure if it comes
after a failing test. Arguably log_test() should do this automatically,
but currently it does not.

Due to all the &&'s peppered down there, do_test() only gets called at
most once, so it's OK in this case.

>  	start_counter link3 $SERVER_NS
>  	do_netperf $CLIENT_NS
>  
> -	if check_counter link1 $ROUTER_NS; then
> -		check_counter link3 $SERVER_NS || ret="FAIL_on_link3"
> -	else
> -		ret="FAIL_on_link1"
> -	fi
> +	check_counter link1 $ROUTER_NS
> +	check_err $? "fail on link1"
> +	check_counter link3 $SERVER_NS
> +	check_err $? "fail on link3"
>  
>  	stop_counter link1 $ROUTER_NS
>  	stop_counter link3 $SERVER_NS
> -	printf "%-9s %-8s %-8s %-8s: [%s]\n" \
> -		$cli_tso $gw_gro $gw_tso $ser_gro $ret
> +	log_test "$(printf "%-9s %-8s %-8s %-8s" \
> +			$cli_tso $gw_gro $gw_tso $ser_gro)"
>  	test $ret = "PASS"
>  }
>  
>  testup() {
> -	echo "CLI GSO | GW GRO | GW GSO | SER GRO" && \
> +	echo "      CLI GSO | GW GRO | GW GSO | SER GRO" && \
>  	do_test "on"  "on"  "on"  "on"  && \
>  	do_test "on"  "off" "on"  "off" && \
>  	do_test "off" "on"  "on"  "on"  && \
> @@ -176,7 +174,8 @@ if ! ip link help 2>&1 | grep gso_ipv4_max_size &> /dev/null; then
>  fi
>  
>  trap cleanup EXIT
> +xfail_on_slow
>  setup && echo "Testing for BIG TCP:" && \
>  NF=4 testup && echo "***v4 Tests Done***" && \
>  NF=6 testup && echo "***v6 Tests Done***"
> -exit $?
> +exit $EXIT_STATUS

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Pablo Martin Medrano]

On Tue, 25 Feb 2025, Petr Machata wrote:

> It is customary to Cc people who have expressed interest in past
> versions of your patches. I had opinions on v1, so please Cc me
> on v2+ as well.

I will keep that in mind for next versions. Thank you Petr!

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Pablo Martin Medrano]

On Tue, 25 Feb 2025, Petr Machata wrote:

> Due to all the &&'s peppered down there, do_test() only gets called at
> most once, so it's OK in this case.

Actually do_test() do always returns 0, so it gets called all times in the
code. check_err is setting RET and keeping it at the failing return
value, so check_err is always returning error after the first error

If I force the error by injecting in do_test():

  if [ $gw_tso = off -a $cli_tso = on ]; then
    check_err 1 "forced to fail when GW_GSO is off and CLI GSO is on"
  else
    check_err $ret_check_counter "fail on link1"
  fi

The output is:

  Testing for BIG TCP:
        CLI GSO | GW GRO | GW GSO | SER GRO
  TEST: on        on       on       on                         [ OK ]
  TEST: on        off      on       off                        [ OK ]
  TEST: off       on       on       on                         [ OK ]
  TEST: on        on       off      on                         [FAIL]
          forced to fail when GW_GSO is off and CLI GSO is on
  TEST: off       on       off      on                         [FAIL]
          forced to fail when GW_GSO is off and CLI GSO is on
  ***v4 Tests Done***

So setting RET at the end of do_test is needed indeed.

Re: [PATCH net v2] selftests/net: big_tcp: return xfail on slow machines

[Petr Machata]

Pablo Martin Medrano <pablmart@redhat.com> writes:

> On Tue, 25 Feb 2025, Petr Machata wrote:
>
>> Due to all the &&'s peppered down there, do_test() only gets called at
>> most once, so it's OK in this case.
>
> Actually do_test() do always returns 0, so it gets called all times in the

You are right, I missed that you kept the ret == PASS test at the end of
the function. So just drop that? It's not adding anything, it could be
replaced with a : or true if you truly want to return 0. Then local
ret="PASS" can go away as well.

> code. check_err is setting RET and keeping it at the failing return
> value, so check_err is always returning error after the first error
>
> If I force the error by injecting in do_test():
>
>   if [ $gw_tso = off -a $cli_tso = on ]; then
>     check_err 1 "forced to fail when GW_GSO is off and CLI GSO is on"
>   else
>     check_err $ret_check_counter "fail on link1"
>   fi
>
> The output is:
>
>   Testing for BIG TCP:
>         CLI GSO | GW GRO | GW GSO | SER GRO
>   TEST: on        on       on       on                         [ OK ]
>   TEST: on        off      on       off                        [ OK ]
>   TEST: off       on       on       on                         [ OK ]
>   TEST: on        on       off      on                         [FAIL]
>           forced to fail when GW_GSO is off and CLI GSO is on
>   TEST: off       on       off      on                         [FAIL]
>           forced to fail when GW_GSO is off and CLI GSO is on
>   ***v4 Tests Done***
>
> So setting RET at the end of do_test is needed indeed.

The convention is to do it at the start of the test, before the first
check_err etc.