[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Tue, 12 Dec 2023 21:48:05 +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
On 12/12/2023 16:57, Vladimir Oltean wrote:
> On Tue, Dec 12, 2023 at 04:07:18PM +0200, Roger Quadros wrote:
>> 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
>
> IDK. I rsync the selftest dir to my board and do:
>
> $ cd selftests/net/forwarding
> $ ./ethtool.mm eth0 eth1
>
> Running through run_kselftest.sh is probably better. I think that also
> supports passing the network interfaces as arguments, no need to hack up
> the script.
>
>>> 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.
>
> Makes sense.
>
>>> + 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?
>
> Maybe giving the stats group as argument instead of hardcoding "eth-mac"
> would make sense. I hoped we could avoid hardcoding one particular group
> of counters in check_ethtool_pmac_std_stats_support().
You mean like this?
check_ethtool_pmac_std_stats_support()
{
local dev=$1; shift
local grp=$1; shift
[ 0 -ne $(ethtool --json -S $dev --all-groups --src pmac 2>/dev/null \
| jq '.[]."$grp" | length') ]
}
Caller will call like so
check_ethtool_pmac_std_stats_support $netif eth-mac
--
cheers,
-roger
Powered by blists - more mailing lists