Re: [PATCH net-next 7/7] testing: net-drv: add a driver test for stats reporting

Petr Machata
2024-04-02T17:13:17Z

Jakub Kicinski <kuba@kernel.org> writes:

> Add a very simple test to make sure drivers report expected
> stats. Drivers which implement FEC or pause configuration
> should report relevant stats. Qstats must be reported,
> at least packet and byte counts, and they must match
> total device stats.
>
> Tested with netdevsim, bnxt, in-tree and installed.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> tools/testing/selftests/drivers/net/stats.py | 85 ++++++++++++++++++++
> 1 file changed, 85 insertions(+)
> create mode 100755 tools/testing/selftests/drivers/net/stats.py
>
> diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
> new file mode 100755
> index 000000000000..751cca2869b8
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/stats.py
> @@ -0,0 +1,85 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +from lib.py import ksft_run, ksft_in, ksft_true, KsftSkipEx, KsftXfailEx
> +from lib.py import EthtoolFamily, NetdevFamily, RtnlFamily, NlError
> +from lib.py import NetDrvEnv
> +
> +cfg = None
> +ethnl = EthtoolFamily()
> +netfam = NetdevFamily()
> +rtnl = RtnlFamily()
> +
> +
> +def check_pause() -> None:
> + global cfg, ethnl
> +
> + try:
> + ethnl.pause_get({"header": {"dev-index": cfg.ifindex}})
> + except NlError as e:
> + if e.error == 95:
> + raise KsftXfailEx("pause not supported by the device")
> + raise
> +
> + data = ethnl.pause_get({"header": {"dev-index": cfg.ifindex,
> + "flags": {'stats'}}})
> + ksft_true(data['stats'], "driver does not report stats")
> +
> +
> +def check_fec() -> None:
> + global ethnl
> +
> + try:
> + ethnl.fec_get({"header": {"dev-index": cfg.ifindex}})
> + except NlError as e:
> + if e.error == 95:
> + raise KsftXfailEx("FEC not supported by the device")
> + raise
> +
> + data = ethnl.fec_get({"header": {"dev-index": cfg.ifindex,
> + "flags": {'stats'}}})
> + ksft_true(data['stats'], "driver does not report stats")
> +
> +
> +def pkt_byte_sum() -> None:
> + global cfg, netfam, rtnl
> +
> + def get_qstat(test):
> + global netfam
> + stats = netfam.qstats_get({}, dump=True)
> + if stats:
> + for qs in stats:
> + if qs["ifindex"]== test.ifindex:
> + return qs
> +
> + qstat = get_qstat(cfg)
> + if qstat is None:
> + raise KsftSkipEx("qstats not supported by the device")
> +
> + for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
> + ksft_in(key, qstat, "Drivers should always report basic keys")
> +
> + # Compare stats, rtnl stats and qstats must match,
> + # but the interface may be up, so do a series of dumps
> + # each time the more "recent" stats must be higher or same.
> + def stat_cmp(rstat, qstat):
> + for key in ['tx-packets', 'tx-bytes', 'rx-packets', 'rx-bytes']:
> + if rstat[key] != qstat[key]:
> + return rstat[key] - qstat[key]
> + return 0
> +
> + for _ in range(10):
> + rtstat = rtnl.getlink({"ifi-index": cfg.ifindex})['stats']
> + if stat_cmp(rtstat, qstat) < 0:
> + raise Exception("RTNL stats are lower, fetched later")
> + qstat = get_qstat(cfg)
> + if stat_cmp(rtstat, qstat) > 0:
> + raise Exception("Qstats are lower, fetched later")
> +
> +
> +if __name__ == "__main__":
> + cfg = NetDrvEnv(__file__)
> + try:
> + ksft_run([check_pause, check_fec, pkt_byte_sum])
> + finally:
> + del cfg

Yeah, this would be usually done through context managers, as I mention
in the other e-mail. But then cfg would be lexically scoped, which IMHO
is a good thing, but then it needs to be passed around as an argument,
and that makes the ksft_run() invocation a bit messy:

with NetDrvEnv(__file__) as cfg:
ksft_run([lambda: check_pause(cfg),
lambda: check_fec(cfg),
lambda: pkt_byte_sum(cfg)])

Dunno, maybe it could forward *args **kwargs to the cases? But then it
loses some of the readability again.