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(-)
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;
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.
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.
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.
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; }
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
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
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.
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
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!
linux-kselftest-mirror@lists.linaro.org