selftests: Fixes for kernel CI

RFC PATCH In review Petr Machata 2 participants 23 messages Mar 25, 2024 – Mar 26, 2024

Subsystems (8)

Networking Network drivers Kselftest IPv4/IPv6 +4 more subsystems

lore

Share & tools

lore

Petr Machata

author maintainer

2024-03-25 17:32 UTC #0 lore raw

cover selftests: Fixes for kernel CI

As discussed on the bi-weekly call on Jan 30, and in mailing around kernel CI effort, some changes are desirable in the suite of forwarding selftests the better to work with the CI tooling. Namely:

- The forwarding selftests use a configuration file where names of interfaces are defined and various variables can be overridden. There is also forwarding.config.sample that users can use as a template to refer to when creating the config file. What happens a fair bit is that users either do not know about this at all, or simply forget, and are confused by cryptic failures about interfaces that cannot be created.

In patches #1 - #3 have lib.sh just be the single source of truth with regards to which variables exist. That includes the topology variables which were previously only in the sample file, and any "tweak variables", such as what tools to use, sleep times, etc.

forwarding.config.sample then becomes just a placeholder with a couple examples. Unless specific HW should be exercised, or specific tools used, the defaults are usually just fine.

- Several net/forwarding/ selftests (and one net/ one) cannot be run on veth pairs, they need an actual HW interface to run on. They are generic in the sense that any capable HW should pass them, which is why they have been put to net/forwarding/ as opposed to drivers/net/, but they do not generalize to veth. The fact that these tests are in net/forwarding/, but still complaining when run, is confusing.

In patches #4 - #6 move these tests to a new directory drivers/net/hw.

- The following patches extend the codebase to handle well test results other than pass and fail.

Patch #7 is preparatory. It converts several log_test_skip to XFAIL, so that tests do not spuriously end up returning non-0 when they are not supposed to.

In patches #8 - #10, introduce some missing ksft constants, then support having those constants in RET, and then finally in EXIT_STATUS.

- The traffic scheduler tests generate a large amount of network traffic to test the behavior of the scheduler. This demands a relatively high-performance computer. On slow machines, such as with a debugging kernel, the test would spuriously fail.

It can still be useful to "go through the motions" though, to possibly catch bugs in setup of the scheduler graph and passing packets around. Thus we still want to run the tests, just with lowered demands.

To that end, in patches #11 - #12, introduce an environment variable KSFT_MACHINE_SLOW, with obvious meaning. Tests can then make checks more lenient, such as mark failures as XFAIL. A helper, xfail_on_slow, is provided to mark performance-sensitive parts of the selftest.

- In patch #13, use a similar mechanism to mark a NH group stats selftest to XFAIL HW stats tests when run on VETH pairs.

- All these changes complicate the hitherto straightforward logging and checking logic, so in patch #14, add a selftest that checks this functionality in lib.sh.

Petr Machata (14):
selftests: net: libs: Change variable fallback syntax
selftests: forwarding.config.sample: Move overrides to lib.sh
selftests: forwarding: README: Document customization
selftests: forwarding: ipip_lib: Do not import lib.sh
selftests: forwarding: Move several selftests
selftests: forwarding: Ditch skip_on_veth()
selftests: forwarding: Change inappropriate log_test_skip() calls
selftests: lib: Define more kselftest exit codes
selftests: forwarding: Have RET track kselftest framework constants
selftests: forwarding: Convert log_test() to recognize RET values
selftests: forwarding: Support for performance sensitive tests
selftests: forwarding: Mark performance-sensitive tests
selftests: forwarding: router_mpath_nh_lib: Don't skip, xfail on veth
selftests: forwarding: Add a test for testing lib.sh functionality

 .../testing/selftests/drivers/net/hw/Makefile |  25 ++
 .../net/hw}/devlink_port_split.py             |   0
 .../forwarding => drivers/net/hw}/ethtool.sh  |   5 +-
 .../net/hw}/ethtool_extended_state.sh         |   5 +-
 .../net/hw}/ethtool_lib.sh                    |   0
 .../net/hw}/ethtool_mm.sh                     |   3 +-
 .../net/hw}/ethtool_rmon.sh                   |   7 +-
 .../net/hw}/hw_stats_l3.sh                    |  19 +-
 .../net/hw}/hw_stats_l3_gre.sh                |   7 +-
 .../forwarding => drivers/net/hw}/loopback.sh |   5 +-
 .../testing/selftests/drivers/net/hw/settings |   1 +
 .../selftests/drivers/net/mlxsw/mlxsw_lib.sh  |   2 +-
 .../net/mlxsw/spectrum-2/resource_scale.sh    |   1 -
 .../net/mlxsw/spectrum/resource_scale.sh      |   1 -
 tools/testing/selftests/net/Makefile          |   1 -
 .../testing/selftests/net/forwarding/Makefile |   9 +-
 tools/testing/selftests/net/forwarding/README |  33 +++
 .../net/forwarding/forwarding.config.sample   |  53 ++--
 .../selftests/net/forwarding/ipip_lib.sh      |   1 -
 tools/testing/selftests/net/forwarding/lib.sh | 255 +++++++++++++-----
 .../selftests/net/forwarding/lib_sh_test.sh   | 208 ++++++++++++++
 .../net/forwarding/router_mpath_nh_lib.sh     |  12 +-
 .../selftests/net/forwarding/sch_ets_tests.sh |  19 +-
 .../selftests/net/forwarding/sch_red.sh       |  10 +-
 .../selftests/net/forwarding/sch_tbf_core.sh  |   2 +-
 .../selftests/net/forwarding/tc_common.sh     |   2 +-
 .../selftests/net/forwarding/tc_tunnel_key.sh |   2 -
 tools/testing/selftests/net/lib.sh            |  48 +++-
 28 files changed, 565 insertions(+), 171 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/hw/Makefile
 rename tools/testing/selftests/{net => drivers/net/hw}/devlink_port_split.py (100%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool.sh (98%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_extended_state.sh (96%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_lib.sh (100%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_mm.sh (99%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_rmon.sh (92%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/hw_stats_l3.sh (96%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/hw_stats_l3_gre.sh (92%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/loopback.sh (92%)
 create mode 100644 tools/testing/selftests/drivers/net/hw/settings
 create mode 100755 tools/testing/selftests/net/forwarding/lib_sh_test.sh

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #1 lore raw

RFC PATCH 1/14 selftests: net: libs: Change variable fallback syntax

The current syntax of X=${X:=X} first evaluates the ${X:=Y} expression, which either uses the existing value of $X if there is one, or uses the value of "Y" as a fallback, and assigns it to X. The expression is then replaced with the now-current value of $X. Assigning that value to X once more is meaningless.

So avoid the outer X=... bit, and instead express the same idea though the do-nothing ":" built-in as : "${X:=Y}". This also cleans up the block nicely and makes it more readable.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Benjamin Poirier <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 48 +++++++++----------
 .../selftests/net/forwarding/tc_common.sh     |  2 +-
 tools/testing/selftests/net/lib.sh            |  3 +-
 3 files changed, 27 insertions(+), 26 deletions(-)

tools/testing/selftests/net/forwarding/lib.sh +24 ~~-24~~

index e579c2e0c462..ee48c4603ff0 100644
@@ -5,30 +5,30 @@
 # Defines
 
 # Can be overridden by the configuration file.
-PING=${PING:=ping}
-PING6=${PING6:=ping6}
-MZ=${MZ:=mausezahn}
-MZ_DELAY=${MZ_DELAY:=0}
-ARPING=${ARPING:=arping}
-TEAMD=${TEAMD:=teamd}
-WAIT_TIME=${WAIT_TIME:=5}
-PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
-PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
-NETIF_TYPE=${NETIF_TYPE:=veth}
-NETIF_CREATE=${NETIF_CREATE:=yes}
-MCD=${MCD:=smcrouted}
-MC_CLI=${MC_CLI:=smcroutectl}
-PING_COUNT=${PING_COUNT:=10}
-PING_TIMEOUT=${PING_TIMEOUT:=5}
-WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
-INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
-LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
-REQUIRE_JQ=${REQUIRE_JQ:=yes}
-REQUIRE_MZ=${REQUIRE_MZ:=yes}
-REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
-STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
-TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
-TROUTE6=${TROUTE6:=traceroute6}
+: "${PING:=ping}"
+: "${PING6:=ping6}"
+: "${MZ:=mausezahn}"
+: "${MZ_DELAY:=0}"
+: "${ARPING:=arping}"
+: "${TEAMD:=teamd}"
+: "${WAIT_TIME:=5}"
+: "${PAUSE_ON_FAIL:=no}"
+: "${PAUSE_ON_CLEANUP:=no}"
+: "${NETIF_TYPE:=veth}"
+: "${NETIF_CREATE:=yes}"
+: "${MCD:=smcrouted}"
+: "${MC_CLI:=smcroutectl}"
+: "${PING_COUNT:=10}"
+: "${PING_TIMEOUT:=5}"
+: "${WAIT_TIMEOUT:=20}"
+: "${INTERFACE_TIMEOUT:=600}"
+: "${LOW_AGEING_TIME:=1000}"
+: "${REQUIRE_JQ:=yes}"
+: "${REQUIRE_MZ:=yes}"
+: "${REQUIRE_MTOOLS:=no}"
+: "${STABLE_MAC_ADDRS:=no}"
+: "${TCPDUMP_EXTRA_FLAGS:=}"
+: "${TROUTE6:=traceroute6}"
 
 net_forwarding_dir=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")

tools/testing/selftests/net/forwarding/tc_common.sh +1 -1

index bce8bb8d2b6f..2e3326edfa9a 100644
@@ -4,7 +4,7 @@
 CHECK_TC="yes"
 
 # Can be overridden by the configuration file. See lib.sh
-TC_HIT_TIMEOUT=${TC_HIT_TIMEOUT:=1000} # ms
+: "${TC_HIT_TIMEOUT:=1000}" # ms
 
 tc_check_packets()
 {

tools/testing/selftests/net/lib.sh +2 -1

index f9fe182dfbd4..5b366cc4fc43 100644
@@ -4,7 +4,8 @@
 ##############################################################################
 # Defines
 
-WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
+: "${WAIT_TIMEOUT:=20}"
+
 BUSYWAIT_TIMEOUT=$((WAIT_TIMEOUT * 1000)) # ms
 
 # Kselftest framework requirement - SKIP code is 4.

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #2 lore raw

RFC PATCH 2/14 selftests: forwarding.config.sample: Move overrides to lib.sh

In reply to Petr Machata

forwarding.config.sample, net/lib.sh and net/forwarding/lib.sh contain definitions and redefinitions of some of the same variables. The overlap between net/forwarding/lib.sh and forwarding.config.sample is especially large. This duplication is a potential source of confusion and problems.

It would be overall less error prone if each variable were defined in one place only. In this patch set, that place is the library itself. Therefore move all comments from forwarding.config.sample to net/forwarding/lib.sh.

Move over also a definition of TC_FLAG, which was missing from lib.sh entirely.

Additionally, add to lib.sh a default definition of the topology variables. The logic behind this is that forgetting to specify forwarding.config was a frequent source of frustration for the selftest users. But really, most of the time the default veth based topology is just fine. We considered just sourcing forwarding.config.sample instead if forwarding.config is not available, but this is a cleaner solution.

That means the syntax of the forwarding.config.sample override has to change to an array assignment, so that the whole variable is overwritten, not just individual keys, which could leave the value of some keys unchanged. Do the same in lib.sh for any cut'n'pasters out there.

The config file is then given a sort of carte blanche to redefine whatever variables it sees fit from the libraries. This is described in a comment in the file. Only a handful of variables are left behind, to illustrate the customization.

The fact that the variables are now missing from forwarding.config.sample, and therefore would miss from forwarding.config derived from that file as well, should not change anything. This is just the sample file. Users that keep their own forwarding.config would retain it as before.

The only observable change is introduction of TC_FLAG to lib.sh, because now the filters would not be attempted to install to HW datapath. For veth pairs this does not change anything. For HW deployments, users presumably have forwarding.config with this value overridden.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Benjamin Poirier <[email protected]>

---
 .../net/forwarding/forwarding.config.sample   | 53 ++++----------
 tools/testing/selftests/net/forwarding/lib.sh | 69 ++++++++++++++++---
 2 files changed, 74 insertions(+), 48 deletions(-)

tools/testing/selftests/net/forwarding/forwarding.config.sample +15 ~~-38~~

index 1fc4f0242fc5..f1ca95e79a65 100644
@@ -3,51 +3,28 @@
 
 ##############################################################################
 # Topology description. p1 looped back to p2, p3 to p4 and so on.
-declare -A NETIFS
 
-NETIFS[p1]=veth0
-NETIFS[p2]=veth1
-NETIFS[p3]=veth2
-NETIFS[p4]=veth3
-NETIFS[p5]=veth4
-NETIFS[p6]=veth5
-NETIFS[p7]=veth6
-NETIFS[p8]=veth7
-NETIFS[p9]=veth8
-NETIFS[p10]=veth9
+NETIFS=(
+	[p1]=veth0
+	[p2]=veth1
+	[p3]=veth2
+	[p4]=veth3
+	[p5]=veth4
+	[p6]=veth5
+	[p7]=veth6
+	[p8]=veth7
+	[p9]=veth8
+	[p10]=veth9
+)
 
 # Port that does not have a cable connected.
 NETIF_NO_CABLE=eth8
 
 ##############################################################################
-# Defines
+# In addition to the topology-related variables, it is also possible to override
+# in this file other variables that net/lib.sh, net/forwarding/lib.sh or other
+# libraries or selftests use. E.g.:
 
-# IPv4 ping utility name
-PING=ping
-# IPv6 ping utility name. Some distributions use 'ping' for IPv6.
 PING6=ping6
-# Packet generator. Some distributions use 'mz'.
 MZ=mausezahn
-# mausezahn delay between transmissions in microseconds.
-MZ_DELAY=0
-# Time to wait after interfaces participating in the test are all UP
 WAIT_TIME=5
-# Whether to pause on failure or not.
-PAUSE_ON_FAIL=no
-# Whether to pause on cleanup or not.
-PAUSE_ON_CLEANUP=no
-# Type of network interface to create
-NETIF_TYPE=veth
-# Whether to create virtual interfaces (veth) or not
-NETIF_CREATE=yes
-# Timeout (in seconds) before ping exits regardless of how many packets have
-# been sent or received
-PING_TIMEOUT=5
-# Minimum ageing_time (in centiseconds) supported by hardware
-LOW_AGEING_TIME=1000
-# Flag for tc match, supposed to be skip_sw/skip_hw which means do not process
-# filter by software/hardware
-TC_FLAG=skip_hw
-# IPv6 traceroute utility name.
-TROUTE6=traceroute6
-

tools/testing/selftests/net/forwarding/lib.sh +59 ~~-10~~

93 lines not on this page · raw message

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #3 lore raw

RFC PATCH 3/14 selftests: forwarding: README: Document customization

In reply to Petr Machata

That any sort of customization is possible at all, let alone how it should be done, is currently not at all clear. Document the whats and hows in README.

Signed-off-by Petr Machata <[email protected]>

Reviewed-by Benjamin Poirier <[email protected]>

---
 tools/testing/selftests/net/forwarding/README | 33 +++++++++++++++++++
 1 file changed, 33 insertions(+)

tools/testing/selftests/net/forwarding/README +33 -0

index b8a2af8fcfb7..7fdb6a9ca543 100644
@@ -56,3 +56,36 @@ o Checks shall be added to lib.sh for any external dependencies.
 o Code shall be checked using ShellCheck [1] prior to submission.
 
 1. https://www.shellcheck.net/
+
+Customization
+=============
+
+The forwarding selftests framework uses a number of variables that
+influence its behavior and tools it invokes, and how it invokes them, in
+various ways. A number of these variables can be overridden. The way these
+overridable variables are specified is typically one of the following two
+syntaxes:
+
+	: "${VARIABLE:=default_value}"
+	VARIABLE=${VARIABLE:=default_value}
+
+Any of these variables can be overridden. Notably net/forwarding/lib.sh and
+net/lib.sh contain a number of overridable variables.
+
+One way of overriding these variables is through the environment:
+
+	PAUSE_ON_FAIL=yes ./some_test.sh
+
+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}
+
+A way to customize variables in a persistent fashion is to create a file
+named forwarding.config in this directory. lib.sh sources the file if
+present, so it can contain any shell code. Typically it will contain
+assignments of variables whose value should be overridden.
+
+forwarding.config.sample is available in the directory as an example of
+how forwarding.config might look.

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2024-03-26 00:34 UTC #4 lore raw

Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization

Quoted 5 lines · On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:

tools/testing/selftests/net/forwarding/README +5 -0

+The forwarding selftests framework uses a number of variables that
+influence its behavior and tools it invokes, and how it invokes them, in
+various ways. A number of these variables can be overridden. The way these
+overridable variables are specified is typically one of the following two
+syntaxes:

tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 77

Quoted 10 lines

tools/testing/selftests/net/forwarding/README +10 -0

+
+	: "${VARIABLE:=default_value}"
+	VARIABLE=${VARIABLE:=default_value}
+
+Any of these variables can be overridden. Notably net/forwarding/lib.sh and
+net/lib.sh contain a number of overridable variables.
+
+One way of overriding these variables is through the environment:
+
+	PAUSE_ON_FAIL=yes ./some_test.sh

I like this conversion a lot. Makes me want to propose that we make this a standard feature of kselftest. If "env" file exists in the test directory kselftest would load its contents before running every test.

That's more of a broader question to anyone reading on linux-kselftest@ if there's no interest more than happy to merge as is :)

tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 83

Quoted 5 lines

tools/testing/selftests/net/forwarding/README +5 -0

+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}

Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.? We can have lib.sh convert that into an array with a ugly-but-short loop, it's a bit tempting to get rid of the exception.

Petr Machata

author maintainer

2024-03-26 12:14 UTC #5 lore raw

Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization

Quoted 10 lines · Jakub Kicinski

Quoted 9 lines · On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:

tools/testing/selftests/net/forwarding/README +9 -0

+The forwarding selftests framework uses a number of variables that
+influence its behavior and tools it invokes, and how it invokes them, in
+various ways. A number of these variables can be overridden. The way these
+overridable variables are specified is typically one of the following two
+syntaxes:
+
+	: "${VARIABLE:=default_value}"
+	VARIABLE=${VARIABLE:=default_value}
+

Quoted 8 lines

tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 77
Quoted 6 lines
tools/testing/selftests/net/forwarding/README +6 -0
+Any of these variables can be overridden. Notably net/forwarding/lib.sh and
+net/lib.sh contain a number of overridable variables.
+
+One way of overriding these variables is through the environment:
+
+	PAUSE_ON_FAIL=yes ./some_test.sh
I like this conversion a lot. Makes me want to propose that we make this

Convention you mean?
Nothing was converted, this has always worked.

Quoted 6 lines

a standard feature of kselftest. If "env" file exists in the test directory kselftest would load its contents before running every test.
That's more of a broader question to anyone reading on linux-kselftest@ if there's no interest more than happy to merge as is :)

Quoted 9 lines

tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 83
Quoted 5 lines
tools/testing/selftests/net/forwarding/README +5 -0
+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}
Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.? We can have lib.sh convert that into an array with a ugly-but-short loop, it's a bit tempting to get rid of the exception.

The exception is a bit annoying, yeah. But it works today, should stay, and therefore should be documented, so the paragraph won't go away. I use it all the time, too. I basically don't use the config file, I just use the env overrides and the argv interface names. It's very handy.

The alternative is also very verbose:

	NETIF_1=swp1 NETIF_2=swp2 NETIF_3=swp3 [...] ./some_test.sh.

Maybe we could do this though?

NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh

And like this it won't make you want to pull your hair from all the repetition:

	NETIFS=$(echo swp{1..8}) ./some_test.sh

But NETIFS is going to be a special case one way or another. That you need to specify it through several variables, or a variable with a special value, means you need to explain it as a special case in the documentation. At which point you have two exceptions, and an interaction between them, to describe.

Jakub Kicinski

maintainer

2024-03-26 14:13 UTC #6 lore raw

Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization

Quoted 21 lines · On Tue, 26 Mar 2024 11:31:31 +0100 Petr Machata wrote:

Quoted 10 lines · Jakub Kicinski
Quoted 9 lines · On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
tools/testing/selftests/net/forwarding/README +9 -0
+The forwarding selftests framework uses a number of variables that
+influence its behavior and tools it invokes, and how it invokes them, in
+various ways. A number of these variables can be overridden. The way these
+overridable variables are specified is typically one of the following two
+syntaxes:
+
+	: "${VARIABLE:=default_value}"
+	VARIABLE=${VARIABLE:=default_value}
+
Quoted 8 lines
tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 77
Quoted 6 lines
tools/testing/selftests/net/forwarding/README +6 -0
+Any of these variables can be overridden. Notably net/forwarding/lib.sh and
+net/lib.sh contain a number of overridable variables.
+
+One way of overriding these variables is through the environment:
+
+	PAUSE_ON_FAIL=yes ./some_test.sh  
I like this conversion a lot. Makes me want to propose that we make this

Quoted 1 line

Convention you mean?

Yes, sorry

Quoted 1 line

Nothing was converted, this has always worked.

Right, for forwarding and perhaps net.

Quoted 32 lines

Quoted 6 lines
a standard feature of kselftest. If "env" file exists in the test directory kselftest would load its contents before running every test.
That's more of a broader question to anyone reading on linux-kselftest@ if there's no interest more than happy to merge as is :)
Quoted 9 lines
tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 83
Quoted 5 lines
tools/testing/selftests/net/forwarding/README +5 -0
+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}  
Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.? We can have lib.sh convert that into an array with a ugly-but-short loop, it's a bit tempting to get rid of the exception.
The exception is a bit annoying, yeah. But it works today, should stay, and therefore should be documented, so the paragraph won't go away. I use it all the time, too. I basically don't use the config file, I just use the env overrides and the argv interface names. It's very handy.
The alternative is also very verbose:
	NETIF_1=swp1 NETIF_2=swp2 NETIF_3=swp3 [...] ./some_test.sh.
Maybe we could do this though?
NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh
And like this it won't make you want to pull your hair from all the repetition:

Quoted 7 lines

	NETIFS=$(echo swp{1..8}) ./some_test.sh
But NETIFS is going to be a special case one way or another. That you need to specify it through several variables, or a variable with a special value, means you need to explain it as a special case in the documentation. At which point you have two exceptions, and an interaction between them, to describe.

I think there's some value in passing all inputs in the same way (thru env rather than argv). I guess it's subjective, you're coding it up, so you can pick.

Petr Machata

author maintainer

2024-03-26 17:35 UTC #7 lore raw

Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization

Quoted 31 lines · Jakub Kicinski

Quoted 22 lines · On Tue, 26 Mar 2024 11:31:31 +0100 Petr Machata wrote:
Quoted 8 lines · Jakub Kicinski
a standard feature of kselftest. If "env" file exists in the test directory kselftest would load its contents before running every test.
That's more of a broader question to anyone reading on linux-kselftest@ if there's no interest more than happy to merge as is :)
On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
Quoted 9 lines
tools/testing/selftests/net/forwarding/README o Checks shall be added to lib.sh for any external dependencies. · line 83
Quoted 5 lines
tools/testing/selftests/net/forwarding/README +5 -0
+The variable NETIFS is special. Since it is an array variable, there is no
+way to pass it through the environment. Its value can instead be given as
+consecutive arguments to the selftest:
+
+	./some_test.sh swp{1..8}  
Did you consider allowing them to be defined as NETIF_0, NETIF_1 etc.? We can have lib.sh convert that into an array with a ugly-but-short loop, it's a bit tempting to get rid of the exception.
Maybe we could do this though?
Quoted 7 lines
NETIFS="swp1 swp2 swp3 swp4 swp5 swp6 swp7 swp8" ./some_test.sh
But NETIFS is going to be a special case one way or another. That you need to specify it through several variables, or a variable with a special value, means you need to explain it as a special case in the documentation. At which point you have two exceptions, and an interaction between them, to describe.

Quoted 3 lines

I think there's some value in passing all inputs in the same way (thru env rather than argv). I guess it's subjective, you're coding it up, so you can pick.

I kinda like the NETIFS="a b c" approach. If somebody wants to code that up, I'll be happy to review :) I might get around to it at some point.

Petr Machata

author maintainer

2024-03-25 17:32 UTC #8 lore raw

RFC PATCH 4/14 selftests: forwarding: ipip_lib: Do not import lib.sh

In reply to Petr Machata

This library is always sourced in the context where lib.sh has already been sourced as well. Therefore drop the explicit sourcing and expect the client to already have done it. This will simplify moving some of the clients to a different directory.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/ipip_lib.sh | 1 -
 1 file changed, 1 deletion(-)

tools/testing/selftests/net/forwarding/ipip_lib.sh +0 -1

index 30f36a57bae6..01e62c4ac94d 100644
@@ -141,7 +141,6 @@
 # |               $h2 +       |
 # |     192.0.2.18/28         |
 # +---------------------------+
-source lib.sh
 
 h1_create()
 {

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #9 lore raw

RFC PATCH 5/14 selftests: forwarding: Move several selftests

In reply to Petr Machata

The tests in net/forwarding are generally expected to be HW-independent. There are however several tests that, while not depending on any HW in particular, nevertheless depend on being used on HW interfaces. Placing these selftests to net/forwarding is confusing, because the selftest will just report it can't be run on veth pairs. At the same time, placing them to a particular driver's selftests subdirectory would be wrong.

Instead, add a new directory, drivers/net/hw, where these generic but HW independent selftests should be placed. Move over several such tests including one helper library.

Since typically these tests will not be expected to run, omit the directory drivers/net/hw from the TARGETS list in selftests/Makefile. Retain a Makefile in the new directory itself, so that a user can make -C into that directory and act on those tests explicitly.

Cc Roger Quadros <[email protected]>

Cc Tobias Waldekranz <[email protected]>

Cc Danielle Ratson <[email protected]>

Cc Davide Caratti <[email protected]>

Cc Johannes Nixdorf <[email protected]>

Signed-off-by Petr Machata <[email protected]>

---
 .../testing/selftests/drivers/net/hw/Makefile | 25 +++++++++++++++++++
 .../net/hw}/devlink_port_split.py             |  0
 .../forwarding => drivers/net/hw}/ethtool.sh  |  3 ++-
 .../net/hw}/ethtool_extended_state.sh         |  3 ++-
 .../net/hw}/ethtool_lib.sh                    |  0
 .../net/hw}/ethtool_mm.sh                     |  3 ++-
 .../net/hw}/ethtool_rmon.sh                   |  3 ++-
 .../net/hw}/hw_stats_l3.sh                    |  3 ++-
 .../net/hw}/hw_stats_l3_gre.sh                |  5 ++--
 .../forwarding => drivers/net/hw}/loopback.sh |  5 ++--
 .../testing/selftests/drivers/net/hw/settings |  1 +
 tools/testing/selftests/net/Makefile          |  1 -
 .../testing/selftests/net/forwarding/Makefile |  8 ------
 13 files changed, 42 insertions(+), 18 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/hw/Makefile
 rename tools/testing/selftests/{net => drivers/net/hw}/devlink_port_split.py (100%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool.sh (98%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_extended_state.sh (96%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_lib.sh (100%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_mm.sh (99%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/ethtool_rmon.sh (97%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/hw_stats_l3.sh (99%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/hw_stats_l3_gre.sh (93%)
 rename tools/testing/selftests/{net/forwarding => drivers/net/hw}/loopback.sh (92%)
 create mode 100644 tools/testing/selftests/drivers/net/hw/settings

tools/testing/selftests/drivers/net/hw/Makefile new file +25 -0

new file mode 100644
index 000000000000..2259a39a70ed
@@ -0,0 +1,25 @@
+# SPDX-License-Identifier: GPL-2.0+ OR MIT
+
+TEST_PROGS = \
+	devlink_port_split.py \
+	ethtool.sh \
+	ethtool_extended_state.sh \
+	ethtool_mm.sh \
+	ethtool_rmon.sh \
+	hw_stats_l3.sh \
+	hw_stats_l3_gre.sh \
+	loopback.sh \
+	#
+
+TEST_FILES := \
+	ethtool_lib.sh \
+	#
+
+TEST_INCLUDES := \
+	../../../net/lib.sh \
+	../../../net/forwarding/lib.sh \
+	../../../net/forwarding/ipip_lib.sh \
+	../../../net/forwarding/tc_common.sh \
+	#
+
+include ../../../lib.mk

tools/testing/selftests/drivers/net/hw/devlink_port_split.py renamed from tools/testing/selftests/net/devlink_port_split.py +0 -0

similarity index 100%
rename from tools/testing/selftests/net/devlink_port_split.py
rename to tools/testing/selftests/drivers/net/hw/devlink_port_split.py

tools/testing/selftests/drivers/net/hw/ethtool.sh renamed from tools/testing/selftests/net/forwarding/ethtool.sh +2 -1

similarity index 98%
rename from tools/testing/selftests/net/forwarding/ethtool.sh
rename to tools/testing/selftests/drivers/net/hw/ethtool.sh
index aa2eafb7b243..187429670ee7 100755
@@ -10,7 +10,8 @@ ALL_TESTS="
 	different_speeds_autoneg_on
 "
 NUM_NETIFS=2
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
 source ethtool_lib.sh
 
 h1_create()

tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh renamed from tools/testing/selftests/net/forwarding/ethtool_extended_state.sh +2 -1

similarity index 96%
rename from tools/testing/selftests/net/forwarding/ethtool_extended_state.sh
rename to tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
index 17f89c3b7c02..b0f931260a27 100755
@@ -8,7 +8,8 @@ ALL_TESTS="
 "
 
 NUM_NETIFS=2
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
 source ethtool_lib.sh
 
 TIMEOUT=$((WAIT_TIMEOUT * 1000)) # ms

tools/testing/selftests/drivers/net/hw/ethtool_lib.sh renamed from tools/testing/selftests/net/forwarding/ethtool_lib.sh +0 -0

similarity index 100%
rename from tools/testing/selftests/net/forwarding/ethtool_lib.sh
rename to tools/testing/selftests/drivers/net/hw/ethtool_lib.sh

tools/testing/selftests/drivers/net/hw/ethtool_mm.sh renamed from tools/testing/selftests/net/forwarding/ethtool_mm.sh +2 -1

similarity index 99%
rename from tools/testing/selftests/net/forwarding/ethtool_mm.sh
rename to tools/testing/selftests/drivers/net/hw/ethtool_mm.sh
index 50d5bfb17ef1..c301e735c8ab 100755
@@ -14,7 +14,8 @@ ALL_TESTS="
 NUM_NETIFS=2
 REQUIRE_MZ=no
 PREEMPTIBLE_PRIO=0
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
 
 traffic_test()
 {

tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh renamed from tools/testing/selftests/net/forwarding/ethtool_rmon.sh +2 -1

similarity index 97%
rename from tools/testing/selftests/net/forwarding/ethtool_rmon.sh
rename to tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
index 41a34a61f763..709433a4c886 100755
@@ -7,7 +7,8 @@ ALL_TESTS="
 "
 
 NUM_NETIFS=2
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
 
 ETH_FCS_LEN=4
 ETH_HLEN=$((6+6+2))

tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh renamed from tools/testing/selftests/net/forwarding/hw_stats_l3.sh +2 -1

similarity index 99%
rename from tools/testing/selftests/net/forwarding/hw_stats_l3.sh
rename to tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
index 48584a51388f..744760117005 100755
@@ -48,7 +48,8 @@ ALL_TESTS="
 	test_double_enable
 "
 NUM_NETIFS=4
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
 
 h1_create()
 {

tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh renamed from tools/testing/selftests/net/forwarding/hw_stats_l3_gre.sh +3 -2

similarity index 93%
rename from tools/testing/selftests/net/forwarding/hw_stats_l3_gre.sh
rename to tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
index 7594bbb49029..354be353455f 100755
@@ -12,8 +12,9 @@ ALL_TESTS="
 	test_stats_tx
 "
 NUM_NETIFS=6
-source lib.sh
-source ipip_lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/lib.sh
+source "$lib_dir"/../../../net/forwarding/ipip_lib.sh
 
 setup_prepare()
 {

tools/testing/selftests/drivers/net/hw/loopback.sh renamed from tools/testing/selftests/net/forwarding/loopback.sh +3 -2

similarity index 92%
rename from tools/testing/selftests/net/forwarding/loopback.sh
rename to tools/testing/selftests/drivers/net/hw/loopback.sh
index 8f4057310b5b..5acc3ff820aa 100755
@@ -6,8 +6,9 @@ ksft_skip=4
 
 ALL_TESTS="loopback_test"
 NUM_NETIFS=2
-source tc_common.sh
-source lib.sh
+lib_dir=$(dirname "$0")
+source "$lib_dir"/../../../net/forwarding/tc_common.sh
+source "$lib_dir"/../../../net/forwarding/lib.sh
 
 h1_create()
 {

tools/testing/selftests/drivers/net/hw/settings new file +1 -0

new file mode 100644
index 000000000000..e7b9417537fb
@@ -0,0 +1 @@
+timeout=0

tools/testing/selftests/net/Makefile +0 -1

index 7b6918d5f4af..cb418a2346bc 100644
@@ -20,7 +20,6 @@ TEST_PROGS += reuseaddr_ports_exhausted.sh
 TEST_PROGS += txtimestamp.sh
 TEST_PROGS += vrf-xfrm-tests.sh
 TEST_PROGS += rxtimestamp.sh
-TEST_PROGS += devlink_port_split.py
 TEST_PROGS += drop_monitor_tests.sh
 TEST_PROGS += vrf_route_leaking.sh
 TEST_PROGS += bareudp.sh

tools/testing/selftests/net/forwarding/Makefile +0 -8

index 535865b3d1d6..56e3557ba8a6 100644
@@ -15,18 +15,12 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \
 	bridge_vlan_unaware.sh \
 	custom_multipath_hash.sh \
 	dual_vxlan_bridge.sh \
-	ethtool_extended_state.sh \
-	ethtool_mm.sh \
-	ethtool_rmon.sh \
-	ethtool.sh \
 	gre_custom_multipath_hash.sh \
 	gre_inner_v4_multipath.sh \
 	gre_inner_v6_multipath.sh \
 	gre_multipath_nh_res.sh \
 	gre_multipath_nh.sh \
 	gre_multipath.sh \
-	hw_stats_l3.sh \
-	hw_stats_l3_gre.sh \
 	ip6_forward_instats_vrf.sh \
 	ip6gre_custom_multipath_hash.sh \
 	ip6gre_flat_key.sh \

@@ -44,7 +38,6 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \
 	ipip_hier_gre_keys.sh \
 	ipip_hier_gre.sh \
 	local_termination.sh \
-	loopback.sh \
 	mirror_gre_bound.sh \
 	mirror_gre_bridge_1d.sh \
 	mirror_gre_bridge_1d_vlan.sh \

@@ -113,7 +106,6 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \
 	vxlan_symmetric.sh
 
 TEST_FILES := devlink_lib.sh \
-	ethtool_lib.sh \
 	fib_offload_lib.sh \
 	forwarding.config.sample \
 	ip6gre_lib.sh \

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #10 lore raw

RFC PATCH 6/14 selftests: forwarding: Ditch skip_on_veth()

In reply to Petr Machata

Since the selftests that are not supposed to run on veth pairs are now in their own dedicated directory, the skip_on_veth logic can go away. Drop it from the selftests, and from lib.sh.

Cc Danielle Ratson <[email protected]>

Signed-off-by Petr Machata <[email protected]>

---
 .../testing/selftests/drivers/net/hw/ethtool.sh  |  2 --
 .../drivers/net/hw/ethtool_extended_state.sh     |  2 --
 .../selftests/drivers/net/hw/hw_stats_l3.sh      | 16 ++++------------
 .../selftests/drivers/net/hw/hw_stats_l3_gre.sh  |  2 --
 tools/testing/selftests/net/forwarding/lib.sh    | 11 -----------
 5 files changed, 4 insertions(+), 29 deletions(-)

tools/testing/selftests/drivers/net/hw/ethtool.sh +0 -2

index 187429670ee7..bb12d5d70949 100755
@@ -287,8 +287,6 @@ different_speeds_autoneg_on()
 	ethtool -s $h1 autoneg on
 }
 
-skip_on_veth
-
 trap cleanup EXIT
 
 setup_prepare

tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh +0 -2

index b0f931260a27..a7584448416e 100755
@@ -109,8 +109,6 @@ no_cable()
 	ip link set dev $swp3 down
 }
 
-skip_on_veth
-
 setup_prepare
 
 tests_run

tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh +4 ~~-12~~

index 744760117005..7dfc50366c99 100755
@@ -325,17 +325,9 @@ setup_wait
 
 used=$(ip -j stats show dev $rp1.200 group offload subgroup hw_stats_info |
 	   jq '.[].info.l3_stats.used')
-kind=$(ip -j -d link show dev $rp1 |
-	   jq -r '.[].linkinfo.info_kind')
-if [[ $used != true ]]; then
-	if [[ $kind == veth ]]; then
-		log_test_skip "l3_stats not offloaded on veth interface"
-		EXIT_STATUS=$ksft_skip
-	else
-		RET=1 log_test "l3_stats not offloaded"
-	fi
-else
-	tests_run
-fi
+[[ $used = true ]]
+check_err $? "hw_stats_info.used=$used"
+log_test "l3_stats offloaded"
+tests_run
 
 exit $EXIT_STATUS

tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh +0 -2

index 354be353455f..ab8d04855af5 100755
@@ -100,8 +100,6 @@ test_stats_rx()
 	test_stats g2a rx
 }
 
-skip_on_veth
-
 trap cleanup EXIT
 
 setup_prepare

tools/testing/selftests/net/forwarding/lib.sh +0 ~~-11~~

index dbd4348f85b8..ca433ba3612e 100644
@@ -254,17 +254,6 @@ check_port_mab_support()
 	fi
 }
 
-skip_on_veth()
-{
-	local kind=$(ip -j -d link show dev ${NETIFS[p1]} |
-		jq -r '.[].linkinfo.info_kind')
-
-	if [[ $kind == veth ]]; then
-		echo "SKIP: Test cannot be run with veth pairs"
-		exit $ksft_skip
-	fi
-}
-
 if [[ "$(id -u)" -ne 0 ]]; then
 	echo "SKIP: need root privileges"
 	exit $ksft_skip

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:32 UTC #11 lore raw

RFC PATCH 7/14 selftests: forwarding: Change inappropriate log_test_skip() calls

In reply to Petr Machata

The SKIP return should be used for cases where tooling of the machine under test is lacking. For cases where HW is lacking, the appropriate outcome is XFAIL.

This is the case with ethtool_rmon and mlxsw_lib. For these, introduce a new helper, log_test_xfail().

Do the same for router_mpath_nh_lib. Note that it will be fixed using a more reusable way in a following patch.

For the two resource_scale selftests, the log should simply not be written, because there is no problem.

Cc Tobias Waldekranz <[email protected]>

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh   | 4 ++--
 tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh   | 2 +-
 .../drivers/net/mlxsw/spectrum-2/resource_scale.sh       | 1 -
 .../drivers/net/mlxsw/spectrum/resource_scale.sh         | 1 -
 tools/testing/selftests/net/forwarding/lib.sh            | 9 +++++++++
 .../selftests/net/forwarding/router_mpath_nh_lib.sh      | 2 +-
 6 files changed, 13 insertions(+), 6 deletions(-)

tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh +2 -2

index 709433a4c886..e2a1c10d3503 100755
@@ -79,7 +79,7 @@ rmon_histogram()
 
 		for if in $iface $neigh; do
 			if ! ensure_mtu $if ${bucket[0]}; then
-				log_test_skip "$if does not support the required MTU for $step"
+				log_test_xfail "$if does not support the required MTU for $step"
 				return
 			fi
 		done

@@ -94,7 +94,7 @@ rmon_histogram()
 		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high]|@tsv" 2>/dev/null)
 
 	if [ $nbuckets -eq 0 ]; then
-		log_test_skip "$iface does not support $set histogram counters"
+		log_test_xfail "$iface does not support $set histogram counters"
 		return
 	fi
 }

tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh +1 -1

index 6369927e9c37..48395cfd4f95 100644
@@ -42,7 +42,7 @@ __mlxsw_only_on_spectrum()
 	local src=$1; shift
 
 	if ! mlxsw_on_spectrum "$rev"; then
-		log_test_skip $src:$caller "(Spectrum-$rev only)"
+		log_test_xfail $src:$caller "(Spectrum-$rev only)"
 		return 1
 	fi
 }

tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh +0 -1

index a88d8a8c85f2..899b6892603f 100755
@@ -47,7 +47,6 @@ for current_test in ${TESTS:-$ALL_TESTS}; do
 		RET=0
 		target=$(${current_test}_get_target "$should_fail")
 		if ((target == 0)); then
-			log_test_skip "'$current_test' should_fail=$should_fail test"
 			continue
 		fi

tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh +0 -1

index f981c957f097..482ebb744eba 100755
@@ -52,7 +52,6 @@ for current_test in ${TESTS:-$ALL_TESTS}; do
 			RET=0
 			target=$(${current_test}_get_target "$should_fail")
 			if ((target == 0)); then
-				log_test_skip "'$current_test' [$profile] should_fail=$should_fail test"
 				continue
 			fi
 			${current_test}_setup_prepare

tools/testing/selftests/net/forwarding/lib.sh +9 -0

index ca433ba3612e..5415b8d29862 100644
@@ -467,6 +467,15 @@ log_test_skip()
 	return 0
 }
 
+log_test_xfail()
+{
+	local test_name=$1
+	local opt_str=$2
+
+	printf "TEST: %-60s  [XFAIL]\n" "$test_name $opt_str"
+	return 0
+}
+
 log_info()
 {
 	local msg=$1

tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh +1 -1

index 7e7d62161c34..b2d2c6cecc01 100644
@@ -69,7 +69,7 @@ nh_stats_test_dispatch_swhw()
 		nh_stats_do_test "HW $what" "$nh1_id" "$nh2_id" "$group_id" \
 				 nh_stats_get_hw "${mz[@]}"
 	elif [[ $kind == veth ]]; then
-		log_test_skip "HW stats not offloaded on veth topology"
+		log_test_xfail "HW stats not offloaded on veth topology"
 	fi
 }

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #12 lore raw

RFC PATCH 8/14 selftests: lib: Define more kselftest exit codes

In reply to Petr Machata

The following patches will operate with more exit codes besides ksft_skip. Add them here.

Additionally, move a duplicated skip exit code definition from forwarding/tc_tunnel_key.sh. Keep a similar duplicate in forwarding/devlink_lib.sh, because even though lib.sh will have been sourced in all cases where devlink_lib is, the inclusion is not visible in the file itself, and relying on it would be confusing.

Cc Davide Caratti <[email protected]>

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/tc_tunnel_key.sh | 2 --
 tools/testing/selftests/net/lib.sh                      | 6 +++++-
 2 files changed, 5 insertions(+), 3 deletions(-)

tools/testing/selftests/net/forwarding/tc_tunnel_key.sh +0 -2

index 5a5dd9034819..79775b10b99f 100755
@@ -1,7 +1,5 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
-# Kselftest framework requirement - SKIP code is 4.
-ksft_skip=4
 
 ALL_TESTS="tunnel_key_nofrag_test"

tools/testing/selftests/net/lib.sh +5 -1

index 5b366cc4fc43..d9bdf6aa3bf1 100644
@@ -8,8 +8,12 @@
 
 BUSYWAIT_TIMEOUT=$((WAIT_TIMEOUT * 1000)) # ms
 
-# Kselftest framework requirement - SKIP code is 4.
+# Kselftest framework constants.
+ksft_pass=0
+ksft_fail=1
+ksft_xfail=2
 ksft_skip=4
+
 # namespace list created by setup_ns
 NS_LIST=""

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #13 lore raw

RFC PATCH 9/14 selftests: forwarding: Have RET track kselftest framework constants

In reply to Petr Machata

The variable RET keeps track of whether the test under execution has so far failed or not. Currently it works in binary fashion: zero means everything is fine, non-zero means something failed. log_test() then uses the value to given a human-readable message.

In order to allow log_test() to report skips and xfails, the semantics of RET need to be more fine-grained. Therefore have RET value be one of kselftest framework constants: $ksft_fail, $ksft_xfail, etc.

The current logic in check_err() is such that first non-zero value of RET trumps all those that follow. But that is not right when RET has more fine-grained value semantics. Different outcomes have different weights.

The results of PASS and XFAIL are mostly the same: they both communicate a test that did not go wrong. SKIP communicates lack of tooling, which the user should go and try to fix, and as such should not be overridden by the passes. So far, the higher-numbered statuses can be considered weightier. But FAIL should be the weightiest.

Add a helper, ksft_status_merge(), which merges two statuses in a way that respects the above conditions. Express it in a generic manner, because exit status merge is subtly different, and we want to reuse the same logic.

Use the new helper when setting RET in check_err().

Re-express check_fail() in terms of check_err() to avoid duplication.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 21 ++++++++-----
 tools/testing/selftests/net/lib.sh            | 30 +++++++++++++++++++
 2 files changed, 44 insertions(+), 7 deletions(-)

tools/testing/selftests/net/forwarding/lib.sh +14 -7

index 5415b8d29862..e72b2370238c 100644
@@ -396,14 +396,24 @@ EXIT_STATUS=0
 # Per-test return value. Clear at the beginning of each test.
 RET=0
 
+set_ret()
+{
+	local nret=$1; shift
+	local msg=$1; shift
+
+	RET=$(ksft_status_merge $RET $nret)
+	if (( $? )); then
+		retmsg=$msg
+	fi
+}
+
 check_err()
 {
 	local err=$1
 	local msg=$2
 
-	if [[ $RET -eq 0 && $err -ne 0 ]]; then
-		RET=$err
-		retmsg=$msg
+	if ((err)); then
+		set_ret $ksft_fail "$msg"
 	fi
 }

@@ -412,10 +422,7 @@ check_fail()
 	local err=$1
 	local msg=$2
 
-	if [[ $RET -eq 0 && $err -eq 0 ]]; then
-		RET=1
-		retmsg=$msg
-	fi
+	check_err $((!err)) "$msg"
 }
 
 check_err_fail()

tools/testing/selftests/net/lib.sh +30 -0

index d9bdf6aa3bf1..88f6133ca319 100644
@@ -19,6 +19,36 @@ NS_LIST=""
 
 ##############################################################################
 # Helpers
+
+__ksft_status_merge()
+{
+	local a=$1; shift
+	local b=$1; shift
+	local -A weights
+	local weight=0
+
+	for i in "$@"; do
+		weights[$i]=$((weight++))
+	done
+
+	if [[ ${weights[$a]} > ${weights[$b]} ]]; then
+		echo "$a"
+		return 0
+	else
+		echo "$b"
+		return 1
+	fi
+}
+
+ksft_status_merge()
+{
+	local a=$1; shift
+	local b=$1; shift
+
+	__ksft_status_merge "$a" "$b" \
+		$ksft_pass $ksft_xfail $ksft_skip $ksft_fail
+}
+
 busywait()
 {
 	local timeout=$1; shift

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2024-03-26 00:43 UTC #14 lore raw

Re: [RFC PATCH net-next mlxsw 09/14] selftests: forwarding: Have RET track kselftest framework constants

tools/testing/selftests/net/forwarding/lib.sh EXIT_STATUS=0 · line 401

Quoted 3 lines · On Mon, 25 Mar 2024 18:29:16 +0100 Petr Machata wrote:

tools/testing/selftests/net/forwarding/lib.sh +3 -0
+set_ret()
+{
+	local nret=$1; shift

May be worth throwing in a comment that $1 must be a legal ksft ret code, not any exit code from random commands.

Petr Machata

author maintainer

2024-03-26 12:11 UTC #15 lore raw

Re: [RFC PATCH net-next mlxsw 09/14] selftests: forwarding: Have RET track kselftest framework constants

Quoted 7 lines · Jakub Kicinski

tools/testing/selftests/net/forwarding/lib.sh EXIT_STATUS=0 · line 401
Quoted 3 lines · On Mon, 25 Mar 2024 18:29:16 +0100 Petr Machata wrote:
tools/testing/selftests/net/forwarding/lib.sh +3 -0
+set_ret()
+{
+	local nret=$1; shift
May be worth throwing in a comment that $1 must be a legal ksft ret code, not any exit code from random commands.

Hmm, yeah. I'll rename to ret_set_ksft_status() and the variable to ksft_status, I think that will be harder to miss than a comment. The verbosity of the new name doesn't matter, it's an internal helper.

Petr Machata

author maintainer

2024-03-25 17:33 UTC #16 lore raw

RFC PATCH 10/14 selftests: forwarding: Convert log_test() to recognize RET values

In reply to Petr Machata

In a previous patch, the interpretation of RET value was changed to mean the kselftest framework constant with the test outcome: $ksft_pass, $ksft_xfail, etc.

Update log_test() to recognize the various possible RET values.

Then have EXIT_STATUS track the RET value of the current test. This differs subtly from the way RET tracks the value: while for RET we want to recognize XFAIL as a separate status, for purposes of exit code, we want to to conflate XFAIL and PASS, because they both communicate non-failure. Thus add a new helper, ksft_exit_status_merge().

With this log_test_skip() and log_test_xfail() can be reexpressed as thin wrappers around log_test.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 92 ++++++++++++++-----
 tools/testing/selftests/net/lib.sh            |  9 ++
 2 files changed, 77 insertions(+), 24 deletions(-)

tools/testing/selftests/net/forwarding/lib.sh +68 ~~-24~~

index e72b2370238c..942f38988941 100644
@@ -438,6 +438,62 @@ check_err_fail()
 	fi
 }
 
+log_test_result()
+{
+	local test_name=$1; shift
+	local opt_str=$1; shift
+	local result=$1; shift
+	local retmsg=$1; shift
+
+	printf "TEST: %-60s  [%s]\n" "$test_name $opt_str" "$result"
+	if [[ $retmsg ]]; then
+		printf "\t%s\n" "$retmsg"
+	fi
+}
+
+pause_on_fail()
+{
+	if [[ $PAUSE_ON_FAIL == yes ]]; then
+		echo "Hit enter to continue, 'q' to quit"
+		read a
+		[[ $a == q ]] && exit 1
+	fi
+}
+
+handle_test_result_pass()
+{
+	local test_name=$1; shift
+	local opt_str=$1; shift
+
+	log_test_result "$test_name" "$opt_str" " OK "
+}
+
+handle_test_result_fail()
+{
+	local test_name=$1; shift
+	local opt_str=$1; shift
+
+	log_test_result "$test_name" "$opt_str" FAIL "$retmsg"
+	pause_on_fail
+}
+
+handle_test_result_xfail()
+{
+	local test_name=$1; shift
+	local opt_str=$1; shift
+
+	log_test_result "$test_name" "$opt_str" XFAIL "$retmsg"
+	pause_on_fail
+}
+
+handle_test_result_skip()
+{
+	local test_name=$1; shift
+	local opt_str=$1; shift
+
+	log_test_result "$test_name" "$opt_str" SKIP "$retmsg"
+}
+
 log_test()
 {
 	local test_name=$1

@@ -447,40 +503,28 @@ log_test()
 		opt_str="($opt_str)"
 	fi
 
-	if [[ $RET -ne 0 ]]; then
-		EXIT_STATUS=1
-		printf "TEST: %-60s  [FAIL]\n" "$test_name $opt_str"
-		if [[ ! -z "$retmsg" ]]; then
-			printf "\t%s\n" "$retmsg"
-		fi
-		if [ "${PAUSE_ON_FAIL}" = "yes" ]; then
-			echo "Hit enter to continue, 'q' to quit"
-			read a
-			[ "$a" = "q" ] && exit 1
-		fi
-		return 1
+	if ((RET == ksft_pass)); then
+		handle_test_result_pass "$test_name" "$opt_str"
+	elif ((RET == ksft_xfail)); then
+		handle_test_result_xfail "$test_name" "$opt_str"
+	elif ((RET == ksft_skip)); then
+		handle_test_result_skip "$test_name" "$opt_str"
+	else
+		handle_test_result_fail "$test_name" "$opt_str"
 	fi
 
-	printf "TEST: %-60s  [ OK ]\n" "$test_name $opt_str"
-	return 0
+	EXIT_STATUS=$(ksft_exit_status_merge $EXIT_STATUS $RET)
+	return $RET
 }
 
 log_test_skip()
 {
-	local test_name=$1
-	local opt_str=$2
-
-	printf "TEST: %-60s  [SKIP]\n" "$test_name $opt_str"
-	return 0
+	RET=$ksft_skip retmsg= log_test "$@"
 }
 
 log_test_xfail()
 {
-	local test_name=$1
-	local opt_str=$2
-
-	printf "TEST: %-60s  [XFAIL]\n" "$test_name $opt_str"
-	return 0
+	RET=$ksft_xfail retmsg= log_test "$@"
 }
 
 log_info()

tools/testing/selftests/net/lib.sh +9 -0

index 88f6133ca319..b7f7b8695165 100644
@@ -49,6 +49,15 @@ ksft_status_merge()
 		$ksft_pass $ksft_xfail $ksft_skip $ksft_fail
 }
 
+ksft_exit_status_merge()
+{
+	local a=$1; shift
+	local b=$1; shift
+
+	__ksft_status_merge "$a" "$b" \
+		$ksft_xfail $ksft_pass $ksft_skip $ksft_fail
+}
+
 busywait()
 {
 	local timeout=$1; shift

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #17 lore raw

RFC PATCH 11/14 selftests: forwarding: Support for performance sensitive tests

In reply to Petr Machata

Several tests in the suite use large amounts of traffic to e.g. cause congestion and evaluate RED or shaper performance. These tests will not run well on a slow machine, be it one with heavy debug kernel, or a VM, or e.g. a single-board computer. Allow users to specify an environment variable, KSFT_MACHINE_SLOW=yes, to indicate that the tests are being run on one such machine.

Performance sensitive tests can then use a new helper, xfail_on_slow(), to mark parts of the test that are sensitive to low-performance machines. The helper can be used to just mark the whole suite, like so:

xfail_on_slow tests_run

... or, on the other side of the granularity spectrum, to override individual checks:

xfail_on_slow check_err $? "Expected much, got little."

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh | 23 ++++++++++++++++++-
 1 file changed, 22 insertions(+), 1 deletion(-)

tools/testing/selftests/net/forwarding/lib.sh +22 -1

index 942f38988941..cc32bf11739b 100644
@@ -79,6 +79,11 @@ declare -A NETIFS=(
 # Flags for TC filters.
 : "${TC_FLAG:=skip_hw}"
 
+# Whether the machine is "slow" -- i.e. might be incapable of running tests
+# involving heavy traffic. This might be the case on a debug kernel, a VM, or
+# e.g. a low-power board.
+: "${KSFT_MACHINE_SLOW:=no}"
+
 net_forwarding_dir=$(dirname "$(readlink -e "${BASH_SOURCE[0]}")")
 
 if [[ -f $net_forwarding_dir/forwarding.config ]]; then

@@ -407,13 +412,20 @@ set_ret()
 	fi
 }
 
+# Whether FAILs should be interpreted as XFAILs. Internal.
+FAIL_TO_XFAIL=
+
 check_err()
 {
 	local err=$1
 	local msg=$2
 
 	if ((err)); then
-		set_ret $ksft_fail "$msg"
+		if [[ $FAIL_TO_XFAIL = yes ]]; then
+			set_ret $ksft_xfail "$msg"
+		else
+			set_ret $ksft_fail "$msg"
+		fi
 	fi
 }

@@ -438,6 +450,15 @@ check_err_fail()
 	fi
 }
 
+xfail_on_slow()
+{
+	if [[ $KSFT_MACHINE_SLOW = yes ]]; then
+		FAIL_TO_XFAIL=yes "$@"
+	else
+		"$@"
+	fi
+}
+
 log_test_result()
 {
 	local test_name=$1; shift

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #18 lore raw

RFC PATCH 12/14 selftests: forwarding: Mark performance-sensitive tests

In reply to Petr Machata

When run on a slow machine, the scheduler traffic tests can be expected to fail, and should be reported as XFAIL in that case. Therefore run these tests through the perf_sensitive wrapper.

Signed-off-by Petr Machata <[email protected]>

---
 .../selftests/net/forwarding/sch_ets_tests.sh | 19 +++++++++++--------
 .../selftests/net/forwarding/sch_red.sh       | 10 +++++-----
 .../selftests/net/forwarding/sch_tbf_core.sh  |  2 +-
 3 files changed, 17 insertions(+), 14 deletions(-)

tools/testing/selftests/net/forwarding/sch_ets_tests.sh +11 -8

index cdf689e99458..f9d26a7911bb 100644
@@ -199,25 +199,28 @@ ets_set_dwrr_two_bands()
 ets_test_strict()
 {
 	ets_set_strict
-	ets_dwrr_test_01
-	ets_dwrr_test_12
+	xfail_on_slow ets_dwrr_test_01
+	xfail_on_slow ets_dwrr_test_12
 }
 
 ets_test_mixed()
 {
 	ets_set_mixed
-	ets_dwrr_test_01
-	ets_dwrr_test_12
+	xfail_on_slow ets_dwrr_test_01
+	xfail_on_slow ets_dwrr_test_12
 }
 
 ets_test_dwrr()
 {
 	ets_set_dwrr_uniform
-	ets_dwrr_test_012
+	xfail_on_slow ets_dwrr_test_012
+
 	ets_set_dwrr_varying
-	ets_dwrr_test_012
+	xfail_on_slow ets_dwrr_test_012
+
 	ets_change_quantum
-	ets_dwrr_test_012
+	xfail_on_slow ets_dwrr_test_012
+
 	ets_set_dwrr_two_bands
-	ets_dwrr_test_01
+	xfail_on_slow ets_dwrr_test_01
 }

tools/testing/selftests/net/forwarding/sch_red.sh +5 -5

index 81f31179ac88..17f28644568e 100755
@@ -451,35 +451,35 @@ uninstall_qdisc()
 ecn_test()
 {
 	install_qdisc ecn
-	do_ecn_test $BACKLOG
+	xfail_on_slow do_ecn_test $BACKLOG
 	uninstall_qdisc
 }
 
 ecn_nodrop_test()
 {
 	install_qdisc ecn nodrop
-	do_ecn_nodrop_test $BACKLOG
+	xfail_on_slow do_ecn_nodrop_test $BACKLOG
 	uninstall_qdisc
 }
 
 red_test()
 {
 	install_qdisc
-	do_red_test $BACKLOG
+	xfail_on_slow do_red_test $BACKLOG
 	uninstall_qdisc
 }
 
 red_qevent_test()
 {
 	install_qdisc qevent early_drop block 10
-	do_red_qevent_test $BACKLOG
+	xfail_on_slow do_red_qevent_test $BACKLOG
 	uninstall_qdisc
 }
 
 ecn_qevent_test()
 {
 	install_qdisc ecn qevent mark block 10
-	do_ecn_qevent_test $BACKLOG
+	xfail_on_slow do_ecn_qevent_test $BACKLOG
 	uninstall_qdisc
 }

tools/testing/selftests/net/forwarding/sch_tbf_core.sh +1 -1

index d1f26cb7cd73..9cd884d4a5de 100644
@@ -227,7 +227,7 @@ do_tbf_test()
 	local nr=$(rate $t2 $t3 10)
 	local nr_pct=$((100 * (nr - er) / er))
 	((-5 <= nr_pct && nr_pct <= 5))
-	check_err $? "Expected rate $(humanize $er), got $(humanize $nr), which is $nr_pct% off. Required accuracy is +-5%."
+	xfail_on_slow check_err $? "Expected rate $(humanize $er), got $(humanize $nr), which is $nr_pct% off. Required accuracy is +-5%."
 
 	log_test "TC $((vlan - 10)): TBF rate ${mbit}Mbit"
 }

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #19 lore raw

RFC PATCH 13/14 selftests: forwarding: router_mpath_nh_lib: Don't skip, xfail on veth

In reply to Petr Machata

When the NH group stats tests are currently run on a veth topology, the HW-stats leg of each test is SKIP'ped. But kernel networking CI interprets skips as a sign that tooling is missing, and prompts maintainer investigation. Lack of capability to pass a test should be expressed as XFAIL.

Selftests that require HW should normally be put in drivers/net/hw, but doing so for the NH counter selftests would just lead to a lot of duplicity.

So instead, introduce a helper, xfail_on_veth(), which can be used to mark selftests that should XFAIL instead of FAILing when run on a veth topology. On non-veth topology, they don't do anything.

Use the helper in the HW-stats part of router_mpath_nh_lib selftest.

Signed-off-by Petr Machata <[email protected]>

---
 tools/testing/selftests/net/forwarding/lib.sh      | 14 ++++++++++++++
 .../net/forwarding/router_mpath_nh_lib.sh          | 12 +-----------
 2 files changed, 15 insertions(+), 11 deletions(-)

tools/testing/selftests/net/forwarding/lib.sh +14 -0

index cc32bf11739b..6291723580d7 100644
@@ -459,6 +459,20 @@ xfail_on_slow()
 	fi
 }
 
+xfail_on_veth()
+{
+	local dev=$1; shift
+	local kind
+
+	kind=$(ip -j -d link show dev $dev |
+			jq -r '.[].linkinfo.info_kind')
+	if [[ $kind = veth ]]; then
+		FAIL_TO_XFAIL=yes "$@"
+	else
+		"$@"
+	fi
+}
+
 log_test_result()
 {
 	local test_name=$1; shift

tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh +1 ~~-11~~

index b2d2c6cecc01..2903294d8bca 100644
@@ -56,21 +56,12 @@ nh_stats_test_dispatch_swhw()
 	local group_id=$1; shift
 	local mz="$@"
 
-	local used
-
 	nh_stats_do_test "$what" "$nh1_id" "$nh2_id" "$group_id" \
 			 nh_stats_get "${mz[@]}"
 
-	used=$(ip -s -j -d nexthop show id $group_id |
-		   jq '.[].hw_stats.used')
-	kind=$(ip -j -d link show dev $rp11 |
-		   jq -r '.[].linkinfo.info_kind')
-	if [[ $used == true ]]; then
+	xfail_on_veth $rp11 \
 		nh_stats_do_test "HW $what" "$nh1_id" "$nh2_id" "$group_id" \
 				 nh_stats_get_hw "${mz[@]}"
-	elif [[ $kind == veth ]]; then
-		log_test_xfail "HW stats not offloaded on veth topology"
-	fi
 }
 
 nh_stats_test_dispatch()

@@ -83,7 +74,6 @@ nh_stats_test_dispatch()
 	local mz="$@"
 
 	local enabled
-	local kind
 
 	if ! ip nexthop help 2>&1 | grep -q hw_stats; then
 		log_test_skip "NH stats test: ip doesn't support HW stats"

Signature · 1 line

2.43.0

Petr Machata

author maintainer

2024-03-25 17:33 UTC #20 lore raw

RFC PATCH 14/14 selftests: forwarding: Add a test for testing lib.sh functionality

In reply to Petr Machata

Rerunning various scenarios to make sure lib.sh changes do not impact the observable behavior is no fun. Add a selftest at least for the bare basics -- the mechanics of setting RET, retmsg, and EXIT_STATUS.

Since the selftest itself uses lib.sh, it would be possible to break lib.sh in such a way that invalidates result of the selftest. Since the metatest only uses the bare basics (just pass/fail), hopefully such fundamental breakages would be noticed.

Signed-off-by Petr Machata <[email protected]>

---
 .../testing/selftests/net/forwarding/Makefile |   1 +
 .../selftests/net/forwarding/lib_sh_test.sh   | 208 ++++++++++++++++++
 2 files changed, 209 insertions(+)
 create mode 100755 tools/testing/selftests/net/forwarding/lib_sh_test.sh

tools/testing/selftests/net/forwarding/Makefile +1 -0

index 56e3557ba8a6..fa7b59ff4029 100644
@@ -37,6 +37,7 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \
 	ipip_hier_gre_key.sh \
 	ipip_hier_gre_keys.sh \
 	ipip_hier_gre.sh \
+	lib_sh_test.sh \
 	local_termination.sh \
 	mirror_gre_bound.sh \
 	mirror_gre_bridge_1d.sh \

tools/testing/selftests/net/forwarding/lib_sh_test.sh new file +208 -0

208 lines not on this page · raw message

Signature · 1 line

2.43.0

Jakub Kicinski

maintainer

2024-03-26 00:48 UTC #21 lore raw

In reply to Petr Machata

Quoted 3 lines · On Mon, 25 Mar 2024 18:29:07 +0100 Petr Machata wrote:

As discussed on the bi-weekly call on Jan 30, and in mailing around kernel CI effort, some changes are desirable in the suite of forwarding selftests the better to work with the CI tooling.

Excellent.

Did you skip CCing netdev@ on purpose or by accident? Feel free to send it to the list without the RFC tag (pw-bot will auto-discard it if it sees RFC).

Petr Machata

author maintainer

2024-03-26 11:20 UTC #22 lore raw

Quoted 10 lines · Jakub Kicinski

Quoted 3 lines · On Mon, 25 Mar 2024 18:29:07 +0100 Petr Machata wrote:
As discussed on the bi-weekly call on Jan 30, and in mailing around kernel CI effort, some changes are desirable in the suite of forwarding selftests the better to work with the CI tooling.
Excellent.
Did you skip CCing netdev@ on purpose or by accident? Feel free to send it to the list without the RFC tag (pw-bot will auto-discard it if it sees RFC).

D'oh, I forgot to toggle my dev tree from internal to external, so it went to nbu-linux-internal instead. I'll resend.