Re: [PATCH v3 net-next 8/8] selftests: forwarding: ethtool_rmon: Add histogram counter test

[Vladimir Oltean <olteanv@gmail.com>]

Hi Tobias,

On Mon, Dec 11, 2023 at 11:33:46PM +0100, Tobias Waldekranz wrote:
> Validate the operation of rx and tx histogram counters, if supported
> by the interface, by sending batches of packets targeted for each
> bucket.
> 
> Signed-off-by: Tobias Waldekranz <tobias@waldekranz.com>
> ---

Thank you so much for writing down this test.

I tested it on enetc and ocelot/felix, and I can report back that I
already found 2 bugs. One in ocelot, for which I've sent this patch:
https://lore.kernel.org/netdev/20231214000902.545625-1-vladimir.oltean@nxp.com/

and one in this selftest. I hope the logs below make it quite clear what
is going on.

Before the change:

root@debian:~/selftests/net/forwarding# ./ethtool_rmon.sh eno0 swp0
[   37.359447] fsl_enetc 0000:00:00.0 eno0: PHY [0000:00:00.3:02] driver [Qualcomm Atheros AR8031/AR8033] (irq=POLL)
[   37.370906] fsl_enetc 0000:00:00.0 eno0: configuring for inband/sgmii link mode
[   37.387399] mscc_felix 0000:00:00.5 swp0: configuring for inband/qsgmii link mode
[   41.478974] fsl_enetc 0000:00:00.0 eno0: Link is Up - 1Gbps/Full - flow control rx/tx
[   41.479119] mscc_felix 0000:00:00.5 swp0: Link is Up - 1Gbps/Full - flow control rx/tx
TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-1526                    [ OK ]
TEST: rx histogram counters for bucket 1527-65535                   [FAIL]
        Verification failed for swp0 bucket 1527-65535
TEST: tx histogram counters for bucket 64-64                        [ OK ]
TEST: tx histogram counters for bucket 65-127                       [ OK ]
TEST: tx histogram counters for bucket 128-255                      [ OK ]
TEST: tx histogram counters for bucket 256-511                      [ OK ]
TEST: tx histogram counters for bucket 512-1023                     [ OK ]
TEST: tx histogram counters for bucket 1024-1526                    [ OK ]
TEST: tx histogram counters for bucket 1527-65535                   [FAIL]
        Verification failed for swp0 bucket 1527-65535

The change itself:

root@debian:~/selftests/net/forwarding# ip link set swp0 mtu 9000
root@debian:~/selftests/net/forwarding# ip link set eno0 mtu 9000

After the change:

root@debian:~/selftests/net/forwarding# ./ethtool_rmon.sh eno0 swp0
TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-1526                    [ OK ]
TEST: rx histogram counters for bucket 1527-65535                   [ OK ]
TEST: tx histogram counters for bucket 64-64                        [ OK ]
TEST: tx histogram counters for bucket 65-127                       [ OK ]
TEST: tx histogram counters for bucket 128-255                      [ OK ]
TEST: tx histogram counters for bucket 256-511                      [ OK ]
TEST: tx histogram counters for bucket 512-1023                     [ OK ]
TEST: tx histogram counters for bucket 1024-1526                    [ OK ]
TEST: tx histogram counters for bucket 1527-65535                   [ OK ]

We'd need to raise the MTU on both $h1 and $h2 to $len - ETH_HLEN.
Note that $h1 - the device whose counters we are not looking at - may
not have the same histograms, and even the same MTU. It means we may not
be able to test all of $h2's histograms if we can't set the MTU to the
appropriate value, and that should just mean a skipped test.

The initial MTU of the interfaces should be restored at cleanup() time,
and only modified during each test if necessary, I suppose.

I noticed that the test is asymmetric, so I ran it a second time with
the argument order "swp0 eno0" and that passed as well. It's probably
all too easy to miss that it leaves $h1's counters untested, though.

The test also passes on mv88e6390, because all buckets start with a
value smaller than 1518, so the MTU never needs to be increased:

TEST: rx histogram counters for bucket 64-64                        [ OK ]
TEST: rx histogram counters for bucket 65-127                       [ OK ]
TEST: rx histogram counters for bucket 128-255                      [ OK ]
TEST: rx histogram counters for bucket 256-511                      [ OK ]
TEST: rx histogram counters for bucket 512-1023                     [ OK ]
TEST: rx histogram counters for bucket 1024-65535                   [ OK ]
TEST: lan2 does not support tx histogram counters                   [SKIP]

> diff --git a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
> new file mode 100755
> index 000000000000..73e3fbe28f37
> --- /dev/null
> +++ b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
> @@ -0,0 +1,106 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +ALL_TESTS="
> +	rmon_rx_histogram
> +	rmon_tx_histogram
> +"
> +
> +NUM_NETIFS=2
> +source lib.sh
> +
> +bucket_test()
> +{
> +	local set=$1; shift
> +	local bucket=$1; shift
> +	local len=$1; shift
> +	local num_rx=10000
> +	local num_tx=20000
> +	local expected=
> +	local before=
> +	local after=
> +	local delta=
> +
> +	# Mausezahn does not include FCS bytes in its length - but the
> +	# histogram counters do
> +	len=$((len - 4))
> +
> +	before=$(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
> +
> +	# Send 10k one way and 20k in the other, to detect counters
> +	# mapped to the wrong direction
> +	$MZ $h1 -q -c $num_rx -p $len -a own -b bcast -d 10us
> +	$MZ $h2 -q -c $num_tx -p $len -a own -b bcast -d 10us
> +
> +	after=$(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][$bucket].val")
> +
> +	delta=$((after - before))
> +
> +	expected=$([ $set = rx ] && echo $num_rx || echo $num_tx)
> +
> +	# Allow some extra tolerance for other packets sent by the stack
> +	[ $delta -ge $expected ] && [ $delta -le $((expected + 100)) ]
> +}
> +
> +rmon_histogram()
> +{
> +	local set=$1; shift
> +	local nbuckets=0
> +
> +	RET=0
> +
> +	while read -r -a bucket; do
> +		bucket_test $set $nbuckets ${bucket[0]}
> +		check_err "$?" "Verification failed for bucket ${bucket[0]}-${bucket[1]}"
> +		nbuckets=$((nbuckets + 1))
> +	done < <(ethtool --json -S $h2 --groups rmon | \
> +		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high, .val]|@tsv" 2>/dev/null)
> +
> +	if [ $nbuckets -eq 0 ]; then
> +		log_test_skip "$h2 does not support $set histogram counters"
> +		return
> +	fi
> +
> +	log_test "$set histogram counters"

I'm aware this was probably done on purpose, but I felt the test was not
very interactive (it took over 10 seconds to get some output back), so I
took the liberty to log individual buckets as their own tests. And also
to stop at the first failure, rather than continue the iteration which
got me confused during debugging.

diff --git a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
index 73e3fbe28f37..b0f701063822 100755
--- a/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
+++ b/tools/testing/selftests/net/forwarding/ethtool_rmon.sh
@@ -53,7 +53,13 @@ rmon_histogram()
 
 	while read -r -a bucket; do
 		bucket_test $set $nbuckets ${bucket[0]}
-		check_err "$?" "Verification failed for bucket ${bucket[0]}-${bucket[1]}"
+		rc="$?"
+		check_err "$rc" "Verification failed for $h2 bucket ${bucket[0]}-${bucket[1]}"
+		log_test "$set histogram counters for bucket ${bucket[0]}-${bucket[1]}"
+		if [ $rc -ne 0 ]; then
+			return 1
+		fi
+
 		nbuckets=$((nbuckets + 1))
 	done < <(ethtool --json -S $h2 --groups rmon | \
 		jq -r ".[0].rmon[\"${set}-pktsNtoM\"][]|[.low, .high, .val]|@tsv" 2>/dev/null)
@@ -62,8 +68,6 @@ rmon_histogram()
 		log_test_skip "$h2 does not support $set histogram counters"
 		return
 	fi
-
-	log_test "$set histogram counters"
 }
 
 rmon_rx_histogram()

> +}