Re: [PATCH net v3] selftests: net: local_termination: annotate the expected failures
Hangbin Liu writes:
> On Thu, May 16, 2024 at 08:25:13AM -0700, Jakub Kicinski wrote:
>> Vladimir said when adding this test:
>>
>> The bridge driver fares particularly badly [...] mainly because
>> it does not implement IFF_UNICAST_FLT.
>>
>> See commit 90b9566aa5cd ("selftests: forwarding: add a test for
>> local_termination.sh").
>>
>> We don't want to hide the known gaps, but having a test which
>> always fails prevents us from catching regressions. Report
>> the cases we know may fail as XFAIL.
>>
>> Signed-off-by: Jakub Kicinski
I'm still confused that the failure was shown for $rcv_if_name==bridge,
yet we are testing $h1==veth. But mechanically the code is correct.
Reviewed-by: Petr Machata
Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures
Jakub Kicinski writes:
> On Wed, 15 May 2024 11:02:28 +0200 Petr Machata wrote:
>> >> And then either replace the existing xfail_on_veth's (there are just a
>> >> handful) or convert xfail_on_veth to a wrapper around xfail_on_kind.
>> >
>> > I think the bridge thing we can workaround by just checking
>> > if ${NETIFS[p1]} is veth, rather than $rcv_if_name.
>> > Since the two behave the same.
>>
>> I don't follow. The test has two legs, one creates a VRF and attaches
>> p2, the other creates a bridge and attaches p2. Whether p1 and p2 are
>> veth or HW seems orthogonal to whether $rcv_if_name is a bridge or a
>> veth.
>
> Right, my superficial understanding was that the main distinction is
> whether p2/h2 can do the filtering (or possibly some offload happens).
> So if p1,p2 are veths we know to XFAIL, doesn't matter if we're in
> the vrf or bridge configuration, cause these construct will not filter
> either.
>
> If I'm not making sense - I'm probably confused, I can code up what you
> suggested, it will work, just more LoC :)
I'm not sure myself, but from the commit message it looks like the issue
is with $rcv_if_name being the bridge.
But the patch that you inline is R-b'd and T-b'd by Vladimir, so I'm
going to assume it's doing the right thing.
> +# Clear internal failure tracking for the next test case
> +begin_test()
> +{
> +RET=0
> +FAIL_TO_XFAIL=
> +}
> +
> check_err()
> {
> local err=$1
> diff --git a/tools/testing/selftests/net/forwarding/local_termination.sh
> b/tools/testing/selftests/net/forwarding/local_termination.sh
> index c5b0cbc85b3e..a241acc02498 100755
> --- a/tools/testing/selftests/net/forwarding/local_termination.sh
> +++ b/tools/testing/selftests/net/forwarding/local_termination.sh
> @@ -73,9 +73,12 @@ check_rcv()
> local pattern=$3
> local should_receive=$4
> local should_fail=
> + local xfail_sw=$5
>
> [ $should_receive = true ] && should_fail=0 || should_fail=1
> - RET=0
> + begin_test
> + # check if main interface is veth
> + [ "$xfail_sw" == true ] && xfail_on_veth $h1
If xfail_on_veth $h1 is all that's needed, then I really don't see a
reason why not just do this:
check_rcv $rcv_if_name "Unicast IPv4 to primary MAC address" \
"$smac > $rcv_dmac, ethertype IPv4 (0x0800)" \
true
check_rcv $rcv_if_name "Unicast IPv4 to macvlan MAC address" \
"$smac > $MACVLAN_ADDR, ethertype IPv4 (0x0800)" \
true
xfail_on_veth $h1 \
check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
false
This should work now, in much the same way as this patch, but the intent
is IMHO clearer (vs. passing a mystery true), and FAIL_TO_XFAIL is
cleanly scoped and doesn't run the risk of leaking out of the test.
> tcpdump_show $if_name | grep -q "$pattern"
>
> @@ -157,7 +160,7 @@ run_test()
>
> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
> "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
> - false
> + false true
>
> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, promisc" \
> "$smac > $UNKNOWN_UC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -165,7 +168,7 @@ run_test()
>
> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, allmulti" \
> "$smac > $UNKNOWN_UC_ADDR3, ethertype IPv4 (0x0800)" \
> - false
> + false true
>
> check_rcv $rcv_if_name "Multicast IPv4 to joined group" \
> "$smac > $JOINED_MACV4_MC_ADDR, ethertype IPv4 (0x0800)" \
> @@ -173,7 +176,7 @@ run_test()
>
> check_rcv $rcv_if_name "Multicast IPv4 to unknown group" \
> "$smac > $UNKNOWN_MACV4_MC_ADDR1, ethertype IPv4 (0x0800)" \
> - false
> + false true
>
> check_rcv $rcv_if_name "Multicast IPv4 to unknown group, promisc" \
> "$smac > $UNKNOWN_MACV4_MC_ADDR2, ethertype IPv4 (0x0800)" \
> @@ -189,7 +192,7 @@ run_test()
>
> check_rcv $rcv_if_name "Multicast IPv6 to unknown group" \
> "$smac > $UNKNOWN_MACV6_MC_ADDR1, ethertype IPv6 (0x86dd)" \
> - false
> + false true
>
> check_rcv $rcv_if_name "Multicast IPv6 to unknown group, promisc" \
> "$smac > $UNKNOWN_MACV6_MC_ADDR2, ethertype IPv6 (0x86dd)" \
Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures
Jakub Kicinski writes:
> On Mon, 13 May 2024 15:25:38 +0200 Petr Machata wrote:
>> For veth specifically there is xfail_on_veth:
>>
>> xfail_on_veth $rcv_if_name \
>> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>>"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>>false
>>
>> Which is IMHO clearer than passing an extra boolean.
>
> The extra boolean is because we want to only fail particular subcases.
> I think that's legit. If the other cases regress we want to know.
> Otherwise running the test on SW bridge is only useful for catching
> crashes (so less useful).
Likewise you only annotate with xfail_on_* the testcases that you want
to xfail. The FAIL_TO_XFAIL ought to only be set for that one subcase
and then revert to its former value. (That's the intention anyway.)
> So we probably need to reset FAIL_TO_XFAIL in this case.
> Any preference on how to go about that? I'm tempted to:
>
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh
> b/tools/testing/selftests/net/forwarding/lib.sh index
> 112c85c35092..79aa3c8bc288 100644 ---
> a/tools/testing/selftests/net/forwarding/lib.sh +++
> b/tools/testing/selftests/net/forwarding/lib.sh @@ -473,6 +473,13 @@
> ret_set_ksft_status() # Whether FAILs should be interpreted as XFAILs.
> Internal. FAIL_TO_XFAIL=
>
> +# Clear internal failure tracking for the next test case
> +begin_test()
> +{
> +RET=0
> +FAIL_TO_XFAIL=
> +}
> +
> check_err()
> {
> local err=$1
> diff --git
> a/tools/testing/selftests/net/forwarding/local_termination.sh
> b/tools/testing/selftests/net/forwarding/local_termination.sh index
> 65694cdf2dbb..0781ceba1348 100755 ---
> a/tools/testing/selftests/net/forwarding/local_termination.sh +++
> b/tools/testing/selftests/net/forwarding/local_termination.sh @@ -76,7
> +76,7 @@ check_rcv() local xfail_sw=$5
> [ $should_receive = true ] && should_fail=0 || should_fail=1
> - RET=0
> + begin_test
>
> tcpdump_show $if_name | grep -q "$pattern"
>
>
>> Not sure what to do about the bridge bit though. In principle the
>> various xfail_on_'s can be chained, so e.g. this should work:
>>
>> xfail_on_bridge $rcv_if_name \
>> xfail_on_veth $rcv_if_name \
>> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>>"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>>false
>>
>> I find this preferable to adding these ad-hoc tweaks to each test
>> individually. Maybe it would make sense to have:
>>
>> xfail_on_kind $rcv_if_name veth bridge \
>> check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \
>>"$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \
>>false
>>
>> And then either replace the existing xfail_on_veth's (there are just a
>> handful) or convert xfail_on_veth to a wrapper around xfail_on_kind.
>
> I think the bridge thing we can workaround by just checking
> if ${NETIFS[p1]} is veth, rather than $rcv_if_name.
> Since the two behave the same.
I don't follow. The test has two legs, one creates a VRF and attaches
p2, the other creates a bridge and attaches p2. Whether p1 and p2 are
veth or HW seems orthogonal to whether $rcv_if_name is a bridge or a
veth.
Re: [PATCH net-next] selftests: net: local_termination: annotate the expected failures
Jakub Kicinski writes: > @@ -157,7 +168,7 @@ run_test() > > check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \ > "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \ > - false > + false true > > check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address, promisc" \ > "$smac > $UNKNOWN_UC_ADDR2, ethertype IPv4 (0x0800)" \ For veth specifically there is xfail_on_veth: xfail_on_veth $rcv_if_name \ check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \ "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \ false Which is IMHO clearer than passing an extra boolean. Not sure what to do about the bridge bit though. In principle the various xfail_on_'s can be chained, so e.g. this should work: xfail_on_bridge $rcv_if_name \ xfail_on_veth $rcv_if_name \ check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \ "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \ false I find this preferable to adding these ad-hoc tweaks to each test individually. Maybe it would make sense to have: xfail_on_kind $rcv_if_name veth bridge \ check_rcv $rcv_if_name "Unicast IPv4 to unknown MAC address" \ "$smac > $UNKNOWN_UC_ADDR1, ethertype IPv4 (0x0800)" \ false And then either replace the existing xfail_on_veth's (there are just a handful) or convert xfail_on_veth to a wrapper around xfail_on_kind.
Re: [PATCH net-next 1/5] selftests: drv-net: define endpoint structures
Willem de Bruijn writes:
> 1. Cleaning up remote state in all conditions, including timeout/kill.
>
>Some tests require a setup phase before the test, and a matching
>cleanup phase. If any of the configured state is variable (even
>just a randomized filepath) this needs to be communicated to the
>cleanup phase. The remote filepath is handled well here. But if
>a test needs per-test setup? Say, change MTU or an Ethtool feature.
>Multiple related tests may want to share a setup/cleanup.
Personally I like to wrap responsibilities of this sort in context
managers, e.g. something along these lines:
class changed_mtu:
def __init__(self, dev, mtu):
self.dev = dev
self.mtu = mtu
def __enter__(self):
js = cmd(f"ip -j link show dev {self.dev}", json=True)
self.orig_mtu = something_something(js)
cmd(f"ip link set dev {self.dev} mtu {self.mtu}")
def __exit__(self, type, value, traceback):
cmd(f"ip link set dev {self.dev} mtu {self.orig_mtu}")
with changed_mtu(swp1, 1):
# MTU is 10K here
# and back to 1500
A lot of this can be made generic, where some object is given a setup /
cleanup commands and just invokes those. But things like MTU, ethtool
speed, sysctls and what have you that need to save a previous state and
revert back to it will probably need a custom handler. Like we have them
in lib.sh as well.
[PATCH net-next 10/10] selftests: forwarding: router_nh: Add a diagram
This test lacks a topology diagram, making the setup not obvious. Add one. Signed-off-by: Petr Machata --- .../testing/selftests/net/forwarding/router_nh.sh | 14 ++ 1 file changed, 14 insertions(+) diff --git a/tools/testing/selftests/net/forwarding/router_nh.sh b/tools/testing/selftests/net/forwarding/router_nh.sh index f3a53738bdcc..92904b01eae9 100755 --- a/tools/testing/selftests/net/forwarding/router_nh.sh +++ b/tools/testing/selftests/net/forwarding/router_nh.sh @@ -1,6 +1,20 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +# +-+ +-+ +# | H1 | | H2 | +# | $h1 + | | $h2 + | +# | 192.0.2.2/24 | | | 198.51.100.2/24 | | +# | 2001:db8:1::2/64 | | | 2001:db8:2::2/64 | | +# +---|-+ +---|-+ +# || +# +---||-+ +# | R1|| | +# | $rp1 + $rp2 + | +# | 192.0.2.1/24 198.51.100.1/24 | +# | 2001:db8:1::1/64 2001:db8:2::1/64 | +# +--+ + ALL_TESTS=" ping_ipv4 ping_ipv6 -- 2.43.0
[PATCH net-next 09/10] selftests: forwarding: router_mpath_nh_res: Add a diagram
This test lacks a topology diagram, making the setup not obvious. Add one. Signed-off-by: Petr Machata --- .../net/forwarding/router_mpath_nh_res.sh | 35 +++ 1 file changed, 35 insertions(+) diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh_res.sh b/tools/testing/selftests/net/forwarding/router_mpath_nh_res.sh index 4b483d24ad00..cd9e346436fc 100755 --- a/tools/testing/selftests/net/forwarding/router_mpath_nh_res.sh +++ b/tools/testing/selftests/net/forwarding/router_mpath_nh_res.sh @@ -1,6 +1,41 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +# +-+ +# | H1 | +# | $h1 + | +# | 192.0.2.2/24 | | +# | 2001:db8:1::2/64 | | +# +---|-+ +# | +# +---|--+ +# | | R1 | +# | $rp11 + | +# | 192.0.2.1/24| +# | 2001:db8:1::1/64| +# | | +# | + $rp12 + $rp13| +# | | 169.254.2.12/24| 169.254.3.13/24 | +# | | fe80:2::12/64 | fe80:3::13/64| +# +--||--+ +#|| +# +--||--+ +# | + $rp22 + $rp23| +# |169.254.2.22/24 169.254.3.23/24 | +# |fe80:2::22/64fe80:3::23/64| +# | | +# | $rp21 + | +# | 198.51.100.1/24 | | +# | 2001:db8:2::1/64 | R2 | +# +---|--+ +# | +# +---|-+ +# | | | +# | $h2 + | +# | 198.51.100.2/24 | +# | 2001:db8:2::2/64H2 | +# +-+ + ALL_TESTS=" ping_ipv4 ping_ipv6 -- 2.43.0
[PATCH net-next 08/10] selftests: forwarding: router_mpath_nh: Add a diagram
This test lacks a topology diagram, making the setup not obvious. Add one. Cc: David Ahern Signed-off-by: Petr Machata --- .../net/forwarding/router_mpath_nh.sh | 35 +++ 1 file changed, 35 insertions(+) diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh.sh b/tools/testing/selftests/net/forwarding/router_mpath_nh.sh index 3f0f5dc95542..2ba44247c60a 100755 --- a/tools/testing/selftests/net/forwarding/router_mpath_nh.sh +++ b/tools/testing/selftests/net/forwarding/router_mpath_nh.sh @@ -1,6 +1,41 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +# +-+ +# | H1 | +# | $h1 + | +# | 192.0.2.2/24 | | +# | 2001:db8:1::2/64 | | +# +---|-+ +# | +# +---|--+ +# | | R1 | +# | $rp11 + | +# | 192.0.2.1/24| +# | 2001:db8:1::1/64| +# | | +# | + $rp12 + $rp13| +# | | 169.254.2.12/24| 169.254.3.13/24 | +# | | fe80:2::12/64 | fe80:3::13/64| +# +--||--+ +#|| +# +--||--+ +# | + $rp22 + $rp23| +# |169.254.2.22/24 169.254.3.23/24 | +# |fe80:2::22/64fe80:3::23/64| +# | | +# | $rp21 + | +# | 198.51.100.1/24 | | +# | 2001:db8:2::1/64 | R2 | +# +---|--+ +# | +# +---|-+ +# | | | +# | $h2 + | +# | 198.51.100.2/24 | +# | 2001:db8:2::2/64H2 | +# +-+ + ALL_TESTS=" ping_ipv4 ping_ipv6 -- 2.43.0
[PATCH net-next 07/10] selftests: mlxsw: ethtool_lanes: Wait for lanes parameter dump explicitly
From: Danielle Ratson The ethtool dump includes the lanes parameter only when the port is up. Therefore, the ethtool_lanes.sh test waits for ports to come before testing the lanes parameter. In some cases, the test considers the port as up, but the lanes parameter is not yet dumped although assumed to be, resulting in ethtool_lanes.sh test failure. To avoid that, ensure that the lanes parameter is indeed dumped by waiting for it explicitly, before preforming the test cases. Signed-off-by: Danielle Ratson Reviewed-by: Petr Machata Signed-off-by: Petr Machata --- .../selftests/drivers/net/mlxsw/ethtool_lanes.sh | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/tools/testing/selftests/drivers/net/mlxsw/ethtool_lanes.sh b/tools/testing/selftests/drivers/net/mlxsw/ethtool_lanes.sh index 91891b9418d7..877cd6df94a1 100755 --- a/tools/testing/selftests/drivers/net/mlxsw/ethtool_lanes.sh +++ b/tools/testing/selftests/drivers/net/mlxsw/ethtool_lanes.sh @@ -24,8 +24,8 @@ setup_prepare() busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 check_err $? "ports did not come up" - local lanes_exist=$(ethtool $swp1 | grep 'Lanes:') - if [[ -z $lanes_exist ]]; then + busywait $TIMEOUT sh -c "ethtool $swp1 | grep -q Lanes:" + if [[ $? -ne 0 ]]; then log_test "SKIP: driver does not support lanes setting" exit 1 fi @@ -122,8 +122,9 @@ autoneg() ethtool_set $swp1 speed $max_speed lanes $lanes ip link set dev $swp1 up ip link set dev $swp2 up - busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 - check_err $? "ports did not come up" + + busywait $TIMEOUT sh -c "ethtool $swp1 | grep -q Lanes:" + check_err $? "Lanes parameter is not presented on time" check_lanes $swp1 $lanes $max_speed log_test "$lanes lanes is autonegotiated" @@ -160,8 +161,9 @@ autoneg_force_mode() ethtool_set $swp2 speed $max_speed lanes $lanes autoneg off ip link set dev $swp1 up ip link set dev $swp2 up - busywait "$TIMEOUT" wait_for_port_up ethtool $swp2 - check_err $? "ports did not come up" + + busywait $TIMEOUT sh -c "ethtool $swp1 | grep -q Lanes:" + check_err $? "Lanes parameter is not presented on time" check_lanes $swp1 $lanes $max_speed log_test "Autoneg off, $lanes lanes detected during force mode" -- 2.43.0
[PATCH net-next 06/10] selftests: drivers: hw: Include tc_common.sh in hw_stats_l3
The tests use the constant TC_HIT_TIMEOUT when waiting on the counter
values. However it does not include tc_common.sh where the counter is
specified. The test has been robust in our testing, which means the counter
is bumped quickly enough that the updated value is available already on the
first iteration. Nevertheless it's not correct. Include tc_common.sh as
appropriate.
Signed-off-by: Petr Machata
---
tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh | 1 +
tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh | 1 +
2 files changed, 2 insertions(+)
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
index 7dfc50366c99..67fafefc80be 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
@@ -50,6 +50,7 @@ ALL_TESTS="
NUM_NETIFS=4
lib_dir=$(dirname "$0")
source "$lib_dir"/../../../net/forwarding/lib.sh
+source "$lib_dir"/../../../net/forwarding/tc_common.sh
h1_create()
{
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
index ab8d04855af5..a94d92e1abce 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
@@ -15,6 +15,7 @@ NUM_NETIFS=6
lib_dir=$(dirname "$0")
source "$lib_dir"/../../../net/forwarding/lib.sh
source "$lib_dir"/../../../net/forwarding/ipip_lib.sh
+source "$lib_dir"/../../../net/forwarding/tc_common.sh
setup_prepare()
{
--
2.43.0
[PATCH net-next 05/10] selftests: drivers: hw: ethtool.sh: Adjust output
Some log_test calls are done in a loop, and lead to the same log output. This might prove tricky to deduplicate for automated tools. Instead, roll the unique information from log_info to log_test, and drop the log_info. This also leads to more compact and clearer output. This change prompts rewording the messages so that they are not excessively long. Some check_err messages do not indicate what the issue actually is, so reword them to say it's a "ping with", like is the case in some other instances in this test. Signed-off-by: Petr Machata --- tools/testing/selftests/drivers/net/hw/ethtool.sh | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/tools/testing/selftests/drivers/net/hw/ethtool.sh b/tools/testing/selftests/drivers/net/hw/ethtool.sh index bb12d5d70949..fa6953de6b6d 100755 --- a/tools/testing/selftests/drivers/net/hw/ethtool.sh +++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh @@ -65,9 +65,8 @@ same_speeds_autoneg_off() setup_wait_dev_with_timeout $h1 setup_wait_dev_with_timeout $h2 ping_do $h1 192.0.2.2 - check_err $? "speed $speed autoneg off" - log_test "force of same speed autoneg off" - log_info "speed = $speed" + check_err $? "ping with speed $speed autoneg off" + log_test "force speed $speed on both ends" done ethtool -s $h2 autoneg on @@ -112,9 +111,8 @@ combination_of_neg_on_and_off() setup_wait_dev_with_timeout $h1 setup_wait_dev_with_timeout $h2 ping_do $h1 192.0.2.2 - check_err $? "h1-speed=$speed autoneg off, h2 autoneg on" - log_test "one side with autoneg off and another with autoneg on" - log_info "force speed = $speed" + check_err $? "ping with h1-speed=$speed autoneg off, h2 autoneg on" + log_test "force speed $speed vs. autoneg" done ethtool -s $h1 autoneg on @@ -207,10 +205,9 @@ advertise_subset_of_speeds() setup_wait_dev_with_timeout $h1 setup_wait_dev_with_timeout $h2 ping_do $h1 192.0.2.2 - check_err $? "h1=$speed_1_to_advertise, h2=$speed_2_to_advertise ($speed_value)" + check_err $? "ping with h1=$speed_1_to_advertise, h2=$speed_2_to_advertise ($speed_value)" - log_test "advertise subset of speeds" - log_info "h1=$speed_1_to_advertise, h2=$speed_2_to_advertise" + log_test "advertise $speed_1_to_advertise vs. $speed_2_to_advertise" done ethtool -s $h2 autoneg on -- 2.43.0
[PATCH net-next 04/10] selftests: drivers: hw: Fix ethtool_rmon
When rx-pktsNtoM reports a range that involves very low-valued range, such
as 0-64, the calculated length of the packet will be -4, because FCS is
subtracted from the value. mausezahn then confuses the value for an option
and bails out. As a result, the test dumps many mausezahn error messages.
Instead, cap the value at 0. mausezahn will use an appropriate minimum
packet length.
Cc: Vladimir Oltean
Cc: Tobias Waldekranz
Signed-off-by: Petr Machata
---
tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh | 1 +
1 file changed, 1 insertion(+)
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
index e2a1c10d3503..8f60c1685ad4 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
@@ -44,6 +44,7 @@ bucket_test()
# Mausezahn does not include FCS bytes in its length - but the
# histogram counters do
len=$((len - ETH_FCS_LEN))
+ len=$((len > 0 ? len : 0))
before=$(ethtool --json -S $iface --groups rmon | \
jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
--
2.43.0
[PATCH net-next 03/10] selftests: forwarding: bail_on_lldpad() should SKIP
$ksft_skip is used to mark selftests that have tooling issues. The fact
that LLDPad is running, but shouldn't, is one such issue. Therefore have
bail_on_lldpad() bail with $ksft_skip.
Signed-off-by: Petr Machata
Reviewed-by: Benjamin Poirier
---
tools/testing/selftests/net/forwarding/lib.sh | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 3cbbc2fd4d7d..7913c6ee418d 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -2138,6 +2138,8 @@ bail_on_lldpad()
{
local reason1="$1"; shift
local reason2="$1"; shift
+ local caller=${FUNCNAME[1]}
+ local src=${BASH_SOURCE[1]}
if systemctl is-active --quiet lldpad; then
@@ -2158,7 +2160,8 @@ bail_on_lldpad()
an environment variable ALLOW_LLDPAD to a
non-empty string.
EOF
- exit 1
+ log_test_skip $src:$caller
+ exit $EXIT_STATUS
else
return
fi
--
2.43.0
[PATCH net-next 02/10] selftests: forwarding: lib.sh: Validate NETIFS
The variable should contain at least NUM_NETIFS interfaces, stored
as keys named "p$i", for i in `seq $NUM_NETIFS`.
Signed-off-by: Petr Machata
Reviewed-by: Benjamin Poirier
---
tools/testing/selftests/net/forwarding/lib.sh | 22 ++-
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 658e4e7bf4b9..3cbbc2fd4d7d 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -273,11 +273,6 @@ if [[ "$REQUIRE_MTOOLS" = "yes" ]]; then
require_command mreceive
fi
-if [[ ! -v NUM_NETIFS ]]; then
- echo "SKIP: importer does not define \"NUM_NETIFS\""
- exit $ksft_skip
-fi
-
##
# Command line options handling
@@ -296,6 +291,23 @@ done
##
# Network interfaces configuration
+if [[ ! -v NUM_NETIFS ]]; then
+ echo "SKIP: importer does not define \"NUM_NETIFS\""
+ exit $ksft_skip
+fi
+
+if (( NUM_NETIFS > ${#NETIFS[@]} )); then
+ echo "SKIP: Importer requires $NUM_NETIFS NETIFS, but only
${#NETIFS[@]} are defined (${NETIFS[@]})"
+ exit $ksft_skip
+fi
+
+for i in $(seq ${#NETIFS[@]}); do
+ if [[ ! ${NETIFS[p$i]} ]]; then
+ echo "SKIP: NETIFS[p$i] not given"
+ exit $ksft_skip
+ fi
+done
+
create_netif_veth()
{
local i
--
2.43.0
[PATCH net-next 01/10] selftests: net: Unify code of busywait() and slowwait()
Bodies of busywait() and slowwait() functions are almost identical. Extract
the common code into a helper, loopy_wait, and convert busywait() and
slowwait() into trivial wrappers.
Moreover, the fact that slowwait() uses seconds for units is really not
intuitive, and the comment does not help much. Instead make the unit part
of the name of the argument to further clarify what units are expected.
Cc: Hangbin Liu
Signed-off-by: Petr Machata
Reviewed-by: Benjamin Poirier
---
tools/testing/selftests/net/forwarding/lib.sh | 22 ++-
tools/testing/selftests/net/lib.sh| 16 +++---
2 files changed, 15 insertions(+), 23 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 4103ed7afcde..658e4e7bf4b9 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -95,27 +95,9 @@ source "$net_forwarding_dir/../lib.sh"
# timeout in seconds
slowwait()
{
- local timeout=$1; shift
+ local timeout_sec=$1; shift
- local start_time="$(date -u +%s)"
- while true
- do
- local out
- out=$("$@")
- local ret=$?
- if ((!ret)); then
- echo -n "$out"
- return 0
- fi
-
- local current_time="$(date -u +%s)"
- if ((current_time - start_time > timeout)); then
- echo -n "$out"
- return 1
- fi
-
- sleep 0.1
- done
+ loopy_wait "sleep 0.1" "$((timeout_sec * 1000))" "$@"
}
##
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index b7f7b8695165..c868c0aec121 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -58,9 +58,10 @@ ksft_exit_status_merge()
$ksft_xfail $ksft_pass $ksft_skip $ksft_fail
}
-busywait()
+loopy_wait()
{
- local timeout=$1; shift
+ local sleep_cmd=$1; shift
+ local timeout_ms=$1; shift
local start_time="$(date -u +%s%3N)"
while true
@@ -74,13 +75,22 @@ busywait()
fi
local current_time="$(date -u +%s%3N)"
- if ((current_time - start_time > timeout)); then
+ if ((current_time - start_time > timeout_ms)); then
echo -n "$out"
return 1
fi
+
+ $sleep_cmd
done
}
+busywait()
+{
+ local timeout_ms=$1; shift
+
+ loopy_wait : "$timeout_ms" "$@"
+}
+
cleanup_ns()
{
local ns=""
--
2.43.0
[PATCH net-next 00/10] selftests: Assortment of fixes
This is a loose follow-up to the Kernel CI patchset posted recently. It contains various fixes that were supposed to be part of said patchset, but didn't fit due to its size. The latter 4 patches were written independently of the CI effort, but again didn't fit in their intended patchsets. - Patch #1 unifies code of two very similar looking functions, busywait() and slowwait(). - Patch #2 adds sanity checks around the setting of NETIFS, which carries list of interfaces to run on. - Patch #3 changes bail_on_lldpad() to SKIP instead of FAILing. - Patches #4 to #7 fix issues in selftests. - Patches #8 to #10 add topology diagrams to several selftests. This should have been part of the mlxsw leg of NH group stats patches, but again, it did not fit in due to size. Danielle Ratson (1): selftests: mlxsw: ethtool_lanes: Wait for lanes parameter dump explicitly Petr Machata (9): selftests: net: Unify code of busywait() and slowwait() selftests: forwarding: lib.sh: Validate NETIFS selftests: forwarding: bail_on_lldpad() should SKIP selftests: drivers: hw: Fix ethtool_rmon selftests: drivers: hw: ethtool.sh: Adjust output selftests: drivers: hw: Include tc_common.sh in hw_stats_l3 selftests: forwarding: router_mpath_nh: Add a diagram selftests: forwarding: router_mpath_nh_res: Add a diagram selftests: forwarding: router_nh: Add a diagram .../selftests/drivers/net/hw/ethtool.sh | 15 +++--- .../selftests/drivers/net/hw/ethtool_rmon.sh | 1 + .../selftests/drivers/net/hw/hw_stats_l3.sh | 1 + .../drivers/net/hw/hw_stats_l3_gre.sh | 1 + .../drivers/net/mlxsw/ethtool_lanes.sh| 14 +++--- tools/testing/selftests/net/forwarding/lib.sh | 49 +-- .../net/forwarding/router_mpath_nh.sh | 35 + .../net/forwarding/router_mpath_nh_res.sh | 35 + .../selftests/net/forwarding/router_nh.sh | 14 ++ tools/testing/selftests/net/lib.sh| 16 -- 10 files changed, 137 insertions(+), 44 deletions(-) -- 2.43.0
Re: [PATCH net-next v2 6/6] selftests: net: exercise page pool reporting via netlink
Jakub Kicinski writes: > Add a Python test for the basic ops. > > # ./net/nl_netdev.py > KTAP version 1 > 1..3 > ok 1 nl_netdev.empty_check > ok 2 nl_netdev.lo_check > ok 3 nl_netdev.page_pool_check > # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0 > > Signed-off-by: Jakub Kicinski LGTM Reviewed-by: Petr Machata
Re: [PATCH net-next 6/6] selftests: net: exercise page pool reporting via netlink
Jakub Kicinski writes:
> Add a Python test for the basic ops.
>
> # ./net/nl_netdev.py
> KTAP version 1
> 1..3
> ok 1 nl_netdev.empty_check
> ok 2 nl_netdev.lo_check
> ok 3 nl_netdev.page_pool_check
> # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Jakub Kicinski
> ---
> tools/testing/selftests/net/lib/py/nsim.py | 7 ++
> tools/testing/selftests/net/nl_netdev.py | 79 +-
> 2 files changed, 84 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/lib/py/nsim.py
> b/tools/testing/selftests/net/lib/py/nsim.py
> index 94aa32f59fdb..1fd50a308408 100644
> --- a/tools/testing/selftests/net/lib/py/nsim.py
> +++ b/tools/testing/selftests/net/lib/py/nsim.py
> @@ -28,6 +28,13 @@ from .utils import cmd, ip
> self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
> ret = ip("-j link show dev %s" % ifname, ns=ns)
> self.dev = json.loads(ret.stdout)[0]
> +self.ifindex = self.dev["ifindex"]
> +
> +def up(self):
> +ip("link set dev {} up".format(self.ifname))
> +
> +def down(self):
> +ip("link set dev {} down".format(self.ifname))
Yeah, what I meant by integration pain with LNST the other day was
basically this. You end up rewriting the iproute2 API in Python.
But it is what it is.
> def dfs_write(self, path, val):
> self.nsimdev.dfs_write(f'ports/{self.port_index}/' + path, val)
> diff --git a/tools/testing/selftests/net/nl_netdev.py
> b/tools/testing/selftests/net/nl_netdev.py
> index 2b8b488fb419..afc510c044ce 100755
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py
> @@ -1,7 +1,8 @@
> #!/usr/bin/env python3
> # SPDX-License-Identifier: GPL-2.0
>
> -from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily
> +import time
> +from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily,
> NetdevSimDev, ip
>
>
> def empty_check(nf) -> None:
> @@ -15,9 +16,83 @@ from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge,
> NetdevFamily
> ksft_eq(len(lo_info['xdp-rx-metadata-features']), 0)
>
>
> +def page_pool_check(nf) -> None:
> +with NetdevSimDev() as nsimdev:
> +nsim = nsimdev.nsims[0]
> +
> +# No page pools when down
> +nsim.down()
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
This combo of page_pool_get / filter looks like it should be in a helper.
> +ksft_eq(len(pp_list), 0)
> +
> +# Up, empty page pool appears
> +nsim.up()
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +ksft_ge(len(pp_list), 0)
> +refs = sum([pp["inflight"] for pp in pp_list])
> +ksft_eq(refs, 0)
> +
> +# Down, it disappears, again
> +nsim.down()
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +ksft_eq(len(pp_list), 0)
> +
> +# Up, allocate a page
> +nsim.up()
> +nsim.dfs_write("pp_hold", "y")
> +pp_list = nf.page_pool_get({}, dump=True)
> +refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") ==
> nsim.ifindex])
> +ksft_ge(refs, 1)
> +
> +# Now let's leak a page
> +nsim.down()
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +ksft_eq(len(pp_list), 1)
> +refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") ==
> nsim.ifindex])
The second filtering is unnecessary.
> +ksft_eq(refs, 1)
> +undetached = [pp for pp in pp_list if "detach-time" not in pp]
Maybe call this attached to be in sync with the next hunk?
> +ksft_eq(len(undetached), 0)
> +
> +# New pp can get created, and we'll have two
> +nsim.up()
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +attached = [pp for pp in pp_list if "detach-time" not in pp]
> +undetached = [pp for pp in pp_list if "detach-time" in pp]
detached.
> +ksft_eq(len(attached), 1)
> +ksft_eq(len(undetached), 1)
> +
> +# Free the old page and the old pp is gone
> +nsim.dfs_write("pp_hold", "n")
> +# Freeing check is once a second so we may need to retry
> +for i in range(50):
> +pp_list = nf.page_pool_get({}, dump=True)
> +pp_list = [pp for pp in pp_list if pp.get("ifindex") ==
> nsim.ifindex]
> +if len(pp_list) == 1:
> +break
> +time.sleep(0.05)
> +ksft_eq(len(pp_list), 1)
Yeah, I don't know if busywait / slowwait sort of a helper is practical
to write
Re: [PATCH net-next 5/6] selftests: net: support use of NetdevSimDev under "with" in python
Jakub Kicinski writes: > Using "with" on an entire driver test env is supported already, > but it's also useful to use "with" on an individual nsim. > > Signed-off-by: Jakub Kicinski Reviewed-by: Petr Machata
Re: [PATCH net-next 4/6] selftests: net: print full exception on failure
Jakub Kicinski writes:
> Instead of a summary line print the full exception.
> This makes debugging Python tests much easier.
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Petr Machata
> @@ -85,7 +86,8 @@ KSFT_RESULT = None
> totals['xfail'] += 1
> continue
> except Exception as e:
> -for line in str(e).split('\n'):
> +tb = traceback.format_exc()
> +for line in tb.strip().split('\n'):
(The strip is necessary to get rid of trailing newlines.)
> ksft_pr("Exception|", line)
> ktap_result(False, cnt, case)
> totals['fail'] += 1
Re: [PATCH net-next 3/6] selftests: net: print report check location in python tests
Jakub Kicinski writes:
> Developing Python tests is a bit annoying because when test fails
> we only print the fail message and no info about which exact check
> led to it. Print the location (the first line of this example is new):
>
> # At /root/ksft-net-drv/./net/nl_netdev.py line 38:
> # Check failed 0 != 10
> not ok 3 nl_netdev.page_pool_check
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Petr Machata
> +def _fail(*args):
> +global KSFT_RESULT
> +KSFT_RESULT = False
> +
> +frame = inspect.stack()[2]
> +ksft_pr("At " + frame.filename + " line " + str(frame.lineno) + ":")
> +ksft_pr(" ".join([str(a) for a in args]))
I think this could have just been ksft_pr(*args) like before, but
whatever.
> +
> def ksft_eq(a, b, comment=""):
> global KSFT_RESULT
> if a != b:
> -KSFT_RESULT = False
> -ksft_pr("Check failed", a, "!=", b, comment)
> +_fail("Check failed", a, "!=", b, comment)
Re: [PATCH net-next v2 6/7] selftests: drivers: add scaffolding for Netlink tests in Python
Jakub Kicinski writes:
> Add drivers/net as a target for mixed-use tests.
> The setup is expected to work similarly to the forwarding tests.
> Since we only need one interface (unlike forwarding tests)
> read the target device name from NETIF. If not present we'll
> try to run the test against netdevsim.
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Petr Machata
However:
> diff --git a/tools/testing/selftests/net/lib/py/nsim.py
> b/tools/testing/selftests/net/lib/py/nsim.py
> new file mode 100644
> index ..25ae0d081788
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/py/nsim.py
> @@ -0,0 +1,118 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import json
> +import os
> +import random
> +import re
> +import time
> +from .utils import cmd, ip
> +
> +
> +class NetdevSim:
> +"""
> +Class for netdevsim netdevice and its attributes.
> +"""
> +
> +def __init__(self, nsimdev, port_index, ifname, ns=None):
> +# In case udev renamed the netdev to according to new schema,
> +# check if the name matches the port_index.
> +nsimnamere = re.compile(r"eni\d+np(\d+)")
> +match = nsimnamere.match(ifname)
> +if match and int(match.groups()[0]) != port_index + 1:
> +raise Exception("netdevice name mismatches the expected one")
> +
> +self.ifname = ifname
> +self.nsimdev = nsimdev
> +self.port_index = port_index
> +self.ns = ns
> +self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
> +ret = ip("-j link show dev %s" % ifname, ns=ns)
> +self.dev = json.loads(ret.stdout)[0]
I don't think self.ifname, .ns, .dfs_dir, .dev are actually used outside
of this function.
> +
> +def dfs_write(self, path, val):
> +self.nsimdev.dfs_write(f'ports/{self.port_index}/' + path, val)
> +
> +
> +class NetdevSimDev:
> +"""
> +Class for netdevsim bus device and its attributes.
> +"""
> +@staticmethod
> +def ctrl_write(path, val):
> +fullpath = os.path.join("/sys/bus/netdevsim/", path)
> +with open(fullpath, "w") as f:
> +f.write(val)
> +
> +def dfs_write(self, path, val):
> +fullpath =
> os.path.join(f"/sys/kernel/debug/netdevsim/netdevsim{self.addr}/", path)
> +with open(fullpath, "w") as f:
> +f.write(val)
> +
> +def __init__(self, port_count=1, ns=None):
> +# nsim will spawn in init_net, we'll set to actual ns once we switch
> it the.sre
the.sre?
Re: [PATCH net-next v2 5/7] netdevsim: report stats by default, like a real device
Jakub Kicinski writes:
> Real devices should implement qstats. Devices which support
> pause or FEC configuration should also report the relevant stats.
>
> nsim was missing FEC stats completely, some of the qstats
> and pause stats required toggling a debugfs knob.
>
> Note that the tests which used pause always initialize the setting
> so they shouldn't be affected by the different starting value.
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Petr Machata
Just:
> @@ -330,6 +331,49 @@ static const struct net_device_ops nsim_vf_netdev_ops = {
> .ndo_set_features = nsim_set_features,
> };
>
> +/* We don't have true par-queue stats, yet, so do some random fakery here. */
per
> +static void nsim_get_queue_stats_rx(struct net_device *dev, int idx,
> + struct netdev_queue_stats_rx *stats)
> +{
> + struct rtnl_link_stats64 rtstats = {};
> +
> + nsim_get_stats64(dev, );
> +
> + stats->packets = rtstats.rx_packets - !!rtstats.rx_packets;
This is just to make sure that per-queue stats are lower than the
overall rtstats I presume?
> + stats->bytes = rtstats.rx_bytes;
> +}
> +
> +static void nsim_get_queue_stats_tx(struct net_device *dev, int idx,
> + struct netdev_queue_stats_tx *stats)
> +{
> + struct rtnl_link_stats64 rtstats = {};
> +
> + nsim_get_stats64(dev, );
> +
> + stats->packets = rtstats.tx_packets - !!rtstats.tx_packets;
> + stats->bytes = rtstats.tx_bytes;
> +}
Re: [PATCH net-next v2 4/7] selftests: nl_netdev: add a trivial Netlink netdev test
Jakub Kicinski writes: > Add a trivial test using YNL. > > $ ./tools/testing/selftests/net/nl_netdev.py > KTAP version 1 > 1..2 > ok 1 nl_netdev.empty_check > ok 2 nl_netdev.lo_check > > Instantiate the family once, it takes longer than the test itself. > > Signed-off-by: Jakub Kicinski Reviewed-by: Petr Machata
Re: [PATCH net-next v2 3/7] selftests: net: add scaffolding for Netlink tests in Python
Jakub Kicinski writes: > Add glue code for accessing the YNL library which lives under > tools/net and YAML spec files from under Documentation/. > Automatically figure out if tests are run in tree or not. > Since we'll want to use this library both from net and > drivers/net test targets make the library a target as well, > and automatically include it when net or drivers/net are > included. Making net/lib a target ensures that we end up > with only one copy of it, and saves us some path guessing. > > Add a tiny bit of formatting support to be able to output KTAP > from the start. > > Signed-off-by: Jakub Kicinski > --- > v2: > - include the net/lib only in install > - support passing state to tests > - don't apply Path() on what's already a Path() > - fix spacing in Makefile's filter() > - sort imports > > CC: sh...@kernel.org > CC: linux-kselftest@vger.kernel.org > --- > tools/testing/selftests/Makefile | 9 +- > tools/testing/selftests/net/lib/Makefile | 8 ++ > .../testing/selftests/net/lib/py/__init__.py | 6 ++ > tools/testing/selftests/net/lib/py/consts.py | 9 ++ > tools/testing/selftests/net/lib/py/ksft.py| 96 +++ > tools/testing/selftests/net/lib/py/utils.py | 47 + > tools/testing/selftests/net/lib/py/ynl.py | 49 ++ > 7 files changed, 223 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/net/lib/Makefile > create mode 100644 tools/testing/selftests/net/lib/py/__init__.py > create mode 100644 tools/testing/selftests/net/lib/py/consts.py > create mode 100644 tools/testing/selftests/net/lib/py/ksft.py > create mode 100644 tools/testing/selftests/net/lib/py/utils.py > create mode 100644 tools/testing/selftests/net/lib/py/ynl.py > > diff --git a/tools/testing/selftests/Makefile > b/tools/testing/selftests/Makefile > index e1504833654d..f533eb7054fe 100644 > --- a/tools/testing/selftests/Makefile > +++ b/tools/testing/selftests/Makefile > @@ -116,6 +116,13 @@ TARGETS += zram > TARGETS_HOTPLUG = cpu-hotplug > TARGETS_HOTPLUG += memory-hotplug > > +# Networking tests want the net/lib target, include it automatically > +ifneq ($(filter net,$(TARGETS)),) > +ifeq ($(filter net/lib,$(TARGETS)),) > + INSTALL_DEP_TARGETS := net/lib > +endif > +endif > + > # User can optionally provide a TARGETS skiplist. By default we skip > # BPF since it has cutting edge build time dependencies which require > # more effort to install. > @@ -245,7 +252,7 @@ ifdef INSTALL_PATH > install -m 744 run_kselftest.sh $(INSTALL_PATH)/ > rm -f $(TEST_LIST) > @ret=1; \ > - for TARGET in $(TARGETS); do \ > + for TARGET in $(TARGETS) $(INSTALL_DEP_TARGETS); do \ > BUILD_TARGET=$$BUILD/$$TARGET; \ > $(MAKE) OUTPUT=$$BUILD_TARGET -C $$TARGET install \ > INSTALL_PATH=$(INSTALL_PATH)/$$TARGET \ > diff --git a/tools/testing/selftests/net/lib/Makefile > b/tools/testing/selftests/net/lib/Makefile > new file mode 100644 > index ..5730682aeffb > --- /dev/null > +++ b/tools/testing/selftests/net/lib/Makefile > @@ -0,0 +1,8 @@ > +# SPDX-License-Identifier: GPL-2.0 > + > +TEST_FILES := ../../../../../Documentation/netlink/s* Not sure I understand the motivation for the wildcard. Currently this matches Documentation/netlink/specs. Do you expect everything that starts with s to be a testfile? > +TEST_FILES += ../../../../net/* Likewise this -- it's just tools/net/ynl? Will everything that's there be a testfile? > +TEST_INCLUDES := $(wildcard py/*.py) > + > +include ../../lib.mk Other than that it looks OK. Reviewed-by: Petr Machata
Re: [PATCH net-next v2 2/7] tools: ynl: copy netlink error to NlError
Jakub Kicinski writes: > Typing e.nl_msg.error when processing exception is a bit tedious > and counter-intuitive. Set a local .error member to the positive > value of the netlink level error. > > Signed-off-by: Jakub Kicinski Reviewed-by: Petr Machata
Re: [PATCH net-next v2 1/7] netlink: specs: define ethtool header flags
Jakub Kicinski writes:
> When interfacing with the ethtool commands it's handy to
> be able to use the names of the flags. Example:
>
> ethnl.pause_get({"header": {"dev-index": cfg.ifindex,
> "flags": {'stats'}}})
>
> Note that not all commands accept all the flags,
> but the meaning of the bits does not change command
> to command.
>
> Signed-off-by: Jakub Kicinski
Reviewed-by: Petr Machata
Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting
Jakub Kicinski writes:
> On Wed, 3 Apr 2024 10:58:19 +0200 Petr Machata wrote:
>> Also, it's not clear what "del thing" should do in that context, because
>> if cfg also keeps a reference, __del__ won't get called. There could be
>> a direct method, like thing.exit() or whatever, but then you need
>> bookkeeping so as not to clean up the second time through cfg. It's the
>> less straightforward way of going about it IMHO.
>
> I see, having read up on what del actually does - "del thing" would
> indeed not work here.
>
>> I know that I must sound like a broken record at this point, but look:
>>
>> with build("ip link set dev %s master %s" % (swp1, h1),
>>"ip link set dev %s nomaster" % swp1) as thing:
>> ... some code which may rise ...
>> ... more code, interface detached, `thing' gone ...
>>
>> It's just as concise, makes it very clear where the device is part of
>> the bridge and where not anymore, and does away with the intricacies of
>> lifetime management.
>
> My experience [1] is that with "with" we often end up writing tests
> like this:
>
> def test():
> with a() as bunch,
>of() as things:
> ... entire body of the test indented ...
>
> [1] https://github.com/kuba-moo/linux/blob/psp/tools/net/ynl/psp.py
Yeah, that does end up happening. I think there are a couple places
where you could have folded several withs in one, but it is going to be
indented, yeah.
But you end up indenting for try: finally: to make the del work reliably
anyway, so it's kinda lose/lose in that regard.
> Nothing wrong with that. I guess the question in my mind is whether
> we're aiming for making the tests "pythonic" (in which case "with"
> definitely wins), or more of a "bash with classes" style trying to
> avoid any constructs people may have to google. I'm on the fence on
> that one, as the del example proves my python expertise is not high.
> OTOH people who prefer bash will continue to write bash tests,
> so maybe we don't have to worry about non-experts too much. Dunno.
What I'm saying is, bash is currently a bit of a mess when it comes to
cleanups. It's hard to get right, annoying to review, and sometimes
individual cases add state that they don't unwind in cleanup() but only
later in the function, so when you C-c half-way through such case, stuff
stays behind.
Python has tools to just magic all this away.
Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting
Jakub Kicinski writes:
> On Wed, 3 Apr 2024 00:04:14 +0200 Petr Machata wrote:
>> > Yes, I was wondering about that. It must be doable, IIRC
>> > the multi-threading API "injects" args from a tuple.
>> > I was thinking something along the lines of:
>> >
>> > with NetDrvEnv(__file__) as cfg:
>> > ksft_run([check_pause, check_fec, pkt_byte_sum],
>> > args=(cfg, ))
>> >
>> > I got lazy, let me take a closer look. Another benefit
>> > will be that once we pass in "env" / cfg - we can "register"
>> > objects in there for auto-cleanup (in the future, current
>> > tests don't need cleanup)
>>
>> Yeah, though some of those should probably just be their own context
>> managers IMHO, not necessarily hooked to cfg. I'm thinking something
>> fairly general, so that the support boilerplate doesn't end up costing
>> an arm and leg:
>>
>> with build("ip route add 192.0.2.1/28 nexthop via 192.0.2.17",
>>"ip route del 192.0.2.1/28"),
>> build("ip link set dev %s master %s" % (swp1, h1),
>>"ip link set dev %s nomaster" % swp1):
>> le_test()
>>
>> Dunno. I guess it makes sense to have some of the common stuff
>> predefined, e.g. "with vrf() as h1". And then the stuff that's typically
>> in lib.sh's setup() and cleanup(), can be losslessly hooked up to cfg.
>
> I was thinking of something along the lines of:
>
> def test_abc(cfg):
> cfg.build("ip route add 192.0.2.1/28 nexthop via 192.0.2.17",
> "ip route del 192.0.2.1/28")
> cfg.build("ip link set dev %s master %s" % (swp1, h1),
> "ip link set dev %s nomaster" % swp1)
>
> optionally we could then also:
>
> thing = cfg.build("ip link set dev %s master %s" % (swp1, h1),
>"ip link set dev %s nomaster" % swp1)
>
> # ... some code which may raise ...
>
> # unlink to do something else with the device
> del thing
> # ... more code ...
>
> cfg may not be best here, could be cleaner to create a "test" object,
> always pass it in as the first param, and destroy it after each test.
I assume above you mean that cfg inherits the thing, but cfg lifetime
currently looks like it spreads across several test cases. ksft_run()
would need to know about it and call something to issue the postponed
cleanups between cases.
Also, it's not clear what "del thing" should do in that context, because
if cfg also keeps a reference, __del__ won't get called. There could be
a direct method, like thing.exit() or whatever, but then you need
bookkeeping so as not to clean up the second time through cfg. It's the
less straightforward way of going about it IMHO.
I know that I must sound like a broken record at this point, but look:
with build("ip link set dev %s master %s" % (swp1, h1),
"ip link set dev %s nomaster" % swp1) as thing:
... some code which may rise ...
... more code, interface detached, `thing' gone ...
It's just as concise, makes it very clear where the device is part of
the bridge and where not anymore, and does away with the intricacies of
lifetime management.
If lifetimes don't nest, I think it's just going to be ugly either way.
But I don't think this comes up often.
I don't really see stuff that you could just throw at cfg to keep track
of, apart from the suite configuration (i.e. topology set up). But I
suppose if it comes up, we can do something like:
thing = cfg.retain(build(..., ...))
Or maybe have a dedicated retainer object, or whatever, it doesn't
necessarily need to be cfg itself.
>> This is what I ended up gravitating towards after writing a handful of
>> LNST tests anyway. The scoping makes it clear where the object exists,
>> lifetime is taken care of, it's all ponies rainbows basically. At least
>> as long as your object lifetimes can be cleanly nested, which admittedly
>> is not always.
>
> Should be fairly easy to support all cases - "with", "recording on
> cfg/test" and del. Unfortunately in the two tests I came up with
Yup.
> quickly for this series cleanup is only needed for the env itself.
> It's a bit awkward to add the lifetime helpers without any users.
Yeah. I'm basically delving in this now to kinda try and steer future
expectations.
Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting
Jakub Kicinski writes:
> On Tue, 2 Apr 2024 18:37:44 +0200 Petr Machata wrote:
>> Yeah, this would be usually done through context managers, as I mention
>> in the other e-mail. But then cfg would be lexically scoped, which IMHO
>> is a good thing, but then it needs to be passed around as an argument,
>> and that makes the ksft_run() invocation a bit messy:
>>
>> with NetDrvEnv(__file__) as cfg:
>> ksft_run([lambda: check_pause(cfg),
>> lambda: check_fec(cfg),
>> lambda: pkt_byte_sum(cfg)])
>>
>> Dunno, maybe it could forward *args **kwargs to the cases? But then it
>> loses some of the readability again.
>
> Yes, I was wondering about that. It must be doable, IIRC
> the multi-threading API "injects" args from a tuple.
> I was thinking something along the lines of:
>
> with NetDrvEnv(__file__) as cfg:
> ksft_run([check_pause, check_fec, pkt_byte_sum],
> args=(cfg, ))
>
> I got lazy, let me take a closer look. Another benefit
> will be that once we pass in "env" / cfg - we can "register"
> objects in there for auto-cleanup (in the future, current
> tests don't need cleanup)
Yeah, though some of those should probably just be their own context
managers IMHO, not necessarily hooked to cfg. I'm thinking something
fairly general, so that the support boilerplate doesn't end up costing
an arm and leg:
with build("ip route add 192.0.2.1/28 nexthop via 192.0.2.17",
"ip route del 192.0.2.1/28"),
build("ip link set dev %s master %s" % (swp1, h1),
"ip link set dev %s nomaster" % swp1):
le_test()
Dunno. I guess it makes sense to have some of the common stuff
predefined, e.g. "with vrf() as h1". And then the stuff that's typically
in lib.sh's setup() and cleanup(), can be losslessly hooked up to cfg.
This is what I ended up gravitating towards after writing a handful of
LNST tests anyway. The scoping makes it clear where the object exists,
lifetime is taken care of, it's all ponies rainbows basically. At least
as long as your object lifetimes can be cleanly nested, which admittedly
is not always.
Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting
Jakub Kicinski writes:
> On Tue, 2 Apr 2024 10:31:11 -0700 Jakub Kicinski wrote:
>> Yes, I was wondering about that. It must be doable, IIRC
>> the multi-threading API "injects" args from a tuple.
>> I was thinking something along the lines of:
>>
>> with NetDrvEnv(__file__) as cfg:
>> ksft_run([check_pause, check_fec, pkt_byte_sum],
>> args=(cfg, ))
>
> seems to work, is this good?
>
> diff --git a/tools/testing/selftests/net/lib/py/ksft.py
> b/tools/testing/selftests/net/lib/py/ksft.py
> index 7c296fe5e438..c7210525981c 100644
> --- a/tools/testing/selftests/net/lib/py/ksft.py
> +++ b/tools/testing/selftests/net/lib/py/ksft.py
> @@ -60,7 +60,7 @@ KSFT_RESULT = None
> print(res)
>
>
> -def ksft_run(cases):
> +def ksft_run(cases, args=()):
> totals = {"pass": 0, "fail": 0, "skip": 0, "xfail": 0}
>
> print("KTAP version 1")
> @@ -72,7 +72,7 @@ KSFT_RESULT = None
> KSFT_RESULT = True
> cnt += 1
> try:
> -case()
> +case(*args)
> except KsftSkipEx as e:
> ktap_result(True, cnt, case, comment="SKIP " + str(e))
> totals['skip'] += 1
Yep, looks good.
Re: [PATCH net-next 3/7] selftests: net: add scaffolding for Netlink tests in Python
Jakub Kicinski writes:
> Add glue code for accessing the YNL library which lives under
> tools/net and YAML spec files from under Documentation/.
> Automatically figure out if tests are run in tree or not.
> Since we'll want to use this library both from net and
> drivers/net test targets make the library a target as well,
> and automatically include it when net or drivers/net are
> included. Making net/lib a target ensures that we end up
> with only one copy of it, and saves us some path guessing.
>
> Add a tiny bit of formatting support to be able to output KTAP
> from the start.
>
> Signed-off-by: Jakub Kicinski
> ---
> CC: sh...@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
> tools/testing/selftests/Makefile | 7 ++
> tools/testing/selftests/net/lib/Makefile | 8 ++
> .../testing/selftests/net/lib/py/__init__.py | 6 ++
> tools/testing/selftests/net/lib/py/consts.py | 9 ++
> tools/testing/selftests/net/lib/py/ksft.py| 96 +++
> tools/testing/selftests/net/lib/py/utils.py | 47 +
> tools/testing/selftests/net/lib/py/ynl.py | 49 ++
> 7 files changed, 222 insertions(+)
> create mode 100644 tools/testing/selftests/net/lib/Makefile
> create mode 100644 tools/testing/selftests/net/lib/py/__init__.py
> create mode 100644 tools/testing/selftests/net/lib/py/consts.py
> create mode 100644 tools/testing/selftests/net/lib/py/ksft.py
> create mode 100644 tools/testing/selftests/net/lib/py/utils.py
> create mode 100644 tools/testing/selftests/net/lib/py/ynl.py
>
> diff --git a/tools/testing/selftests/Makefile
> b/tools/testing/selftests/Makefile
> index e1504833654d..0cffdfb4b116 100644
> --- a/tools/testing/selftests/Makefile
> +++ b/tools/testing/selftests/Makefile
> @@ -116,6 +116,13 @@ TARGETS += zram
> TARGETS_HOTPLUG = cpu-hotplug
> TARGETS_HOTPLUG += memory-hotplug
>
> +# Networking tests want the net/lib target, include it automatically
> +ifneq ($(filter net ,$(TARGETS)),)
> +ifeq ($(filter net/lib,$(TARGETS)),)
> + override TARGETS := $(TARGETS) net/lib
> +endif
> +endif
> +
> # User can optionally provide a TARGETS skiplist. By default we skip
> # BPF since it has cutting edge build time dependencies which require
> # more effort to install.
> diff --git a/tools/testing/selftests/net/lib/Makefile
> b/tools/testing/selftests/net/lib/Makefile
> new file mode 100644
> index ..5730682aeffb
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/Makefile
> @@ -0,0 +1,8 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +TEST_FILES := ../../../../../Documentation/netlink/s*
> +TEST_FILES += ../../../../net/*
> +
> +TEST_INCLUDES := $(wildcard py/*.py)
> +
> +include ../../lib.mk
> diff --git a/tools/testing/selftests/net/lib/py/__init__.py
> b/tools/testing/selftests/net/lib/py/__init__.py
> new file mode 100644
> index ..81a8d14b68f0
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/py/__init__.py
> @@ -0,0 +1,6 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +from .ksft import *
> +from .ynl import NlError, YnlFamily, EthtoolFamily, NetdevFamily, RtnlFamily
> +from .consts import KSRC
> +from .utils import *
> diff --git a/tools/testing/selftests/net/lib/py/consts.py
> b/tools/testing/selftests/net/lib/py/consts.py
> new file mode 100644
> index ..f518ce79d82c
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/py/consts.py
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import sys
> +from pathlib import Path
> +
> +KSFT_DIR = (Path(__file__).parent / "../../..").resolve()
> +KSRC = (Path(__file__).parent / "../../../../../..").resolve()
> +
> +KSFT_MAIN_NAME = Path(sys.argv[0]).with_suffix("").name
> diff --git a/tools/testing/selftests/net/lib/py/ksft.py
> b/tools/testing/selftests/net/lib/py/ksft.py
> new file mode 100644
> index ..7c296fe5e438
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib/py/ksft.py
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import builtins
> +from .consts import KSFT_MAIN_NAME
> +
> +KSFT_RESULT = None
> +
> +
> +class KsftSkipEx(Exception):
> +pass
> +
> +
> +class KsftXfailEx(Exception):
> +pass
> +
> +
> +def ksft_pr(*objs, **kwargs):
> +print("#", *objs, **kwargs)
> +
> +
> +def ksft_eq(a, b, comment=""):
> +global KSFT_RESULT
> +if a != b:
> +KSFT_RESULT = False
> +ksft_pr("Check failed", a, "!=", b, comment)
> +
> +
> +def ksft_true(a, comment=""):
> +global KSFT_RESULT
> +if not a:
> +KSFT_RESULT = False
> +ksft_pr("Check failed", a, "does not eval to True", comment)
> +
> +
> +def ksft_in(a, b, comment=""):
> +global KSFT_RESULT
> +if a not in b:
> +KSFT_RESULT = False
> +ksft_pr("Check failed", a, "not in", b, comment)
> +
> +
> +def ksft_ge(a, b, comment=""):
> +global KSFT_RESULT
> +if a < b:
> +KSFT_RESULT = False
Hmm, instead of this global KSFT_RESULT business, have you
Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting
Jakub Kicinski writes:
> Add a very simple test to make sure drivers report expected
> stats. Drivers which implement FEC or pause configuration
> should report relevant stats. Qstats must be reported,
> at least packet and byte counts, and they must match
> total device stats.
>
> Tested with netdevsim, bnxt, in-tree and installed.
>
> Signed-off-by: Jakub Kicinski
> ---
> tools/testing/selftests/drivers/net/stats.py | 85
> 1 file changed, 85 insertions(+)
> create mode 100755 tools/testing/selftests/drivers/net/stats.py
>
> diff --git a/tools/testing/selftests/drivers/net/stats.py
> b/tools/testing/selftests/drivers/net/stats.py
> new file mode 100755
> index ..751cca2869b8
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/stats.py
> @@ -0,0 +1,85 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +from lib.py import ksft_run, ksft_in, ksft_true, KsftSkipEx, KsftXfailEx
> +from lib.py import EthtoolFamily, NetdevFamily, RtnlFamily, NlError
> +from lib.py import NetDrvEnv
> +
> +cfg = None
> +ethnl = EthtoolFamily()
> +netfam = NetdevFamily()
> +rtnl = RtnlFamily()
> +
> +
> +def check_pause() -> None:
> +global cfg, ethnl
> +
> +try:
> +ethnl.pause_get({"header": {"dev-index": cfg.ifindex}})
> +except NlError as e:
> +if e.error == 95:
> +raise KsftXfailEx("pause not supported by the device")
> +raise
> +
> +data = ethnl.pause_get({"header": {"dev-index": cfg.ifindex,
> + "flags": {'stats'}}})
> +ksft_true(data['stats'], "driver does not report stats")
> +
> +
> +def check_fec() -> None:
> +global ethnl
> +
> +try:
> +ethnl.fec_get({"header": {"dev-index": cfg.ifindex}})
> +except NlError as e:
> +if e.error == 95:
> +raise KsftXfailEx("FEC not supported by the device")
> +raise
> +
> +data = ethnl.fec_get({"header": {"dev-index": cfg.ifindex,
> + "flags": {'stats'}}})
> +ksft_true(data['stats'], "driver does not report stats")
> +
> +
> +def pkt_byte_sum() -> None:
> +global cfg, netfam, rtnl
> +
> +def get_qstat(test):
> +global netfam
> +stats = netfam.qstats_get({}, dump=True)
> +if stats:
> +for qs in stats:
> +if qs["ifindex"]== test.ifindex:
> +return qs
> +
> +qstat = get_qstat(cfg)
> +if qstat is None:
> +raise KsftSkipEx("qstats not supported by the device")
> +
> +for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
> +ksft_in(key, qstat, "Drivers should always report basic keys")
> +
> +# Compare stats, rtnl stats and qstats must match,
> +# but the interface may be up, so do a series of dumps
> +# each time the more "recent" stats must be higher or same.
> +def stat_cmp(rstat, qstat):
> +for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
> +if rstat[key] != qstat[key]:
> +return rstat[key] - qstat[key]
> +return 0
> +
> +for _ in range(10):
> +rtstat = rtnl.getlink({"ifi-index": cfg.ifindex})['stats']
> +if stat_cmp(rtstat, qstat) < 0:
> +raise Exception("RTNL stats are lower, fetched later")
> +qstat = get_qstat(cfg)
> +if stat_cmp(rtstat, qstat) > 0:
> +raise Exception("Qstats are lower, fetched later")
> +
> +
> +if __name__ == "__main__":
> +cfg = NetDrvEnv(__file__)
> +try:
> +ksft_run([check_pause, check_fec, pkt_byte_sum])
> +finally:
> +del cfg
Yeah, this would be usually done through context managers, as I mention
in the other e-mail. But then cfg would be lexically scoped, which IMHO
is a good thing, but then it needs to be passed around as an argument,
and that makes the ksft_run() invocation a bit messy:
with NetDrvEnv(__file__) as cfg:
ksft_run([lambda: check_pause(cfg),
lambda: check_fec(cfg),
lambda: pkt_byte_sum(cfg)])
Dunno, maybe it could forward *args **kwargs to the cases? But then it
loses some of the readability again.
Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization
Jakub Kicinski writes:
> On Tue, 26 Mar 2024 11:31:31 +0100 Petr Machata wrote:
>> Jakub Kicinski writes:
>>
>> > 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:
>> >
>> >> +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?
>>
>> 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.
>
> 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.
[PATCH net-next 14/14] selftests: forwarding: Add a test for testing lib.sh functionality
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
---
.../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
diff --git a/tools/testing/selftests/net/forwarding/Makefile
b/tools/testing/selftests/net/forwarding/Makefile
index 56e3557ba8a6..fa7b59ff4029 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -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 \
diff --git a/tools/testing/selftests/net/forwarding/lib_sh_test.sh
b/tools/testing/selftests/net/forwarding/lib_sh_test.sh
new file mode 100755
index ..ff2aaf4d
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/lib_sh_test.sh
@@ -0,0 +1,208 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This tests the operation of lib.sh itself.
+
+ALL_TESTS="
+ test_ret
+ test_exit_status
+"
+NUM_NETIFS=0
+source lib.sh
+
+# Simulated checks.
+
+do_test()
+{
+ local msg=$1; shift
+
+ "$@"
+ check_err $? "$msg"
+}
+
+tpass()
+{
+ do_test "tpass" true
+}
+
+tfail()
+{
+ do_test "tfail" false
+}
+
+txfail()
+{
+ FAIL_TO_XFAIL=yes do_test "txfail" false
+}
+
+# Simulated tests.
+
+pass()
+{
+ RET=0
+ do_test "true" true
+ log_test "true"
+}
+
+fail()
+{
+ RET=0
+ do_test "false" false
+ log_test "false"
+}
+
+xfail()
+{
+ RET=0
+ FAIL_TO_XFAIL=yes do_test "xfalse" false
+ log_test "xfalse"
+}
+
+skip()
+{
+ RET=0
+ log_test_skip "skip"
+}
+
+slow_xfail()
+{
+ RET=0
+ xfail_on_slow do_test "slow_false" false
+ log_test "slow_false"
+}
+
+# lib.sh tests.
+
+ret_tests_run()
+{
+ local t
+
+ RET=0
+ retmsg=
+ for t in "$@"; do
+ $t
+ done
+ echo "$retmsg"
+ return $RET
+}
+
+ret_subtest()
+{
+ local expect_ret=$1; shift
+ local expect_retmsg=$1; shift
+ local -a tests=( "$@" )
+
+ local status_names=(pass fail xfail xpass skip)
+ local ret
+ local out
+
+ RET=0
+
+ # Run this in a subshell, so that our environment is intact.
+ out=$(ret_tests_run "${tests[@]}")
+ ret=$?
+
+ (( ret == expect_ret ))
+ check_err $? "RET=$ret expected $expect_ret"
+
+ [[ $out == $expect_retmsg ]]
+ check_err $? "retmsg=$out expected $expect_retmsg"
+
+ log_test "RET $(echo ${tests[@]}) -> ${status_names[$ret]}"
+}
+
+test_ret()
+{
+ ret_subtest $ksft_pass ""
+
+ ret_subtest $ksft_pass "" tpass
+ ret_subtest $ksft_fail "tfail" tfail
+ ret_subtest $ksft_xfail "txfail" txfail
+
+ ret_subtest $ksft_pass "" tpass tpass
+ ret_subtest $ksft_fail "tfail" tpass tfail
+ ret_subtest $ksft_xfail "txfail" tpass txfail
+
+ ret_subtest $ksft_fail "tfail" tfail tpass
+ ret_subtest $ksft_xfail "txfail" txfail tpass
+
+ ret_subtest $ksft_fail "tfail" tfail tfail
+ ret_subtest $ksft_fail "tfail" tfail txfail
+
+ ret_subtest $ksft_fail "tfail" txfail tfail
+
+ ret_subtest $ksft_xfail "txfail" txfail txfail
+}
+
+exit_status_tests_run()
+{
+ EXIT_STATUS=0
+ tests_run > /dev/null
+ return $EXIT_STATUS
+}
+
+exit_status_subtest()
+{
+ local expect_exit_status=$1; shift
+ local tests=$1; shift
+ local what=$1; shift
+
+ local status_names=(pass fail xfail xpass skip)
+ local exit_status
+ local out
+
+ RET=0
+
+ # Run this in a subshell, so that our environment is intact.
+ out=$(TESTS="$tests" exit_status_tests_run)
+ exit_status=$?
+
+ (( exit_status == expect_exit_status ))
+ check_err $? "EXIT_STATUS=$exit_status, expected $expect_exit_status"
+
+ log_test "EXIT_STATUS $te
[PATCH net-next 13/14] selftests: forwarding: router_mpath_nh_lib: Don't skip, xfail on veth
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
---
tools/testing/selftests/net/forwarding/lib.sh | 14 ++
.../net/forwarding/router_mpath_nh_lib.sh | 12 +---
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 58df57855bff..4103ed7afcde 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
index b2d2c6cecc01..2903294d8bca 100644
--- a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
+++ b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
@@ -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"
--
2.43.0
[PATCH net-next 10/14] selftests: forwarding: Convert log_test() to recognize RET values
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
---
tools/testing/selftests/net/forwarding/lib.sh | 92 ++-
tools/testing/selftests/net/lib.sh| 9 ++
2 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index ee8153651b38..370fc377249b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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()
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index 88f6133ca319..b7f7b8695165 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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
--
2.43.0
[PATCH net-next 12/14] selftests: forwarding: Mark performance-sensitive tests
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
---
.../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(-)
diff --git a/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
b/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
index cdf689e99458..f9d26a7911bb 100644
--- a/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
+++ b/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
@@ -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
}
diff --git a/tools/testing/selftests/net/forwarding/sch_red.sh
b/tools/testing/selftests/net/forwarding/sch_red.sh
index 81f31179ac88..17f28644568e 100755
--- a/tools/testing/selftests/net/forwarding/sch_red.sh
+++ b/tools/testing/selftests/net/forwarding/sch_red.sh
@@ -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
}
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
index d1f26cb7cd73..9cd884d4a5de 100644
--- a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
@@ -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"
}
--
2.43.0
[PATCH net-next 11/14] selftests: forwarding: Support for performance sensitive tests
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
---
tools/testing/selftests/net/forwarding/lib.sh | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 370fc377249b..58df57855bff 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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 @@ ret_set_ksft_status()
fi
}
+# Whether FAILs should be interpreted as XFAILs. Internal.
+FAIL_TO_XFAIL=
+
check_err()
{
local err=$1
local msg=$2
if ((err)); then
- ret_set_ksft_status $ksft_fail "$msg"
+ if [[ $FAIL_TO_XFAIL = yes ]]; then
+ ret_set_ksft_status $ksft_xfail "$msg"
+ else
+ ret_set_ksft_status $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
--
2.43.0
[PATCH net-next 08/14] selftests: lib: Define more kselftest exit codes
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 Signed-off-by: Petr Machata --- tools/testing/selftests/net/forwarding/tc_tunnel_key.sh | 2 -- tools/testing/selftests/net/lib.sh | 6 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh b/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh index 5a5dd9034819..79775b10b99f 100755 --- a/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh +++ b/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh @@ -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" diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh index 5b366cc4fc43..d9bdf6aa3bf1 100644 --- a/tools/testing/selftests/net/lib.sh +++ b/tools/testing/selftests/net/lib.sh @@ -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="" -- 2.43.0
[PATCH net-next 09/14] selftests: forwarding: Have RET track kselftest framework constants
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
---
Notes:
v1:
- Clarify intended usage by s/set_ret/ret_set_ksft_status/,
s/nret/ksft_status/
tools/testing/selftests/net/forwarding/lib.sh | 21 -
tools/testing/selftests/net/lib.sh| 30 +++
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 5415b8d29862..ee8153651b38 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -396,14 +396,24 @@ EXIT_STATUS=0
# Per-test return value. Clear at the beginning of each test.
RET=0
+ret_set_ksft_status()
+{
+ local ksft_status=$1; shift
+ local msg=$1; shift
+
+ RET=$(ksft_status_merge $RET $ksft_status)
+ 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
+ ret_set_ksft_status $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()
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index d9bdf6aa3bf1..88f6133ca319 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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
--
2.43.0
[PATCH net-next 07/14] selftests: forwarding: Change inappropriate log_test_skip() calls
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
Signed-off-by: Petr Machata
---
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(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
index 709433a4c886..e2a1c10d3503 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
@@ -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
}
diff --git a/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
b/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
index 6369927e9c37..48395cfd4f95 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
@@ -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
}
diff --git
a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
index a88d8a8c85f2..899b6892603f 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
@@ -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
diff --git
a/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
index f981c957f097..482ebb744eba 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index ca433ba3612e..5415b8d29862 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
index 7e7d62161c34..b2d2c6cecc01 100644
--- a/tools/testing/selftest
[PATCH net-next 06/14] selftests: forwarding: Ditch skip_on_veth()
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
Signed-off-by: Petr Machata
---
.../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(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool.sh
b/tools/testing/selftests/drivers/net/hw/ethtool.sh
index 187429670ee7..bb12d5d70949 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
@@ -287,8 +287,6 @@ different_speeds_autoneg_on()
ethtool -s $h1 autoneg on
}
-skip_on_veth
-
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
index b0f931260a27..a7584448416e 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
@@ -109,8 +109,6 @@ no_cable()
ip link set dev $swp3 down
}
-skip_on_veth
-
setup_prepare
tests_run
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
index 744760117005..7dfc50366c99 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
@@ -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
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
index 354be353455f..ab8d04855af5 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
@@ -100,8 +100,6 @@ test_stats_rx()
test_stats g2a rx
}
-skip_on_veth
-
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index dbd4348f85b8..ca433ba3612e 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
--
2.43.0
[PATCH net-next 05/14] selftests: forwarding: Move several selftests
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
Cc: Tobias Waldekranz
Cc: Danielle Ratson
Cc: Davide Caratti
Cc: Johannes Nixdorf
Suggested-by: Jakub Kicinski
Signed-off-by: Petr Machata
---
.../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
diff --git a/tools/testing/selftests/drivers/net/hw/Makefile
b/tools/testing/selftests/drivers/net/hw/Makefile
new file mode 100644
index ..2259a39a70ed
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -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
diff --git a/tools/testing/selftests/net/devlink_port_split.py
b/tools/testing/selftests/drivers/net/hw/devlink_port_split.py
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
diff --git a/tools/testing/selftests/net/forwarding/ethtool.sh
b/tools/testing/selftests/drivers/net/hw/ethtool.sh
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
--- a/tools/testing/selftests/net/forwarding/ethtool.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
@@ -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()
diff --git a/tools/testing/selftests/net/forwarding/ethtool_extended_state.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
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
--- a/tools/testing/selftests/net/forwarding/ethtool_extended_state.sh
+++ b/tools/tes
[PATCH net-next 04/14] selftests: forwarding: ipip_lib: Do not import lib.sh
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
---
tools/testing/selftests/net/forwarding/ipip_lib.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/ipip_lib.sh
b/tools/testing/selftests/net/forwarding/ipip_lib.sh
index 30f36a57bae6..01e62c4ac94d 100644
--- a/tools/testing/selftests/net/forwarding/ipip_lib.sh
+++ b/tools/testing/selftests/net/forwarding/ipip_lib.sh
@@ -141,7 +141,6 @@
# | $h2 + |
# | 192.0.2.18/28 |
# +---+
-source lib.sh
h1_create()
{
--
2.43.0
[PATCH net-next 03/14] selftests: forwarding: README: Document customization
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
Reviewed-by: Benjamin Poirier
---
tools/testing/selftests/net/forwarding/README | 33 +++
1 file changed, 33 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/README
b/tools/testing/selftests/net/forwarding/README
index b8a2af8fcfb7..7fdb6a9ca543 100644
--- a/tools/testing/selftests/net/forwarding/README
+++ b/tools/testing/selftests/net/forwarding/README
@@ -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.
--
2.43.0
[PATCH net-next 02/14] selftests: forwarding.config.sample: Move overrides to lib.sh
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 Reviewed-by: Benjamin Poirier --- .../net/forwarding/forwarding.config.sample | 53 -- tools/testing/selftests/net/forwarding/lib.sh | 69 --- 2 files changed, 74 insertions(+), 48 deletions(-) diff --git a/tools/testing/selftests/net/forwarding/forwarding.config.sample b/tools/testing/selftests/net/forwarding/forwarding.config.sample index 1fc4f0242fc5..f1ca95e79a65 100644 --- a/tools/testing/selftests/net/forwarding/forwarding.config.sample +++ b/tools/testing/selftests/net/forwarding/forwarding.config.sample @@ -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 - diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index ee48c4603ff0..dbd4348f85b8 100644 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -1,34 +1,83 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +## +# Topology description
[PATCH net-next 01/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
Reviewed-by: Benjamin Poirier
---
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(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index e579c2e0c462..ee48c4603ff0 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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]}")")
diff --git a/tools/testing/selftests/net/forwarding/tc_common.sh
b/tools/testing/selftests/net/forwarding/tc_common.sh
index bce8bb8d2b6f..2e3326edfa9a 100644
--- a/tools/testing/selftests/net/forwarding/tc_common.sh
+++ b/tools/testing/selftests/net/forwarding/tc_common.sh
@@ -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()
{
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index f9fe182dfbd4..5b366cc4fc43 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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.
--
2.43.0
[PATCH net-next 00/14] 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. v1 (vs. an RFC circulated through linux-kselftest): - Patch #9: - Clarify intended usage by s/set_ret/ret_set_ksft_status/, s/nret/ksft_status/ 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/n
Re: [RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization
Jakub Kicinski writes:
> On Mon, 25 Mar 2024 18:29:10 +0100 Petr Machata wrote:
>> +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
>
> 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.
> 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 :)
>
>> +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.
Re: [RFC PATCH net-next mlxsw 09/14] selftests: forwarding: Have RET track kselftest framework constants
Jakub Kicinski writes:
> On Mon, 25 Mar 2024 18:29:16 +0100 Petr Machata wrote:
>> +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.
Re: [RFC PATCH net-next mlxsw 00/14] selftests: Fixes for kernel CI
Jakub Kicinski writes: > 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.
[RFC PATCH net-next mlxsw 10/14] selftests: forwarding: Convert log_test() to recognize RET values
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
---
tools/testing/selftests/net/forwarding/lib.sh | 92 ++-
tools/testing/selftests/net/lib.sh| 9 ++
2 files changed, 77 insertions(+), 24 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index e72b2370238c..942f38988941 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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()
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index 88f6133ca319..b7f7b8695165 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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
--
2.43.0
[RFC PATCH net-next mlxsw 14/14] selftests: forwarding: Add a test for testing lib.sh functionality
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
---
.../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
diff --git a/tools/testing/selftests/net/forwarding/Makefile
b/tools/testing/selftests/net/forwarding/Makefile
index 56e3557ba8a6..fa7b59ff4029 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -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 \
diff --git a/tools/testing/selftests/net/forwarding/lib_sh_test.sh
b/tools/testing/selftests/net/forwarding/lib_sh_test.sh
new file mode 100755
index ..ff2aaf4d
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/lib_sh_test.sh
@@ -0,0 +1,208 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# This tests the operation of lib.sh itself.
+
+ALL_TESTS="
+ test_ret
+ test_exit_status
+"
+NUM_NETIFS=0
+source lib.sh
+
+# Simulated checks.
+
+do_test()
+{
+ local msg=$1; shift
+
+ "$@"
+ check_err $? "$msg"
+}
+
+tpass()
+{
+ do_test "tpass" true
+}
+
+tfail()
+{
+ do_test "tfail" false
+}
+
+txfail()
+{
+ FAIL_TO_XFAIL=yes do_test "txfail" false
+}
+
+# Simulated tests.
+
+pass()
+{
+ RET=0
+ do_test "true" true
+ log_test "true"
+}
+
+fail()
+{
+ RET=0
+ do_test "false" false
+ log_test "false"
+}
+
+xfail()
+{
+ RET=0
+ FAIL_TO_XFAIL=yes do_test "xfalse" false
+ log_test "xfalse"
+}
+
+skip()
+{
+ RET=0
+ log_test_skip "skip"
+}
+
+slow_xfail()
+{
+ RET=0
+ xfail_on_slow do_test "slow_false" false
+ log_test "slow_false"
+}
+
+# lib.sh tests.
+
+ret_tests_run()
+{
+ local t
+
+ RET=0
+ retmsg=
+ for t in "$@"; do
+ $t
+ done
+ echo "$retmsg"
+ return $RET
+}
+
+ret_subtest()
+{
+ local expect_ret=$1; shift
+ local expect_retmsg=$1; shift
+ local -a tests=( "$@" )
+
+ local status_names=(pass fail xfail xpass skip)
+ local ret
+ local out
+
+ RET=0
+
+ # Run this in a subshell, so that our environment is intact.
+ out=$(ret_tests_run "${tests[@]}")
+ ret=$?
+
+ (( ret == expect_ret ))
+ check_err $? "RET=$ret expected $expect_ret"
+
+ [[ $out == $expect_retmsg ]]
+ check_err $? "retmsg=$out expected $expect_retmsg"
+
+ log_test "RET $(echo ${tests[@]}) -> ${status_names[$ret]}"
+}
+
+test_ret()
+{
+ ret_subtest $ksft_pass ""
+
+ ret_subtest $ksft_pass "" tpass
+ ret_subtest $ksft_fail "tfail" tfail
+ ret_subtest $ksft_xfail "txfail" txfail
+
+ ret_subtest $ksft_pass "" tpass tpass
+ ret_subtest $ksft_fail "tfail" tpass tfail
+ ret_subtest $ksft_xfail "txfail" tpass txfail
+
+ ret_subtest $ksft_fail "tfail" tfail tpass
+ ret_subtest $ksft_xfail "txfail" txfail tpass
+
+ ret_subtest $ksft_fail "tfail" tfail tfail
+ ret_subtest $ksft_fail "tfail" tfail txfail
+
+ ret_subtest $ksft_fail "tfail" txfail tfail
+
+ ret_subtest $ksft_xfail "txfail" txfail txfail
+}
+
+exit_status_tests_run()
+{
+ EXIT_STATUS=0
+ tests_run > /dev/null
+ return $EXIT_STATUS
+}
+
+exit_status_subtest()
+{
+ local expect_exit_status=$1; shift
+ local tests=$1; shift
+ local what=$1; shift
+
+ local status_names=(pass fail xfail xpass skip)
+ local exit_status
+ local out
+
+ RET=0
+
+ # Run this in a subshell, so that our environment is intact.
+ out=$(TESTS="$tests" exit_status_tests_run)
+ exit_status=$?
+
+ (( exit_status == expect_exit_status ))
+ check_err $? "EXIT_STATUS=$exit_status, expected $expect_exit_status"
+
+ log_test "EXIT_STATUS $te
[RFC PATCH net-next mlxsw 11/14] selftests: forwarding: Support for performance sensitive tests
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
---
tools/testing/selftests/net/forwarding/lib.sh | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 942f38988941..cc32bf11739b 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
--
2.43.0
[RFC PATCH net-next mlxsw 13/14] selftests: forwarding: router_mpath_nh_lib: Don't skip, xfail on veth
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
---
tools/testing/selftests/net/forwarding/lib.sh | 14 ++
.../net/forwarding/router_mpath_nh_lib.sh | 12 +---
2 files changed, 15 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index cc32bf11739b..6291723580d7 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
index b2d2c6cecc01..2903294d8bca 100644
--- a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
+++ b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
@@ -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"
--
2.43.0
[RFC PATCH net-next mlxsw 12/14] selftests: forwarding: Mark performance-sensitive tests
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
---
.../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(-)
diff --git a/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
b/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
index cdf689e99458..f9d26a7911bb 100644
--- a/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
+++ b/tools/testing/selftests/net/forwarding/sch_ets_tests.sh
@@ -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
}
diff --git a/tools/testing/selftests/net/forwarding/sch_red.sh
b/tools/testing/selftests/net/forwarding/sch_red.sh
index 81f31179ac88..17f28644568e 100755
--- a/tools/testing/selftests/net/forwarding/sch_red.sh
+++ b/tools/testing/selftests/net/forwarding/sch_red.sh
@@ -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
}
diff --git a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
index d1f26cb7cd73..9cd884d4a5de 100644
--- a/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
+++ b/tools/testing/selftests/net/forwarding/sch_tbf_core.sh
@@ -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"
}
--
2.43.0
[RFC PATCH net-next mlxsw 05/14] selftests: forwarding: Move several selftests
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
Cc: Tobias Waldekranz
Cc: Danielle Ratson
Cc: Davide Caratti
Cc: Johannes Nixdorf
Suggested-by: Jakub Kicinski
Signed-off-by: Petr Machata
---
.../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
diff --git a/tools/testing/selftests/drivers/net/hw/Makefile
b/tools/testing/selftests/drivers/net/hw/Makefile
new file mode 100644
index ..2259a39a70ed
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -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
diff --git a/tools/testing/selftests/net/devlink_port_split.py
b/tools/testing/selftests/drivers/net/hw/devlink_port_split.py
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
diff --git a/tools/testing/selftests/net/forwarding/ethtool.sh
b/tools/testing/selftests/drivers/net/hw/ethtool.sh
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
--- a/tools/testing/selftests/net/forwarding/ethtool.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
@@ -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()
diff --git a/tools/testing/selftests/net/forwarding/ethtool_extended_state.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
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
--- a/tools/testing/selftests/net/forwarding/ethtool_extended_state.sh
+++ b/tools/tes
[RFC PATCH net-next mlxsw 06/14] selftests: forwarding: Ditch skip_on_veth()
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
Signed-off-by: Petr Machata
---
.../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(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool.sh
b/tools/testing/selftests/drivers/net/hw/ethtool.sh
index 187429670ee7..bb12d5d70949 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool.sh
@@ -287,8 +287,6 @@ different_speeds_autoneg_on()
ethtool -s $h1 autoneg on
}
-skip_on_veth
-
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
index b0f931260a27..a7584448416e 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_extended_state.sh
@@ -109,8 +109,6 @@ no_cable()
ip link set dev $swp3 down
}
-skip_on_veth
-
setup_prepare
tests_run
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
index 744760117005..7dfc50366c99 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3.sh
@@ -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
diff --git a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
index 354be353455f..ab8d04855af5 100755
--- a/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
+++ b/tools/testing/selftests/drivers/net/hw/hw_stats_l3_gre.sh
@@ -100,8 +100,6 @@ test_stats_rx()
test_stats g2a rx
}
-skip_on_veth
-
trap cleanup EXIT
setup_prepare
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index dbd4348f85b8..ca433ba3612e 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
--
2.43.0
[RFC PATCH net-next mlxsw 07/14] selftests: forwarding: Change inappropriate log_test_skip() calls
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
Signed-off-by: Petr Machata
---
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(-)
diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
index 709433a4c886..e2a1c10d3503 100755
--- a/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
+++ b/tools/testing/selftests/drivers/net/hw/ethtool_rmon.sh
@@ -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
}
diff --git a/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
b/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
index 6369927e9c37..48395cfd4f95 100644
--- a/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/mlxsw_lib.sh
@@ -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
}
diff --git
a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
index a88d8a8c85f2..899b6892603f 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum-2/resource_scale.sh
@@ -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
diff --git
a/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
index f981c957f097..482ebb744eba 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/spectrum/resource_scale.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index ca433ba3612e..5415b8d29862 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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
diff --git a/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
b/tools/testing/selftests/net/forwarding/router_mpath_nh_lib.sh
index 7e7d62161c34..b2d2c6cecc01 100644
--- a/tools/testing/selftest
[RFC PATCH net-next mlxsw 09/14] selftests: forwarding: Have RET track kselftest framework constants
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
---
tools/testing/selftests/net/forwarding/lib.sh | 21 -
tools/testing/selftests/net/lib.sh| 30 +++
2 files changed, 44 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 5415b8d29862..e72b2370238c 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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()
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index d9bdf6aa3bf1..88f6133ca319 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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
--
2.43.0
[RFC PATCH net-next mlxsw 08/14] selftests: lib: Define more kselftest exit codes
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 Signed-off-by: Petr Machata --- tools/testing/selftests/net/forwarding/tc_tunnel_key.sh | 2 -- tools/testing/selftests/net/lib.sh | 6 +- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh b/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh index 5a5dd9034819..79775b10b99f 100755 --- a/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh +++ b/tools/testing/selftests/net/forwarding/tc_tunnel_key.sh @@ -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" diff --git a/tools/testing/selftests/net/lib.sh b/tools/testing/selftests/net/lib.sh index 5b366cc4fc43..d9bdf6aa3bf1 100644 --- a/tools/testing/selftests/net/lib.sh +++ b/tools/testing/selftests/net/lib.sh @@ -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="" -- 2.43.0
[RFC PATCH net-next mlxsw 04/14] selftests: forwarding: ipip_lib: Do not import lib.sh
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
---
tools/testing/selftests/net/forwarding/ipip_lib.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/net/forwarding/ipip_lib.sh
b/tools/testing/selftests/net/forwarding/ipip_lib.sh
index 30f36a57bae6..01e62c4ac94d 100644
--- a/tools/testing/selftests/net/forwarding/ipip_lib.sh
+++ b/tools/testing/selftests/net/forwarding/ipip_lib.sh
@@ -141,7 +141,6 @@
# | $h2 + |
# | 192.0.2.18/28 |
# +---+
-source lib.sh
h1_create()
{
--
2.43.0
[RFC PATCH net-next mlxsw 03/14] selftests: forwarding: README: Document customization
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
Reviewed-by: Benjamin Poirier
---
tools/testing/selftests/net/forwarding/README | 33 +++
1 file changed, 33 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/README
b/tools/testing/selftests/net/forwarding/README
index b8a2af8fcfb7..7fdb6a9ca543 100644
--- a/tools/testing/selftests/net/forwarding/README
+++ b/tools/testing/selftests/net/forwarding/README
@@ -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.
--
2.43.0
[RFC PATCH net-next mlxsw 00/14] 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/selft
[RFC PATCH net-next mlxsw 01/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
Reviewed-by: Benjamin Poirier
---
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(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index e579c2e0c462..ee48c4603ff0 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -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]}")")
diff --git a/tools/testing/selftests/net/forwarding/tc_common.sh
b/tools/testing/selftests/net/forwarding/tc_common.sh
index bce8bb8d2b6f..2e3326edfa9a 100644
--- a/tools/testing/selftests/net/forwarding/tc_common.sh
+++ b/tools/testing/selftests/net/forwarding/tc_common.sh
@@ -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()
{
diff --git a/tools/testing/selftests/net/lib.sh
b/tools/testing/selftests/net/lib.sh
index f9fe182dfbd4..5b366cc4fc43 100644
--- a/tools/testing/selftests/net/lib.sh
+++ b/tools/testing/selftests/net/lib.sh
@@ -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.
--
2.43.0
[RFC PATCH net-next mlxsw 02/14] selftests: forwarding.config.sample: Move overrides to lib.sh
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 Reviewed-by: Benjamin Poirier --- .../net/forwarding/forwarding.config.sample | 53 -- tools/testing/selftests/net/forwarding/lib.sh | 69 --- 2 files changed, 74 insertions(+), 48 deletions(-) diff --git a/tools/testing/selftests/net/forwarding/forwarding.config.sample b/tools/testing/selftests/net/forwarding/forwarding.config.sample index 1fc4f0242fc5..f1ca95e79a65 100644 --- a/tools/testing/selftests/net/forwarding/forwarding.config.sample +++ b/tools/testing/selftests/net/forwarding/forwarding.config.sample @@ -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 - diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index ee48c4603ff0..dbd4348f85b8 100644 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -1,34 +1,83 @@ #!/bin/bash # SPDX-License-Identifier: GPL-2.0 +## +# Topology description
Re: [ANN] net-next is OPEN
Jakub Kicinski writes:
> On Tue, 23 Jan 2024 18:04:19 +0100 Petr Machata wrote:
>> > Unless I'm doing it wrong and the sub-directories are supposed to
>> > inherit the parent directory's config? So net/forwarding/ should
>> > be built with net/'s config? I could not find the info in docs,
>> > does anyone know?
>>
>> I don't think they are, net/config defines CONFIG_VXLAN, but then the
>> vxlan tests still complain about unknown device type. Though maybe
>> there's another device type that it's missing...
>>
>> What do I do to feed the config file to some build script to get a
>> kernel image to test? I can of course just do something like
>> cat config | xargs -n1 scripts/config -m, but I expect there's some
>> automation for it and I just can't find it.
>
> The CI script is based on virtme-ng. So it does this:
>
> # $target is net or net/forwarding or drivers/net/bonding etc.
> make mrproper
> vng -v -b -f tools/testing/selftests/$target
Actually:
# vng -v -b -f tools/testing/selftests/${target}/config
Didn't know about vng, it's way cool!
(Despite having used virtme for a long time.)
> # build the scripts
> make headers
> make -C tools/testing/selftests/$target
>
> vng -v -r arch/x86/boot/bzImage --user root
> # inside the VM
> make -C tools/testing/selftests TARGETS=$target run_tests
I'm working my way through the selftests.
> https://github.com/kuba-moo/nipa/blob/master/contest/remote/vmksft.py#L138
>
> You're right, it definitely does not "inherit" net's config when
> running forwarding/net. I can easily make it do so, but I'm not clear
>
> what the expectation from the kselftest subsystem is. Because if other
> testers (people testing stable, KernelCI etc. et.c) don't "inherit" we
> better fill in the config completely so that the tests pass for
> everyone.
Oh, gotcha, the question was not whether it does, but whether it's
supposed to. OK.
IMHO not necessarily. net/config is for net/*.sh, net/forwarding/config
for net/forwarding/*.sh. It's not a given that whatever is needed for
net/ is needed for net/forwarding/ as well.
It will also lead to simpler patches, where enabling config options in
X/ doesn't imply checking for newly useless duplicities in X's child
directories.
Re: [ANN] net-next is OPEN
Jakub Kicinski writes: > On Tue, 23 Jan 2024 10:55:09 +0100 Petr Machata wrote: >> > If you authored any net or drivers/net selftests, please look around >> > and see if they are passing. If not - send patches or LMK what I need >> > to do to make them pass on the runner.. Make sure to scroll down to >> > the "Not reporting to patchwork" section. >> >> A whole bunch of them fail because of no IPv6 support in the runner >> kernel. E.g. this from bridge-mdb.sh[0]: > > Thanks a lot for investigating! I take it that you're looking at > forwarding? Please send a patch to add the missing configs to > > tools/testing/selftests/net/forwarding/config OK. > The runner uses that to configure the kernel on top of defconfig. > > Unless I'm doing it wrong and the sub-directories are supposed to > inherit the parent directory's config? So net/forwarding/ should > be built with net/'s config? I could not find the info in docs, > does anyone know? I don't think they are, net/config defines CONFIG_VXLAN, but then the vxlan tests still complain about unknown device type. Though maybe there's another device type that it's missing... What do I do to feed the config file to some build script to get a kernel image to test? I can of course just do something like cat config | xargs -n1 scripts/config -m, but I expect there's some automation for it and I just can't find it.
[PATCH net 6/6] selftests: mlxsw: qos_pfc: Adjust the test to support 8 lanes
From: Amit Cohen
'qos_pfc' test checks PFC behavior. The idea is to limit the traffic
using a shaper somewhere in the flow of the packets. In this area, the
buffer is smaller than the buffer at the beginning of the flow, so it fills
up until there is no more space left. The test configures there PFC
which is supposed to notice that the headroom is filling up and send PFC
Xoff to indicate the transmitter to stop sending traffic for the priorities
sharing this PG.
The Xon/Xoff threshold is auto-configured and always equal to
2*(MTU rounded up to cell size). Even after sending the PFC Xoff packet,
traffic will keep arriving until the transmitter receives and processes
the PFC packet. This amount of traffic is known as the PFC delay allowance.
Currently the buffer for the delay traffic is configured as 100KB. The
MTU in the test is 10KB, therefore the threshold for Xoff is about 20KB.
This allows 80KB extra to be stored in this buffer.
8-lane ports use two buffers among which the configured buffer is split,
the Xoff threshold then applies to each buffer in parallel.
The test does not take into account the behavior of 8-lane ports, when the
ports are configured to 400Gbps with 8 lanes or 800Gbps with 8 lanes,
packets are dropped and the test fails.
Check if the relevant ports use 8 lanes, in such case double the size of
the buffer, as the headroom is split half-half.
Cc: Shuah Khan
Cc: linux-kselftest@vger.kernel.org
Fixes: bfa804784e32 ("selftests: mlxsw: Add a PFC test")
Signed-off-by: Amit Cohen
Reviewed-by: Ido Schimmel
Signed-off-by: Petr Machata
---
.../selftests/drivers/net/mlxsw/qos_pfc.sh | 18 +-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
index 49bef76083b8..0f0f4f05807c 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
@@ -119,6 +119,9 @@ h2_destroy()
switch_create()
{
+ local lanes_swp4
+ local pg1_size
+
# pools
# -
@@ -228,7 +231,20 @@ switch_create()
dcb pfc set dev $swp4 prio-pfc all:off 1:on
# PG0 will get autoconfigured to Xoff, give PG1 arbitrarily 100K, which
# is (-2*MTU) about 80K of delay provision.
- dcb buffer set dev $swp4 buffer-size all:0 1:$_100KB
+ pg1_size=$_100KB
+
+ setup_wait_dev_with_timeout $swp4
+
+ lanes_swp4=$(ethtool $swp4 | grep 'Lanes:')
+ lanes_swp4=${lanes_swp4#*"Lanes: "}
+
+ # 8-lane ports use two buffers among which the configured buffer
+ # is split, so double the size to get twice (20K + 80K).
+ if [[ $lanes_swp4 -eq 8 ]]; then
+ pg1_size=$((pg1_size * 2))
+ fi
+
+ dcb buffer set dev $swp4 buffer-size all:0 1:$pg1_size
# bridges
# ---
--
2.42.0
[PATCH net 5/6] selftests: mlxsw: qos_pfc: Remove wrong description
From: Amit Cohen
In the diagram of the topology, $swp3 and $swp4 are described as 1Gbps
ports. This is wrong information, the test does not configure such speed.
Cc: Shuah Khan
Cc: linux-kselftest@vger.kernel.org
Fixes: bfa804784e32 ("selftests: mlxsw: Add a PFC test")
Signed-off-by: Amit Cohen
Reviewed-by: Ido Schimmel
Signed-off-by: Petr Machata
---
tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh | 1 -
1 file changed, 1 deletion(-)
diff --git a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
index 42ce602d8d49..49bef76083b8 100755
--- a/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
+++ b/tools/testing/selftests/drivers/net/mlxsw/qos_pfc.sh
@@ -40,7 +40,6 @@
# | + $swp1 $swp3 ++ $swp4 |
# | | iPOOL1iPOOL0 || iPOOL2|
# | | ePOOL4ePOOL5 || ePOOL4|
-# | |1Gbps || 1Gbps |
# | |PFC:enabled=1 || PFC:enabled=1 |
# | +-|--|-++-|+ |
# | | + $swp1.111 $swp3.111 + || + $swp4.111 | |
--
2.42.0
Re: [PATCHv3 net-next 01/14] selftests/net: add lib.sh
Paolo Abeni writes:
> On Wed, 2023-12-06 at 13:32 +0100, Petr Machata wrote:
>> Paolo Abeni writes:
>>
>> > Side note for a possible follow-up: if you maintain $ns_list as global
>> > variable, and remove from such list the ns deleted by cleanup_ns, you
>> > could remove the cleanup trap from the individual test with something
>> > alike:
>> >
>> > final_cleanup_ns()
>> > {
>> >cleanup_ns $ns_list
>> > }
>> >
>> > trap final_cleanup_ns EXIT
>> >
>> > No respin needed for the above, could be a follow-up if agreed upon.
>>
>> If you propose this for the library then I'm against it. The exit trap
>> is a global resource that the client scripts sometimes need to use as
>> well, to do topology teardowns or just general cleanups.Â
>> So either the library would have to provide APIs for cleanup management, or
>> the trap
>> is for exclusive use by clients. The latter is IMHO simpler.
>
> Even the former would not be very complex:
>
> TRAPS=""
> do_at_exit() {
> TRAPS="${TRAPS}$@;"
>
> trap "${TRAPS}" EXIT
> }
>
> And then use "do_at_exit " instead of "trap EXIT"
Yep. I mentioned this during v2 review:
https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L13
Not much code at all, though you need to convert all EXIT trap users to
this contraption. Again, a mechanical process, just needs to be done.
>> It also puts the cleanups at the same place where the acquisition is
>> prompted: the client allocates the NS, the client should prompt its
>> cleanup.
>
> I guess I could argue that the the script is asking the library to
> allocate the namespaces, and the library could take care of disposing
> them.
It could also be said that since the script asked for NS creation, the
script should ask for NS disposal :)
But what I object against is that the library uses trap without having a
way for user scripts to schedule at-exit work, because that's used
literally everywhere in forwarding tests. If people are willing to do
the conversion, I'm OK with that.
> But I'm not pushing the proposed option, if there is no agreement no
> need for additional work ;)
>
> Cheers,
>
> Paolo
Re: [PATCHv3 net-next 01/14] selftests/net: add lib.sh
Paolo Abeni writes:
> Side note for a possible follow-up: if you maintain $ns_list as global
> variable, and remove from such list the ns deleted by cleanup_ns, you
> could remove the cleanup trap from the individual test with something
> alike:
>
> final_cleanup_ns()
> {
> cleanup_ns $ns_list
> }
>
> trap final_cleanup_ns EXIT
>
> No respin needed for the above, could be a follow-up if agreed upon.
If you propose this for the library then I'm against it. The exit trap
is a global resource that the client scripts sometimes need to use as
well, to do topology teardowns or just general cleanups. So either the
library would have to provide APIs for cleanup management, or the trap
is for exclusive use by clients. The latter is IMHO simpler.
It also puts the cleanups at the same place where the acquisition is
prompted: the client allocates the NS, the client should prompt its
cleanup.
Re: [PATCHv2 net-next 01/14] selftests/net: add lib.sh
Hangbin Liu writes: > Add a lib.sh for net selftests. This file can be used to define commonly > used variables and functions. Some commonly used functions can be moved > from forwarding/lib.sh to this lib file. e.g. busywait(). > > Add function setup_ns() for user to create unique namespaces with given > prefix name. > > Signed-off-by: Hangbin Liu LGTM. I took it for a spin with a couple tests that we use for mlxsw, mainly to make sure the "source ../lib.sh" bit works when invoking tests from other directories. All seems fine. Reviewed-by: Petr Machata
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> On Fri, Nov 24, 2023 at 03:35:51PM +0100, Petr Machata wrote:
>>
>> Hangbin Liu writes:
>>
>> > + fi
>> > + done
>> > +
>> > + [ $errexit -eq 1 ] && set -e
>> > + return 0
>> > +}
>> > +
>> > +# By default, remove all netns before EXIT.
>> > +cleanup_all_ns()
>> > +{
>> > + cleanup_ns $NS_LIST
>> > +}
>> > +trap cleanup_all_ns EXIT
>>
>> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
>> because basically all users of forwarding/lib.sh use the EXIT trap.
>>
>> I wonder if we need something like these push_cleanup / on_exit helpers:
>>
>> https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15
>
> When I added this, I just want to make sure the netns are cleaned up if the
> client script forgot. I think the client script trap function should
> cover this one, no?
So the motivation makes sense. But in general, invoking cleanup from the
same abstraction layer that acquired the resource, makes the code easier
to analyze. And in particular here that we are talking about traps,
which are a global resource, and one that the client might well want to
use for their own management. The client should be using the trap
instead of the framework.
The framework might expose APIs to allow clients to register cleanups
etc., which the framework itself is then free to use of course, for
resources that it itself has acquired. But even with these APIs in place
I think it would be better if the client that acquires a resource also
schedules its release. (Though it's not as clear-cut in that case.)
>>
>> But I don't want to force this on your already large patchset :)
>
> Yes, Paolo also told me that this is too large. I will break it to
> 2 path set or merge some small patches together for next version.
>
>> So just ignore the bit about including from forwarding/lib.sh.
>
>> Actually I take this back. The cleanup should be invoked from where the
>> init was called. I don't think the library should be auto-invoking it,
>> the client scripts should. Whether through a trap or otherwise.
>
> OK, also makes sense. I will remove this trap.
>
> Thanks for all your comments.
> Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> On Fri, Nov 24, 2023 at 03:05:18PM +0100, Petr Machata wrote:
>>
>> Hangbin Liu writes:
>>
>> > +# Helpers
>> > +busywait()
>> > +{
>> > + local timeout=$1; shift
>> > +
>> > + local start_time="$(date -u +%s%3N)"
>> > + while true
>> > + do
>> > + local out
>> > + out=$($@)
>> > + local ret=$?
>> > + if ((!ret)); then
>> > + echo -n "$out"
>> > + return 0
>> > + fi
>> > +
>> > + local current_time="$(date -u +%s%3N)"
>> > + if ((current_time - start_time > timeout)); then
>> > + echo -n "$out"
>> > + return 1
>> > + fi
>> > + done
>> > +}
>>
>> This is lifted from forwarding/lib.sh, right? Would it make sense to
>
> Yes.
>
>> just source this new file from forwarding/lib.sh instead of copying
>
> Do you mean let net/forwarding/lib.sh source net.lib, and let other net
> tests source the net/forwarding/lib.sh?
>
> Or move the busywait() function from net/forwarding/lib.sh to net.lib.
> Then let net/forwarding/lib.sh source net.lib?
This.
>> stuff around? I imagine there will eventually be more commonality, and
>> when that pops up, we can just shuffle the forwarding code to
>> net/lib.sh.
>
> Yes, make sense.
>
> Thanks
> Hangbin
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Petr Machata writes:
> Hangbin Liu writes:
>
>> +# By default, remove all netns before EXIT.
>> +cleanup_all_ns()
>> +{
>> +cleanup_ns $NS_LIST
>> +}
>> +trap cleanup_all_ns EXIT
>
> Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
> because basically all users of forwarding/lib.sh use the EXIT trap.
[...]
> So just ignore the bit about including from forwarding/lib.sh.
Actually I take this back. The cleanup should be invoked from where the
init was called. I don't think the library should be auto-invoking it,
the client scripts should. Whether through a trap or otherwise.
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> +cleanup_ns()
> +{
> + local ns=""
> + local errexit=0
> +
> + # disable errexit temporary
> + if [[ $- =~ "e" ]]; then
> + errexit=1
> + set +e
> + fi
> +
> + for ns in "$@"; do
> + ip netns delete "${ns}" &> /dev/null
> + busywait 2 "ip netns list | grep -vq $1" &> /dev/null
The grep would get confused by substrings of other names.
This should be grep -vq "^$ns$".
> + if ip netns list | grep -q $1; then
Busywait returns != 0 when the wait condition is not reached within a
given time. So it should be possible to roll the duplicated if-grep into
the busywait line like so:
if ! busywait 2 "ip netns etc."; then
> + echo "Failed to remove namespace $1"
> + return $ksft_skip
This does not restore the errexit.
I think it might be clearest to have this function as a helper, say
__cleanup_ns, and then have a wrapper that does the errexit management:
cleanup_ns()
{
local errexit
local rc
# disable errexit temporarily
if [[ $- =~ "e" ]]; then
errexit=1
set +e
fi
__cleanup_ns "$@"
rc=$?
[ $errexit -eq 1 ] && set -e
return $rc
}
If this comes up more often, we can have a helper like
with_disabled_errexit or whatever, that does this management and
dispatches to "$@", so cleanup_ns() would become:
cleanup_ns()
{
with_disabled_errexit __cleanup_ns "$@"
}
> + fi
> + done
> +
> + [ $errexit -eq 1 ] && set -e
> + return 0
> +}
> +
> +# By default, remove all netns before EXIT.
> +cleanup_all_ns()
> +{
> + cleanup_ns $NS_LIST
> +}
> +trap cleanup_all_ns EXIT
Hmm, OK, this is a showstopper for inclusion from forwarding/lib.sh,
because basically all users of forwarding/lib.sh use the EXIT trap.
I wonder if we need something like these push_cleanup / on_exit helpers:
https://github.com/pmachata/stuff/blob/master/ptp-test/lib.sh#L15
But I don't want to force this on your already large patchset :)
So just ignore the bit about including from forwarding/lib.sh.
> +# setup netns with given names as prefix. e.g
> +# setup_ns local remote
> +setup_ns()
> +{
> + local ns=""
> + # the ns list we created in this call
> + local ns_list=""
> + while [ -n "$1" ]; do
I would find it more readable if this used the same iteration approach
as the 'for ns in "$@"' above. The $1/shift approach used here is
somewhat confusing.
> + # Some test may setup/remove same netns multi times
> + if unset $1 2> /dev/null; then
> + ns="${1,,}-$(mktemp -u XX)"
> + eval readonly $1=$ns
> + else
> + eval ns='$'$1
> + cleanup_ns $ns
> +
> + fi
> +
> + ip netns add $ns
> + if ! ip netns list | grep -q $ns; then
As above, the grep could get confused. But in fact wouldn't just
checking the exit code of ip netns add be enough?
> + echo "Failed to create namespace $1"
> + cleanup_ns $ns_list
> + return $ksft_skip
> + fi
> + ip -n $ns link set lo up
> + ns_list="$ns_list $ns"
> +
> + shift
> + done
> + NS_LIST="$NS_LIST $ns_list"
> +}
Re: [PATCH net-next 01/38] selftests/net: add lib.sh
Hangbin Liu writes:
> Add a lib.sh for net selftests. This file can be used to define commonly
> used variables and functions.
>
> Add function setup_ns() for user to create unique namespaces with given
> prefix name.
>
> Signed-off-by: Hangbin Liu
> ---
> tools/testing/selftests/net/Makefile | 2 +-
> tools/testing/selftests/net/lib.sh | 98
> 2 files changed, 99 insertions(+), 1 deletion(-)
> create mode 100644 tools/testing/selftests/net/lib.sh
>
> diff --git a/tools/testing/selftests/net/Makefile
> b/tools/testing/selftests/net/Makefile
> index 9274edfb76ff..14bd68da7466 100644
> --- a/tools/testing/selftests/net/Makefile
> +++ b/tools/testing/selftests/net/Makefile
> @@ -54,7 +54,7 @@ TEST_PROGS += ip_local_port_range.sh
> TEST_PROGS += rps_default_mask.sh
> TEST_PROGS += big_tcp.sh
> TEST_PROGS_EXTENDED := in_netns.sh setup_loopback.sh setup_veth.sh
> -TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh
> +TEST_PROGS_EXTENDED += toeplitz_client.sh toeplitz.sh lib.sh
> TEST_GEN_FILES = socket nettest
> TEST_GEN_FILES += psock_fanout psock_tpacket msg_zerocopy reuseport_addr_any
> TEST_GEN_FILES += tcp_mmap tcp_inq psock_snd txring_overwrite
> diff --git a/tools/testing/selftests/net/lib.sh
> b/tools/testing/selftests/net/lib.sh
> new file mode 100644
> index ..239ab2beb438
> --- /dev/null
> +++ b/tools/testing/selftests/net/lib.sh
> @@ -0,0 +1,98 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +##
> +# Defines
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +# namespace list created by setup_ns
> +NS_LIST=""
> +
> +##
> +# Helpers
> +busywait()
> +{
> + local timeout=$1; shift
> +
> + local start_time="$(date -u +%s%3N)"
> + while true
> + do
> + local out
> + out=$($@)
> + local ret=$?
> + if ((!ret)); then
> + echo -n "$out"
> + return 0
> + fi
> +
> + local current_time="$(date -u +%s%3N)"
> + if ((current_time - start_time > timeout)); then
> + echo -n "$out"
> + return 1
> + fi
> + done
> +}
This is lifted from forwarding/lib.sh, right? Would it make sense to
just source this new file from forwarding/lib.sh instead of copying
stuff around? I imagine there will eventually be more commonality, and
when that pops up, we can just shuffle the forwarding code to
net/lib.sh.
Re: [Bridge] [PATCH iproute2-next v5] iplink: bridge: Add support for bridge FDB learning limits
Johannes Nixdorf writes:
> Support setting the FDB limit through ip link. The arguments is:
> - fdb_max_learned: A 32-bit unsigned integer specifying the maximum
> number of learned FDB entries, with 0 disabling
> the limit.
>
> Also support reading back the current number of learned FDB entries in
> the bridge by this count. The returned value's name is:
> - fdb_n_learned: A 32-bit unsigned integer specifying the current number
> of learned FDB entries.
>
> Example:
>
> # ip -d -j -p link show br0
> [ {
> ...
> "linkinfo": {
> "info_kind": "bridge",
> "info_data": {
> ...
> "fdb_n_learned": 2,
> "fdb_max_learned": 0,
> ...
> }
> },
> ...
> } ]
> # ip link set br0 type bridge fdb_max_learned 1024
> # ip -d -j -p link show br0
> [ {
> ...
> "linkinfo": {
> "info_kind": "bridge",
> "info_data": {
> ...
> "fdb_n_learned": 2,
> "fdb_max_learned": 1024,
> ...
> }
> },
> ...
> } ]
>
> Signed-off-by: Johannes Nixdorf
Reviewed-by: Petr Machata
Re: [Bridge] [PATCH iproute2-next v3] iplink: bridge: Add support for bridge FDB learning limits
(I pruned the CC list, hopefully I didn't leave out anybody who cares.) Johannes Nixdorf via Bridge writes: > Support setting the FDB limit through ip link. The arguments is: > - fdb_max_learned_entries: A 32-bit unsigned integer specifying the > maximum number of learned FDB entries, with 0 > disabling the limit. ... > Signed-off-by: Johannes Nixdorf Code looks good to me. A couple points though: - The corresponding kernel changes have not yet been merged, have they? This patch should obviously only be merged after that happens. - The UAPI changes should not be part of the patch, the maintainers will update themselves. - The names fdb_n_learned_entries, fdb_max_learned_entries... they are somewhat unwieldy IMHO. Just for consideration, I don't feel strongly about this: Your kconfig option is named BRIDGE_DEFAULT_FDB_MAX_LEARNED, and that makes sense to me, because yeah, given the word "learned" in context of FDB, "entries" is the obvious continuation, so why mention it explicitly. Consider following suit with the other source artifacts -- the attribute names, struct fields, keywords in this patch. "fdb_n_learned", "fdb_max_learned" is IMHO just as understandable and will be easier to type.
[Bridge] [PATCH net-next 02/17] net: switchdev: Add a helper to replay objects on a bridge port
When a front panel joins a bridge via another netdevice (typically a LAG),
the driver needs to learn about the objects configured on the bridge port.
When the bridge port is offloaded by the driver for the first time, this
can be achieved by passing a notifier to switchdev_bridge_port_offload().
The notifier is then invoked for the individual objects (such as VLANs)
configured on the bridge, and can look for the interesting ones.
Calling switchdev_bridge_port_offload() when the second port joins the
bridge lower is unnecessary, but the replay is still needed. To that end,
add a new function, switchdev_bridge_port_replay(), which does only the
replay part of the _offload() function in exactly the same way as that
function.
Cc: Jiri Pirko
Cc: Ivan Vecera
Cc: Roopa Prabhu
Cc: Nikolay Aleksandrov
Cc: bridge@lists.linux-foundation.org
Signed-off-by: Petr Machata
Reviewed-by: Danielle Ratson
---
include/net/switchdev.h | 6 ++
net/bridge/br.c | 8
net/bridge/br_private.h | 16
net/bridge/br_switchdev.c | 9 +
net/switchdev/switchdev.c | 25 +
5 files changed, 64 insertions(+)
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index ca0312b78294..4d324e2a2eef 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -231,6 +231,7 @@ enum switchdev_notifier_type {
SWITCHDEV_BRPORT_OFFLOADED,
SWITCHDEV_BRPORT_UNOFFLOADED,
+ SWITCHDEV_BRPORT_REPLAY,
};
struct switchdev_notifier_info {
@@ -299,6 +300,11 @@ void switchdev_bridge_port_unoffload(struct net_device
*brport_dev,
const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb);
+int switchdev_bridge_port_replay(struct net_device *brport_dev,
+struct net_device *dev, const void *ctx,
+struct notifier_block *atomic_nb,
+struct notifier_block *blocking_nb,
+struct netlink_ext_ack *extack);
void switchdev_deferred_process(void);
int switchdev_port_attr_set(struct net_device *dev,
diff --git a/net/bridge/br.c b/net/bridge/br.c
index 4f5098d33a46..a6e94ceb7c9a 100644
--- a/net/bridge/br.c
+++ b/net/bridge/br.c
@@ -234,6 +234,14 @@ static int br_switchdev_blocking_event(struct
notifier_block *nb,
br_switchdev_port_unoffload(p, b->ctx, b->atomic_nb,
b->blocking_nb);
break;
+ case SWITCHDEV_BRPORT_REPLAY:
+ brport_info = ptr;
+ b = _info->brport;
+
+ err = br_switchdev_port_replay(p, b->dev, b->ctx, b->atomic_nb,
+ b->blocking_nb, extack);
+ err = notifier_from_errno(err);
+ break;
}
out:
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index a63b32c1638e..a69774131535 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -2115,6 +2115,12 @@ void br_switchdev_port_unoffload(struct net_bridge_port
*p, const void *ctx,
struct notifier_block *atomic_nb,
struct notifier_block *blocking_nb);
+int br_switchdev_port_replay(struct net_bridge_port *p,
+struct net_device *dev, const void *ctx,
+struct notifier_block *atomic_nb,
+struct notifier_block *blocking_nb,
+struct netlink_ext_ack *extack);
+
bool br_switchdev_frame_uses_tx_fwd_offload(struct sk_buff *skb);
void br_switchdev_frame_set_offload_fwd_mark(struct sk_buff *skb);
@@ -2165,6 +2171,16 @@ br_switchdev_port_unoffload(struct net_bridge_port *p,
const void *ctx,
{
}
+static inline int
+br_switchdev_port_replay(struct net_bridge_port *p,
+struct net_device *dev, const void *ctx,
+struct notifier_block *atomic_nb,
+struct notifier_block *blocking_nb,
+struct netlink_ext_ack *extack)
+{
+ return -EOPNOTSUPP;
+}
+
static inline bool br_switchdev_frame_uses_tx_fwd_offload(struct sk_buff *skb)
{
return false;
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index e92e0338afee..ee84e783e1df 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -829,3 +829,12 @@ void br_switchdev_port_unoffload(struct net_bridge_port
*p, const void *ctx,
nbp_switchdev_del(p);
}
+
+int br_switchdev_port_replay(struct net_bridge_port *p,
+struct net_device *dev, const void *ctx,
+struct notifier_block *atomic_nb,
+struct not
[Bridge] [PATCH net-next 01/17] net: bridge: br_switchdev: Tolerate -EOPNOTSUPP when replaying MDB
There are two kinds of MDB entries to be replayed: port MDB entries, and
host MDB entries. They are both replayed by br_switchdev_mdb_replay(). If
the driver supports one kind, but lacks the other, the first -EOPNOTSUPP
returned terminates the whole replay, including any further still-supported
objects in the list.
For this to cause issues, there must be MDB entries for both the host and
the port being replayed. In that case, if the driver bails out from
handling the host entry, the port entries are never replayed. However, the
replay is currently only done when a switchdev port joins a bridge. There
would be no port memberships at that point. Thus despite being erroneous,
the code does not cause observable bugs.
This is not an issue with other object kinds either, because there, each
function replays one object kind. If a driver does not support that kind,
it makes sense to bail out early. -EOPNOTSUPP is then ignored in
nbp_switchdev_sync_objs().
For MDB, suppress the -EOPNOTSUPP error code in br_switchdev_mdb_replay()
already, so that the whole list gets replayed.
The reason we need this patch is that a future patch will introduce a
replay that should be used when a front-panel port netdevice is enslaved to
a bridge lower, in particular a LAG. The LAG netdevice can already have
both host and port MDB entries. The port entries need to be replayed so
that they are offloaded on the port that joins the LAG.
Cc: Jiri Pirko
Cc: Ivan Vecera
Cc: Roopa Prabhu
Cc: Nikolay Aleksandrov
Cc: bridge@lists.linux-foundation.org
Signed-off-by: Petr Machata
Reviewed-by: Danielle Ratson
---
net/bridge/br_switchdev.c | 6 +-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_switchdev.c b/net/bridge/br_switchdev.c
index ba95c4d74a60..e92e0338afee 100644
--- a/net/bridge/br_switchdev.c
+++ b/net/bridge/br_switchdev.c
@@ -727,6 +727,8 @@ br_switchdev_mdb_replay(struct net_device *br_dev, struct
net_device *dev,
err = br_switchdev_mdb_replay_one(nb, dev,
SWITCHDEV_OBJ_PORT_MDB(obj),
action, ctx, extack);
+ if (err == -EOPNOTSUPP)
+ err = 0;
if (err)
goto out_free_mdb;
}
@@ -759,8 +761,10 @@ static int nbp_switchdev_sync_objs(struct net_bridge_port
*p, const void *ctx,
err = br_switchdev_mdb_replay(br_dev, dev, ctx, true, blocking_nb,
extack);
- if (err && err != -EOPNOTSUPP)
+ if (err) {
+ /* -EOPNOTSUPP not propagated from MDB replay. */
return err;
+ }
err = br_switchdev_fdb_replay(br_dev, ctx, true, atomic_nb);
if (err && err != -EOPNOTSUPP)
--
2.40.1
Re: [Bridge] llc needs namespace awareness asap, was Re: Patch fixing STP if bridge in non-default namespace.
(CC'ing bridge maintainers.)
Kuniyuki Iwashima writes:
> From: Harry Coin
> Date: Tue, 11 Jul 2023 16:40:03 -0500
>> On 7/11/23 15:44, Andrew Lunn wrote:
>> >> The current llc_rcv.c around line 166 in net/llc/llc_input.c has
>> >>
>> >>if (!net_eq(dev_net(dev), _net))
>> >>goto drop;
>> >>
>> >> Thank you! When you offer your patches, and you hear worries about being
>> >> 'invasive', it's worth asking 'compared to what' -- since the 'status quo'
>> >> is every bridge with STP in a non default namespace with a loop in it
>> >> somewhere will freeze every connected system more solid than ice in
>> >> Antarctica.
>> > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
>> >
>> > say:
>> >
>> > o It must be obviously correct and tested.
>> > o It cannot be bigger than 100 lines, with context.
>> > o It must fix only one thing.
>> > o It must fix a real bug that bothers people (not a, "This could be a
>> > problem..." type thing).
>> >
>> > git blame shows:
>> >
>> > commit 721499e8931c5732202481ae24f2dfbf9910f129
>> > Author: YOSHIFUJI Hideaki
>> > Date: Sat Jul 19 22:34:43 2008 -0700
>> >
>> > netns: Use net_eq() to compare net-namespaces for optimization.
>> >
>> > diff --git a/net/llc/llc_input.c b/net/llc/llc_input.c
>> > index 1c45f172991e..57ad974e4d94 100644
>> > --- a/net/llc/llc_input.c
>> > +++ b/net/llc/llc_input.c
>> > @@ -150,7 +150,7 @@ int llc_rcv(struct sk_buff *skb, struct net_device
>> > *dev,
>> > int (*rcv)(struct sk_buff *, struct net_device *,
>> > struct packet_type *, struct net_device *);
>> >
>> > - if (dev_net(dev) != _net)
>> > + if (!net_eq(dev_net(dev), _net))
>> > goto drop;
>> >
>> > /*
>> >
>> > So this is just an optimization.
>> >
>> > The test itself was added in
>> >
>> > ommit e730c15519d09ea528b4d2f1103681fa5937c0e6
>> > Author: Eric W. Biederman
>> > Date: Mon Sep 17 11:53:39 2007 -0700
>> >
>> > [NET]: Make packet reception network namespace safe
>> >
>> > This patch modifies every packet receive function
>> > registered with dev_add_pack() to drop packets if they
>> > are not from the initial network namespace.
>> >
>> > This should ensure that the various network stacks do
>> > not receive packets in a anything but the initial network
>> > namespace until the code has been converted and is ready
>> > for them.
>> >
>> > Signed-off-by: Eric W. Biederman
>> > Signed-off-by: David S. Miller
>> >
>> > So that was over 15 years ago.
>> >
>> > It appears it has not bothered people for over 15 years.
>> >
>> > Lets wait until we get to see the actual fix. We can then decide how
>> > simple/hard it is to back port to stable, if it fulfils the stable
>> > rules or not.
>> >
>> >Andrew
>>
>> Andrew, fair enough. In the time until it's fixed, the kernel folks
>> should publish an advisory and block any attempt to set bridge stp state
>> to other than 0 if in a non-default namespace. The alternative is a
>> packet flood at whatever the top line speed is should there be a loop
>> somewhere in even one connected link.
>
> Like this ? Someone who didn't notice the issue might complain about
> it as regression.
>
> ---8<---
> diff --git a/net/bridge/br_stp_if.c b/net/bridge/br_stp_if.c
> index 75204d36d7f9..a807996ac56b 100644
> --- a/net/bridge/br_stp_if.c
> +++ b/net/bridge/br_stp_if.c
> @@ -201,6 +201,11 @@ int br_stp_set_enabled(struct net_bridge *br, unsigned
> long val,
> {
> ASSERT_RTNL();
>
> + if (!net_eq(dev_net(br->dev), _net)) {
> + NL_SET_ERR_MSG_MOD(extack, "STP can't be enabled in non-root
> netns");
> + return -EINVAL;
> + }
> +
> if (br_mrp_enabled(br)) {
> NL_SET_ERR_MSG_MOD(extack,
> "STP can't be enabled if MRP is already
> enabled");
> ---8<---
[Bridge] [PATCH net-next v3 15/16] selftests: forwarding: lib: Add helpers to build IGMP/MLD leave packets
The testsuite that checks for mcast_max_groups functionality will need to
wipe the added groups as well. Add helpers to build an IGMP or MLD packets
announcing that host is leaving a given group.
Signed-off-by: Petr Machata
Acked-by: Nikolay Aleksandrov
---
tools/testing/selftests/net/forwarding/lib.sh | 50 +++
1 file changed, 50 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 12ef34ebcbbf..969e570f609e 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1815,6 +1815,21 @@ igmpv3_is_in_get()
payload_template_expand_checksum "$igmpv3" $checksum
}
+igmpv2_leave_get()
+{
+ local GRP=$1; shift
+
+ local payload=$(:
+ )"17:"$(: Type - Leave Group
+ )"00:"$(: Max Resp Time - not meaningful
+ )"CHECKSUM:"$( : Checksum
+ )"$(ipv4_to_bytes $GRP)"$( : Group Address
+ )
+ local checksum=$(payload_template_calc_checksum "$payload")
+
+ payload_template_expand_checksum "$payload" $checksum
+}
+
mldv2_is_in_get()
{
local SIP=$1; shift
@@ -1858,3 +1873,38 @@ mldv2_is_in_get()
payload_template_expand_checksum "$hbh$icmpv6" $checksum
}
+
+mldv1_done_get()
+{
+ local SIP=$1; shift
+ local GRP=$1; shift
+
+ local hbh
+ local icmpv6
+
+ hbh=$(:
+ )"3a:"$(: Next Header - ICMPv6
+ )"00:"$(: Hdr Ext Len
+ )"00:00:00:00:00:00:"$( : Options and Padding
+ )
+
+ icmpv6=$(:
+ )"84:"$(: Type - MLDv1 Done
+ )"00:"$(: Code
+ )"CHECKSUM:"$( : Checksum
+ )"00:00:"$( : Max Resp Delay - not
meaningful
+ )"00:00:"$( : Reserved
+ )"$(ipv6_to_bytes $GRP):"$( : Multicast address
+ )
+
+ local len=$(u16_to_bytes $(payload_template_nbytes $icmpv6))
+ local sudohdr=$(:
+ )"$(ipv6_to_bytes $SIP):"$( : SIP
+ )"$(ipv6_to_bytes $GRP):"$( : DIP is multicast address
+ )"${len}:"$(: Upper-layer length
+ )"00:3a:"$( : Zero and next-header
+ )
+ local checksum=$(payload_template_calc_checksum ${sudohdr}${icmpv6})
+
+ payload_template_expand_checksum "$hbh$icmpv6" $checksum
+}
--
2.39.0
[Bridge] [PATCH net-next v3 16/16] selftests: forwarding: bridge_mdb_max: Add a new selftest
Add a suite covering mcast_n_groups and mcast_max_groups bridge features.
Signed-off-by: Petr Machata
---
Notes:
v2:
- Adjust the tests that check setting max below n and
reset of max on VLAN snooping enablement
- Make test naming uniform
- Enable testing of control path (IGMP/MLD) in
mcast_vlan_snooping bridge
- Reorganize the code so that test instances (per bridge
type and configuration type) always come right after
the test, in order of {d,q,qvs}{4,6}{cfg,ctl}.
Then groups of selftests are at the end of the file.
Similarly adjust invocation order of the tests.
.../testing/selftests/net/forwarding/Makefile |1 +
.../net/forwarding/bridge_mdb_max.sh | 1336 +
2 files changed, 1337 insertions(+)
create mode 100755 tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
diff --git a/tools/testing/selftests/net/forwarding/Makefile
b/tools/testing/selftests/net/forwarding/Makefile
index 453ae006fbcf..91201ab3c4fc 100644
--- a/tools/testing/selftests/net/forwarding/Makefile
+++ b/tools/testing/selftests/net/forwarding/Makefile
@@ -4,6 +4,7 @@ TEST_PROGS = bridge_igmp.sh \
bridge_locked_port.sh \
bridge_mdb.sh \
bridge_mdb_host.sh \
+ bridge_mdb_max.sh \
bridge_mdb_port_down.sh \
bridge_mld.sh \
bridge_port_isolation.sh \
diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
b/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
new file mode 100755
index ..ae255b662ba3
--- /dev/null
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb_max.sh
@@ -0,0 +1,1336 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0
+
+# +---+ ++
+# | H1 (vrf) | | H2 (vrf) |
+# | + $h1.10 | | + $h2.10 |
+# | | 192.0.2.1/28| | | 192.0.2.2/28 |
+# | | 2001:db8:1::1/64| | | 2001:db8:1::2/64 |
+# | | | | | |
+# | | + $h1.20 | | | + $h2.20|
+# | \ | 198.51.100.1/24 | | \ | 198.51.100.2/24 |
+# | \ | 2001:db8:2::1/64 | | \ | 2001:db8:2::2/64 |
+# | \| | | \| |
+# |+ $h1 | |+ $h2 |
+# +|--+ +|---+
+# | |
+# +|--|---+
+# | SW | | |
+# | +--|--|-+ |
+# | | + $swp1 BR0 (802.1q) + $swp2 | |
+# | | vid 10 vid 10 | |
+# | | vid 20 vid 20 | |
+# | | | |
+# | +---+ |
+# +---+
+
+ALL_TESTS="
+ test_8021d
+ test_8021q
+ test_8021qvs
+"
+
+NUM_NETIFS=4
+source lib.sh
+source tc_common.sh
+
+h1_create()
+{
+ simple_if_init $h1
+ vlan_create $h1 10 v$h1 192.0.2.1/28 2001:db8:1::1/64
+ vlan_create $h1 20 v$h1 198.51.100.1/24 2001:db8:2::1/64
+}
+
+h1_destroy()
+{
+ vlan_destroy $h1 20
+ vlan_destroy $h1 10
+ simple_if_fini $h1
+}
+
+h2_create()
+{
+ simple_if_init $h2
+ vlan_create $h2 10 v$h2 192.0.2.2/28
+ vlan_create $h2 20 v$h2 198.51.100.2/24
+}
+
+h2_destroy()
+{
+ vlan_destroy $h2 20
+ vlan_destroy $h2 10
+ simple_if_fini $h2
+}
+
+switch_create_8021d()
+{
+ log_info "802.1d tests"
+
+ ip link add name br0 type bridge vlan_filtering 0 \
+ mcast_snooping 1 \
+ mcast_igmp_version 3 mcast_mld_version 2
+ ip link set dev br0 up
+
+ ip link set dev $swp1 master br0
+ ip link set dev $swp1 up
+ bridge link set dev $swp1 fastleave on
+
+ ip link set dev $swp2 master br0
+ ip link set dev $swp2 up
+}
+
+switch_create_8021q()
+{
+ local br_flags=$1; shift
+
+ log_info "802.1q $br_flags${br_flags:+ }tests"
+
+ ip link add name br0 type bridge vlan_filtering 1 vlan_default_pvid 0 \
+ mcast_snooping 1 $br_flags \
+ mcast_igmp_version 3 mcast_mld_version 2
+ bridge vlan add vid 10 dev br0 self
+ bridge vlan add vid 20 dev br0 se
[Bridge] [PATCH net-next v3 13/16] selftests: forwarding: lib: Parameterize IGMPv3/MLDv2 generation
In order to generate IGMPv3 and MLDv2 packets on the fly, the
functions that generate these packets need to be able to generate
packets for different groups and different sources. Generating MLDv2
packets further needs the source address of the packet for purposes of
checksum calculation. Add the necessary parameters, and generate the
payload accordingly by dispatching to helpers added in the previous
patches.
Adjust the sole client, bridge_mdb.sh, as well.
Signed-off-by: Petr Machata
Acked-by: Nikolay Aleksandrov
---
.../selftests/net/forwarding/bridge_mdb.sh| 9 ++---
tools/testing/selftests/net/forwarding/lib.sh | 36 +--
2 files changed, 31 insertions(+), 14 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 4e16677f02ba..b48867d8cadf 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -1029,7 +1029,7 @@ ctrl_igmpv3_is_in_test()
# IS_IN ( 192.0.2.2 )
$MZ $h1.10 -c 1 -A 192.0.2.1 -B 239.1.1.1 \
- -t ip proto=2,p=$(igmpv3_is_in_get) -q
+ -t ip proto=2,p=$(igmpv3_is_in_get 239.1.1.1 192.0.2.2) -q
bridge -d mdb show dev br0 vid 10 | grep 239.1.1.1 | grep -q 192.0.2.2
check_fail $? "Permanent entry affected by IGMP packet"
@@ -1042,7 +1042,7 @@ ctrl_igmpv3_is_in_test()
# IS_IN ( 192.0.2.2 )
$MZ $h1.10 -c 1 -A 192.0.2.1 -B 239.1.1.1 \
- -t ip proto=2,p=$(igmpv3_is_in_get) -q
+ -t ip proto=2,p=$(igmpv3_is_in_get 239.1.1.1 192.0.2.2) -q
bridge -d mdb show dev br0 vid 10 | grep 239.1.1.1 | grep -v "src" | \
grep -q 192.0.2.2
@@ -1067,8 +1067,9 @@ ctrl_mldv2_is_in_test()
filter_mode include source_list 2001:db8:1::1
# IS_IN ( 2001:db8:1::2 )
+ local p=$(mldv2_is_in_get fe80::1 ff0e::1 2001:db8:1::2)
$MZ -6 $h1.10 -c 1 -A fe80::1 -B ff0e::1 \
- -t ip hop=1,next=0,p=$(mldv2_is_in_get) -q
+ -t ip hop=1,next=0,p="$p" -q
bridge -d mdb show dev br0 vid 10 | grep ff0e::1 | \
grep -q 2001:db8:1::2
@@ -1082,7 +1083,7 @@ ctrl_mldv2_is_in_test()
# IS_IN ( 2001:db8:1::2 )
$MZ -6 $h1.10 -c 1 -A fe80::1 -B ff0e::1 \
- -t ip hop=1,next=0,p=$(mldv2_is_in_get) -q
+ -t ip hop=1,next=0,p="$p" -q
bridge -d mdb show dev br0 vid 10 | grep ff0e::1 | grep -v "src" | \
grep -q 2001:db8:1::2
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index b10c903d9abd..190e49e60508 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1788,26 +1788,35 @@ payload_template_nbytes()
igmpv3_is_in_get()
{
+ local GRP=$1; shift
+ local IP=$1; shift
+
local igmpv3
+ # IS_IN ( $IP )
igmpv3=$(:
)"22:"$(: Type - Membership Report
)"00:"$(: Reserved
- )"2a:f8:"$( : Checksum
+ )"CHECKSUM:"$( : Checksum
)"00:00:"$( : Reserved
)"00:01:"$( : Number of Group Records
)"01:"$(: Record Type - IS_IN
)"00:"$(: Aux Data Len
)"00:01:"$( : Number of Sources
- )"ef:01:01:01:"$( : Multicast Address - 239.1.1.1
- )"c0:00:02:02"$(: Source Address - 192.0.2.2
+ )"$(ipv4_to_bytes $GRP):"$( : Multicast Address
+ )"$(ipv4_to_bytes $IP)"$( : Source Address
)
+ local checksum=$(payload_template_calc_checksum "$igmpv3")
- echo $igmpv3
+ payload_template_expand_checksum "$igmpv3" $checksum
}
mldv2_is_in_get()
{
+ local SIP=$1; shift
+ local GRP=$1; shift
+ local IP=$1; shift
+
local hbh
local icmpv6
@@ -1820,17 +1829,24 @@ mldv2_is_in_get()
icmpv6=$(:
)"8f:"$(: Type - MLDv2 Report
)"00:"$(: Code
- )"45:39:"$( : Checksum
+ )"CHECKSUM:"$( : Checksum
)"00:00:"$( : Reserved
)"00:01:"$( : Number of Group Records
)"01:"$(:
[Bridge] [PATCH net-next v3 10/16] selftests: forwarding: bridge_mdb: Fix a typo
Add the letter missing from the word "INCLUDE". Signed-off-by: Petr Machata Reviewed-by: Ido Schimmel Acked-by: Nikolay Aleksandrov --- tools/testing/selftests/net/forwarding/bridge_mdb.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh index 51f2b0d77067..4e16677f02ba 100755 --- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh +++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh @@ -1054,7 +1054,7 @@ ctrl_igmpv3_is_in_test() bridge mdb del dev br0 port $swp1 grp 239.1.1.1 vid 10 - log_test "IGMPv3 MODE_IS_INCLUE tests" + log_test "IGMPv3 MODE_IS_INCLUDE tests" } ctrl_mldv2_is_in_test() -- 2.39.0
[Bridge] [PATCH net-next v3 14/16] selftests: forwarding: lib: Allow list of IPs for IGMPv3/MLDv2
The testsuite that checks for mcast_max_groups functionality will need
to generate IGMP and MLD packets with configurable number of (S,G)
addresses. To that end, further extend igmpv3_is_in_get() and
mldv2_is_in_get() to allow a list of IP addresses instead of one
address.
Signed-off-by: Petr Machata
Acked-by: Nikolay Aleksandrov
---
tools/testing/selftests/net/forwarding/lib.sh | 22 +--
1 file changed, 15 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 190e49e60508..12ef34ebcbbf 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1789,11 +1789,12 @@ payload_template_nbytes()
igmpv3_is_in_get()
{
local GRP=$1; shift
- local IP=$1; shift
+ local sources=("$@")
local igmpv3
+ local nsources=$(u16_to_bytes ${#sources[@]})
- # IS_IN ( $IP )
+ # IS_IN ( $sources )
igmpv3=$(:
)"22:"$(: Type - Membership Report
)"00:"$(: Reserved
@@ -1802,9 +1803,12 @@ igmpv3_is_in_get()
)"00:01:"$( : Number of Group Records
)"01:"$(: Record Type - IS_IN
)"00:"$(: Aux Data Len
- )"00:01:"$( : Number of Sources
+ )"${nsources}:"$( : Number of Sources
)"$(ipv4_to_bytes $GRP):"$( : Multicast Address
- )"$(ipv4_to_bytes $IP)"$( : Source Address
+ )"$(for src in "${sources[@]}"; do
+ ipv4_to_bytes $src
+ echo -n :
+ done)"$(: Source Addresses
)
local checksum=$(payload_template_calc_checksum "$igmpv3")
@@ -1815,10 +1819,11 @@ mldv2_is_in_get()
{
local SIP=$1; shift
local GRP=$1; shift
- local IP=$1; shift
+ local sources=("$@")
local hbh
local icmpv6
+ local nsources=$(u16_to_bytes ${#sources[@]})
hbh=$(:
)"3a:"$(: Next Header - ICMPv6
@@ -1834,9 +1839,12 @@ mldv2_is_in_get()
)"00:01:"$( : Number of Group Records
)"01:"$(: Record Type - IS_IN
)"00:"$(: Aux Data Len
- )"00:01:"$( : Number of Sources
+ )"${nsources}:"$( : Number of Sources
)"$(ipv6_to_bytes $GRP):"$( : Multicast address
- )"$(ipv6_to_bytes $IP):"$( : Source Address
+ )"$(for src in "${sources[@]}"; do
+ ipv6_to_bytes $src
+ echo -n :
+ done)"$(: Source Addresses
)
local len=$(u16_to_bytes $(payload_template_nbytes $icmpv6))
--
2.39.0
[Bridge] [PATCH net-next v3 09/16] selftests: forwarding: Move IGMP- and MLD-related functions to lib
These functions will be helpful for other testsuites as well. Extract them
to a common place.
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
.../selftests/net/forwarding/bridge_mdb.sh| 49 ---
tools/testing/selftests/net/forwarding/lib.sh | 49 +++
2 files changed, 49 insertions(+), 49 deletions(-)
diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 2fa5973c0c28..51f2b0d77067 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -1018,26 +1018,6 @@ fwd_test()
ip -6 address del fe80::1/64 dev br0
}
-igmpv3_is_in_get()
-{
- local igmpv3
-
- igmpv3=$(:
- )"22:"$(: Type - Membership Report
- )"00:"$(: Reserved
- )"2a:f8:"$( : Checksum
- )"00:00:"$( : Reserved
- )"00:01:"$( : Number of Group Records
- )"01:"$(: Record Type - IS_IN
- )"00:"$(: Aux Data Len
- )"00:01:"$( : Number of Sources
- )"ef:01:01:01:"$( : Multicast Address - 239.1.1.1
- )"c0:00:02:02"$(: Source Address - 192.0.2.2
- )
-
- echo $igmpv3
-}
-
ctrl_igmpv3_is_in_test()
{
RET=0
@@ -1077,35 +1057,6 @@ ctrl_igmpv3_is_in_test()
log_test "IGMPv3 MODE_IS_INCLUE tests"
}
-mldv2_is_in_get()
-{
- local hbh
- local icmpv6
-
- hbh=$(:
- )"3a:"$(: Next Header - ICMPv6
- )"00:"$(: Hdr Ext Len
- )"00:00:00:00:00:00:"$( : Options and Padding
- )
-
- icmpv6=$(:
- )"8f:"$(: Type - MLDv2 Report
- )"00:"$(: Code
- )"45:39:"$( : Checksum
- )"00:00:"$( : Reserved
- )"00:01:"$( : Number of Group Records
- )"01:"$(: Record Type - IS_IN
- )"00:"$(: Aux Data Len
- )"00:01:"$( : Number of Sources
- )"ff:0e:00:00:00:00:00:00:"$( : Multicast address - ff0e::1
- )"00:00:00:00:00:00:00:01:"$( :
- )"20:01:0d:b8:00:01:00:00:"$( : Source Address - 2001:db8:1::2
- )"00:00:00:00:00:00:00:02:"$( :
- )
-
- echo ${hbh}${icmpv6}
-}
-
ctrl_mldv2_is_in_test()
{
RET=0
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index ded967d204d3..0cfa0b699803 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1692,3 +1692,52 @@ hw_stats_monitor_test()
log_test "${type}_stats notifications"
}
+
+igmpv3_is_in_get()
+{
+ local igmpv3
+
+ igmpv3=$(:
+ )"22:"$(: Type - Membership Report
+ )"00:"$(: Reserved
+ )"2a:f8:"$( : Checksum
+ )"00:00:"$( : Reserved
+ )"00:01:"$( : Number of Group Records
+ )"01:"$(: Record Type - IS_IN
+ )"00:"$(: Aux Data Len
+ )"00:01:"$( : Number of Sources
+ )"ef:01:01:01:"$( : Multicast Address - 239.1.1.1
+ )"c0:00:02:02"$(: Source Address - 192.0.2.2
+ )
+
+ echo $igmpv3
+}
+
+mldv2_is_in_get()
+{
+ local hbh
+ local icmpv6
+
+ hbh=$(:
+ )"3a:"$(: Next Header - ICMPv6
+ )"00:"$(: Hdr Ext Len
+ )"00:00:00:00:00:00:"$( : Options and Padding
+ )
+
+ icmpv6=$(:
+ )"8f:"$(: Type - MLDv2 Report
+ )"00:"$(: Code
+ )"45:39:"$( : Checksum
+ )"
[Bridge] [PATCH net-next v3 12/16] selftests: forwarding: lib: Add helpers for checksum handling
In order to generate IGMPv3 and MLDv2 packets on the fly, we will need
helpers to calculate the packet checksum.
The approach presented in this patch revolves around payload templates
for mausezahn. These are mausezahn-like payload strings (01:23:45:...)
with possibly one 2-byte sequence replaced with the word PAYLOAD. The
main function is payload_template_calc_checksum(), which calculates
RFC 1071 checksum of the message. There are further helpers to then
convert the checksum to the payload format, and to expand it.
For IPv6, MLDv2 message checksum is computed using a pseudoheader that
differs from the header used in the payload itself. The fact that the
two messages are different means that the checksum needs to be
returned as a separate quantity, instead of being expanded in-place in
the payload itself. Furthermore, the pseudoheader includes a length of
the message. Much like the checksum, this needs to be expanded in
mausezahn format. And likewise for number of addresses for (S,G)
entries. Thus we have several places where a computed quantity needs
to be presented in the payload format. Add a helper u16_to_bytes(),
which will be used in all these cases.
Signed-off-by: Petr Machata
Acked-by: Nikolay Aleksandrov
---
Notes:
v2:
- In the comment at payload_template_calc_checksum(),
s/%#02x/%02x/, that's the mausezahn payload format.
tools/testing/selftests/net/forwarding/lib.sh | 56 +++
1 file changed, 56 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 409ff3799b55..b10c903d9abd 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1730,6 +1730,62 @@ ipv6_to_bytes()
expand_ipv6 "$IP" :
}
+u16_to_bytes()
+{
+ local u16=$1; shift
+
+ printf "%04x" $u16 | sed 's/^/000/;s/^.*\(..\)\(..\)$/\1:\2/'
+}
+
+# Given a mausezahn-formatted payload (colon-separated bytes given as %02x),
+# possibly with a keyword CHECKSUM stashed where a 16-bit checksum should be,
+# calculate checksum as per RFC 1071, assuming the CHECKSUM field (if any)
+# stands for 00:00.
+payload_template_calc_checksum()
+{
+ local payload=$1; shift
+
+ (
+ # Set input radix.
+ echo "16i"
+ # Push zero for the initial checksum.
+ echo 0
+
+ # Pad the payload with a terminating 00: in case we get an odd
+ # number of bytes.
+ echo "${payload%:}:00:" |
+ sed 's/CHECKSUM/00:00/g' |
+ tr '[:lower:]' '[:upper:]' |
+ # Add the word to the checksum.
+ sed 's/\(..\):\(..\):/\1\2+\n/g' |
+ # Strip the extra odd byte we pushed if left unconverted.
+ sed 's/\(..\):$//'
+
+ echo "1 ~ +"# Calculate and add carry.
+ echo " r - p" # Bit-flip and print.
+ ) |
+ dc |
+ tr '[:upper:]' '[:lower:]'
+}
+
+payload_template_expand_checksum()
+{
+ local payload=$1; shift
+ local checksum=$1; shift
+
+ local ckbytes=$(u16_to_bytes $checksum)
+
+ echo "$payload" | sed "s/CHECKSUM/$ckbytes/g"
+}
+
+payload_template_nbytes()
+{
+ local payload=$1; shift
+
+ payload_template_expand_checksum "${payload%:}" 0 |
+ sed 's/:/\n/g' | wc -l
+}
+
igmpv3_is_in_get()
{
local igmpv3
--
2.39.0
[Bridge] [PATCH net-next v3 11/16] selftests: forwarding: lib: Add helpers for IP address handling
In order to generate IGMPv3 and MLDv2 packets on the fly, we will need
helpers to expand IPv4 and IPv6 addresses given as parameters in
mausezahn payload notation. Add helpers that do it.
Signed-off-by: Petr Machata
Acked-by: Nikolay Aleksandrov
---
tools/testing/selftests/net/forwarding/lib.sh | 37 +++
1 file changed, 37 insertions(+)
diff --git a/tools/testing/selftests/net/forwarding/lib.sh
b/tools/testing/selftests/net/forwarding/lib.sh
index 0cfa0b699803..409ff3799b55 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -1693,6 +1693,43 @@ hw_stats_monitor_test()
log_test "${type}_stats notifications"
}
+ipv4_to_bytes()
+{
+ local IP=$1; shift
+
+ printf '%02x:' ${IP//./ } |
+ sed 's/:$//'
+}
+
+# Convert a given IPv6 address, `IP' such that the :: token, if present, is
+# expanded, and each 16-bit group is padded with zeroes to be 4 hexadecimal
+# digits. An optional `BYTESEP' parameter can be given to further separate
+# individual bytes of each 16-bit group.
+expand_ipv6()
+{
+ local IP=$1; shift
+ local bytesep=$1; shift
+
+ local cvt_ip=${IP/::/_}
+ local colons=${cvt_ip//[^:]/}
+ local allcol=:::
+ # IP where :: -> the appropriate number of colons:
+ local allcol_ip=${cvt_ip/_/${allcol:${#colons}}}
+
+ echo $allcol_ip | tr : '\n' |
+ sed s/^// |
+ sed 's/.*\(..\)\(..\)/\1'"$bytesep"'\2/' |
+ tr '\n' : |
+ sed 's/:$//'
+}
+
+ipv6_to_bytes()
+{
+ local IP=$1; shift
+
+ expand_ipv6 "$IP" :
+}
+
igmpv3_is_in_get()
{
local igmpv3
--
2.39.0
[Bridge] [PATCH net-next v3 08/16] net: bridge: Add netlink knobs for number / maximum MDB entries
The previous patch added accounting for number of MDB entries per port and
per port-VLAN, and the logic to verify that these values stay within
configured bounds. However it didn't provide means to actually configure
those bounds or read the occupancy. This patch does that.
Two new netlink attributes are added for the MDB occupancy:
IFLA_BRPORT_MCAST_N_GROUPS for the per-port occupancy and
BRIDGE_VLANDB_ENTRY_MCAST_N_GROUPS for the per-port-VLAN occupancy.
And another two for the maximum number of MDB entries:
IFLA_BRPORT_MCAST_MAX_GROUPS for the per-port maximum, and
BRIDGE_VLANDB_ENTRY_MCAST_MAX_GROUPS for the per-port-VLAN one.
Note that the two new IFLA_BRPORT_ attributes prompt bumping of
RTNL_SLAVE_MAX_TYPE to size the slave attribute tables large enough.
The new attributes are used like this:
# ip link add name br up type bridge vlan_filtering 1 mcast_snooping 1 \
mcast_vlan_snooping 1 mcast_querier 1
# ip link set dev v1 master br
# bridge vlan add dev v1 vid 2
# bridge vlan set dev v1 vid 1 mcast_max_groups 1
# bridge mdb add dev br port v1 grp 230.1.2.3 temp vid 1
# bridge mdb add dev br port v1 grp 230.1.2.4 temp vid 1
Error: bridge: Port-VLAN is already in 1 groups, and mcast_max_groups=1.
# bridge link set dev v1 mcast_max_groups 1
# bridge mdb add dev br port v1 grp 230.1.2.3 temp vid 2
Error: bridge: Port is already in 1 groups, and mcast_max_groups=1.
# bridge -d link show
5: v1@v2: mtu 1500 master br [...]
[...] mcast_n_groups 1 mcast_max_groups 1
# bridge -d vlan show
port vlan-id
br1 PVID Egress Untagged
state forwarding mcast_router 1
v11 PVID Egress Untagged
[...] mcast_n_groups 1 mcast_max_groups 1
2
[...] mcast_n_groups 0 mcast_max_groups 0
Signed-off-by: Petr Machata
---
Notes:
v3:
- Move the br_multicast_port_ctx_vlan_disabled() check
out to the _vlan_ helpers callers. Thus these helpers
cannot fail, which makes them very similar to the
_port_ helpers. Have them take the MC context directly
and unify them.
v2:
- Drop locks around accesses in
br_multicast_{port,vlan}_ngroups_{get,set_max}(),
- Drop bounces due to maxmulticast_ctx);
}
+u32 br_multicast_ngroups_get(const struct net_bridge_mcast_port *pmctx)
+{
+ return READ_ONCE(pmctx->mdb_n_entries);
+}
+
+void br_multicast_ngroups_set_max(struct net_bridge_mcast_port *pmctx, u32 max)
+{
+ WRITE_ONCE(pmctx->mdb_max_entries, max);
+}
+
+u32 br_multicast_ngroups_get_max(const struct net_bridge_mcast_port *pmctx)
+{
+ return READ_ONCE(pmctx->mdb_max_entries);
+}
+
static void br_multicast_destroy_port_group(struct net_bridge_mcast_gc *gc)
{
struct net_bridge_port_group *pg;
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index a6133d469885..9173e52b89e2 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -202,6 +202,8 @@ static inline size_t br_port_info_size(void)
+ nla_total_size_64bit(sizeof(u64)) /* IFLA_BRPORT_HOLD_TIMER */
#ifdef CONFIG_BRIDGE_IGMP_SNOOPING
+ nla_total_size(sizeof(u8))/* IFLA_BRPORT_MULTICAST_ROUTER
*/
+ + nla_total_size(sizeof(u32)) /* IFLA_BRPORT_MCAST_N_GROUPS */
+ + nla_total_size(sizeof(u32)) /* IFLA_BRPORT_MCAST_MAX_GROUPS
*/
#endif
+ nla_total_size(sizeof(u16)) /* IFLA_BRPORT_GROUP_FWD_MASK */
+ nla_total_size(sizeof(u8))/* IFLA_BRPORT_MRP_RING_OPEN */
@@ -298,7 +300,11 @@ static int br_port_fill_attrs(struct sk_buff *skb,
nla_put_u32(skb, IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT,
p->multicast_eht_hosts_limit) ||
nla_put_u32(skb, IFLA_BRPORT_MCAST_EHT_HOSTS_CNT,
- p->multicast_eht_hosts_cnt))
+ p->multicast_eht_hosts_cnt) ||
+ nla_put_u32(skb, IFLA_BRPORT_MCAST_N_GROUPS,
+ br_multicast_ngroups_get(>multicast_ctx)) ||
+ nla_put_u32(skb, IFLA_BRPORT_MCAST_MAX_GROUPS,
+ br_multicast_ngroups_get_max(>multicast_ctx)))
return -EMSGSIZE;
#endif
@@ -883,6 +889,8 @@ static const struct nla_policy
br_port_policy[IFLA_BRPORT_MAX + 1] = {
[IFLA_BRPORT_MAB] = { .type = NLA_U8 },
[IFLA_BRPORT_BACKUP_PORT] = { .type = NLA_U32 },
[IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT] = { .type = NLA_U32 },
+ [IFLA_BRPORT_MCAST_N_GROUPS] = { .type = NLA_REJECT },
+ [IFLA_BRPORT_MCAST_MAX_GROUPS] = { .type = NLA_U32 },
};
/* Change the state of the port and notify spanning tree */
@@ -1017,6 +1025,13 @@ static int br_setport(struct net_bridge_port *p, struct
nlattr *tb[],
if (err)
return err;
}
+
+ if (tb[IFLA_BRPORT_MCAST_MAX_GROUPS
[Bridge] [PATCH net-next v3 06/16] net: bridge: Add a tracepoint for MDB overflows
The following patch will add two more maximum MDB allowances to the global
one, mcast_hash_max, that exists today. In all these cases, attempts to add
MDB entries above the configured maximums through netlink, fail noisily and
obviously. Such visibility is missing when adding entries through the
control plane traffic, by IGMP or MLD packets.
To improve visibility in those cases, add a trace point that reports the
violation, including the relevant netdevice (be it a slave or the bridge
itself), and the MDB entry parameters:
# perf record -e bridge:br_mdb_full &
# [...]
# perf script | cut -d: -f4-
dev v2 af 2 src :::0.0.0.0 grp
:::239.1.1.112/00:00:00:00:00:00 vid 0
dev v2 af 10 src :: grp ff0e::112/00:00:00:00:00:00 vid 0
dev v2 af 2 src :::0.0.0.0 grp
:::239.1.1.112/00:00:00:00:00:00 vid 10
dev v2 af 10 src 2001:db8:1::1 grp ff0e::1/00:00:00:00:00:00 vid 10
dev v2 af 2 src :::192.0.2.1 grp
:::239.1.1.1/00:00:00:00:00:00 vid 10
CC: Steven Rostedt
CC: linux-trace-ker...@vger.kernel.org
Signed-off-by: Petr Machata
Reviewed-by: Steven Rostedt (Google)
---
Notes:
v2:
- Report IPv4 as an IPv6-mapped address through the IPv6 buffer
as well, to save ring buffer space.
include/trace/events/bridge.h | 58 +++
net/core/net-traces.c | 1 +
2 files changed, 59 insertions(+)
diff --git a/include/trace/events/bridge.h b/include/trace/events/bridge.h
index 6b200059c2c5..a6b3a4e409f0 100644
--- a/include/trace/events/bridge.h
+++ b/include/trace/events/bridge.h
@@ -122,6 +122,64 @@ TRACE_EVENT(br_fdb_update,
__entry->flags)
);
+TRACE_EVENT(br_mdb_full,
+
+ TP_PROTO(const struct net_device *dev,
+const struct br_ip *group),
+
+ TP_ARGS(dev, group),
+
+ TP_STRUCT__entry(
+ __string(dev, dev->name)
+ __field(int, af)
+ __field(u16, vid)
+ __array(__u8, src, 16)
+ __array(__u8, grp, 16)
+ __array(__u8, grpmac, ETH_ALEN) /* For af == 0. */
+ ),
+
+ TP_fast_assign(
+ struct in6_addr *in6;
+
+ __assign_str(dev, dev->name);
+ __entry->vid = group->vid;
+
+ if (!group->proto) {
+ __entry->af = 0;
+
+ memset(__entry->src, 0, sizeof(__entry->src));
+ memset(__entry->grp, 0, sizeof(__entry->grp));
+ memcpy(__entry->grpmac, group->dst.mac_addr, ETH_ALEN);
+ } else if (group->proto == htons(ETH_P_IP)) {
+ __entry->af = AF_INET;
+
+ in6 = (struct in6_addr *)__entry->src;
+ ipv6_addr_set_v4mapped(group->src.ip4, in6);
+
+ in6 = (struct in6_addr *)__entry->grp;
+ ipv6_addr_set_v4mapped(group->dst.ip4, in6);
+
+ memset(__entry->grpmac, 0, ETH_ALEN);
+
+#if IS_ENABLED(CONFIG_IPV6)
+ } else {
+ __entry->af = AF_INET6;
+
+ in6 = (struct in6_addr *)__entry->src;
+ *in6 = group->src.ip6;
+
+ in6 = (struct in6_addr *)__entry->grp;
+ *in6 = group->dst.ip6;
+
+ memset(__entry->grpmac, 0, ETH_ALEN);
+#endif
+ }
+ ),
+
+ TP_printk("dev %s af %u src %pI6c grp %pI6c/%pM vid %u",
+ __get_str(dev), __entry->af, __entry->src, __entry->grp,
+ __entry->grpmac, __entry->vid)
+);
#endif /* _TRACE_BRIDGE_H */
diff --git a/net/core/net-traces.c b/net/core/net-traces.c
index c40cd8dd75c7..c6820ad2183f 100644
--- a/net/core/net-traces.c
+++ b/net/core/net-traces.c
@@ -41,6 +41,7 @@ EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_add);
EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_external_learn_add);
EXPORT_TRACEPOINT_SYMBOL_GPL(fdb_delete);
EXPORT_TRACEPOINT_SYMBOL_GPL(br_fdb_update);
+EXPORT_TRACEPOINT_SYMBOL_GPL(br_mdb_full);
#endif
#if IS_ENABLED(CONFIG_PAGE_POOL)
--
2.39.0
[Bridge] [PATCH net-next v3 07/16] net: bridge: Maintain number of MDB entries in net_bridge_mcast_port
The MDB maintained by the bridge is limited. When the bridge is configured
for IGMP / MLD snooping, a buggy or malicious client can easily exhaust its
capacity. In SW datapath, the capacity is configurable through the
IFLA_BR_MCAST_HASH_MAX parameter, but ultimately is finite. Obviously a
similar limit exists in the HW datapath for purposes of offloading.
In order to prevent the issue of unilateral exhaustion of MDB resources,
introduce two parameters in each of two contexts:
- Per-port and per-port-VLAN number of MDB entries that the port
is member in.
- Per-port and (when BROPT_MCAST_VLAN_SNOOPING_ENABLED is enabled)
per-port-VLAN maximum permitted number of MDB entries, or 0 for
no limit.
The per-port multicast context is used for tracking of MDB entries for the
port as a whole. This is available for all bridges.
The per-port-VLAN multicast context is then only available on
VLAN-filtering bridges on VLANs that have multicast snooping on.
With these changes in place, it will be possible to configure MDB limit for
bridge as a whole, or any one port as a whole, or any single port-VLAN.
Note that unlike the global limit, exhaustion of the per-port and
per-port-VLAN maximums does not cause disablement of multicast snooping.
It is also permitted to configure the local limit larger than hash_max,
even though that is not useful.
In this patch, introduce only the accounting for number of entries, and the
max field itself, but not the means to toggle the max. The next patch
introduces the netlink APIs to toggle and read the values.
Signed-off-by: Petr Machata
---
Notes:
v3:
- Access mdb_max_/_n_entries through READ_/WRITE_ONCE
- Move extack setting to br_multicast_port_ngroups_inc_one().
Since we use NL_SET_ERR_MSG_FMT_MOD, the correct context
(port / port-vlan) can be passed through an argument.
This also removes the need for more READ/WRITE_ONCE's
at the extack-setting site.
v2:
- In br_multicast_port_ngroups_inc_one(), bounce
if n>=max, not if n==max
- Adjust extack messages to mention ngroups, now that
the bounces appear when n>=max, not n==max
- In __br_multicast_enable_port_ctx(), do not reset
max to 0. Also do not count number of entries by
going through _inc, as that would end up incorrectly
bouncing the entries.
net/bridge/br_multicast.c | 136 +-
net/bridge/br_private.h | 2 +
2 files changed, 137 insertions(+), 1 deletion(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 51b622afdb67..b6aa0bad5817 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -31,6 +31,7 @@
#include
#include
#endif
+#include
#include "br_private.h"
#include "br_private_mcast_eht.h"
@@ -234,6 +235,29 @@ br_multicast_pg_to_port_ctx(const struct
net_bridge_port_group *pg)
return pmctx;
}
+static struct net_bridge_mcast_port *
+br_multicast_port_vid_to_port_ctx(struct net_bridge_port *port, u16 vid)
+{
+ struct net_bridge_mcast_port *pmctx = NULL;
+ struct net_bridge_vlan *vlan;
+
+ lockdep_assert_held_once(>br->multicast_lock);
+
+ if (!br_opt_get(port->br, BROPT_MCAST_VLAN_SNOOPING_ENABLED))
+ return NULL;
+
+ /* Take RCU to access the vlan. */
+ rcu_read_lock();
+
+ vlan = br_vlan_find(nbp_vlan_group_rcu(port), vid);
+ if (vlan && !br_multicast_port_ctx_vlan_disabled(>port_mcast_ctx))
+ pmctx = >port_mcast_ctx;
+
+ rcu_read_unlock();
+
+ return pmctx;
+}
+
/* when snooping we need to check if the contexts should be used
* in the following order:
* - if pmctx is non-NULL (port), check if it should be used
@@ -668,6 +692,86 @@ void br_multicast_del_group_src(struct
net_bridge_group_src *src,
__br_multicast_del_group_src(src);
}
+static int
+br_multicast_port_ngroups_inc_one(struct net_bridge_mcast_port *pmctx,
+ struct netlink_ext_ack *extack,
+ const char *what)
+{
+ u32 max = READ_ONCE(pmctx->mdb_max_entries);
+ u32 n = READ_ONCE(pmctx->mdb_n_entries);
+
+ if (max && n >= max) {
+ NL_SET_ERR_MSG_FMT_MOD(extack, "%s is already in %u groups, and
mcast_max_groups=%u",
+ what, n, max);
+ return -E2BIG;
+ }
+
+ WRITE_ONCE(pmctx->mdb_n_entries, n + 1);
+ return 0;
+}
+
+static void br_multicast_port_ngroups_dec_one(struct net_bridge_mcast_port
*pmctx)
+{
+ u32 n = READ_ONCE(pmctx->mdb_n_entries);
+
+ WARN_ON_ONCE(n == 0);
+ WRITE_ONCE(pmctx->mdb_n_entries, n - 1);
+}
+
+static int br_multicast_port_ngroups_inc(struct net_bridge_port *port,
+const struct br_ip *group,
+struct
[Bridge] [PATCH net-next v3 04/16] net: bridge: Add br_multicast_del_port_group()
Since cleaning up the effects of br_multicast_new_port_group() just
consists of delisting and freeing the memory, the function
br_mdb_add_group_star_g() inlines the corresponding code. In the following
patches, number of per-port and per-port-VLAN MDB entries is going to be
maintained, and that counter will have to be updated. Because that logic
is going to be hidden in the br_multicast module, introduce a new hook
intended to again remove a newly-created group.
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
net/bridge/br_mdb.c | 3 +--
net/bridge/br_multicast.c | 11 +++
net/bridge/br_private.h | 1 +
3 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 139de8ac532c..9f22ebfdc518 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -1099,8 +1099,7 @@ static int br_mdb_add_group_star_g(const struct
br_mdb_config *cfg,
return 0;
err_del_port_group:
- hlist_del_init(>mglist);
- kfree(p);
+ br_multicast_del_port_group(p);
return err;
}
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index f9f4d54226fd..08da724ebfdd 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1326,6 +1326,17 @@ struct net_bridge_port_group
*br_multicast_new_port_group(
return p;
}
+void br_multicast_del_port_group(struct net_bridge_port_group *p)
+{
+ struct net_bridge_port *port = p->key.port;
+
+ hlist_del_init(>mglist);
+ if (!br_multicast_is_star_g(>key.addr))
+ rhashtable_remove_fast(>br->sg_port_tbl, >rhnode,
+ br_sg_port_rht_params);
+ kfree(p);
+}
+
void br_multicast_host_join(const struct net_bridge_mcast *brmctx,
struct net_bridge_mdb_entry *mp, bool notify)
{
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 1805c468ae03..e4069e27b5c6 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -958,6 +958,7 @@ br_multicast_new_port_group(struct net_bridge_port *port,
unsigned char flags, const unsigned char *src,
u8 filter_mode, u8 rt_protocol,
struct netlink_ext_ack *extack);
+void br_multicast_del_port_group(struct net_bridge_port_group *p);
int br_mdb_hash_init(struct net_bridge *br);
void br_mdb_hash_fini(struct net_bridge *br);
void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
--
2.39.0
[Bridge] [PATCH net-next v3 02/16] net: bridge: Add extack to br_multicast_new_port_group()
Make it possible to set an extack in br_multicast_new_port_group().
Eventually, this function will check for per-port and per-port-vlan
MDB maximums, and will use the extack to communicate the reason for
the bounce.
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
net/bridge/br_mdb.c | 5 +++--
net/bridge/br_multicast.c | 5 +++--
net/bridge/br_private.h | 3 ++-
3 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 00e5743647b0..069061366541 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -849,7 +849,7 @@ static int br_mdb_add_group_sg(const struct br_mdb_config
*cfg,
}
p = br_multicast_new_port_group(cfg->p, >group, *pp, flags, NULL,
- MCAST_INCLUDE, cfg->rt_protocol);
+ MCAST_INCLUDE, cfg->rt_protocol,
extack);
if (unlikely(!p)) {
NL_SET_ERR_MSG_MOD(extack, "Couldn't allocate new (S, G) port
group");
return -ENOMEM;
@@ -1075,7 +1075,8 @@ static int br_mdb_add_group_star_g(const struct
br_mdb_config *cfg,
}
p = br_multicast_new_port_group(cfg->p, >group, *pp, flags, NULL,
- cfg->filter_mode, cfg->rt_protocol);
+ cfg->filter_mode, cfg->rt_protocol,
+ extack);
if (unlikely(!p)) {
NL_SET_ERR_MSG_MOD(extack, "Couldn't allocate new (*, G) port
group");
return -ENOMEM;
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index dea1ee1bd095..de67d176838f 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1284,7 +1284,8 @@ struct net_bridge_port_group *br_multicast_new_port_group(
unsigned char flags,
const unsigned char *src,
u8 filter_mode,
- u8 rt_protocol)
+ u8 rt_protocol,
+ struct netlink_ext_ack *extack)
{
struct net_bridge_port_group *p;
@@ -1387,7 +1388,7 @@ __br_multicast_add_group(struct net_bridge_mcast *brmctx,
}
p = br_multicast_new_port_group(pmctx->port, group, *pp, 0, src,
- filter_mode, RTPROT_KERNEL);
+ filter_mode, RTPROT_KERNEL, NULL);
if (unlikely(!p)) {
p = ERR_PTR(-ENOMEM);
goto out;
diff --git a/net/bridge/br_private.h b/net/bridge/br_private.h
index 15ef7fd508ee..1805c468ae03 100644
--- a/net/bridge/br_private.h
+++ b/net/bridge/br_private.h
@@ -956,7 +956,8 @@ br_multicast_new_port_group(struct net_bridge_port *port,
const struct br_ip *group,
struct net_bridge_port_group __rcu *next,
unsigned char flags, const unsigned char *src,
- u8 filter_mode, u8 rt_protocol);
+ u8 filter_mode, u8 rt_protocol,
+ struct netlink_ext_ack *extack);
int br_mdb_hash_init(struct net_bridge *br);
void br_mdb_hash_fini(struct net_bridge *br);
void br_mdb_notify(struct net_device *dev, struct net_bridge_mdb_entry *mp,
--
2.39.0
[Bridge] [PATCH net-next v3 05/16] net: bridge: Change a cleanup in br_multicast_new_port_group() to goto
This function is getting more to clean up in the following patches.
Structuring the cleanups in one labeled block will allow reusing the same
cleanup from several places.
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
net/bridge/br_multicast.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index 08da724ebfdd..51b622afdb67 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1309,8 +1309,7 @@ struct net_bridge_port_group *br_multicast_new_port_group(
rhashtable_lookup_insert_fast(>br->sg_port_tbl, >rhnode,
br_sg_port_rht_params)) {
NL_SET_ERR_MSG_MOD(extack, "Couldn't insert new port group");
- kfree(p);
- return NULL;
+ goto free_out;
}
rcu_assign_pointer(p->next, next);
@@ -1324,6 +1323,10 @@ struct net_bridge_port_group
*br_multicast_new_port_group(
eth_broadcast_addr(p->eth_addr);
return p;
+
+free_out:
+ kfree(p);
+ return NULL;
}
void br_multicast_del_port_group(struct net_bridge_port_group *p)
--
2.39.0
[Bridge] [PATCH net-next v3 03/16] net: bridge: Move extack-setting to br_multicast_new_port_group()
Now that br_multicast_new_port_group() takes an extack argument, move
setting the extack there. The downside is that the error messages end
up being less specific (the function cannot distinguish between (S,G)
and (*,G) groups). However, the alternative is to check in the caller
whether the callee set the extack, and if it didn't, set it. But that
is only done when the callee is not exactly known. (E.g. in case of a
notifier invocation.)
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
net/bridge/br_mdb.c | 9 +++--
net/bridge/br_multicast.c | 5 -
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/bridge/br_mdb.c b/net/bridge/br_mdb.c
index 069061366541..139de8ac532c 100644
--- a/net/bridge/br_mdb.c
+++ b/net/bridge/br_mdb.c
@@ -850,10 +850,9 @@ static int br_mdb_add_group_sg(const struct br_mdb_config
*cfg,
p = br_multicast_new_port_group(cfg->p, >group, *pp, flags, NULL,
MCAST_INCLUDE, cfg->rt_protocol,
extack);
- if (unlikely(!p)) {
- NL_SET_ERR_MSG_MOD(extack, "Couldn't allocate new (S, G) port
group");
+ if (unlikely(!p))
return -ENOMEM;
- }
+
rcu_assign_pointer(*pp, p);
if (!(flags & MDB_PG_FLAGS_PERMANENT) && !cfg->src_entry)
mod_timer(>timer,
@@ -1077,10 +1076,8 @@ static int br_mdb_add_group_star_g(const struct
br_mdb_config *cfg,
p = br_multicast_new_port_group(cfg->p, >group, *pp, flags, NULL,
cfg->filter_mode, cfg->rt_protocol,
extack);
- if (unlikely(!p)) {
- NL_SET_ERR_MSG_MOD(extack, "Couldn't allocate new (*, G) port
group");
+ if (unlikely(!p))
return -ENOMEM;
- }
err = br_mdb_add_group_srcs(cfg, p, brmctx, extack);
if (err)
diff --git a/net/bridge/br_multicast.c b/net/bridge/br_multicast.c
index de67d176838f..f9f4d54226fd 100644
--- a/net/bridge/br_multicast.c
+++ b/net/bridge/br_multicast.c
@@ -1290,8 +1290,10 @@ struct net_bridge_port_group
*br_multicast_new_port_group(
struct net_bridge_port_group *p;
p = kzalloc(sizeof(*p), GFP_ATOMIC);
- if (unlikely(!p))
+ if (unlikely(!p)) {
+ NL_SET_ERR_MSG_MOD(extack, "Couldn't allocate new port group");
return NULL;
+ }
p->key.addr = *group;
p->key.port = port;
@@ -1306,6 +1308,7 @@ struct net_bridge_port_group *br_multicast_new_port_group(
if (!br_multicast_is_star_g(group) &&
rhashtable_lookup_insert_fast(>br->sg_port_tbl, >rhnode,
br_sg_port_rht_params)) {
+ NL_SET_ERR_MSG_MOD(extack, "Couldn't insert new port group");
kfree(p);
return NULL;
}
--
2.39.0
[Bridge] [PATCH net-next v3 01/16] net: bridge: Set strict_start_type at two policies
Make any attributes newly-added to br_port_policy or vlan_tunnel_policy
parsed strictly, to prevent userspace from passing garbage. Note that this
patchset only touches the former policy. The latter was adjusted for
completeness' sake. There do not appear to be other _deprecated calls
with non-NULL policies.
Suggested-by: Ido Schimmel
Signed-off-by: Petr Machata
Reviewed-by: Ido Schimmel
Acked-by: Nikolay Aleksandrov
---
net/bridge/br_netlink.c| 2 ++
net/bridge/br_netlink_tunnel.c | 3 +++
2 files changed, 5 insertions(+)
diff --git a/net/bridge/br_netlink.c b/net/bridge/br_netlink.c
index 4316cc82ae17..a6133d469885 100644
--- a/net/bridge/br_netlink.c
+++ b/net/bridge/br_netlink.c
@@ -858,6 +858,8 @@ static int br_afspec(struct net_bridge *br,
}
static const struct nla_policy br_port_policy[IFLA_BRPORT_MAX + 1] = {
+ [IFLA_BRPORT_UNSPEC]= { .strict_start_type =
+ IFLA_BRPORT_MCAST_EHT_HOSTS_LIMIT + 1 },
[IFLA_BRPORT_STATE] = { .type = NLA_U8 },
[IFLA_BRPORT_COST] = { .type = NLA_U32 },
[IFLA_BRPORT_PRIORITY] = { .type = NLA_U16 },
diff --git a/net/bridge/br_netlink_tunnel.c b/net/bridge/br_netlink_tunnel.c
index 8914290c75d4..17abf092f7ca 100644
--- a/net/bridge/br_netlink_tunnel.c
+++ b/net/bridge/br_netlink_tunnel.c
@@ -188,6 +188,9 @@ int br_fill_vlan_tunnel_info(struct sk_buff *skb,
}
static const struct nla_policy vlan_tunnel_policy[IFLA_BRIDGE_VLAN_TUNNEL_MAX
+ 1] = {
+ [IFLA_BRIDGE_VLAN_TUNNEL_UNSPEC] = {
+ .strict_start_type = IFLA_BRIDGE_VLAN_TUNNEL_FLAGS + 1
+ },
[IFLA_BRIDGE_VLAN_TUNNEL_ID] = { .type = NLA_U32 },
[IFLA_BRIDGE_VLAN_TUNNEL_VID] = { .type = NLA_U16 },
[IFLA_BRIDGE_VLAN_TUNNEL_FLAGS] = { .type = NLA_U16 },
--
2.39.0
