[PATCH net-next 0/5] selftests: forwarding: misc bug fixes and enhancements

Bug fixes and an enhancement for the recent forwarding tests:
- only check tc version on tc tests
- only check for mz for tests that will use it
- handle multipath tests failing with 0 packet count
- fix ping command for IPv6 on Debian jessie
- improve summary of multipath tests

BTW: who is the cruel person that made whitespace 7 spaces in these files?

David Ahern (5):
  selftests: forwarding: Only check tc version for tc tests
  selftests: forwarding: Only check for mz when it is needed
  selftests: forwarding: Handle 0 for packet difference in multipath
    tests
  selftests: forwarding: Use PING6 instead of ping for ipv6 multipath
    test
  selftests: forwarding: Add description to the multipath tests

 tools/testing/selftests/net/forwarding/lib.sh      | 37 ++++++++++-------
 .../selftests/net/forwarding/router_multipath.sh   | 47 +++++++++++++---------
 .../testing/selftests/net/forwarding/tc_actions.sh |  2 +-
 .../testing/selftests/net/forwarding/tc_chains.sh  |  2 +-
 .../testing/selftests/net/forwarding/tc_common.sh  |  3 ++
 .../testing/selftests/net/forwarding/tc_flower.sh  |  2 +-
 .../selftests/net/forwarding/tc_shblocks.sh        |  2 +-
 7 files changed, 59 insertions(+), 36 deletions(-)

-- 
2.11.0

Capabilities of tc command are irrelevant for router tests:
    $ ./router.sh
    SKIP: iproute2 too old, missing shared block support

Add a CHECK_TC flag and only check tc capabilities if set. Add flag to
tc_common.sh and have it sourced before lib.sh

Also, if the command lacks some feature the test should exit non-0.

Signed-off-by: David Ahern <redacted>
---
 tools/testing/selftests/net/forwarding/lib.sh      | 29 ++++++++++++++--------
 .../testing/selftests/net/forwarding/tc_actions.sh |  2 +-
 .../testing/selftests/net/forwarding/tc_chains.sh  |  2 +-
 .../testing/selftests/net/forwarding/tc_common.sh  |  2 ++
 .../testing/selftests/net/forwarding/tc_flower.sh  |  2 +-
 .../selftests/net/forwarding/tc_shblocks.sh        |  2 +-
 6 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 23866a685f77..d0af52109360 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh

@@ -19,26 +19,33 @@ fi
 ##############################################################################
 # Sanity checks
 
+check_tc_version()
+{
+	tc -j &> /dev/null
+	if [[ $? -ne 0 ]]; then
+		echo "SKIP: iproute2 too old; tc is missing JSON support"
+		exit 1
+	fi
+
+	tc filter help 2>&1 | grep block &> /dev/null
+	if [[ $? -ne 0 ]]; then
+		echo "SKIP: iproute2 too old; tc is missing shared block support"
+		exit 1
+	fi
+}
+
 if [[ "$(id -u)" -ne 0 ]]; then
 	echo "SKIP: need root privileges"
 	exit 0
 fi
 
-tc -j &> /dev/null
-if [[ $? -ne 0 ]]; then
-	echo "SKIP: iproute2 too old, missing JSON support"
-	exit 0
-fi
-
-tc filter help 2>&1 | grep block &> /dev/null
-if [[ $? -ne 0 ]]; then
-	echo "SKIP: iproute2 too old, missing shared block support"
-	exit 0
+if [[ "$CHECK_TC" = "yes" ]]; then
+	check_tc_version
 fi
 
 if [[ ! -x "$(command -v jq)" ]]; then
 	echo "SKIP: jq not installed"
-	exit 0
+	exit 1
 fi
 
 if [[ ! -x "$(command -v $MZ)" ]]; then

diff --git a/tools/testing/selftests/net/forwarding/tc_actions.sh b/tools/testing/selftests/net/forwarding/tc_actions.sh
index 84234317a25d..8ab5cf0a960b 100755
--- a/tools/testing/selftests/net/forwarding/tc_actions.sh
+++ b/tools/testing/selftests/net/forwarding/tc_actions.sh

@@ -2,8 +2,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
 NUM_NETIFS=4
-source lib.sh
 source tc_common.sh
+source lib.sh
 
 tcflags="skip_hw"
 

diff --git a/tools/testing/selftests/net/forwarding/tc_chains.sh b/tools/testing/selftests/net/forwarding/tc_chains.sh
index 94c114ad8b44..2fd15226974b 100755
--- a/tools/testing/selftests/net/forwarding/tc_chains.sh
+++ b/tools/testing/selftests/net/forwarding/tc_chains.sh

@@ -2,8 +2,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
 NUM_NETIFS=2
-source lib.sh
 source tc_common.sh
+source lib.sh
 
 tcflags="skip_hw"
 

diff --git a/tools/testing/selftests/net/forwarding/tc_common.sh b/tools/testing/selftests/net/forwarding/tc_common.sh
index acd0b520241c..9d3b64a2a264 100644
--- a/tools/testing/selftests/net/forwarding/tc_common.sh
+++ b/tools/testing/selftests/net/forwarding/tc_common.sh

@@ -1,6 +1,8 @@
 #!/bin/bash
 # SPDX-License-Identifier: GPL-2.0
 
+CHECK_TC="yes"
+
 tc_check_packets()
 {
 	local id=$1

diff --git a/tools/testing/selftests/net/forwarding/tc_flower.sh b/tools/testing/selftests/net/forwarding/tc_flower.sh
index 026a4ea4b2fb..032b882adfc0 100755
--- a/tools/testing/selftests/net/forwarding/tc_flower.sh
+++ b/tools/testing/selftests/net/forwarding/tc_flower.sh

@@ -2,8 +2,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
 NUM_NETIFS=2
-source lib.sh
 source tc_common.sh
+source lib.sh
 
 tcflags="skip_hw"
 

diff --git a/tools/testing/selftests/net/forwarding/tc_shblocks.sh b/tools/testing/selftests/net/forwarding/tc_shblocks.sh
index cfc8a2ace388..077b98048ef4 100755
--- a/tools/testing/selftests/net/forwarding/tc_shblocks.sh
+++ b/tools/testing/selftests/net/forwarding/tc_shblocks.sh

@@ -2,8 +2,8 @@
 # SPDX-License-Identifier: GPL-2.0
 
 NUM_NETIFS=4
-source lib.sh
 source tc_common.sh
+source lib.sh
 
 tcflags="skip_hw"
 

-- 
2.11.0

Add a CHECK_MZ flag and only check if mz exists when flag is set.
If it does not exist, exit non-0.

Signed-off-by: David Ahern <redacted>
---
 tools/testing/selftests/net/forwarding/lib.sh              | 8 +++++---
 tools/testing/selftests/net/forwarding/router_multipath.sh | 1 +
 tools/testing/selftests/net/forwarding/tc_common.sh        | 1 +
 3 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index d0af52109360..f21b3ac11d27 100644
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh

@@ -48,9 +48,11 @@ if [[ ! -x "$(command -v jq)" ]]; then
 	exit 1
 fi
 
-if [[ ! -x "$(command -v $MZ)" ]]; then
-	echo "SKIP: $MZ not installed"
-	exit 0
+if [[ "$CHECK_MZ" = "yes" ]]; then
+	if [[ ! -x "$(command -v $MZ)" ]]; then
+		echo "SKIP: $MZ not installed"
+		exit 1
+	fi
 fi
 
 if [[ ! -v NUM_NETIFS ]]; then

diff --git a/tools/testing/selftests/net/forwarding/router_multipath.sh b/tools/testing/selftests/net/forwarding/router_multipath.sh
index d31888e3133e..d40016443272 100755
--- a/tools/testing/selftests/net/forwarding/router_multipath.sh
+++ b/tools/testing/selftests/net/forwarding/router_multipath.sh

@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 NUM_NETIFS=8
+CHECK_MZ="yes"
 source lib.sh
 
 h1_create()

diff --git a/tools/testing/selftests/net/forwarding/tc_common.sh b/tools/testing/selftests/net/forwarding/tc_common.sh
index 9d3b64a2a264..d93cd7599bc1 100644
--- a/tools/testing/selftests/net/forwarding/tc_common.sh
+++ b/tools/testing/selftests/net/forwarding/tc_common.sh

@@ -2,6 +2,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 CHECK_TC="yes"
+CHECK_MZ="yes"
 
 tc_check_packets()
 {

-- 
2.11.0

If the packet stats have a difference of 0, the test output shows:
INFO: Expected ratio 2.00 Measured ratio
Runtime error (func=(main), adr=9): Divide by zero
(standard_in) 2: syntax error
(standard_in) 1: syntax error
./router_multipath.sh: line 187: test: : integer expression expected
TEST: Multipath                                                     [FAIL]
	Too large discrepancy between expected and measured ratios

Handle the 0 and display a cleaner message:
INFO: Running IPv6 multipath tests
TEST: Multipath                                                     [FAIL]
	Packet difference is 0

Signed-off-by: David Ahern <redacted>
---
 tools/testing/selftests/net/forwarding/router_multipath.sh | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/testing/selftests/net/forwarding/router_multipath.sh b/tools/testing/selftests/net/forwarding/router_multipath.sh
index d40016443272..9c66b3fbf994 100755
--- a/tools/testing/selftests/net/forwarding/router_multipath.sh
+++ b/tools/testing/selftests/net/forwarding/router_multipath.sh

@@ -169,6 +169,13 @@ multipath_eval()
 
        RET=0
 
+       if [[ "$packets_rp12" -eq "0" || "$packets_rp13" -eq "0" ]]; then
+              check_err 1 "Packet difference is 0"
+              log_test "Multipath"
+              log_info "Expected ratio $weights_ratio"
+              return
+       fi
+
        if [[ "$weight_rp12" -gt "$weight_rp13" ]]; then
                weights_ratio=$(echo "scale=2; $weight_rp12 / $weight_rp13" \
 		       | bc -l)

-- 
2.11.0

On Debian jessie ping can not handle IPv6 addresses so the command
fails. Use PING6 which is set to ping6.

Signed-off-by: David Ahern <redacted>
---
 tools/testing/selftests/net/forwarding/router_multipath.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/router_multipath.sh b/tools/testing/selftests/net/forwarding/router_multipath.sh
index 9c66b3fbf994..e227ec052804 100755
--- a/tools/testing/selftests/net/forwarding/router_multipath.sh
+++ b/tools/testing/selftests/net/forwarding/router_multipath.sh

@@ -250,7 +250,7 @@ multipath6_test()
 
        # Generate 16384 echo requests, each with a random flow label.
        for _ in $(seq 1 16384); do
-	       ip vrf exec vrf-h1 ping 2001:db8:2::2 -F 0 -c 1 -q &> /dev/null
+	       ip vrf exec vrf-h1 $PING6 2001:db8:2::2 -F 0 -c 1 -q &> /dev/null
        done
 
        t1_rp12=$(link_stats_tx_packets_get $rp12)

-- 
2.11.0

Add a better description to the summary for multipath tests. e.g.,

INFO: Running IPv6 multipath tests
TEST: Multipath: ECMP                                               [PASS]
INFO: Expected ratio 1.00 Measured ratio 1.02
TEST: Multipath: Weighted MP 2:1                                    [PASS]
INFO: Expected ratio 2.00 Measured ratio 2.02
TEST: Multipath: Weighted MP 11:45                                  [PASS]
INFO: Expected ratio 4.09 Measured ratio 4.03

Signed-off-by: David Ahern <redacted>
---
 .../selftests/net/forwarding/router_multipath.sh   | 37 ++++++++++++----------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/tools/testing/selftests/net/forwarding/router_multipath.sh b/tools/testing/selftests/net/forwarding/router_multipath.sh
index e227ec052804..f03f628b6dd6 100755
--- a/tools/testing/selftests/net/forwarding/router_multipath.sh
+++ b/tools/testing/selftests/net/forwarding/router_multipath.sh

@@ -161,10 +161,11 @@ router2_destroy()
 
 multipath_eval()
 {
-       local weight_rp12=$1
-       local weight_rp13=$2
-       local packets_rp12=$3
-       local packets_rp13=$4
+       local desc="$1"
+       local weight_rp12=$2
+       local weight_rp13=$3
+       local packets_rp12=$4
+       local packets_rp13=$5
        local weights_ratio packets_ratio diff
 
        RET=0

@@ -193,14 +194,15 @@ multipath_eval()
 
        test "$(echo "$diff / $weights_ratio > 0.1" | bc -l)" -eq 0
        check_err $? "Too large discrepancy between expected and measured ratios"
-       log_test "Multipath"
+       log_test "$desc"
        log_info "Expected ratio $weights_ratio Measured ratio $packets_ratio"
 }
 
 multipath4_test()
 {
-       local weight_rp12=$1
-       local weight_rp13=$2
+       local desc="$1"
+       local weight_rp12=$2
+       local weight_rp13=$3
        local t0_rp12 t0_rp13 t1_rp12 t1_rp13
        local packets_rp12 packets_rp13
        local hash_policy

@@ -225,7 +227,7 @@ multipath4_test()
 
        let "packets_rp12 = $t1_rp12 - $t0_rp12"
        let "packets_rp13 = $t1_rp13 - $t0_rp13"
-       multipath_eval $weight_rp12 $weight_rp13 $packets_rp12 $packets_rp13
+       multipath_eval "$desc" $weight_rp12 $weight_rp13 $packets_rp12 $packets_rp13
 
        # Restore settings.
        ip route replace 198.51.100.0/24 vrf vrf-r1 \

@@ -236,8 +238,9 @@ multipath4_test()
 
 multipath6_test()
 {
-       local weight_rp12=$1
-       local weight_rp13=$2
+       local desc="$1"
+       local weight_rp12=$2
+       local weight_rp13=$3
        local t0_rp12 t0_rp13 t1_rp12 t1_rp13
        local packets_rp12 packets_rp13
 

@@ -258,7 +261,7 @@ multipath6_test()
 
        let "packets_rp12 = $t1_rp12 - $t0_rp12"
        let "packets_rp13 = $t1_rp13 - $t0_rp13"
-       multipath_eval $weight_rp12 $weight_rp13 $packets_rp12 $packets_rp13
+       multipath_eval "$desc" $weight_rp12 $weight_rp13 $packets_rp12 $packets_rp13
 
        ip route replace 2001:db8:2::/64 vrf vrf-r1 \
 	       nexthop via fe80:2::22 dev $rp12 \

@@ -268,14 +271,14 @@ multipath6_test()
 multipath_test()
 {
 	log_info "Running IPv4 multipath tests"
-	multipath4_test 1 1
-	multipath4_test 2 1
-	multipath4_test 11 45
+	multipath4_test "ECMP" 1 1
+	multipath4_test "Weighted MP 2:1" 2 1
+	multipath4_test "Weighted MP 11:45" 11 45
 
 	log_info "Running IPv6 multipath tests"
-	multipath6_test 1 1
-	multipath6_test 2 1
-	multipath6_test 11 45
+	multipath6_test "ECMP" 1 1
+	multipath6_test "Weighted MP 2:1" 2 1
+	multipath6_test "Weighted MP 11:45" 11 45
 }
 
 setup_prepare()

-- 
2.11.0

On Thu, Mar 01, 2018 at 11:09:42AM -0800, David Ahern wrote:

Bug fixes and an enhancement for the recent forwarding tests:
- only check tc version on tc tests
- only check for mz for tests that will use it
- handle multipath tests failing with 0 packet count
- fix ping command for IPv6 on Debian jessie
- improve summary of multipath tests

BTW: who is the cruel person that made whitespace 7 spaces in these files?

I see this in some functions in router_multipath.sh . Not sure how that
happened... Will be more careful in the future.

On Thu, Mar 01, 2018 at 11:09:44AM -0800, David Ahern wrote:

Add a CHECK_MZ flag and only check if mz exists when flag is set.
If it does not exist, exit non-0.

Signed-off-by: David Ahern <redacted>

The only test that doesn't require it is router.sh so I'm not sure it's
worth the effort. If you still want to keep the patch then
bridge_vlan_aware.sh needs to be patched as well.

On Thu, Mar 01, 2018 at 11:09:45AM -0800, David Ahern wrote:

If the packet stats have a difference of 0, the test output shows:
INFO: Expected ratio 2.00 Measured ratio
Runtime error (func=(main), adr=9): Divide by zero
(standard_in) 2: syntax error
(standard_in) 1: syntax error
./router_multipath.sh: line 187: test: : integer expression expected
TEST: Multipath                                                     [FAIL]
	Too large discrepancy between expected and measured ratios

Handle the 0 and display a cleaner message:
INFO: Running IPv6 multipath tests
TEST: Multipath                                                     [FAIL]
	Packet difference is 0

Signed-off-by: David Ahern <redacted>

Reviewed-by: Ido Schimmel <redacted>

Thanks!

On Thu, Mar 01, 2018 at 11:09:46AM -0800, David Ahern wrote:

On Debian jessie ping can not handle IPv6 addresses so the command
fails. Use PING6 which is set to ping6.

Signed-off-by: David Ahern <redacted>

Yep, I forgot to replace that one.

Reviewed-by: Ido Schimmel <redacted>

BTW, I have a patch to convert the IPv6 multipath test to use mz. I'm
waiting for your L4 hash patches to be accepted :)

On Thu, Mar 01, 2018 at 11:09:47AM -0800, David Ahern wrote:

Add a better description to the summary for multipath tests. e.g.,

INFO: Running IPv6 multipath tests
TEST: Multipath: ECMP                                               [PASS]
INFO: Expected ratio 1.00 Measured ratio 1.02
TEST: Multipath: Weighted MP 2:1                                    [PASS]
INFO: Expected ratio 2.00 Measured ratio 2.02
TEST: Multipath: Weighted MP 11:45                                  [PASS]
INFO: Expected ratio 4.09 Measured ratio 4.03

Signed-off-by: David Ahern <redacted>

I tried to make the output as nice as the tests you sent me, but I
failed. Happy you pitched in :)

Reviewed-by: Ido Schimmel <redacted>

On 3/1/18 12:39 PM, Ido Schimmel wrote:

On Thu, Mar 01, 2018 at 11:09:46AM -0800, David Ahern wrote:

quoted

On Debian jessie ping can not handle IPv6 addresses so the command
fails. Use PING6 which is set to ping6.

Signed-off-by: David Ahern <redacted>

Yep, I forgot to replace that one.

Reviewed-by: Ido Schimmel <redacted>

BTW, I have a patch to convert the IPv6 multipath test to use mz. I'm
waiting for your L4 hash patches to be accepted :)

Can you forward it? That is what is driving this series.

On Thu, Mar 01, 2018 at 12:43:42PM -0700, David Ahern wrote:

On 3/1/18 12:39 PM, Ido Schimmel wrote:

quoted

On Thu, Mar 01, 2018 at 11:09:46AM -0800, David Ahern wrote:

quoted

On Debian jessie ping can not handle IPv6 addresses so the command
fails. Use PING6 which is set to ping6.

Signed-off-by: David Ahern <redacted>

Yep, I forgot to replace that one.

Reviewed-by: Ido Schimmel <redacted>

BTW, I have a patch to convert the IPv6 multipath test to use mz. I'm
waiting for your L4 hash patches to be accepted :)

Can you forward it? That is what is driving this series.

Feel free to submit with your patches (sorry about the spaces!):

diff --git a/tools/testing/selftests/net/forwarding/router_multipath.sh b/tools/testing/selftests/net/forwarding/router_multipath.sh
index d31888e3133e..3ebd9cca06d0 100755
--- a/tools/testing/selftests/net/forwarding/router_multipath.sh
+++ b/tools/testing/selftests/net/forwarding/router_multipath.sh

@@ -232,7 +232,10 @@ multipath6_test()
        local weight_rp13=$2
        local t0_rp12 t0_rp13 t1_rp12 t1_rp13
        local packets_rp12 packets_rp13
+       local hash_policy
 
+       hash_policy=$(sysctl -n net.ipv6.fib_multipath_hash_policy)
+       sysctl -q -w net.ipv6.fib_multipath_hash_policy=1
        ip route replace 2001:db8:2::/64 vrf vrf-r1 \
 	       nexthop via fe80:2::22 dev $rp12 weight $weight_rp12 \
 	       nexthop via fe80:3::23 dev $rp13 weight $weight_rp13

@@ -240,10 +243,8 @@ multipath6_test()
        t0_rp12=$(link_stats_tx_packets_get $rp12)
        t0_rp13=$(link_stats_tx_packets_get $rp13)
 
-       # Generate 16384 echo requests, each with a random flow label.
-       for _ in $(seq 1 16384); do
-	       ip vrf exec vrf-h1 ping 2001:db8:2::2 -F 0 -c 1 -q &> /dev/null
-       done
+       $MZ $h1 -6 -q -p 64 -A 2001:db8:1::2 -B 2001:db8:2::2 -d 1msec \
+	       -t udp "sp=1024,dp=0-32768"
 
        t1_rp12=$(link_stats_tx_packets_get $rp12)
        t1_rp13=$(link_stats_tx_packets_get $rp13)

@@ -255,6 +256,7 @@ multipath6_test()
        ip route replace 2001:db8:2::/64 vrf vrf-r1 \
 	       nexthop via fe80:2::22 dev $rp12 \
 	       nexthop via fe80:3::23 dev $rp13
+       sysctl -q -w net.ipv6.fib_multipath_hash_policy=$hash_policy
 }
 
 multipath_test()

On Thu, Mar 01, 2018 at 11:09:43AM -0800, David Ahern wrote:

Capabilities of tc command are irrelevant for router tests:
    $ ./router.sh
    SKIP: iproute2 too old, missing shared block support

Add a CHECK_TC flag and only check tc capabilities if set. Add flag to
tc_common.sh and have it sourced before lib.sh

Also, if the command lacks some feature the test should exit non-0.

Signed-off-by: David Ahern <redacted>
---
 tools/testing/selftests/net/forwarding/lib.sh      | 29 ++++++++++++++--------
 .../testing/selftests/net/forwarding/tc_actions.sh |  2 +-
 .../testing/selftests/net/forwarding/tc_chains.sh  |  2 +-
 .../testing/selftests/net/forwarding/tc_common.sh  |  2 ++
 .../testing/selftests/net/forwarding/tc_flower.sh  |  2 +-
 .../selftests/net/forwarding/tc_shblocks.sh        |  2 +-

bridge_vlan_aware.sh also uses tc to check flooding and learning. Looks
good otherwise.

Reviewed-by: Ido Schimmel <redacted>

Thanks, David!

On 3/1/18 12:36 PM, Ido Schimmel wrote:

On Thu, Mar 01, 2018 at 11:09:44AM -0800, David Ahern wrote:

quoted

Add a CHECK_MZ flag and only check if mz exists when flag is set.
If it does not exist, exit non-0.

Signed-off-by: David Ahern <redacted>

The only test that doesn't require it is router.sh so I'm not sure it's
worth the effort. If you still want to keep the patch then
bridge_vlan_aware.sh needs to be patched as well.

ok, I'll drop this one