[PATCH v2 0/3] selftests/net: deflake GRO tests and fix return value and output

[PATCH v2 0/3] selftests/net: deflake GRO tests and fix return value and output

[Kevin Krakauer]

The GRO selftests can flake and have some confusing behavior. These
changes make the output and return value of GRO behave as expected, then
deflake the tests.

v2:
- Split into multiple commits.
- Reduced napi_defer_hard_irqs to 1.
- Reduced gro_flush_timeout to 100us.
- Fixed comment that wasn't updated.

v1: https://lore.kernel.org/netdev/20250218164555.1955400-1-krakauer@google.com/

Kevin Krakauer (3):
  selftests/net: have `gro.sh -t` return a correct exit code
  selftests/net: only print passing message in GRO tests when tests pass
  selftests/net: deflake GRO tests

 tools/testing/selftests/net/gro.c         | 8 +++++---
 tools/testing/selftests/net/gro.sh        | 7 ++++---
 tools/testing/selftests/net/setup_veth.sh | 3 ++-
 3 files changed, 11 insertions(+), 7 deletions(-)

-- 
2.48.1.658.g4767266eb4-goog

[PATCH v2 1/3] selftests/net: have `gro.sh -t` return a correct exit code

[Kevin Krakauer]

Modify gro.sh to return a useful exit code when the -t flag is used. It
formerly returned 0 no matter what.

Tested: Ran `gro.sh -t large` and verified that test failures return 1.
Signed-off-by: Kevin Krakauer <krakauer@google.com>
---
 tools/testing/selftests/net/gro.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
index 02c21ff4ca81..aabd6e5480b8 100755
--- a/tools/testing/selftests/net/gro.sh
+++ b/tools/testing/selftests/net/gro.sh
@@ -100,5 +100,6 @@ trap cleanup EXIT
 if [[ "${test}" == "all" ]]; then
   run_all_tests
 else
-  run_test "${proto}" "${test}"
+  exit_code=$(run_test "${proto}" "${test}")
+  exit $exit_code
 fi;
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v2 1/3] selftests/net: have `gro.sh -t` return a correct exit code

[Willem de Bruijn]

Kevin Krakauer wrote:
> Modify gro.sh to return a useful exit code when the -t flag is used. It
> formerly returned 0 no matter what.
> 
> Tested: Ran `gro.sh -t large` and verified that test failures return 1.
> Signed-off-by: Kevin Krakauer <krakauer@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  tools/testing/selftests/net/gro.sh | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
> index 02c21ff4ca81..aabd6e5480b8 100755
> --- a/tools/testing/selftests/net/gro.sh
> +++ b/tools/testing/selftests/net/gro.sh
> @@ -100,5 +100,6 @@ trap cleanup EXIT
>  if [[ "${test}" == "all" ]]; then
>    run_all_tests
>  else
> -  run_test "${proto}" "${test}"
> +  exit_code=$(run_test "${proto}" "${test}")
> +  exit $exit_code
>  fi;

This is due to run_test ending with echo ${exit_code}, which itself
always succeeds. Rather than the actual exit_code of the process it
ran, right?

It looks a bit odd, but this is always how run_all_tests uses
run_test.

Re: [PATCH v2 1/3] selftests/net: have `gro.sh -t` return a correct exit code

[Kevin Krakauer]

On Thu, Feb 27, 2025 at 11:20:15AM -0500, Willem de Bruijn wrote:
> > ---
> >  tools/testing/selftests/net/gro.sh | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
> > index 02c21ff4ca81..aabd6e5480b8 100755
> > --- a/tools/testing/selftests/net/gro.sh
> > +++ b/tools/testing/selftests/net/gro.sh
> > @@ -100,5 +100,6 @@ trap cleanup EXIT
> >  if [[ "${test}" == "all" ]]; then
> >    run_all_tests
> >  else
> > -  run_test "${proto}" "${test}"
> > +  exit_code=$(run_test "${proto}" "${test}")
> > +  exit $exit_code
> >  fi;
> 
> This is due to run_test ending with echo ${exit_code}, which itself
> always succeeds. Rather than the actual exit_code of the process it
> ran, right?
> 
> It looks a bit odd, but this is always how run_all_tests uses
> run_test.

Yep. I could change this to use exit codes and $? if that's desirable,
but IME using echo to return is fairly common.

Re: [PATCH v2 1/3] selftests/net: have `gro.sh -t` return a correct exit code

[Willem de Bruijn]

Kevin Krakauer wrote:
> On Thu, Feb 27, 2025 at 11:20:15AM -0500, Willem de Bruijn wrote:
> > > ---
> > >  tools/testing/selftests/net/gro.sh | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
> > > index 02c21ff4ca81..aabd6e5480b8 100755
> > > --- a/tools/testing/selftests/net/gro.sh
> > > +++ b/tools/testing/selftests/net/gro.sh
> > > @@ -100,5 +100,6 @@ trap cleanup EXIT
> > >  if [[ "${test}" == "all" ]]; then
> > >    run_all_tests
> > >  else
> > > -  run_test "${proto}" "${test}"
> > > +  exit_code=$(run_test "${proto}" "${test}")
> > > +  exit $exit_code
> > >  fi;
> > 
> > This is due to run_test ending with echo ${exit_code}, which itself
> > always succeeds. Rather than the actual exit_code of the process it
> > ran, right?
> > 
> > It looks a bit odd, but this is always how run_all_tests uses
> > run_test.
> 
> Yep. I could change this to use exit codes and $? if that's desirable,
> but IME using echo to return is fairly common.

Thanks. No need to change it.

[PATCH v2 2/3] selftests/net: only print passing message in GRO tests when tests pass

[Kevin Krakauer]

gro.c:main no longer erroneously claims a test passes when running as a
sender.

Tested: Ran `gro.sh -t large` to verify the sender no longer prints a
status.

Signed-off-by: Kevin Krakauer <krakauer@google.com>
---
 tools/testing/selftests/net/gro.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/gro.c b/tools/testing/selftests/net/gro.c
index b2184847e388..d5824eadea10 100644
--- a/tools/testing/selftests/net/gro.c
+++ b/tools/testing/selftests/net/gro.c
@@ -1318,11 +1318,13 @@ int main(int argc, char **argv)
 	read_MAC(src_mac, smac);
 	read_MAC(dst_mac, dmac);
 
-	if (tx_socket)
+	if (tx_socket) {
 		gro_sender();
-	else
+	} else {
+		/* Only the receiver exit status determines test success. */
 		gro_receiver();
+		fprintf(stderr, "Gro::%s test passed.\n", testname);
+	}
 
-	fprintf(stderr, "Gro::%s test passed.\n", testname);
 	return 0;
 }
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v2 2/3] selftests/net: only print passing message in GRO tests when tests pass

[Willem de Bruijn]

Kevin Krakauer wrote:
> gro.c:main no longer erroneously claims a test passes when running as a
> sender.
> 
> Tested: Ran `gro.sh -t large` to verify the sender no longer prints a
> status.
> 
> Signed-off-by: Kevin Krakauer <krakauer@google.com>

Reviewed-by: Willem de Bruijn <willemb@google.com>

[PATCH v2 3/3] selftests/net: deflake GRO tests

[Kevin Krakauer]

GRO tests are timing dependent and can easily flake. This is partially
mitigated in gro.sh by giving each subtest 3 chances to pass. However,
this still flakes on some machines. Reduce the flakiness by:

- Bumping retries to 6.
- Setting napi_defer_hard_irqs to 1 to reduce the chance that GRO is
  flushed prematurely. This also lets us reduce the gro_flush_timeout
  from 1ms to 100us.

Tested: Ran `gro.sh -t large` 1000 times. There were no failures with
this change. Ran inside strace to increase flakiness.

Signed-off-by: Kevin Krakauer <krakauer@google.com>
---
 tools/testing/selftests/net/gro.sh        | 4 ++--
 tools/testing/selftests/net/setup_veth.sh | 3 ++-
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
index aabd6e5480b8..9e3f186bc2a1 100755
--- a/tools/testing/selftests/net/gro.sh
+++ b/tools/testing/selftests/net/gro.sh
@@ -18,10 +18,10 @@ run_test() {
   "--smac" "${CLIENT_MAC}" "--test" "${test}" "--verbose" )
 
   setup_ns
-  # Each test is run 3 times to deflake, because given the receive timing,
+  # Each test is run 6 times to deflake, because given the receive timing,
   # not all packets that should coalesce will be considered in the same flow
   # on every try.
-  for tries in {1..3}; do
+  for tries in {1..6}; do
     # Actual test starts here
     ip netns exec $server_ns ./gro "${ARGS[@]}" "--rx" "--iface" "server" \
       1>>log.txt &
diff --git a/tools/testing/selftests/net/setup_veth.sh b/tools/testing/selftests/net/setup_veth.sh
index 1f78a87f6f37..eb3182066d12 100644
--- a/tools/testing/selftests/net/setup_veth.sh
+++ b/tools/testing/selftests/net/setup_veth.sh
@@ -11,7 +11,8 @@ setup_veth_ns() {
 	local -r ns_mac="$4"
 
 	[[ -e /var/run/netns/"${ns_name}" ]] || ip netns add "${ns_name}"
-	echo 1000000 > "/sys/class/net/${ns_dev}/gro_flush_timeout"
+	echo 100000 > "/sys/class/net/${ns_dev}/gro_flush_timeout"
+	echo 1 > "/sys/class/net/${ns_dev}/napi_defer_hard_irqs"
 	ip link set dev "${ns_dev}" netns "${ns_name}" mtu 65535
 	ip -netns "${ns_name}" link set dev "${ns_dev}" up
 
-- 
2.48.1.658.g4767266eb4-goog

Re: [PATCH v2 3/3] selftests/net: deflake GRO tests

[Willem de Bruijn]

Kevin Krakauer wrote:
> GRO tests are timing dependent and can easily flake. This is partially
> mitigated in gro.sh by giving each subtest 3 chances to pass. However,
> this still flakes on some machines. Reduce the flakiness by:
> 
> - Bumping retries to 6.
> - Setting napi_defer_hard_irqs to 1 to reduce the chance that GRO is
>   flushed prematurely. This also lets us reduce the gro_flush_timeout
>   from 1ms to 100us.
> 
> Tested: Ran `gro.sh -t large` 1000 times. There were no failures with
> this change. Ran inside strace to increase flakiness.
> 
> Signed-off-by: Kevin Krakauer <krakauer@google.com>

Nice! Thanks

Reviewed-by: Willem de Bruijn <willemb@google.com>

> ---
>  tools/testing/selftests/net/gro.sh        | 4 ++--
>  tools/testing/selftests/net/setup_veth.sh | 3 ++-
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/gro.sh b/tools/testing/selftests/net/gro.sh
> index aabd6e5480b8..9e3f186bc2a1 100755
> --- a/tools/testing/selftests/net/gro.sh
> +++ b/tools/testing/selftests/net/gro.sh
> @@ -18,10 +18,10 @@ run_test() {
>    "--smac" "${CLIENT_MAC}" "--test" "${test}" "--verbose" )
>  
>    setup_ns
> -  # Each test is run 3 times to deflake, because given the receive timing,
> +  # Each test is run 6 times to deflake, because given the receive timing,

Only if respinning: this was always imprecise: it is run up to X times. But
exits immediately on success.

Re: [PATCH v2 0/3] selftests/net: deflake GRO tests and fix return value and output

[Willem de Bruijn]

Kevin Krakauer wrote:
> The GRO selftests can flake and have some confusing behavior. These
> changes make the output and return value of GRO behave as expected, then
> deflake the tests.
> 
> v2:
> - Split into multiple commits.
> - Reduced napi_defer_hard_irqs to 1.
> - Reduced gro_flush_timeout to 100us.
> - Fixed comment that wasn't updated.
> 
> v1: https://lore.kernel.org/netdev/20250218164555.1955400-1-krakauer@google.com/

For next time: add target: [PATCH net-next]

 
> Kevin Krakauer (3):
>   selftests/net: have `gro.sh -t` return a correct exit code
>   selftests/net: only print passing message in GRO tests when tests pass
>   selftests/net: deflake GRO tests
> 
>  tools/testing/selftests/net/gro.c         | 8 +++++---
>  tools/testing/selftests/net/gro.sh        | 7 ++++---
>  tools/testing/selftests/net/setup_veth.sh | 3 ++-
>  3 files changed, 11 insertions(+), 7 deletions(-)
> 
> -- 
> 2.48.1.658.g4767266eb4-goog
> 

Re: [PATCH v2 0/3] selftests/net: deflake GRO tests and fix return value and output

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Wed, 26 Feb 2025 11:27:22 -0800 you wrote:
> The GRO selftests can flake and have some confusing behavior. These
> changes make the output and return value of GRO behave as expected, then
> deflake the tests.
> 
> v2:
> - Split into multiple commits.
> - Reduced napi_defer_hard_irqs to 1.
> - Reduced gro_flush_timeout to 100us.
> - Fixed comment that wasn't updated.
> 
> [...]

Here is the summary with links:
  - [v2,1/3] selftests/net: have `gro.sh -t` return a correct exit code
    https://git.kernel.org/netdev/net-next/c/784e6abd99f2
  - [v2,2/3] selftests/net: only print passing message in GRO tests when tests pass
    https://git.kernel.org/netdev/net-next/c/41cda5728470
  - [v2,3/3] selftests/net: deflake GRO tests
    https://git.kernel.org/netdev/net-next/c/51bef03e1a71

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html