Re: [PATCH net-next 4/4] selftests: drv-net: rss_ctx: add tests for RSS configuration and contexts

[Jakub Kicinski <kuba@kernel.org>]

On Mon, 24 Jun 2024 13:55:34 +0100 Edward Cree wrote:
> > +    try:
> > +        # Use queues 0 and 1 for normal traffic
> > +        ethtool(f"-X {cfg.ifname} equal 2")
> > +
> > +        for i in range(ctx_cnt):
> > +            try:
> > +                ctx_id.append(ethtool_create(cfg, "-X", "context new"))
> > +            except CmdExitFailure:
> > +                # try to carry on and skip at the end
> > +                if i == 0:
> > +                    raise
> > +                ksft_pr(f"Failed to create context {i + 1}, trying to test what we got")
> > +                ctx_cnt = i
> > +                break
> > +
> > +            ethtool(f"-X {cfg.ifname} context {ctx_id[i]} start {2 + i * 2} equal 2")  
> 
> Is it worth also testing the single command
>     f"ethtool -X {cfg.ifname} context new start {2 + i * 2} equal 2"
>  as that will exercise the kernel & driver slightly differently to
>  first creating a context and then configuring it?

Will add!

> > diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
> > index 4769b4eb1ea1..91648c5baf40 100644
> > --- a/tools/testing/selftests/net/lib/py/ksft.py
> > +++ b/tools/testing/selftests/net/lib/py/ksft.py
> > @@ -57,6 +57,11 @@ KSFT_RESULT_ALL = True
> >          _fail("Check failed", a, "<", b, comment)
> >  
> >  
> > +def ksft_lt(a, b, comment=""):
> > +    if a > b:
> > +        _fail("Check failed", a, ">", b, comment)  
> 
> AFAICT this implements 'le' (less-or-equal), not 'lt' (less than) as
>  the name implies.

Good catch!

> Apart from that these tests LGTM as far as they go.  One thing that I
>  notice *isn't* tested here, that I generally make a point of testing,
>  is: add a bunch of contexts (and ntuple filters), remove some of
>  them, then run your traffic and make sure that the ones you left
>  intact still work (and that the deleted ones are actually gone).

Good idea, will add.

> Also wonder if it's worth adding tests for 'ethtool -x ... context N'?
>  You have it for context 0 in test_rss_key_indir(), but on custom
>  contexts it can exercise different code in the kernel.

Good point, in my head I deferred all "context N read" tests until we
have dump, but the simple read test should be added here.