| Message ID | 20241220003116.1458863-1-kuba@kernel.org (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] selftests: drv-net: assume stats refresh is 0 if no ethtool -c support | expand |
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: > > + 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.
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
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>
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):
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(-)