[net-next,v2,1/2] selftests: net-drv: exercise queue stats when the device is down

Verify that total device stats don't decrease after it has been turned down.
Also make sure the device doesn't crash when we access per-queue stats
when it's down (in case it tries to access some pointers that are NULL).

  KTAP version 1
  1..5
  ok 1 stats.check_pause
  ok 2 stats.check_fec
  ok 3 stats.pkt_byte_sum
  ok 4 stats.qstat_by_ifindex
  ok 5 stats.check_down
  # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0

v2:
- KTAP output formatting (Jakub)
- defer instead of try/finally (Jakub)
- disappearing stats is an error (Jakub)
- ksft_ge instead of open coding (Jakub)

Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
--
Cc: Shuah Khan <shuah@kernel.org>
Cc: Joe Damato <jdamato@fastly.com>
Cc: Petr Machata <petrm@nvidia.com>
Cc: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/stats.py | 25 +++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

Stanislav Fomichev <sdf@fomichev.me> writes:

> Verify that total device stats don't decrease after it has been turned down.
> Also make sure the device doesn't crash when we access per-queue stats
> when it's down (in case it tries to access some pointers that are NULL).
>
>   KTAP version 1
>   1..5
>   ok 1 stats.check_pause
>   ok 2 stats.check_fec
>   ok 3 stats.pkt_byte_sum
>   ok 4 stats.qstat_by_ifindex
>   ok 5 stats.check_down
>   # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> v2:
> - KTAP output formatting (Jakub)
> - defer instead of try/finally (Jakub)
> - disappearing stats is an error (Jakub)
> - ksft_ge instead of open coding (Jakub)
>
> Signed-off-by: Stanislav Fomichev <sdf@fomichev.me>
> --
> Cc: Shuah Khan <shuah@kernel.org>
> Cc: Joe Damato <jdamato@fastly.com>
> Cc: Petr Machata <petrm@nvidia.com>
> Cc: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/stats.py | 25 +++++++++++++++++++-
>  1 file changed, 24 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
> index 820b8e0a22c6..93f9204f51c4 100755
> --- a/tools/testing/selftests/drivers/net/stats.py
> +++ b/tools/testing/selftests/drivers/net/stats.py
> @@ -5,6 +5,7 @@ from lib.py import ksft_run, ksft_exit, ksft_pr
>  from lib.py import ksft_ge, ksft_eq, ksft_in, ksft_true, ksft_raises, KsftSkipEx, KsftXfailEx
>  from lib.py import EthtoolFamily, NetdevFamily, RtnlFamily, NlError
>  from lib.py import NetDrvEnv
> +from lib.py import ip, defer
>  
>  ethnl = EthtoolFamily()
>  netfam = NetdevFamily()
> @@ -133,9 +134,31 @@ rtnl = RtnlFamily()
>      ksft_eq(cm.exception.nl_msg.extack['bad-attr'], '.ifindex')
>  
>  
> +def check_down(cfg) -> None:
> +    try:
> +        qstat = netfam.qstats_get({"ifindex": cfg.ifindex}, dump=True)
> +    except NlError as e:
> +        if e.error == 95:

Could you do this as if e.error == errno.ENOTSUP?

> +            raise KsftSkipEx("qstats not supported by the device")
> +        raise
> +
> +    ip(f"link set dev {cfg.dev['ifname']} down")
> +    defer(ip, f"link set dev {cfg.dev['ifname']} up")
> +
> +    qstat = qstat[0]

Consider moving the [0] inside the try to make the two qstats_get
statements obviously the same.

> +    qstat2 = netfam.qstats_get({"ifindex": cfg.ifindex}, dump=True)[0]
> +    for k, v in qstat.items():
> +        ksft_ge(qstat2[k], qstat[k], comment=f"{k} went backwards on device down")
> +
> +    # exercise per-queue API to make sure that "device down" state
> +    # is handled correctly and doesn't crash
> +    netfam.qstats_get({"ifindex": cfg.ifindex, "scope": "queue"}, dump=True)
> +
> +
>  def main() -> None:
>      with NetDrvEnv(__file__) as cfg:
> -        ksft_run([check_pause, check_fec, pkt_byte_sum, qstat_by_ifindex],
> +        ksft_run([check_pause, check_fec, pkt_byte_sum, qstat_by_ifindex,
> +                  check_down],
>                   args=(cfg, ))
>      ksft_exit()

Stanislav Fomichev <sdf@fomichev.me> writes:

> On 08/01, Petr Machata wrote:
>> 
>> Stanislav Fomichev <sdf@fomichev.me> writes:
>> 
>> > On 07/31, Jakub Kicinski wrote:
>> >> On Wed, 31 Jul 2024 13:34:58 +0200 Petr Machata wrote:
>> >> > > +        qstat = netfam.qstats_get({"ifindex": cfg.ifindex}, dump=True)
>> >> > > +    except NlError as e:
>> >> > > +        if e.error == 95:  
>> >> > 
>> >> > Could you do this as if e.error == errno.ENOTSUP?
>> >> 
>> >> just to be clear EOPNOTSUPP ..
>> >
>> > That might be the reason it's coded explicitly as 95? :-D
>> 
>> Both exist, I just didn't notice the latter.
>> 
>> >>> import errno
>> >>> errno.ENOTSUP
>> 95
>> >>> errno.EOPNOTSUPP
>> 95
>
> I believe Jakub was talking about kernel's ENOTSUPP (524) vs EOPNOTSUPP (95):
>
> $ grep ENOTSUPP include/linux/errno.h
> #define ENOTSUPP        524     /* Operation is not supported */
>
> $ grep EOPNOTSUPP include/uapi/asm-generic/errno.h
> #define EOPNOTSUPP      95      /* Operation not supported on transport endpoint */
>
> These two are frequently confused.
>
> OTOH, ENOTSUP looks like a userspace/libc invention:
>
> $ grep -w ENOTSUP /usr/include/bits/errno.h
> # ifndef ENOTSUP
> #  define ENOTSUP               EOPNOTSUPP
>
> I'm gonna stick to kernel's EOPNOTSUPP to make it look similar to what
> we have on the kernel side.

Yep, sounds good.