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):
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
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:raiseHow 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.
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:raiseHow 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
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
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!
linux-kselftest-mirror@lists.linaro.org
-
Andrew Lunn -
Jakub Kicinski -
patchwork-bot+netdevbpf＠kernel.org -
Willem de Bruijn