| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 12 Dec 2023 16:07:18 +0200
From: Roger Quadros <rogerq@...nel.org>
To: Vladimir Oltean <vladimir.oltean@....com>
Cc: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, shuah@...nel.org, s-vadapalli@...com,
r-gunasekaran@...com, vigneshr@...com, srk@...com, horms@...nel.org,
p-varis@...com, netdev@...r.kernel.org
Subject: Re: [PATCH 2/2] selftests: forwarding: ethtool_mm: support devices
that don't support pmac stats
Hi Vladimir,
On 11/12/2023 15:24, Vladimir Oltean wrote:
> On Mon, Dec 11, 2023 at 02:01:38PM +0200, Roger Quadros wrote:
>> Some devices do not support individual 'pmac' and 'emac' stats.
>> For such devices, resort to 'aggregate' stats.
>>
>> Signed-off-by: Roger Quadros <rogerq@...nel.org>
>> ---
>> tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++
>> 1 file changed, 7 insertions(+)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> index 6212913f4ad1..e3f2e62029ca 100755
>> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
>> @@ -26,6 +26,13 @@ traffic_test()
>> local delta=
>>
>> before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>> + # some devices don't support individual pmac/emac stats,
>> + # use aggregate stats for them.
>> + if [ "$before" == null ]; then
>> + src="aggregate"
>> + before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOO
>> +K" $src)
>> + fi
>
> 1. please follow the existing indentation scheme, don't mix tabs with spaces
> 2. someone mangled your patch into invalid bash syntax, splitting a line
> into 2
> 3. "FramesTransmittedOOK" has an extra "O"
> 4. it would be preferable if you could evaluate only once whether pMAC
> counters are reported, set a global variable, and in traffic_test(),
> if that variable is true, override $src with "aggregate".
> 5. why did you split the selftest patches out of the am65-cpsw patch
> set? It is for the am65-cpsw that they are needed.
>
>>
>> $MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
>>
>> --
>> 2.34.1
>>
>
> Something like this?
>
> From ef5688a78908d99b607909fd7c93829c6a018b61 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@....com>
> Date: Mon, 11 Dec 2023 15:21:25 +0200
> Subject: [PATCH] selftests: forwarding: ethtool_mm: fall back to aggregate if
> device does not report pMAC stats
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> ---
> tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++
> tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++++
> 2 files changed, 19 insertions(+)
What is the proper way to run the script?
I've been hardcoding the following in the script.
NETIFS=( "eth0" "eth1" )
in setup_prepare()
h1=eth0
h2=eth1
and run the script like so
./run_kselftest.sh -t net/forwarding:ethtool_mm.sh
>
> diff --git a/tools/testing/selftests/net/forwarding/ethtool_mm.sh b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> index 39e736f30322..2740133f95ec 100755
> --- a/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> +++ b/tools/testing/selftests/net/forwarding/ethtool_mm.sh
> @@ -25,6 +25,10 @@ traffic_test()
> local after=
> local delta=
>
> + if [ has_pmac_stats[$netif] = false ]; then
This should be
if [ ${has_pmac_stats[$if]} = false ]; then
otherwise it doesn't work.
> + src="aggregate"
> + fi
> +
> before=$(ethtool_std_stats_get $if "eth-mac" "FramesTransmittedOK" $src)
>
> $MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
> @@ -284,6 +288,13 @@ for netif in ${NETIFS[@]}; do
> echo "SKIP: $netif does not support MAC Merge"
> exit $ksft_skip
> fi
> +
> + if check_ethtool_pmac_std_stats_support $netif; then
> + has_pmac_stats[$netif]=true
> + else
> + has_pmac_stats[$netif]=false
> + echo "$netif does not report pMAC statistics, falling back to aggregate"
> + fi
> done
>
> trap cleanup EXIT
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 8f6ca458af9a..82ac6a066729 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -146,6 +146,14 @@ check_ethtool_mm_support()
> fi
> }
>
> +check_ethtool_pmac_std_stats_support()
> +{
> + local dev=$1; shift
> +
> + [ -n "$(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null | \
> + jq '.[]')" ]
This is evaluating to true instead of false on my platform so something needs to be fixed here.
Below is the output of "ethtool --json -S eth0 --all-groups --src pmac"
[ {
"ifname": "eth0",
"eth-phy": {},
"eth-mac": {},
"eth-ctrl": {},
"rmon": {}
} ]
I suppose we want to check if eth-mac has anything or not.
Something like this works
[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
| jq '.[]."eth-mac" | length') ]
OK?
> +}
> +
> check_locked_port_support()
> {
> if ! bridge -d link show | grep -q " locked"; then
also I had to revert a recent commit
25ae948b4478 ("selftests/net: add lib.sh")
else i get an error message syaing ../lib.sh not found.
Looks like that is not getting deployed on kselftest-install
I will report this in the original patch thread as well.
--
cheers,
-roger
Powered by blists - more mailing lists