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

[Edward Cree <ecree.xilinx@gmail.com>]

On 21/06/2024 00:29, Jakub Kicinski wrote:
> Add tests focusing on indirection table configuration and
> creating extra RSS contexts in drivers which support it.
> 
>   $ ./drivers/net/hw/rss_ctx.py
>   KTAP version 1
>   1..6
>   ok 1 rss_ctx.test_rss_key_indir
>   ok 2 rss_ctx.test_rss_context
>   ok 3 rss_ctx.test_rss_context4
>   # Increasing queue count 44 -> 66
>   # Failed to create context 32, trying to test what we got
>   ok 4 rss_ctx.test_rss_context32 # SKIP Tested only 31 contexts, wanted 32
>   ok 5 rss_ctx.test_rss_context_overlap
>   ok 6 rss_ctx.test_rss_context_overlap2
>   # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:1 error:0
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
...
> +def test_rss_context(cfg, ctx_cnt=1):
> +    """
> +    Test separating traffic into RSS contexts.
> +    The queues will be allocated 2 for each context:
> +     ctx0  ctx1  ctx2  ctx3
> +    [0 1] [2 3] [4 5] [6 7] ...
> +    """
> +
> +    requested_ctx_cnt = ctx_cnt
> +
> +    # Try to allocate more queues when necessary
> +    qcnt = len(_get_rx_cnts(cfg))
> +    if qcnt >= 2 + 2 * ctx_cnt:
> +        qcnt = None
> +    else:
> +        try:
> +            ksft_pr(f"Increasing queue count {qcnt} -> {2 + 2 * ctx_cnt}")
> +            ethtool(f"-L {cfg.ifname} combined {2 + 2 * ctx_cnt}")
> +        except:
> +            raise KsftSkipEx("Not enough queues for the test")
> +
> +    ntuple = []
> +    ctx_id = []
> +    ports = []
> +    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?

...

> 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.

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).
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.

-ed