Re: [PATCH net-next 5/5] selftests: drivers: hw: add tests for the ethtool standard counters

[Petr Machata]

Ioana Ciornei <ioana.ciornei@xxxxxxx> writes:

> Add a new selftest - ethtool_std_stats.sh - which validates the
> eth-ctrl, eth-mac and pause standard statistics exported by an
> interface. Not all counters can be validated in software, especially
> those that are keeping track of errors. Counters such as
> SingleCollisionFrames, FrameCheckSequenceErrors etc are not tested nor
> included in this new selftest.
>
> The central part of this patch is the traffic_test() function which
> gathers the 'before' counter values, sends a batch of traffic and then
> interrogates again the same counters in order to determine if the delta
> is on target. The function receives an array through which the caller
> can request what counters to be interrogated and, for each of them, what
> is their target delta value.
>
> The output from this selftest looks as follows on a LX2160ARDB board:
>
> $ ./ethtool_std_stats.sh eth0 eth1
> TEST: eth-ctrl tx on eth0 (MACControlFramesTransmitted on eth0)     [ OK ]
> TEST: eth-ctrl tx on eth0 (MACControlFramesReceived on eth1)        [ OK ]
> TEST: eth-ctrl tx on eth1 (MACControlFramesTransmitted on eth1)     [ OK ]
> TEST: eth-ctrl tx on eth1 (MACControlFramesReceived on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac bcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac bcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac bcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac bcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac bcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac bcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac mcast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac mcast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac mcast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac mcast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac mcast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac mcast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac mcast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac mcast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac ucast tx on eth0 (BroadcastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac ucast tx on eth0 (FramesTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac ucast tx on eth0 (OctetsTransmittedOK on eth0)        [ OK ]
> TEST: eth-mac ucast tx on eth0 (MulticastFramesXmittedOK on eth0)   [ OK ]
> TEST: eth-mac ucast tx on eth0 (BroadcastFramesReceivedOK on eth1)  [ OK ]
> TEST: eth-mac ucast tx on eth0 (FramesReceivedOK on eth1)           [ OK ]
> TEST: eth-mac ucast tx on eth0 (OctetsReceivedOK on eth1)           [ OK ]
> TEST: eth-mac ucast tx on eth0 (MulticastFramesReceivedOK on eth1)  [ OK ]
> TEST: pause tx on eth0 (tx_pause_frames on eth0)                    [ OK ]
> TEST: pause tx on eth0 (rx_pause_frames on eth1)                    [ OK ]
> TEST: pause tx on eth1 (tx_pause_frames on eth1)                    [ OK ]
> TEST: pause tx on eth1 (rx_pause_frames on eth0)                    [ OK ]
>
> Please note that not all MACs are counting the software injected pause
> frames as real Tx pause. For example, on a LS1028ARDB the selftest
> output will reflect the fact that neither the ENETC MAC, nor the Felix
> switch MAC are able to detect Tx pause frames injected by software.
>
> $ ./ethtool_std_stats.sh eno0 swp0
> (...)
> TEST: Not all MACs detect injected pause frames                     [XFAIL]
> TEST: pause tx on eno0 (rx_pause_frames on swp0)                    [ OK ]
> TEST: Not all MACs detect injected pause frames                     [XFAIL]
> TEST: pause tx on swp0 (rx_pause_frames on eno0)                    [ OK ]
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@xxxxxxx>
> ---
>  .../testing/selftests/drivers/net/hw/Makefile |   1 +
>  .../drivers/net/hw/ethtool_std_stats.sh       | 192 ++++++++++++++++++
>  2 files changed, 193 insertions(+)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
>
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index a64140333a46..d447813a14b2 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -26,6 +26,7 @@ TEST_PROGS = \
>  	ethtool_extended_state.sh \
>  	ethtool_mm.sh \
>  	ethtool_rmon.sh \
> +	ethtool_std_stats.sh \
>  	hw_stats_l3.sh \
>  	hw_stats_l3_gre.sh \
>  	iou-zcrx.py \
> diff --git a/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> new file mode 100755
> index 000000000000..4ce8f140a18c
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/ethtool_std_stats.sh
> @@ -0,0 +1,192 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +#shellcheck disable=SC2034 # SC does not see the global variables
> +
> +ALL_TESTS="
> +	test_eth_ctrl_stats
> +	test_eth_mac_stats
> +	test_pause_stats
> +"
> +STABLE_MAC_ADDRS=yes
> +NUM_NETIFS=2
> +lib_dir=$(dirname "$0")
> +# shellcheck source=./../../../net/forwarding/lib.sh
> +source "$lib_dir"/../../../net/forwarding/lib.sh
> +
> +traffic_test()
> +{
> +	local iface=$1; shift
> +	local neigh=$1; shift
> +	local num_tx=$1; shift
> +	local pkt_format="$1"; shift
> +	local title="$1"; shift
> +	declare -a counters=( "${@:2:$1}" ); shift "$(( $1 + 1 ))"

Please make this local -a. declare inside a function also introduces a
local variable, so the effect is the same, but using local is more
obvious.

BTW, just a suggestion, personally I'd just make the counters a pure
"rest" argument:

	local -a counters=("$@")

simply because functions usually don't need more than one, and it's
easier to use on call site, and easier to understand what's going on.

> +	local before after delta target_high extra
> +	local int grp counter target unit
> +	local num_rx=$((num_tx * 2))
> +	local xfail_message
> +	local src="aggregate"

And local i.

> +
> +	for i in "${!counters[@]}"; do
> +		read -r int grp counter target unit xfail_message <<< "${counters[$i]}"
> +		before[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> +	done
> +
> +	# shellcheck disable=SC2086 # needs split options
> +	$MZ "$iface" -q -c "$num_tx" $pkt_format
> +
> +	# shellcheck disable=SC2086 # needs split options
> +	$MZ "$neigh" -q -c "$num_rx" $pkt_format
> +
> +	for i in "${!counters[@]}"; do

There should be a RET=0 here, so that the check_err below gets a clean slate.

> +		read -r int grp counter target unit xfail_message<<< "${counters[$i]}"
> +
> +		after[i]=$(ethtool_std_stats_get "$int" "$grp" "$counter" "$src")
> +		if [[ "${after[$i]}" == "null" ]]; then
> +			log_test_skip "$int does not support $grp-$counter"
> +			continue;
> +		fi
> +
> +		delta=$((after[i] - before[i]))
> +
> +		# Allow an extra 1% tolerance for random packets sent by the stack
> +		extra=$((num_pkts * unit / 100))
> +		target_high=$((target + extra))
> +
> +		RET=0
> +		[ "$delta" -ge "$target" ] && [ "$delta" -le "$target_high" ]
> +		err="$?"
> +		if [[ $err != 0  ]] && [[ -n $xfail_message ]]; then
> +			log_test_xfail "$xfail_message"

I think this should mention the $title as well. I think that the
xfail_message could be the log_test's opt_str, so:

                        log_test_xfail "$title" "$xfail_message"

A bit annoying that log_test_xfail clears the retmsg. It does so so that
messages from previous runs do not show up, which is desirable in
general, but here it would be handy.

> +			continue;
> +		fi
> +		check_err "$err" "$grp-$counter is not valid on $int (expected $target, got $delta)"
> +		log_test "$title" "$counter on $int"
> +	done
> +}
> +
> +test_eth_ctrl_stats()
> +{
> +	local pkt_format="-a own -b bcast 88:08 -p 64"
> +	local num_pkts=1000
> +	local counters

local -a

(Though yeah, bash doesn't care.)

> +
> +	counters=("$h1 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> +		  "$h2 eth-ctrl MACControlFramesReceived $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> +		"eth-ctrl tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h2 eth-ctrl MACControlFramesTransmitted $num_pkts 1"
> +		  "$h1 eth-ctrl MACControlFramesReceived $num_pkts 1")
> +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> +		"eth-ctrl tx on $h2" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +test_eth_mac_stats()
> +{
> +	local pkt_size=100
> +	local pkt_size_fcs=$((pkt_size + 4))
> +	local bcast_pkt_format="-a own -b bcast -p $pkt_size"
> +	local mcast_pkt_format="-a own -b -b 01:00:5E:00:00:01 -p $pkt_size"
> +	local ucast_pkt_format="-a own -b $h2_mac -p $pkt_size"
> +	local num_pkts=2000
> +	local octets=$((pkt_size_fcs * num_pkts))
> +	local counters

local -a

> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK $num_pkts 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$bcast_pkt_format" \
> +		"eth-mac bcast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK $num_pkts 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$mcast_pkt_format" \
> +		"eth-mac mcast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h1 eth-mac BroadcastFramesXmittedOK 0 1"
> +		  "$h1 eth-mac FramesTransmittedOK $num_pkts 1"
> +		  "$h1 eth-mac OctetsTransmittedOK $octets $pkt_size_fcs"
> +		  "$h1 eth-mac MulticastFramesXmittedOK 0 1"
> +		  "$h2 eth-mac BroadcastFramesReceivedOK 0 1"
> +		  "$h2 eth-mac FramesReceivedOK $num_pkts 1"
> +		  "$h2 eth-mac OctetsReceivedOK $octets $pkt_size_fcs"
> +		  "$h2 eth-mac MulticastFramesReceivedOK 0 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$ucast_pkt_format" \
> +		"eth-mac ucast tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +test_pause_stats()
> +{
> +	local pkt_format="-a own -b 01:80:c2:00:00:01 88:08:00:01:00:01"
> +	local xfail_message="Not all MACs detect injected pause frames"
> +	local num_pkts=2000
> +	local counters i

counters should be declared local -a

> +
> +	# Check that there is pause frame support
> +	for ((i = 1; i <= NUM_NETIFS; ++i)); do
> +		if ! ethtool -I --json -a "${NETIFS[p$i]}" > /dev/null 2>&1; then
> +			log_test_skip "No support for pause frames, skip tests"
> +			exit
> +		fi
> +	done
> +
> +	counters=("$h1 pause tx_pause_frames $num_pkts 1 $xfail_message"
> +		  "$h2 pause rx_pause_frames $num_pkts 1")
> +	traffic_test "$h1" "$h2" "$num_pkts" "$pkt_format" \
> +		"pause tx on $h1" \
> +		"${#counters[@]}" "${counters[@]}"
> +
> +	counters=("$h2 pause tx_pause_frames $num_pkts 1 $xfail_message"
> +		  "$h1 pause rx_pause_frames $num_pkts 1")
> +	traffic_test "$h2" "$h1" "$num_pkts" "$pkt_format" \
> +		"pause tx on $h2" \
> +		"${#counters[@]}" "${counters[@]}"
> +}
> +
> +setup_prepare()
> +{
> +	h1=${NETIFS[p1]}
> +	h2=${NETIFS[p2]}
> +
> +	h2_mac=$(mac_get "$h2")
> +
> +	for iface in $h1 $h2; do

These should be quoted.
iface should be local.

> +		ip link set dev "$iface" up

Plesae use defer:

		ip link set dev "$iface" up
		defer ip link set dev "$iface" down

Then drop the hand-rolled cleanup() altogether. Retain the "trap cleanup
EXIT" -- it will pick up forwarding/lib.sh's cleanup(), which calls
pre_cleanup and defer_scopes_cleanup().

> +	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"