* [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
@ 2024-12-20 0:31 Jakub Kicinski
2024-12-20 9:09 ` Andrew Lunn
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Jakub Kicinski @ 2024-12-20 0:31 UTC (permalink / raw)
To: davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, shuah, willemb, petrm,
linux-kselftest
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
2024-12-20 0:31 [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support Jakub Kicinski
@ 2024-12-20 9:09 ` Andrew Lunn
2024-12-20 14:22 ` Jakub Kicinski
2024-12-21 4:05 ` Willem de Bruijn
2024-12-23 18:40 ` patchwork-bot+netdevbpf
2 siblings, 1 reply; 6+ messages in thread
From: Andrew Lunn @ 2024-12-20 9:09 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, shuah, willemb, petrm,
linux-kselftest
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
2024-12-20 9:09 ` Andrew Lunn
@ 2024-12-20 14:22 ` Jakub Kicinski
2024-12-20 15:03 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: Jakub Kicinski @ 2024-12-20 14:22 UTC (permalink / raw)
To: Andrew Lunn
Cc: davem, netdev, edumazet, pabeni, shuah, willemb, petrm,
linux-kselftest
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
2024-12-20 14:22 ` Jakub Kicinski
@ 2024-12-20 15:03 ` Andrew Lunn
0 siblings, 0 replies; 6+ messages in thread
From: Andrew Lunn @ 2024-12-20 15:03 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, shuah, willemb, petrm,
linux-kselftest
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
2024-12-20 0:31 [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support Jakub Kicinski
2024-12-20 9:09 ` Andrew Lunn
@ 2024-12-21 4:05 ` Willem de Bruijn
2024-12-23 18:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: Willem de Bruijn @ 2024-12-21 4:05 UTC (permalink / raw)
To: Jakub Kicinski, davem
Cc: netdev, edumazet, pabeni, Jakub Kicinski, shuah, willemb, petrm,
linux-kselftest
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>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support
2024-12-20 0:31 [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support Jakub Kicinski
2024-12-20 9:09 ` Andrew Lunn
2024-12-21 4:05 ` Willem de Bruijn
@ 2024-12-23 18:40 ` patchwork-bot+netdevbpf
2 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-12-23 18:40 UTC (permalink / raw)
To: Jakub Kicinski
Cc: davem, netdev, edumazet, pabeni, shuah, willemb, petrm,
linux-kselftest
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2024-12-23 18:40 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-20 0:31 [PATCH net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support Jakub Kicinski
2024-12-20 9:09 ` Andrew Lunn
2024-12-20 14:22 ` Jakub Kicinski
2024-12-20 15:03 ` Andrew Lunn
2024-12-21 4:05 ` Willem de Bruijn
2024-12-23 18:40 ` patchwork-bot+netdevbpf
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).