[PATCH net-next v2 0/4] selftests: drv-net: rss_ctx: add tests for RSS contexts

[PATCH net-next v2 0/4] selftests: drv-net: rss_ctx: add tests for RSS contexts

[Jakub Kicinski]

Add a few tests exercising RSS context API.
In addition to basic sanity checks, tests add RSS contexts,
n-tuple rule to direct traffic to them (based on dst port),
and qstats to make sure traffic landed where we expected.

v2 adds a test for removing contexts out of order. When testing
bnxt - either the new test or running more tests after the overlap
test makes the device act strangely. To the point where it may start
giving out ntuple IDs of 0 for all rules :S

Ed, could you try the tests with your device?

  $ export NETIF=eth0 REMOTE_...
  $ ./drivers/net/hw/rss_ctx.py
  KTAP version 1
  1..8
  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
  # .. sprays traffic like a headless chicken ..
  not ok 7 rss_ctx.test_rss_context_out_of_order
  ok 8 rss_ctx.test_rss_context4_create_with_cfg
  # Totals: pass:6 fail:1 xfail:0 xpass:0 skip:1 error:0

Jakub Kicinski (4):
  selftests: drv-net: try to check if port is in use
  selftests: drv-net: add helper to wait for HW stats to sync
  selftests: drv-net: add ability to wait for at least N packets to load
    gen
  selftests: drv-net: rss_ctx: add tests for RSS configuration and
    contexts

 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../selftests/drivers/net/hw/rss_ctx.py       | 383 ++++++++++++++++++
 .../selftests/drivers/net/lib/py/env.py       |  20 +-
 .../selftests/drivers/net/lib/py/load.py      |  37 +-
 tools/testing/selftests/net/lib/py/ksft.py    |   5 +
 tools/testing/selftests/net/lib/py/utils.py   |  26 +-
 6 files changed, 457 insertions(+), 15 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/hw/rss_ctx.py

-- 
2.45.2

[PATCH net-next v2 1/4] selftests: drv-net: try to check if port is in use

[Jakub Kicinski]

We use random ports for communication. As Willem predicted
this leads to occasional failures. Try to check if port is
already in use by opening a socket and binding to that port.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - remove v4 check (Willem)
 - update comment (David, Przemek)
 - cap the iterations (Przemek)
---
 tools/testing/selftests/net/lib/py/utils.py | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 0540ea24921d..16907b51e034 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -3,6 +3,7 @@
 import json as _json
 import random
 import re
+import socket
 import subprocess
 import time
 
@@ -79,9 +80,18 @@ import time
 
 def rand_port():
     """
-    Get unprivileged port, for now just random, one day we may decide to check if used.
+    Get a random unprivileged port, try to make sure it's not already used.
     """
-    return random.randint(10000, 65535)
+    for _ in range(1000):
+        port = random.randint(10000, 65535)
+        try:
+            with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+                s.bind(("", port))
+            return port
+        except OSError as e:
+            if e.errno != 98:  # already in use
+                raise
+    raise Exception("Can't find any free unprivileged port")
 
 
 def wait_port_listen(port, proto="tcp", ns=None, host=None, sleep=0.005, deadline=5):
-- 
2.45.2

Re: [PATCH net-next v2 1/4] selftests: drv-net: try to check if port is in use

[Przemek Kitszel]

On 6/25/24 03:02, Jakub Kicinski wrote:
> We use random ports for communication. As Willem predicted
> this leads to occasional failures. Try to check if port is
> already in use by opening a socket and binding to that port.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>   - remove v4 check (Willem)
>   - update comment (David, Przemek)
>   - cap the iterations (Przemek)
> ---
>   tools/testing/selftests/net/lib/py/utils.py | 14 ++++++++++++--
>   1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index 0540ea24921d..16907b51e034 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py
> @@ -3,6 +3,7 @@
>   import json as _json
>   import random
>   import re
> +import socket
>   import subprocess
>   import time
>   
> @@ -79,9 +80,18 @@ import time
>   
>   def rand_port():
>       """
> -    Get unprivileged port, for now just random, one day we may decide to check if used.
> +    Get a random unprivileged port, try to make sure it's not already used.
>       """
> -    return random.randint(10000, 65535)
> +    for _ in range(1000):
> +        port = random.randint(10000, 65535)
> +        try:
> +            with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
> +                s.bind(("", port))
> +            return port
> +        except OSError as e:
> +            if e.errno != 98:  # already in use
> +                raise
> +    raise Exception("Can't find any free unprivileged port")
>   
>   
>   def wait_port_listen(port, proto="tcp", ns=None, host=None, sleep=0.005, deadline=5):

Reviewed-by: Przemek Kitszel <przemyslaw.kitszel@intel.com>

Re: [PATCH net-next v2 1/4] selftests: drv-net: try to check if port is in use

[Breno Leitao]

On Mon, Jun 24, 2024 at 06:02:07PM -0700, Jakub Kicinski wrote:
> We use random ports for communication. As Willem predicted
> this leads to occasional failures. Try to check if port is
> already in use by opening a socket and binding to that port.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - remove v4 check (Willem)
>  - update comment (David, Przemek)
>  - cap the iterations (Przemek)
> ---
>  tools/testing/selftests/net/lib/py/utils.py | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index 0540ea24921d..16907b51e034 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py

> +        except OSError as e:
> +            if e.errno != 98:  # already in use

To make it a bit clearer, you can use something as:

	import errno

	if e.errno != errno.EADDRINUSE

[PATCH net-next v2 2/4] selftests: drv-net: add helper to wait for HW stats to sync

[Jakub Kicinski]

Some devices DMA stats to the host periodically. Add a helper
which can wait for that to happen, based on frequency reported
by the driver in ethtool.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - sleep for 25ms on top of the driver DMA period
   (and remove confusing comment)
---
 .../selftests/drivers/net/lib/py/env.py       | 20 ++++++++++++++++++-
 tools/testing/selftests/net/lib/py/utils.py   |  4 ++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
index edcedd7bffab..16d24fe7107d 100644
--- a/tools/testing/selftests/drivers/net/lib/py/env.py
+++ b/tools/testing/selftests/drivers/net/lib/py/env.py
@@ -1,9 +1,10 @@
 # SPDX-License-Identifier: GPL-2.0
 
 import os
+import time
 from pathlib import Path
 from lib.py import KsftSkipEx, KsftXfailEx
-from lib.py import cmd, ip
+from lib.py import cmd, ethtool, ip
 from lib.py import NetNS, NetdevSimDev
 from .remote import Remote
 
@@ -82,6 +83,8 @@ from .remote import Remote
 
         self.env = _load_env_file(src_path)
 
+        self._stats_settle_time = None
+
         # Things we try to destroy
         self.remote = None
         # These are for local testing state
@@ -222,3 +225,18 @@ from .remote import Remote
         if remote:
             if not self._require_cmd(comm, "remote"):
                 raise KsftSkipEx("Test requires (remote) command: " + comm)
+
+    def wait_hw_stats_settle(self):
+        """
+        Wait for HW stats to become consistent, some devices DMA HW stats
+        periodically so events won't be reflected until next sync.
+        Good drivers will tell us via ethtool what their sync period is.
+        """
+        if self._stats_settle_time is None:
+            self._stats_settle_time = 0.025
+
+            data = ethtool("-c " + self.ifname, json=True)[0]
+            if 'stats-block-usecs' in data:
+                self._stats_settle_time += data['stats-block-usecs'] / 1000 / 1000
+
+        time.sleep(self._stats_settle_time)
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 16907b51e034..11dbdd3b7612 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -78,6 +78,10 @@ import time
     return tool('ip', args, json=json, host=host)
 
 
+def ethtool(args, json=None, ns=None, host=None):
+    return tool('ethtool', args,  json=json, ns=ns, host=host)
+
+
 def rand_port():
     """
     Get a random unprivileged port, try to make sure it's not already used.
-- 
2.45.2

Re: [PATCH net-next v2 2/4] selftests: drv-net: add helper to wait for HW stats to sync

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Some devices DMA stats to the host periodically. Add a helper
> which can wait for that to happen, based on frequency reported
> by the driver in ethtool.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - sleep for 25ms on top of the driver DMA period
>    (and remove confusing comment)
> ---
>  .../selftests/drivers/net/lib/py/env.py       | 20 ++++++++++++++++++-
>  tools/testing/selftests/net/lib/py/utils.py   |  4 ++++
>  2 files changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/drivers/net/lib/py/env.py b/tools/testing/selftests/drivers/net/lib/py/env.py
> index edcedd7bffab..16d24fe7107d 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/env.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/env.py
> @@ -1,9 +1,10 @@
>  # SPDX-License-Identifier: GPL-2.0
>  
>  import os
> +import time
>  from pathlib import Path
>  from lib.py import KsftSkipEx, KsftXfailEx
> -from lib.py import cmd, ip
> +from lib.py import cmd, ethtool, ip
>  from lib.py import NetNS, NetdevSimDev
>  from .remote import Remote
>  
> @@ -82,6 +83,8 @@ from .remote import Remote
>  
>          self.env = _load_env_file(src_path)
>  
> +        self._stats_settle_time = None
> +
>          # Things we try to destroy
>          self.remote = None
>          # These are for local testing state
> @@ -222,3 +225,18 @@ from .remote import Remote
>          if remote:
>              if not self._require_cmd(comm, "remote"):
>                  raise KsftSkipEx("Test requires (remote) command: " + comm)
> +
> +    def wait_hw_stats_settle(self):
> +        """
> +        Wait for HW stats to become consistent, some devices DMA HW stats
> +        periodically so events won't be reflected until next sync.
> +        Good drivers will tell us via ethtool what their sync period is.
> +        """
> +        if self._stats_settle_time is None:
> +            self._stats_settle_time = 0.025
> +
> +            data = ethtool("-c " + self.ifname, json=True)[0]
> +            if 'stats-block-usecs' in data:
> +                self._stats_settle_time += data['stats-block-usecs'] / 1000 / 1000

If there's a v3, this dual check-then-access could be folded into a
.get() and the whole expression then gets a bit simpler:

            data = ethtool("-c " + self.ifname, json=True)[0]
            self._stats_settle_time = 0.25 + \
                    data.get('stats-block-usecs', 0) / 1000 / 1000

> +
> +        time.sleep(self._stats_settle_time)
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index 16907b51e034..11dbdd3b7612 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py
> @@ -78,6 +78,10 @@ import time
>      return tool('ip', args, json=json, host=host)
>  
>  
> +def ethtool(args, json=None, ns=None, host=None):
> +    return tool('ethtool', args,  json=json, ns=ns, host=host)

Double space here.

> +
> +
>  def rand_port():
>      """
>      Get a random unprivileged port, try to make sure it's not already used.

[PATCH net-next v2 3/4] selftests: drv-net: add ability to wait for at least N packets to load gen

[Jakub Kicinski]

Teach the load generator how to wait for at least given number
of packets to be received. This will be useful for filtering
where we'll want to send a non-trivial number of packets and
make sure they landed in right queues.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - add comment that pps or pkt_cnt are mutually exclusive (David)
 - rename variables (David)
---
 .../selftests/drivers/net/lib/py/load.py      | 30 ++++++++++++++-----
 1 file changed, 23 insertions(+), 7 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/lib/py/load.py b/tools/testing/selftests/drivers/net/lib/py/load.py
index abdb677bdb1c..31f82f1e32c1 100644
--- a/tools/testing/selftests/drivers/net/lib/py/load.py
+++ b/tools/testing/selftests/drivers/net/lib/py/load.py
@@ -18,15 +18,31 @@ from lib.py import ksft_pr, cmd, ip, rand_port, wait_port_listen
                                  background=True, host=env.remote)
 
         # Wait for traffic to ramp up
-        pkt = ip("-s link show dev " + env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
+        if not self._wait_pkts(pps=1000):
+            self.stop(verbose=True)
+            raise Exception("iperf3 traffic did not ramp up")
+
+    def _wait_pkts(self, pkt_cnt=None, pps=None):
+        """
+        Wait until we've seen pkt_cnt or until traffic ramps up to pps.
+        Only one of pkt_cnt or pss can be specified.
+        """
+        pkt_end = ip("-s link show dev " + self.env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
         for _ in range(50):
             time.sleep(0.1)
-            now = ip("-s link show dev " + env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
-            if now - pkt > 1000:
-                return
-            pkt = now
-        self.stop(verbose=True)
-        raise Exception("iperf3 traffic did not ramp up")
+            pkt_start = ip("-s link show dev " + self.env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
+            if pps:
+                if pkt_start - pkt_end > pps / 10:
+                    return True
+                pkt_end = pkt_start
+            elif pkt_cnt:
+                if pkt_start - pkt_end > pkt_cnt:
+                    return True
+        return False
+
+    def wait_pkts_and_stop(self, pkt_cnt):
+        failed = not self._wait_pkts(pkt_cnt=pkt_cnt)
+        self.stop(verbose=failed)
 
     def stop(self, verbose=None):
         self._iperf_client.process(terminate=True)
-- 
2.45.2

Re: [PATCH net-next v2 3/4] selftests: drv-net: add ability to wait for at least N packets to load gen

[Breno Leitao]

On Mon, Jun 24, 2024 at 06:02:09PM -0700, Jakub Kicinski wrote:
> Teach the load generator how to wait for at least given number
> of packets to be received. This will be useful for filtering
> where we'll want to send a non-trivial number of packets and
> make sure they landed in right queues.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviwed-by: Breno Leitao <leitao@debian.org>

Re: [PATCH net-next v2 3/4] selftests: drv-net: add ability to wait for at least N packets to load gen

[Willem de Bruijn]

Jakub Kicinski wrote:
> Teach the load generator how to wait for at least given number
> of packets to be received. This will be useful for filtering
> where we'll want to send a non-trivial number of packets and
> make sure they landed in right queues.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - add comment that pps or pkt_cnt are mutually exclusive (David)
>  - rename variables (David)
> ---
>  .../selftests/drivers/net/lib/py/load.py      | 30 ++++++++++++++-----
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/lib/py/load.py b/tools/testing/selftests/drivers/net/lib/py/load.py
> index abdb677bdb1c..31f82f1e32c1 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/load.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/load.py
> @@ -18,15 +18,31 @@ from lib.py import ksft_pr, cmd, ip, rand_port, wait_port_listen
>                                   background=True, host=env.remote)
>  
>          # Wait for traffic to ramp up
> -        pkt = ip("-s link show dev " + env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
> +        if not self._wait_pkts(pps=1000):
> +            self.stop(verbose=True)
> +            raise Exception("iperf3 traffic did not ramp up")
> +
> +    def _wait_pkts(self, pkt_cnt=None, pps=None):
> +        """
> +        Wait until we've seen pkt_cnt or until traffic ramps up to pps.
> +        Only one of pkt_cnt or pss can be specified.
> +        """
> +        pkt_end = ip("-s link show dev " + self.env.ifname, json=True)[0]["stats64"]["rx"]["packets"]

nit: kind of confusing that the first reading is called pkt_end.

I'd s/pkt_end/pkt_start and pkt_start/pkt_now.

Obviously only bikeshedding at this point. Feel free to ignore.

>          for _ in range(50):
>              time.sleep(0.1)
> -            now = ip("-s link show dev " + env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
> -            if now - pkt > 1000:
> -                return
> -            pkt = now
> -        self.stop(verbose=True)
> -        raise Exception("iperf3 traffic did not ramp up")
> +            pkt_start = ip("-s link show dev " + self.env.ifname, json=True)[0]["stats64"]["rx"]["packets"]
> +            if pps:
> +                if pkt_start - pkt_end > pps / 10:
> +                    return True
> +                pkt_end = pkt_start
> +            elif pkt_cnt:
> +                if pkt_start - pkt_end > pkt_cnt:
> +                    return True
> +        return False
> +
> +    def wait_pkts_and_stop(self, pkt_cnt):
> +        failed = not self._wait_pkts(pkt_cnt=pkt_cnt)
> +        self.stop(verbose=failed)
>  
>      def stop(self, verbose=None):
>          self._iperf_client.process(terminate=True)
> -- 
> 2.45.2
> 

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

[Jakub Kicinski]

Add tests focusing on indirection table configuration and
creating extra RSS contexts in drivers which support it.

  $ export NETIF=eth0 REMOTE_...
  $ ./drivers/net/hw/rss_ctx.py
  KTAP version 1
  1..8
  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
  # .. sprays traffic like a headless chicken ..
  not ok 7 rss_ctx.test_rss_context_out_of_order
  ok 8 rss_ctx.test_rss_context4_create_with_cfg
  # Totals: pass:6 fail:1 xfail:0 xpass:0 skip:1 error:0

Note that rss_ctx.test_rss_context_out_of_order fails with the device
I tested with, but it seems to be a device / driver bug.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - ensure right queue count for each test (David)
 - check driver has ntuple filters before starting (Willem)
 - add test for creating contexts with indir table specified (Ed)
 - fix ksft_lt (Ed)
 - query and validate indirection tables of non-first context (Ed)
 - test traffic steering vs OOO context removal (Ed)
 - make sure overlap test deletes all rules, 0 is a valid ntuple ID,
   so we can't do "if ntuple: remove()"
---
 .../testing/selftests/drivers/net/hw/Makefile |   1 +
 .../selftests/drivers/net/hw/rss_ctx.py       | 383 ++++++++++++++++++
 .../selftests/drivers/net/lib/py/load.py      |   7 +-
 tools/testing/selftests/net/lib/py/ksft.py    |   5 +
 tools/testing/selftests/net/lib/py/utils.py   |   8 +-
 5 files changed, 399 insertions(+), 5 deletions(-)
 create mode 100755 tools/testing/selftests/drivers/net/hw/rss_ctx.py

diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
index 4933d045ab66..c9f2f48fc30f 100644
--- a/tools/testing/selftests/drivers/net/hw/Makefile
+++ b/tools/testing/selftests/drivers/net/hw/Makefile
@@ -11,6 +11,7 @@ TEST_PROGS = \
 	hw_stats_l3_gre.sh \
 	loopback.sh \
 	pp_alloc_fail.py \
+	rss_ctx.py \
 	#
 
 TEST_FILES := \
diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
new file mode 100755
index 000000000000..c9c864d5f7d1
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
@@ -0,0 +1,383 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+import datetime
+import random
+from lib.py import ksft_run, ksft_pr, ksft_exit, ksft_eq, ksft_ge, ksft_lt
+from lib.py import NetDrvEpEnv
+from lib.py import NetdevFamily
+from lib.py import KsftSkipEx
+from lib.py import rand_port
+from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
+
+
+def _rss_key_str(key):
+    return ":".join(["{:02x}".format(x) for x in key])
+
+
+def _rss_key_rand(length):
+    return [random.randint(0, 255) for _ in range(length)]
+
+
+def get_rss(cfg, context=0):
+    return ethtool(f"-x {cfg.ifname} context {context}", json=True)[0]
+
+
+def get_drop_err_sum(cfg):
+    stats = ip("-s -s link show dev " + cfg.ifname, json=True)[0]
+    cnt = 0
+    for key in ['errors', 'dropped', 'over_errors', 'fifo_errors',
+                'length_errors', 'crc_errors', 'missed_errors',
+                'frame_errors']:
+        cnt += stats["stats64"]["rx"][key]
+    return cnt, stats["stats64"]["tx"]["carrier_changes"]
+
+
+def ethtool_create(cfg, act, opts):
+    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
+    # Output will be something like: "New RSS context is 1" or
+    # "Added rule with ID 7", we want the integer from the end
+    return int(output.split()[-1])
+
+
+def require_ntuple(cfg):
+    features = ethtool(f"-k {cfg.ifname}", json=True)[0]
+    if not features["ntuple-filters"]["active"]:
+        # ntuple is more of a capability than a config knob, don't bother
+        # trying to enable it (until some driver actually needs it).
+        raise KsftSkipEx("Ntuple filters not enabled on the device: " + str(features["ntuple-filters"]))
+
+
+# Get Rx packet counts for all queues, as a simple list of integers
+# if @prev is specified the prev counts will be subtracted
+def _get_rx_cnts(cfg, prev=None):
+    cfg.wait_hw_stats_settle()
+    data = cfg.netdevnl.qstats_get({"ifindex": cfg.ifindex, "scope": ["queue"]}, dump=True)
+    data = [x for x in data if x['queue-type'] == "rx"]
+    max_q = max([x["queue-id"] for x in data])
+    queue_stats = [0] * (max_q + 1)
+    for q in data:
+        queue_stats[q["queue-id"]] = q["rx-packets"]
+        if prev and q["queue-id"] < len(prev):
+            queue_stats[q["queue-id"]] -= prev[q["queue-id"]]
+    return queue_stats
+
+
+def test_rss_key_indir(cfg):
+    """
+    Test basics like updating the main RSS key and indirection table.
+    """
+    if len(_get_rx_cnts(cfg)) < 2:
+        KsftSkipEx("Device has only one queue (or doesn't support queue stats)")
+
+    data = get_rss(cfg)
+    want_keys = ['rss-hash-key', 'rss-hash-function', 'rss-indirection-table']
+    for k in want_keys:
+        if k not in data:
+            raise KsftFailEx("ethtool results missing key: " + k)
+        if not data[k]:
+            raise KsftFailEx(f"ethtool results empty for '{k}': {data[k]}")
+
+    key_len = len(data['rss-hash-key'])
+
+    # Set the key
+    key = _rss_key_rand(key_len)
+    ethtool(f"-X {cfg.ifname} hkey " + _rss_key_str(key))
+
+    data = get_rss(cfg)
+    ksft_eq(key, data['rss-hash-key'])
+
+    # Set the indirection table
+    ethtool(f"-X {cfg.ifname} equal 2")
+    data = get_rss(cfg)
+    ksft_eq(0, min(data['rss-indirection-table']))
+    ksft_eq(1, max(data['rss-indirection-table']))
+
+    # Check we only get traffic on the first 2 queues
+    cnts = _get_rx_cnts(cfg)
+    GenerateTraffic(cfg).wait_pkts_and_stop(20000)
+    cnts = _get_rx_cnts(cfg, prev=cnts)
+    # 2 queues, 20k packets, must be at least 5k per queue
+    ksft_ge(cnts[0], 5000, "traffic on main context (1/2): " + str(cnts))
+    ksft_ge(cnts[1], 5000, "traffic on main context (2/2): " + str(cnts))
+    # The other queues should be unused
+    ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
+
+    # Restore, and check traffic gets spread again
+    ethtool(f"-X {cfg.ifname} default")
+
+    cnts = _get_rx_cnts(cfg)
+    GenerateTraffic(cfg).wait_pkts_and_stop(20000)
+    cnts = _get_rx_cnts(cfg, prev=cnts)
+    # First two queues get less traffic than all the rest
+    ksft_ge(sum(cnts[2:]), sum(cnts[:2]), "traffic distributed: " + str(cnts))
+
+
+def test_rss_context(cfg, ctx_cnt=1, create_with_cfg=None):
+    """
+    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] ...
+    """
+
+    require_ntuple(cfg)
+
+    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):
+            want_cfg = f"start {2 + i * 2} equal 2"
+            create_cfg = want_cfg if create_with_cfg else ""
+
+            try:
+                ctx_id.append(ethtool_create(cfg, "-X", f"context new {create_cfg}"))
+            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
+
+            if not create_with_cfg:
+                ethtool(f"-X {cfg.ifname} context {ctx_id[i]} {want_cfg}")
+
+            # Sanity check the context we just created
+            data = get_rss(cfg, ctx_id[i])
+            ksft_eq(min(data['rss-indirection-table']), 2 + i * 2, "Unexpected context cfg: " + str(data))
+            ksft_eq(max(data['rss-indirection-table']), 2 + i * 2 + 1, "Unexpected context cfg: " + str(data))
+
+            ports.append(rand_port())
+            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
+            ntuple.append(ethtool_create(cfg, "-N", flow))
+
+        for i in range(ctx_cnt):
+            cnts = _get_rx_cnts(cfg)
+            GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
+            cnts = _get_rx_cnts(cfg, prev=cnts)
+
+            ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
+            ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
+            ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
+    finally:
+        for nid in ntuple:
+            ethtool(f"-N {cfg.ifname} delete {nid}")
+        for cid in ctx_id:
+            ethtool(f"-X {cfg.ifname} context {cid} delete")
+        ethtool(f"-X {cfg.ifname} default")
+        if qcnt:
+            ethtool(f"-L {cfg.ifname} combined {qcnt}")
+
+    if requested_ctx_cnt != ctx_cnt:
+        raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
+
+
+def test_rss_context4(cfg):
+    test_rss_context(cfg, 4)
+
+
+def test_rss_context32(cfg):
+    test_rss_context(cfg, 32)
+
+
+def test_rss_context4_create_with_cfg(cfg):
+    test_rss_context(cfg, 4, create_with_cfg=True)
+
+
+def test_rss_context_out_of_order(cfg, ctx_cnt=4):
+    """
+    Test separating traffic into RSS contexts.
+    Contexts are removed in semi-random order, and steering re-tested
+    to make sure removal doesn't break steering to surviving contexts.
+    Test requires 3 contexts to work.
+    """
+
+    require_ntuple(cfg)
+
+    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")
+
+    removed = [False] * ctx_cnt
+    ntuple = []
+    ctx_id = []
+    ports = []
+
+    def remove_ctx(idx):
+        ethtool(f"-N {cfg.ifname} delete {ntuple[idx]}")
+        del ntuple[idx]
+        ethtool(f"-X {cfg.ifname} context {ctx_id[idx]} delete")
+        del ctx_id[idx]
+        removed[idx] = True
+
+    def check_traffic():
+        for i in range(ctx_cnt):
+            cnts = _get_rx_cnts(cfg)
+            GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
+            cnts = _get_rx_cnts(cfg, prev=cnts)
+
+            if not removed[i]:
+                ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
+                ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
+                ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
+            else:
+                ksft_ge(sum(cnts[ :2]), 20000, "traffic on main context:" + str(cnts))
+                ksft_eq(sum(cnts[2: ]),     0, "traffic on other contexts: " + str(cnts))
+
+    try:
+        # Use queues 0 and 1 for normal traffic
+        ethtool(f"-X {cfg.ifname} equal 2")
+
+        for i in range(ctx_cnt):
+            ctx_id.append(ethtool_create(cfg, "-X", f"context new start {2 + i * 2} equal 2"))
+
+            ports.append(rand_port())
+            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
+            ntuple.append(ethtool_create(cfg, "-N", flow))
+
+        check_traffic()
+
+        # Remove middle context
+        remove_ctx(ctx_cnt // 2)
+        check_traffic()
+
+        # Remove first context
+        remove_ctx(0)
+        check_traffic()
+
+        # Remove last context
+        remove_ctx(-1)
+        check_traffic()
+
+    finally:
+        for nid in ntuple:
+            ethtool(f"-N {cfg.ifname} delete {nid}")
+        for cid in ctx_id:
+            ethtool(f"-X {cfg.ifname} context {cid} delete")
+        ethtool(f"-X {cfg.ifname} default")
+        if qcnt:
+            ethtool(f"-L {cfg.ifname} combined {qcnt}")
+
+    if requested_ctx_cnt != ctx_cnt:
+        raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
+
+
+def test_rss_context_overlap(cfg, other_ctx=0):
+    """
+    Test contexts overlapping with each other.
+    Use 4 queues for the main context, but only queues 2 and 3 for context 1.
+    """
+
+    require_ntuple(cfg)
+
+    queue_cnt = len(_get_rx_cnts(cfg))
+    if queue_cnt >= 4:
+        queue_cnt = None
+    else:
+        try:
+            ksft_pr(f"Increasing queue count {queue_cnt} -> 4")
+            ethtool(f"-L {cfg.ifname} combined 4")
+        except:
+            raise KsftSkipEx("Not enough queues for the test")
+
+    ctx_id = None
+    ntuple = None
+    if other_ctx == 0:
+        ethtool(f"-X {cfg.ifname} equal 4")
+    else:
+        other_ctx = ethtool_create(cfg, "-X", "context new")
+        ethtool(f"-X {cfg.ifname} context {other_ctx} equal 4")
+
+    try:
+        ctx_id = ethtool_create(cfg, "-X", "context new")
+        ethtool(f"-X {cfg.ifname} context {ctx_id} start 2 equal 2")
+
+        port = rand_port()
+        if other_ctx:
+            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {other_ctx}"
+            ntuple = ethtool_create(cfg, "-N", flow)
+
+        # Test the main context
+        cnts = _get_rx_cnts(cfg)
+        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
+        cnts = _get_rx_cnts(cfg, prev=cnts)
+
+        ksft_ge(sum(cnts[ :4]), 20000, "traffic on main context: " + str(cnts))
+        ksft_ge(sum(cnts[ :2]),  7000, "traffic on main context (1/2): " + str(cnts))
+        ksft_ge(sum(cnts[2:4]),  7000, "traffic on main context (2/2): " + str(cnts))
+        if other_ctx == 0:
+            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
+
+        # Now create a rule for context 1 and make sure traffic goes to a subset
+        if other_ctx:
+            ethtool(f"-N {cfg.ifname} delete {ntuple}")
+            ntuple = None
+        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {ctx_id}"
+        ntuple = ethtool_create(cfg, "-N", flow)
+
+        cnts = _get_rx_cnts(cfg)
+        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
+        cnts = _get_rx_cnts(cfg, prev=cnts)
+
+        ksft_lt(sum(cnts[ :2]),  7000, "traffic on main context: " + str(cnts))
+        ksft_ge(sum(cnts[2:4]), 20000, "traffic on extra context: " + str(cnts))
+        if other_ctx == 0:
+            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
+    finally:
+        if ntuple is not None:
+            ethtool(f"-N {cfg.ifname} delete {ntuple}")
+        if ctx_id:
+            ethtool(f"-X {cfg.ifname} context {ctx_id} delete")
+        if other_ctx == 0:
+            ethtool(f"-X {cfg.ifname} default")
+        else:
+            ethtool(f"-X {cfg.ifname} context {other_ctx} delete")
+        if queue_cnt:
+            ethtool(f"-L {cfg.ifname} combined {queue_cnt}")
+
+
+def test_rss_context_overlap2(cfg):
+    test_rss_context_overlap(cfg, True)
+
+
+def main() -> None:
+    with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
+        cfg.netdevnl = NetdevFamily()
+
+        ksft_run([test_rss_key_indir,
+                  test_rss_context, test_rss_context4, test_rss_context32,
+                  test_rss_context_overlap, test_rss_context_overlap2,
+                  test_rss_context_out_of_order, test_rss_context4_create_with_cfg],
+                 args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
diff --git a/tools/testing/selftests/drivers/net/lib/py/load.py b/tools/testing/selftests/drivers/net/lib/py/load.py
index 31f82f1e32c1..0f40a13926d0 100644
--- a/tools/testing/selftests/drivers/net/lib/py/load.py
+++ b/tools/testing/selftests/drivers/net/lib/py/load.py
@@ -5,13 +5,14 @@ import time
 from lib.py import ksft_pr, cmd, ip, rand_port, wait_port_listen
 
 class GenerateTraffic:
-    def __init__(self, env):
+    def __init__(self, env, port=None):
         env.require_cmd("iperf3", remote=True)
 
         self.env = env
 
-        port = rand_port()
-        self._iperf_server = cmd(f"iperf3 -s -p {port}", background=True)
+        if port is None:
+            port = rand_port()
+        self._iperf_server = cmd(f"iperf3 -s -1 -p {port}", background=True)
         wait_port_listen(port)
         time.sleep(0.1)
         self._iperf_client = cmd(f"iperf3 -c {env.addr} -P 16 -p {port} -t 86400",
diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index 4769b4eb1ea1..45ffe277d94a 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)
+
+
 class ksft_raises:
     def __init__(self, expected_type):
         self.exception = None
diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
index 11dbdd3b7612..231e4a2f0252 100644
--- a/tools/testing/selftests/net/lib/py/utils.py
+++ b/tools/testing/selftests/net/lib/py/utils.py
@@ -8,6 +8,10 @@ import subprocess
 import time
 
 
+class CmdExitFailure(Exception):
+    pass
+
+
 class cmd:
     def __init__(self, comm, shell=True, fail=True, ns=None, background=False, host=None, timeout=5):
         if ns:
@@ -42,8 +46,8 @@ import time
         if self.proc.returncode != 0 and fail:
             if len(stderr) > 0 and stderr[-1] == "\n":
                 stderr = stderr[:-1]
-            raise Exception("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
-                            (self.proc.args, stdout, stderr))
+            raise CmdExitFailure("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
+                                 (self.proc.args, stdout, stderr))
 
 
 class bkg(cmd):
-- 
2.45.2

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

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Add tests focusing on indirection table configuration and
> creating extra RSS contexts in drivers which support it.
>
>   $ export NETIF=eth0 REMOTE_...
>   $ ./drivers/net/hw/rss_ctx.py
>   KTAP version 1
>   1..8
>   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
>   # .. sprays traffic like a headless chicken ..
>   not ok 7 rss_ctx.test_rss_context_out_of_order
>   ok 8 rss_ctx.test_rss_context4_create_with_cfg
>   # Totals: pass:6 fail:1 xfail:0 xpass:0 skip:1 error:0
>
> Note that rss_ctx.test_rss_context_out_of_order fails with the device
> I tested with, but it seems to be a device / driver bug.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> v2:
>  - ensure right queue count for each test (David)
>  - check driver has ntuple filters before starting (Willem)
>  - add test for creating contexts with indir table specified (Ed)
>  - fix ksft_lt (Ed)
>  - query and validate indirection tables of non-first context (Ed)
>  - test traffic steering vs OOO context removal (Ed)
>  - make sure overlap test deletes all rules, 0 is a valid ntuple ID,
>    so we can't do "if ntuple: remove()"
> ---
>  .../testing/selftests/drivers/net/hw/Makefile |   1 +
>  .../selftests/drivers/net/hw/rss_ctx.py       | 383 ++++++++++++++++++
>  .../selftests/drivers/net/lib/py/load.py      |   7 +-
>  tools/testing/selftests/net/lib/py/ksft.py    |   5 +
>  tools/testing/selftests/net/lib/py/utils.py   |   8 +-
>  5 files changed, 399 insertions(+), 5 deletions(-)
>  create mode 100755 tools/testing/selftests/drivers/net/hw/rss_ctx.py
>
> diff --git a/tools/testing/selftests/drivers/net/hw/Makefile b/tools/testing/selftests/drivers/net/hw/Makefile
> index 4933d045ab66..c9f2f48fc30f 100644
> --- a/tools/testing/selftests/drivers/net/hw/Makefile
> +++ b/tools/testing/selftests/drivers/net/hw/Makefile
> @@ -11,6 +11,7 @@ TEST_PROGS = \
>  	hw_stats_l3_gre.sh \
>  	loopback.sh \
>  	pp_alloc_fail.py \
> +	rss_ctx.py \
>  	#
>  
>  TEST_FILES := \
> diff --git a/tools/testing/selftests/drivers/net/hw/rss_ctx.py b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> new file mode 100755
> index 000000000000..c9c864d5f7d1
> --- /dev/null
> +++ b/tools/testing/selftests/drivers/net/hw/rss_ctx.py
> @@ -0,0 +1,383 @@
> +#!/usr/bin/env python3
> +# SPDX-License-Identifier: GPL-2.0
> +
> +import datetime
> +import random
> +from lib.py import ksft_run, ksft_pr, ksft_exit, ksft_eq, ksft_ge, ksft_lt
> +from lib.py import NetDrvEpEnv
> +from lib.py import NetdevFamily
> +from lib.py import KsftSkipEx
> +from lib.py import rand_port
> +from lib.py import ethtool, ip, GenerateTraffic, CmdExitFailure
> +
> +
> +def _rss_key_str(key):
> +    return ":".join(["{:02x}".format(x) for x in key])
> +
> +
> +def _rss_key_rand(length):
> +    return [random.randint(0, 255) for _ in range(length)]
> +
> +
> +def get_rss(cfg, context=0):
> +    return ethtool(f"-x {cfg.ifname} context {context}", json=True)[0]
> +
> +
> +def get_drop_err_sum(cfg):
> +    stats = ip("-s -s link show dev " + cfg.ifname, json=True)[0]
> +    cnt = 0
> +    for key in ['errors', 'dropped', 'over_errors', 'fifo_errors',
> +                'length_errors', 'crc_errors', 'missed_errors',
> +                'frame_errors']:
> +        cnt += stats["stats64"]["rx"][key]
> +    return cnt, stats["stats64"]["tx"]["carrier_changes"]
> +
> +
> +def ethtool_create(cfg, act, opts):
> +    output = ethtool(f"{act} {cfg.ifname} {opts}").stdout
> +    # Output will be something like: "New RSS context is 1" or
> +    # "Added rule with ID 7", we want the integer from the end
> +    return int(output.split()[-1])
> +
> +
> +def require_ntuple(cfg):
> +    features = ethtool(f"-k {cfg.ifname}", json=True)[0]
> +    if not features["ntuple-filters"]["active"]:
> +        # ntuple is more of a capability than a config knob, don't bother
> +        # trying to enable it (until some driver actually needs it).
> +        raise KsftSkipEx("Ntuple filters not enabled on the device: " + str(features["ntuple-filters"]))
> +
> +
> +# Get Rx packet counts for all queues, as a simple list of integers
> +# if @prev is specified the prev counts will be subtracted
> +def _get_rx_cnts(cfg, prev=None):
> +    cfg.wait_hw_stats_settle()
> +    data = cfg.netdevnl.qstats_get({"ifindex": cfg.ifindex, "scope": ["queue"]}, dump=True)
> +    data = [x for x in data if x['queue-type'] == "rx"]
> +    max_q = max([x["queue-id"] for x in data])
> +    queue_stats = [0] * (max_q + 1)
> +    for q in data:
> +        queue_stats[q["queue-id"]] = q["rx-packets"]
> +        if prev and q["queue-id"] < len(prev):
> +            queue_stats[q["queue-id"]] -= prev[q["queue-id"]]
> +    return queue_stats
> +
> +
> +def test_rss_key_indir(cfg):
> +    """
> +    Test basics like updating the main RSS key and indirection table.
> +    """
> +    if len(_get_rx_cnts(cfg)) < 2:
> +        KsftSkipEx("Device has only one queue (or doesn't support queue stats)")

I'm not sure, is this admin-correctible configuration issue? It looks
like this and some others should rather be XFAIL.

> +    data = get_rss(cfg)
> +    want_keys = ['rss-hash-key', 'rss-hash-function', 'rss-indirection-table']
> +    for k in want_keys:
> +        if k not in data:
> +            raise KsftFailEx("ethtool results missing key: " + k)
> +        if not data[k]:
> +            raise KsftFailEx(f"ethtool results empty for '{k}': {data[k]}")
> +
> +    key_len = len(data['rss-hash-key'])
> +
> +    # Set the key
> +    key = _rss_key_rand(key_len)
> +    ethtool(f"-X {cfg.ifname} hkey " + _rss_key_str(key))
> +
> +    data = get_rss(cfg)
> +    ksft_eq(key, data['rss-hash-key'])
> +
> +    # Set the indirection table
> +    ethtool(f"-X {cfg.ifname} equal 2")
> +    data = get_rss(cfg)
> +    ksft_eq(0, min(data['rss-indirection-table']))
> +    ksft_eq(1, max(data['rss-indirection-table']))
> +
> +    # Check we only get traffic on the first 2 queues
> +    cnts = _get_rx_cnts(cfg)
> +    GenerateTraffic(cfg).wait_pkts_and_stop(20000)
> +    cnts = _get_rx_cnts(cfg, prev=cnts)
> +    # 2 queues, 20k packets, must be at least 5k per queue
> +    ksft_ge(cnts[0], 5000, "traffic on main context (1/2): " + str(cnts))
> +    ksft_ge(cnts[1], 5000, "traffic on main context (2/2): " + str(cnts))
> +    # The other queues should be unused
> +    ksft_eq(sum(cnts[2:]), 0, "traffic on unused queues: " + str(cnts))
> +
> +    # Restore, and check traffic gets spread again
> +    ethtool(f"-X {cfg.ifname} default")
> +
> +    cnts = _get_rx_cnts(cfg)
> +    GenerateTraffic(cfg).wait_pkts_and_stop(20000)
> +    cnts = _get_rx_cnts(cfg, prev=cnts)
> +    # First two queues get less traffic than all the rest
> +    ksft_ge(sum(cnts[2:]), sum(cnts[:2]), "traffic distributed: " + str(cnts))

Now that you added ksft_lt, this can be made to actually match the comment:

    ksft_lt(sum(cnts[:2]), sum(cnts[2:]), "traffic distributed: " + str(cnts))

> +
> +
> +def test_rss_context(cfg, ctx_cnt=1, create_with_cfg=None):
> +    """
> +    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] ...
> +    """
> +
> +    require_ntuple(cfg)
> +
> +    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):
> +            want_cfg = f"start {2 + i * 2} equal 2"
> +            create_cfg = want_cfg if create_with_cfg else ""
> +
> +            try:
> +                ctx_id.append(ethtool_create(cfg, "-X", f"context new {create_cfg}"))
> +            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
> +
> +            if not create_with_cfg:
> +                ethtool(f"-X {cfg.ifname} context {ctx_id[i]} {want_cfg}")
> +
> +            # Sanity check the context we just created
> +            data = get_rss(cfg, ctx_id[i])
> +            ksft_eq(min(data['rss-indirection-table']), 2 + i * 2, "Unexpected context cfg: " + str(data))
> +            ksft_eq(max(data['rss-indirection-table']), 2 + i * 2 + 1, "Unexpected context cfg: " + str(data))
> +
> +            ports.append(rand_port())
> +            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
> +            ntuple.append(ethtool_create(cfg, "-N", flow))
> +
> +        for i in range(ctx_cnt):
> +            cnts = _get_rx_cnts(cfg)
> +            GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
> +            cnts = _get_rx_cnts(cfg, prev=cnts)
> +
> +            ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
> +            ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
> +            ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
> +    finally:
> +        for nid in ntuple:
> +            ethtool(f"-N {cfg.ifname} delete {nid}")
> +        for cid in ctx_id:
> +            ethtool(f"-X {cfg.ifname} context {cid} delete")
> +        ethtool(f"-X {cfg.ifname} default")
> +        if qcnt:
> +            ethtool(f"-L {cfg.ifname} combined {qcnt}")
> +
> +    if requested_ctx_cnt != ctx_cnt:
> +        raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
> +
> +
> +def test_rss_context4(cfg):
> +    test_rss_context(cfg, 4)
> +
> +
> +def test_rss_context32(cfg):
> +    test_rss_context(cfg, 32)
> +
> +
> +def test_rss_context4_create_with_cfg(cfg):
> +    test_rss_context(cfg, 4, create_with_cfg=True)
> +
> +
> +def test_rss_context_out_of_order(cfg, ctx_cnt=4):
> +    """
> +    Test separating traffic into RSS contexts.
> +    Contexts are removed in semi-random order, and steering re-tested
> +    to make sure removal doesn't break steering to surviving contexts.
> +    Test requires 3 contexts to work.
> +    """
> +
> +    require_ntuple(cfg)
> +
> +    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")

There are variations on this in each of the three tests. It would make
sense to extract to a helper, or perhaps even write as a context
manager. Untested:

class require_contexts:
    def __init__(self, cfg, count):
        self._cfg = cfg
        self._count = count
        self._qcnt = None

    def __enter__(self):
        qcnt = len(_get_rx_cnts(self._cfg))
        if qcnt >= self._count:
            return
        try:
            ksft_pr(f"Increasing queue count {qcnt} -> {self._count}")
            ethtool(f"-L {self._cfg.ifname} combined {self._count}")
            self._qcnt = qcnt
        except:
            raise KsftSkipEx("Not enough queues for the test")

    def __exit__(self, exc_type, exc_value, traceback):
        if self._qcnt is not None:
            ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")

Then:

    with require_contexts(cfg, 2 + 2 * ctx_cnt):
        ...

> +
> +    removed = [False] * ctx_cnt
> +    ntuple = []
> +    ctx_id = []
> +    ports = []
> +
> +    def remove_ctx(idx):
> +        ethtool(f"-N {cfg.ifname} delete {ntuple[idx]}")
> +        del ntuple[idx]
> +        ethtool(f"-X {cfg.ifname} context {ctx_id[idx]} delete")
> +        del ctx_id[idx]
> +        removed[idx] = True
> +
> +    def check_traffic():
> +        for i in range(ctx_cnt):
> +            cnts = _get_rx_cnts(cfg)
> +            GenerateTraffic(cfg, port=ports[i]).wait_pkts_and_stop(20000)
> +            cnts = _get_rx_cnts(cfg, prev=cnts)
> +
> +            if not removed[i]:
> +                ksft_lt(sum(cnts[ :2]), 10000, "traffic on main context:" + str(cnts))
> +                ksft_ge(sum(cnts[2+i*2:4+i*2]), 20000, f"traffic on context {i}: " + str(cnts))
> +                ksft_eq(sum(cnts[2:2+i*2] + cnts[4+i*2:]), 0, "traffic on other contexts: " + str(cnts))
> +            else:
> +                ksft_ge(sum(cnts[ :2]), 20000, "traffic on main context:" + str(cnts))
> +                ksft_eq(sum(cnts[2: ]),     0, "traffic on other contexts: " + str(cnts))
> +
> +    try:
> +        # Use queues 0 and 1 for normal traffic
> +        ethtool(f"-X {cfg.ifname} equal 2")
> +
> +        for i in range(ctx_cnt):
> +            ctx_id.append(ethtool_create(cfg, "-X", f"context new start {2 + i * 2} equal 2"))
> +
> +            ports.append(rand_port())
> +            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {ports[i]} context {ctx_id[i]}"
> +            ntuple.append(ethtool_create(cfg, "-N", flow))
> +
> +        check_traffic()
> +
> +        # Remove middle context
> +        remove_ctx(ctx_cnt // 2)
> +        check_traffic()
> +
> +        # Remove first context
> +        remove_ctx(0)
> +        check_traffic()
> +
> +        # Remove last context
> +        remove_ctx(-1)
> +        check_traffic()

I feel like this works by luck, the removed[] indices are straight,
whereas the ntuple[] and ctx_id[] ones shift around as elements are
removed from the array. I don't mind terribly much, it's not very likely
that somebody adds more legs into this test, but IMHO it would be
cleaner instead of deleting, to replace the deleted element with a None.
And then deleted[] is not even needed. (But then the loops below need an
extra condition.)

> +
> +    finally:
> +        for nid in ntuple:
> +            ethtool(f"-N {cfg.ifname} delete {nid}")
> +        for cid in ctx_id:
> +            ethtool(f"-X {cfg.ifname} context {cid} delete")
> +        ethtool(f"-X {cfg.ifname} default")
> +        if qcnt:
> +            ethtool(f"-L {cfg.ifname} combined {qcnt}")
> +
> +    if requested_ctx_cnt != ctx_cnt:
> +        raise KsftSkipEx(f"Tested only {ctx_cnt} contexts, wanted {requested_ctx_cnt}")
> +
> +
> +def test_rss_context_overlap(cfg, other_ctx=0):
> +    """
> +    Test contexts overlapping with each other.
> +    Use 4 queues for the main context, but only queues 2 and 3 for context 1.
> +    """
> +
> +    require_ntuple(cfg)
> +
> +    queue_cnt = len(_get_rx_cnts(cfg))
> +    if queue_cnt >= 4:
> +        queue_cnt = None
> +    else:
> +        try:
> +            ksft_pr(f"Increasing queue count {queue_cnt} -> 4")
> +            ethtool(f"-L {cfg.ifname} combined 4")
> +        except:
> +            raise KsftSkipEx("Not enough queues for the test")
> +
> +    ctx_id = None
> +    ntuple = None
> +    if other_ctx == 0:
> +        ethtool(f"-X {cfg.ifname} equal 4")
> +    else:
> +        other_ctx = ethtool_create(cfg, "-X", "context new")
> +        ethtool(f"-X {cfg.ifname} context {other_ctx} equal 4")
> +
> +    try:
> +        ctx_id = ethtool_create(cfg, "-X", "context new")
> +        ethtool(f"-X {cfg.ifname} context {ctx_id} start 2 equal 2")
> +
> +        port = rand_port()
> +        if other_ctx:
> +            flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {other_ctx}"
> +            ntuple = ethtool_create(cfg, "-N", flow)
> +
> +        # Test the main context
> +        cnts = _get_rx_cnts(cfg)
> +        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
> +        cnts = _get_rx_cnts(cfg, prev=cnts)
> +
> +        ksft_ge(sum(cnts[ :4]), 20000, "traffic on main context: " + str(cnts))
> +        ksft_ge(sum(cnts[ :2]),  7000, "traffic on main context (1/2): " + str(cnts))
> +        ksft_ge(sum(cnts[2:4]),  7000, "traffic on main context (2/2): " + str(cnts))
> +        if other_ctx == 0:
> +            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
> +
> +        # Now create a rule for context 1 and make sure traffic goes to a subset
> +        if other_ctx:
> +            ethtool(f"-N {cfg.ifname} delete {ntuple}")
> +            ntuple = None
> +        flow = f"flow-type tcp{cfg.addr_ipver} dst-port {port} context {ctx_id}"
> +        ntuple = ethtool_create(cfg, "-N", flow)
> +
> +        cnts = _get_rx_cnts(cfg)
> +        GenerateTraffic(cfg, port=port).wait_pkts_and_stop(20000)
> +        cnts = _get_rx_cnts(cfg, prev=cnts)
> +
> +        ksft_lt(sum(cnts[ :2]),  7000, "traffic on main context: " + str(cnts))
> +        ksft_ge(sum(cnts[2:4]), 20000, "traffic on extra context: " + str(cnts))
> +        if other_ctx == 0:
> +            ksft_eq(sum(cnts[4: ]),     0, "traffic on other queues: " + str(cnts))
> +    finally:
> +        if ntuple is not None:
> +            ethtool(f"-N {cfg.ifname} delete {ntuple}")
> +        if ctx_id:
> +            ethtool(f"-X {cfg.ifname} context {ctx_id} delete")
> +        if other_ctx == 0:
> +            ethtool(f"-X {cfg.ifname} default")
> +        else:
> +            ethtool(f"-X {cfg.ifname} context {other_ctx} delete")
> +        if queue_cnt:
> +            ethtool(f"-L {cfg.ifname} combined {queue_cnt}")
> +
> +
> +def test_rss_context_overlap2(cfg):
> +    test_rss_context_overlap(cfg, True)
> +
> +
> +def main() -> None:
> +    with NetDrvEpEnv(__file__, nsim_test=False) as cfg:
> +        cfg.netdevnl = NetdevFamily()
> +
> +        ksft_run([test_rss_key_indir,
> +                  test_rss_context, test_rss_context4, test_rss_context32,
> +                  test_rss_context_overlap, test_rss_context_overlap2,
> +                  test_rss_context_out_of_order, test_rss_context4_create_with_cfg],
> +                 args=(cfg, ))
> +    ksft_exit()
> +
> +
> +if __name__ == "__main__":
> +    main()
> diff --git a/tools/testing/selftests/drivers/net/lib/py/load.py b/tools/testing/selftests/drivers/net/lib/py/load.py
> index 31f82f1e32c1..0f40a13926d0 100644
> --- a/tools/testing/selftests/drivers/net/lib/py/load.py
> +++ b/tools/testing/selftests/drivers/net/lib/py/load.py
> @@ -5,13 +5,14 @@ import time
>  from lib.py import ksft_pr, cmd, ip, rand_port, wait_port_listen
>  
>  class GenerateTraffic:
> -    def __init__(self, env):
> +    def __init__(self, env, port=None):
>          env.require_cmd("iperf3", remote=True)
>  
>          self.env = env
>  
> -        port = rand_port()
> -        self._iperf_server = cmd(f"iperf3 -s -p {port}", background=True)
> +        if port is None:
> +            port = rand_port()
> +        self._iperf_server = cmd(f"iperf3 -s -1 -p {port}", background=True)
>          wait_port_listen(port)
>          time.sleep(0.1)
>          self._iperf_client = cmd(f"iperf3 -c {env.addr} -P 16 -p {port} -t 86400",
> diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
> index 4769b4eb1ea1..45ffe277d94a 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)
> +
> +
>  class ksft_raises:
>      def __init__(self, expected_type):
>          self.exception = None
> diff --git a/tools/testing/selftests/net/lib/py/utils.py b/tools/testing/selftests/net/lib/py/utils.py
> index 11dbdd3b7612..231e4a2f0252 100644
> --- a/tools/testing/selftests/net/lib/py/utils.py
> +++ b/tools/testing/selftests/net/lib/py/utils.py
> @@ -8,6 +8,10 @@ import subprocess
>  import time
>  
>  
> +class CmdExitFailure(Exception):
> +    pass
> +
> +
>  class cmd:
>      def __init__(self, comm, shell=True, fail=True, ns=None, background=False, host=None, timeout=5):
>          if ns:
> @@ -42,8 +46,8 @@ import time
>          if self.proc.returncode != 0 and fail:
>              if len(stderr) > 0 and stderr[-1] == "\n":
>                  stderr = stderr[:-1]
> -            raise Exception("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
> -                            (self.proc.args, stdout, stderr))
> +            raise CmdExitFailure("Command failed: %s\nSTDOUT: %s\nSTDERR: %s" %
> +                                 (self.proc.args, stdout, stderr))
>  
>  
>  class bkg(cmd):

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

[Jakub Kicinski]

On Tue, 25 Jun 2024 12:42:22 +0200 Petr Machata wrote:
> > +def test_rss_key_indir(cfg):
> > +    """
> > +    Test basics like updating the main RSS key and indirection table.
> > +    """
> > +    if len(_get_rx_cnts(cfg)) < 2:
> > +        KsftSkipEx("Device has only one queue (or doesn't support queue stats)")  
> 
> I'm not sure, is this admin-correctible configuration issue? It looks
> like this and some others should rather be XFAIL.

TBH I don't have a good compass on what should be XFAIL and what should
be SKIP in HW tests. Once vendors start running these we'll get more
experience (there's only one test using Xfail in HW now).
> > +    cnts = _get_rx_cnts(cfg)
> > +    GenerateTraffic(cfg).wait_pkts_and_stop(20000)
> > +    cnts = _get_rx_cnts(cfg, prev=cnts)
> > +    # First two queues get less traffic than all the rest
> > +    ksft_ge(sum(cnts[2:]), sum(cnts[:2]), "traffic distributed: " + str(cnts))  
> 
> Now that you added ksft_lt, this can be made to actually match the comment:
> 
>     ksft_lt(sum(cnts[:2]), sum(cnts[2:]), "traffic distributed: " + str(cnts))

ack

> > +    # 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")  
> 
> There are variations on this in each of the three tests. It would make
> sense to extract to a helper, or perhaps even write as a context
> manager. Untested:
> 
> class require_contexts:
>     def __init__(self, cfg, count):
>         self._cfg = cfg
>         self._count = count
>         self._qcnt = None
> 
>     def __enter__(self):
>         qcnt = len(_get_rx_cnts(self._cfg))
>         if qcnt >= self._count:
>             return
>         try:
>             ksft_pr(f"Increasing queue count {qcnt} -> {self._count}")
>             ethtool(f"-L {self._cfg.ifname} combined {self._count}")
>             self._qcnt = qcnt
>         except:
>             raise KsftSkipEx("Not enough queues for the test")
> 
>     def __exit__(self, exc_type, exc_value, traceback):
>         if self._qcnt is not None:
>             ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
> 
> Then:
> 
>     with require_contexts(cfg, 2 + 2 * ctx_cnt):
>         ...

There are 4 things to clean up, with doesn't cover all of them
naturally and complicates the code.

Once again, I'm thinking about adding some form of deferred execution.
	
	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")

Where cleanups will be executed in reverse order by ksft_run() after
the test, with the option to delete them.

	nid = ethtool_create(cfg, "-N", flow)
	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
	# .. code using ntuple ...
	ntuple.exec()
	# .. now ntuple is gone

or/and:

	nid = ethtool_create(cfg, "-N", flow)
	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
		# .. code using ntuple ...
	# .. now ntuple is gone

Thoughts?

> > +        # Remove last context
> > +        remove_ctx(-1)
> > +        check_traffic()  
> 
> I feel like this works by luck, the removed[] indices are straight,
> whereas the ntuple[] and ctx_id[] ones shift around as elements are
> removed from the array. I don't mind terribly much, it's not very likely
> that somebody adds more legs into this test, but IMHO it would be
> cleaner instead of deleting, to replace the deleted element with a None.
> And then deleted[] is not even needed. (But then the loops below need an
> extra condition.)

I'd argue with the "by luck" but I see your point that it's suspicious,
I'll change it :)

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

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 25 Jun 2024 12:42:22 +0200 Petr Machata wrote:
>> > +def test_rss_key_indir(cfg):
>> > +    """
>> > +    Test basics like updating the main RSS key and indirection table.
>> > +    """
>> > +    if len(_get_rx_cnts(cfg)) < 2:
>> > +        KsftSkipEx("Device has only one queue (or doesn't support queue stats)")  
>> 
>> I'm not sure, is this admin-correctible configuration issue? It looks
>> like this and some others should rather be XFAIL.
>
> TBH I don't have a good compass on what should be XFAIL and what should
> be SKIP in HW tests. Once vendors start running these we'll get more
> experience (there's only one test using Xfail in HW now).

Sure, me neither.

>> > +    # 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")  
>> 
>> There are variations on this in each of the three tests. It would make
>> sense to extract to a helper, or perhaps even write as a context
>> manager. Untested:
>> 
>> class require_contexts:
>>     def __init__(self, cfg, count):
>>         self._cfg = cfg
>>         self._count = count
>>         self._qcnt = None
>> 
>>     def __enter__(self):
>>         qcnt = len(_get_rx_cnts(self._cfg))
>>         if qcnt >= self._count:
>>             return
>>         try:
>>             ksft_pr(f"Increasing queue count {qcnt} -> {self._count}")
>>             ethtool(f"-L {self._cfg.ifname} combined {self._count}")
>>             self._qcnt = qcnt
>>         except:
>>             raise KsftSkipEx("Not enough queues for the test")
>> 
>>     def __exit__(self, exc_type, exc_value, traceback):
>>         if self._qcnt is not None:
>>             ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
>> 
>> Then:
>> 
>>     with require_contexts(cfg, 2 + 2 * ctx_cnt):
>>         ...
>
> There are 4 things to clean up, with doesn't cover all of them
> naturally and complicates the code.

Yeah, you can't use it everywhere, but you can use it for the ethtool
config here.

Re complexity, how about this?

import contextlib

@contextlib.contextmanager
def require_contexts(cfg, count):
    qcnt = len(_get_rx_cnts(cfg))
    if qcnt >= count:
        qcnt = None
    else:
        try:
            ksft_pr(f"Increasing queue count {qcnt} -> {count}")
            ethtool(f"-L {cfg.ifname} combined {count}")
        except:
            raise KsftSkipEx("Not enough queues for the test")

    try:
        yield
    finally:
        if qcnt is not None:
            ethtool(f"-L {cfg.ifname} combined {qcnt}")

This is mostly just business logic, hardly any boilerplate, and still
just uses standard Python. You get the setup and cleanup next to each
other, which is important for cross-comparing the two.

Anyway, if I don't persuade you for The Right Path, something like this
would at least get rid of the duplication:

    qcnt = contexts_setup(cfg, 2 + 2 * ctx_cnt)
    try:
        ...
    finally:
        if qcnt:
            contexts_teardown(cfg, qcnt)

> Once again, I'm thinking about adding some form of deferred execution.
> 	
> 	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
> 	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")
>
> Where cleanups will be executed in reverse order by ksft_run() after
> the test, with the option to delete them.
>
> 	nid = ethtool_create(cfg, "-N", flow)
> 	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
> 	# .. code using ntuple ...
> 	ntuple.exec()
> 	# .. now ntuple is gone
>
> or/and:
>
> 	nid = ethtool_create(cfg, "-N", flow)
> 	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
> 		# .. code using ntuple ...
> 	# .. now ntuple is gone
>
> Thoughts?

Sure, this can be done, but you are introducing a new mechanism to solve
something that the language has had support for for 15 years or so.

Like, it's not terrible. I like it better than the try/finally aprroach,
because at least the setup and cleanup are localized.

Call it defer though? It doesn't "undo" there and then, but at some
later point.

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

[Jakub Kicinski]

On Tue, 25 Jun 2024 16:43:55 +0200 Petr Machata wrote:
> > There are 4 things to clean up, with doesn't cover all of them
> > naturally and complicates the code.  
> 
> Yeah, you can't use it everywhere, but you can use it for the ethtool
> config here.
> 
> Re complexity, how about this?
> 
> import contextlib
> 
> @contextlib.contextmanager
> def require_contexts(cfg, count):
>     qcnt = len(_get_rx_cnts(cfg))
>     if qcnt >= count:
>         qcnt = None
>     else:
>         try:
>             ksft_pr(f"Increasing queue count {qcnt} -> {count}")
>             ethtool(f"-L {cfg.ifname} combined {count}")
>         except:
>             raise KsftSkipEx("Not enough queues for the test")
> 
>     try:
>         yield
>     finally:
>         if qcnt is not None:
>             ethtool(f"-L {cfg.ifname} combined {qcnt}")
> 
> This is mostly just business logic, hardly any boilerplate, and still
> just uses standard Python. You get the setup and cleanup next to each
> other, which is important for cross-comparing the two.

TBH I don't really understand of how the above works.

> Anyway, if I don't persuade you for The Right Path, something like this
> would at least get rid of the duplication:
> 
>     qcnt = contexts_setup(cfg, 2 + 2 * ctx_cnt)
>     try:
>         ...
>     finally:
>         if qcnt:
>             contexts_teardown(cfg, qcnt)

Are we discussing this exact test script or general guidance?

If the general guidance, my principle is to make the test look like
a list of bash commands as much as possible. Having to wrap
every single command you need to undo with a context manager
will take us pretty far from a linear script.

That's why I'd prefer if we provided a mechanism which makes
it easy to defer execution, rather than focus on particular cases.

> > Once again, I'm thinking about adding some form of deferred execution.
> > 	
> > 	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
> > 	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")
> >
> > Where cleanups will be executed in reverse order by ksft_run() after
> > the test, with the option to delete them.
> >
> > 	nid = ethtool_create(cfg, "-N", flow)
> > 	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
> > 	# .. code using ntuple ...
> > 	ntuple.exec()
> > 	# .. now ntuple is gone
> >
> > or/and:
> >
> > 	nid = ethtool_create(cfg, "-N", flow)
> > 	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
> > 		# .. code using ntuple ...
> > 	# .. now ntuple is gone
> >
> > Thoughts?  
> 
> Sure, this can be done, but you are introducing a new mechanism to solve
> something that the language has had support for for 15 years or so.

Well, I can't make the try: yield work for me :(

#!/bin/python3

import contextlib

@contextlib.contextmanager
def bla():
    try:
        yield
    except:
        print("deferred thing")

bla()
print("done")


Gives me:
$ ./test.py 
done


I don't know enough Python, IDK if we can assume much Python expertise
from others.

What we basically want is a form of atexit:

https://docs.python.org/3/library/atexit.html

The fact atexit module exists makes me wonder whether the problem is
really solved by the language itself. But maybe there's a deeper reason
for atexit.

> Like, it's not terrible. I like it better than the try/finally aprroach,
> because at least the setup and cleanup are localized.
> 
> Call it defer though? It doesn't "undo" there and then, but at some
> later point.

I like "defer". We're enqueuing for deferred exec. So another option
would be "enqueue". But I think "defer" is indeed better.

rm = defer(cmd, "rm example.txt")
rm.exec()   # run now, remove from internal queue
rm.cancel() # remove from queue, don't run

?

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

[Jakub Kicinski]

On Tue, 25 Jun 2024 10:06:49 -0700 Jakub Kicinski wrote:
> @contextlib.contextmanager
> def bla():
>     try:
>         yield
>     except:
>         print("deferred thing")

Ah, finally, not except. That works.

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

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> On Tue, 25 Jun 2024 16:43:55 +0200 Petr Machata wrote:
>> > There are 4 things to clean up, with doesn't cover all of them
>> > naturally and complicates the code.  
>> 
>> Yeah, you can't use it everywhere, but you can use it for the ethtool
>> config here.
>> 
>> Re complexity, how about this?
>> 
>> import contextlib
>> 
>> @contextlib.contextmanager
>> def require_contexts(cfg, count):
>>     qcnt = len(_get_rx_cnts(cfg))
>>     if qcnt >= count:
>>         qcnt = None
>>     else:
>>         try:
>>             ksft_pr(f"Increasing queue count {qcnt} -> {count}")
>>             ethtool(f"-L {cfg.ifname} combined {count}")
>>         except:
>>             raise KsftSkipEx("Not enough queues for the test")
>> 
>>     try:
>>         yield
>>     finally:
>>         if qcnt is not None:
>>             ethtool(f"-L {cfg.ifname} combined {qcnt}")
>> 
>> This is mostly just business logic, hardly any boilerplate, and still
>> just uses standard Python. You get the setup and cleanup next to each
>> other, which is important for cross-comparing the two.
>
> TBH I don't really understand of how the above works.

The decorator transforms the function into a context manager. yield
marks the point where the with: body runs, whetever is before is
initialization, whatever is after is cleanup. The try: finally: wrapper
is there in case the with body ends with an exception.

>> Anyway, if I don't persuade you for The Right Path, something like this
>> would at least get rid of the duplication:
>> 
>>     qcnt = contexts_setup(cfg, 2 + 2 * ctx_cnt)
>>     try:
>>         ...
>>     finally:
>>         if qcnt:
>>             contexts_teardown(cfg, qcnt)
>
> Are we discussing this exact test script or general guidance?
>
> If the general guidance, my principle is to make the test look like
> a list of bash commands as much as possible. Having to wrap
> every single command you need to undo with a context manager
> will take us pretty far from a linear script.
>
> That's why I'd prefer if we provided a mechanism which makes
> it easy to defer execution, rather than focus on particular cases.

I meant it as a general principle. Python has tools to solve the cleanup
problem very elegantly. I asked around in the meantime, people don't
generally find it hard to understand. It's a simple concept. It might be
_foreign_, I'll grant you that, but so will whatever new API you cook
up. And you can google the former easily enough.

OK, look, I believe I made my point. I don't want to split this
particular hair too much. I see you already sent something for defer,
so I'll take a look at that.

>> > Once again, I'm thinking about adding some form of deferred execution.
>> > 	
>> > 	ethtool(f"-L {self._cfg.ifname} combined {self._qcnt}")
>> > 	undo(ethtool, f"-L {self._cfg.ifname} combined {old_qcnt}")
>> >
>> > Where cleanups will be executed in reverse order by ksft_run() after
>> > the test, with the option to delete them.
>> >
>> > 	nid = ethtool_create(cfg, "-N", flow)
>> > 	ntuple = undo(ethtool, f"-N {cfg.ifname} delete {nid}")
>> > 	# .. code using ntuple ...
>> > 	ntuple.exec()
>> > 	# .. now ntuple is gone
>> >
>> > or/and:
>> >
>> > 	nid = ethtool_create(cfg, "-N", flow)
>> > 	with undo(ethtool, f"-N {cfg.ifname} delete {nid}"):
>> > 		# .. code using ntuple ...
>> > 	# .. now ntuple is gone
>> >
>> > Thoughts?  
>> 
>> Sure, this can be done, but you are introducing a new mechanism to solve
>> something that the language has had support for for 15 years or so.
>
> Well, I can't make the try: yield work for me :(
>
> #!/bin/python3
>
> import contextlib
>
> @contextlib.contextmanager
> def bla():
>     try:
>         yield
>     except:
>         print("deferred thing")
>
> bla()
> print("done")
>
>
> Gives me:
> $ ./test.py 
> done
>
> I don't know enough Python, IDK if we can assume much Python expertise
> from others.
>
> What we basically want is a form of atexit:
>
> https://docs.python.org/3/library/atexit.html
>
> The fact atexit module exists makes me wonder whether the problem is
> really solved by the language itself. But maybe there's a deeper reason
> for atexit.

I think it's just incremental. atexit was introduced in Python 2.0, with
statements in Python 2.6, some eight years later.

>> Like, it's not terrible. I like it better than the try/finally aprroach,
>> because at least the setup and cleanup are localized.
>> 
>> Call it defer though? It doesn't "undo" there and then, but at some
>> later point.
>
> I like "defer". We're enqueuing for deferred exec. So another option
> would be "enqueue". But I think "defer" is indeed better.
>
> rm = defer(cmd, "rm example.txt")
> rm.exec()   # run now, remove from internal queue
> rm.cancel() # remove from queue, don't run

Looks good.

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

[Jakub Kicinski]

On Mon, 24 Jun 2024 18:02:06 -0700 Jakub Kicinski wrote:
> v2 adds a test for removing contexts out of order. When testing
> bnxt - either the new test or running more tests after the overlap
> test makes the device act strangely. To the point where it may start
> giving out ntuple IDs of 0 for all rules :S

FWIW if I create the context with an indirection table, and then change
the key (without touching the indirection table) - it all works as
expected. So really seems like bnxt has a problem with setting the
indir table when context is created. That said I don't see anything
obviously wrong with the driver code.

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

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 766 bytes --]

On Mon, Jun 24, 2024 at 6:40 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon, 24 Jun 2024 18:02:06 -0700 Jakub Kicinski wrote:
> > v2 adds a test for removing contexts out of order. When testing
> > bnxt - either the new test or running more tests after the overlap
> > test makes the device act strangely. To the point where it may start
> > giving out ntuple IDs of 0 for all rules :S
>
> FWIW if I create the context with an indirection table, and then change
> the key (without touching the indirection table) - it all works as
> expected. So really seems like bnxt has a problem with setting the
> indir table when context is created. That said I don't see anything
> obviously wrong with the driver code.

We will look into it.  Thanks.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

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

[Willem de Bruijn]

Jakub Kicinski wrote:
> Add a few tests exercising RSS context API.
> In addition to basic sanity checks, tests add RSS contexts,
> n-tuple rule to direct traffic to them (based on dst port),
> and qstats to make sure traffic landed where we expected.
> 
> v2 adds a test for removing contexts out of order. When testing
> bnxt - either the new test or running more tests after the overlap
> test makes the device act strangely. To the point where it may start
> giving out ntuple IDs of 0 for all rules :S
> 
> Ed, could you try the tests with your device?
> 
>   $ export NETIF=eth0 REMOTE_...
>   $ ./drivers/net/hw/rss_ctx.py
>   KTAP version 1
>   1..8
>   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
>   # .. sprays traffic like a headless chicken ..
>   not ok 7 rss_ctx.test_rss_context_out_of_order
>   ok 8 rss_ctx.test_rss_context4_create_with_cfg
>   # Totals: pass:6 fail:1 xfail:0 xpass:0 skip:1 error:0
> 
> Jakub Kicinski (4):
>   selftests: drv-net: try to check if port is in use
>   selftests: drv-net: add helper to wait for HW stats to sync
>   selftests: drv-net: add ability to wait for at least N packets to load
>     gen
>   selftests: drv-net: rss_ctx: add tests for RSS configuration and
>     contexts

Good suggestion in patch 1 on symbolic errno.

Will leave one minor comment inline.

But overall LGTM, thanks.

Reviewed-by: Willem de Bruijn <willemb@google.com>