| Message ID | 20231211223346.2497157-9-tobias@waldekranz.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: dsa: mv88e6xxx: Add "eth-mac" and "rmon" counter group support | expand |
On 12/11/23 14:33, 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> Reviewed-by: Florian Fainelli <florian.fainelli@broadcom.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() > +}
diff --git a/tools/testing/selftests/net/forwarding/Makefile b/tools/testing/selftests/net/forwarding/Makefile index df593b7b3e6b..452693514be4 100644 --- a/tools/testing/selftests/net/forwarding/Makefile +++ b/tools/testing/selftests/net/forwarding/Makefile @@ -17,6 +17,7 @@ TEST_PROGS = bridge_fdb_learning_limit.sh \ dual_vxlan_bridge.sh \ ethtool_extended_state.sh \ ethtool_mm.sh \ + ethtool_rmon.sh \ ethtool.sh \ gre_custom_multipath_hash.sh \ gre_inner_v4_multipath.sh \ 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" +} + +rmon_rx_histogram() +{ + rmon_histogram rx +} + +rmon_tx_histogram() +{ + rmon_histogram tx +} + +setup_prepare() +{ + h1=${NETIFS[p1]} + h2=${NETIFS[p2]} + + for iface in $h1 $h2; do + ip link set dev $iface up + done +} + +cleanup() +{ + pre_cleanup + + for iface in $h2 $h1; do + ip link set dev $iface down + done +} + +check_ethtool_counter_group_support +trap cleanup EXIT + +setup_prepare +setup_wait + +tests_run + +exit $EXIT_STATUS diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh index 8f6ca458af9a..e3740163c384 100755 --- a/tools/testing/selftests/net/forwarding/lib.sh +++ b/tools/testing/selftests/net/forwarding/lib.sh @@ -146,6 +146,15 @@ check_ethtool_mm_support() fi } +check_ethtool_counter_group_support() +{ + ethtool --help 2>&1| grep -- '--all-groups' &> /dev/null + if [[ $? -ne 0 ]]; then + echo "SKIP: ethtool too old; it is missing standard counter group support" + exit $ksft_skip + fi +} + check_locked_port_support() { if ! bridge -d link show | grep -q " locked"; then
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> --- .../testing/selftests/net/forwarding/Makefile | 1 + .../selftests/net/forwarding/ethtool_rmon.sh | 106 ++++++++++++++++++ tools/testing/selftests/net/forwarding/lib.sh | 9 ++ 3 files changed, 116 insertions(+) create mode 100755 tools/testing/selftests/net/forwarding/ethtool_rmon.sh