| Message ID | 20231211120138.5461-3-rogerq@kernel.org (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | selftests: net: ethtool_mm: Support devices with higher rx-min-frag-size | expand |
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@kernel.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@nxp.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@nxp.com> --- tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++ tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++++ 2 files changed, 19 insertions(+) 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 + 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 '.[]')" ] +} + check_locked_port_support() { if ! bridge -d link show | grep -q " locked"; then
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@kernel.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 Sure. Will fix. > 2. someone mangled your patch into invalid bash syntax, splitting a line > into 2 > 3. "FramesTransmittedOOK" has an extra "O" Will fix. The mangling happened at my end. > 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. Needed for the tests to pass, yes. I'll add them back at the beginning of the series. > >> >> $MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO >> >> -- >> 2.34.1 >> > > Something like this? Thanks for the hint! > > From ef5688a78908d99b607909fd7c93829c6a018b61 Mon Sep 17 00:00:00 2001 > From: Vladimir Oltean <vladimir.oltean@nxp.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@nxp.com> > --- > tools/testing/selftests/net/forwarding/ethtool_mm.sh | 11 +++++++++++ > tools/testing/selftests/net/forwarding/lib.sh | 8 ++++++++ > 2 files changed, 19 insertions(+) > > 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 > + 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 '.[]')" ] > +} > + > check_locked_port_support() > { > if ! bridge -d link show | grep -q " locked"; then
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@kernel.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@nxp.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@nxp.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.
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(). > > +} > > + > > 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. I'm not using that part, so I didn't notice it :)
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
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 $MZ $if -q -c $num_pkts -p 64 -b bcast -t ip -R $PREEMPTIBLE_PRIO
Some devices do not support individual 'pmac' and 'emac' stats. For such devices, resort to 'aggregate' stats. Signed-off-by: Roger Quadros <rogerq@kernel.org> --- tools/testing/selftests/net/forwarding/ethtool_mm.sh | 7 +++++++ 1 file changed, 7 insertions(+)