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

[Petr Machata <petrm@nvidia.com>]

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.