[PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[Jakub Kicinski]

Tests using HW stats wait for them to stabilize, using data from
ethtool -c as the delay. Not all drivers implement ethtool -c
so handle the errors gracefully.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: willemb@google.com
CC: petrm@nvidia.com
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/lib/py/env.py | 9 +++++++--
 tools/testing/selftests/net/lib/py/utils.py       | 6 ++++--
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index 1ea9bb695e94..fea343f209ea 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -5,7 +5,7 @@ import time
 from pathlib import Path
 from lib.py import KsftSkipEx, KsftXfailEx
 from lib.py import ksft_setup
-from lib.py import cmd, ethtool, ip
+from lib.py import cmd, ethtool, ip, CmdExitFailure
 from lib.py import NetNS, NetdevSimDev
 from .remote import Remote
 
@@ -234,7 +234,12 @@ from .remote import Remote
         Good drivers will tell us via ethtool what their sync period is.
         """
         if self._stats_settle_time is None:
-            data = ethtool("-c " + self.ifname, json=True)[0]
+            data = {}
+            try:
+                data = ethtool("-c " + self.ifname, json=True)[0]
+            except CmdExitFailure as e:
+                if "Operation not supported" not in e.cmd.stderr:
+                    raise
 
             self._stats_settle_time = 0.025 + \
                 data.get('stats-block-usecs', 0) / 1000 / 1000
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 72590c3f90f1..9e3bcddcf3e8 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -10,7 +10,9 @@ import time
 
 
 class CmdExitFailure(Exception):
-    pass
+    def __init__(self, msg, cmd_obj):
+        super().__init__(msg)
+        self.cmd = cmd_obj
 
 
 class cmd:
@@ -48,7 +50,7 @@ import time
             if len(stderr) > 0 and stderr[-1] == "\n":
                 stderr = stderr[:-1]
             raise CmdExitFailure("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
-                                 (self.proc.args, stdout, stderr))
+                                 (self.proc.args, stdout, stderr), self)
 
 
 class bkg(cmd):
-- 
2.47.1

Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[Andrew Lunn]

On Thu, Dec 19, 2024 at 04:31:16PM -0800, Jakub Kicinski wrote:
> Tests using HW stats wait for them to stabilize, using data from
> ethtool -c as the delay. Not all drivers implement ethtool -c
> so handle the errors gracefully.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: willemb@google.com
> CC: petrm@nvidia.com
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/lib/py/env.py | 9 +++++++--
>  tools/testing/selftests/net/lib/py/utils.py       | 6 ++++--
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index 1ea9bb695e94..fea343f209ea 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -5,7 +5,7 @@ import time
>  from pathlib import Path
>  from lib.py import KsftSkipEx, KsftXfailEx
>  from lib.py import ksft_setup
> -from lib.py import cmd, ethtool, ip
> +from lib.py import cmd, ethtool, ip, CmdExitFailure
>  from lib.py import NetNS, NetdevSimDev
>  from .remote import Remote
>  
> @@ -234,7 +234,12 @@ from .remote import Remote
>          Good drivers will tell us via ethtool what their sync period is.
>          """
>          if self._stats_settle_time is None:
> -            data = ethtool("-c " + self.ifname, json=True)[0]
> +            data = {}
> +            try:
> +                data = ethtool("-c " + self.ifname, json=True)[0]
> +            except CmdExitFailure as e:
> +                if "Operation not supported" not in e.cmd.stderr:
> +                    raise

How important is this time to the test itself? If it is not available,
can the test just default to 50ms and keep going? I would of thought
we find more issues by running the test too slowly, than not running
it at all, unless having the wrong timer makes it more flaky.

	Andrew

Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[Jakub Kicinski]

On Fri, 20 Dec 2024 10:09:06 +0100 Andrew Lunn wrote:
> > @@ -234,7 +234,12 @@ from .remote import Remote
> >          Good drivers will tell us via ethtool what their sync period is.
> >          """
> >          if self._stats_settle_time is None:
> > -            data = ethtool("-c " + self.ifname, json=True)[0]
> > +            data = {}
> > +            try:
> > +                data = ethtool("-c " + self.ifname, json=True)[0]
> > +            except CmdExitFailure as e:
> > +                if "Operation not supported" not in e.cmd.stderr:
> > +                    raise  
> 
> How important is this time to the test itself? 

Just to be clear (because unfortunately git doesn't do a good job of
calling out Python method names in the diff :() this is part of a
method called wait_hw_stats_settle() within the test env class. 
It's used by various tests which use/check device stats.

> If it is not available,
> can the test just default to 50ms and keep going? I would of thought
> we find more issues by running the test too slowly, than not running
> it at all, unless having the wrong timer makes it more flaky.

We already use zero for majority of driver which don't report stat
refresh:

                 data.get('stats-block-usecs', 0) / 1000 / 1000
                                              ^^^
this patch just does the same thing not only if the driver doesn't
report 'stats-block-usecs' but also if it doesn't support -c at all.

Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[Andrew Lunn]

On Fri, Dec 20, 2024 at 06:22:14AM -0800, Jakub Kicinski wrote:
> On Fri, 20 Dec 2024 10:09:06 +0100 Andrew Lunn wrote:
> > > @@ -234,7 +234,12 @@ from .remote import Remote
> > >          Good drivers will tell us via ethtool what their sync period is.
> > >          """
> > >          if self._stats_settle_time is None:
> > > -            data = ethtool("-c " + self.ifname, json=True)[0]
> > > +            data = {}
> > > +            try:
> > > +                data = ethtool("-c " + self.ifname, json=True)[0]
> > > +            except CmdExitFailure as e:
> > > +                if "Operation not supported" not in e.cmd.stderr:
> > > +                    raise  
> > 
> > How important is this time to the test itself? 
> 
> Just to be clear (because unfortunately git doesn't do a good job of
> calling out Python method names in the diff :() this is part of a
> method called wait_hw_stats_settle() within the test env class. 
> It's used by various tests which use/check device stats.
> 
> > If it is not available,
> > can the test just default to 50ms and keep going? I would of thought
> > we find more issues by running the test too slowly, than not running
> > it at all, unless having the wrong timer makes it more flaky.
> 
> We already use zero for majority of driver which don't report stat
> refresh:
> 
>                  data.get('stats-block-usecs', 0) / 1000 / 1000
>                                               ^^^
> this patch just does the same thing not only if the driver doesn't
> report 'stats-block-usecs' but also if it doesn't support -c at all.

[Looks at it again] So the raise is if ethtool give an error other
than EOPNOTUPP, so a real error. O.K. Then i makes sense.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[Willem de Bruijn]

Jakub Kicinski wrote:
> Tests using HW stats wait for them to stabilize, using data from
> ethtool -c as the delay. Not all drivers implement ethtool -c
> so handle the errors gracefully.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Willem de Bruijn <willemb@google.com>

Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 19 Dec 2024 16:31:16 -0800 you wrote:
> Tests using HW stats wait for them to stabilize, using data from
> ethtool -c as the delay. Not all drivers implement ethtool -c
> so handle the errors gracefully.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: willemb@google.com
> CC: petrm@nvidia.com
> CC: linux-kselftest@vger.kernel.org
> 
> [...]

Here is the summary with links:
  - [net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
    https://git.kernel.org/netdev/net-next/c/f288c7a1ba26

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html