[PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[Jakub Kicinski]

We are trying to move away from netdevsim-only tests and towards
tests which can be run both against netdevsim and real drivers.

Replace the simple bash script we have for checking ethtool -g/-G
on netdevsim with a Python test tweaking those params as well
as channel count.

The new test is not exactly equivalent to the netdevsim one,
but real drivers don't often support random ring sizes,
let alone modifying max values via debugfs.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
v2:
 - add the new test to Makefile and remove the old one
   turns out NIPA checking for Makefile presence was busted
v1: https://lore.kernel.org/20251024215552.1249838-1-kuba@kernel.org

CC: andrew@lunn.ch
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/Makefile  |   1 +
 .../selftests/drivers/net/netdevsim/Makefile  |   1 -
 .../drivers/net/netdevsim/ethtool-ring.sh     |  85 ---------
 .../selftests/drivers/net/ring_reconfig.py    | 167 ++++++++++++++++++
 4 files changed, 168 insertions(+), 86 deletions(-)
 delete mode 100755 tools/testing/selftests/drivers/net/netdevsim/ethtool-ring.sh
 create mode 100755 tools/testing/selftests/drivers/net/ring_reconfig.py

diff --git a/tools/testing/selftests/drivers/net/Makefile b/tools/testing/selftests/drivers/net/Makefile
index 6e41635bd55a..68e0bb603a9d 100644
--- a/tools/testing/selftests/drivers/net/Makefile
+++ b/tools/testing/selftests/drivers/net/Makefile
@@ -22,6 +22,7 @@ TEST_PROGS := \
 	ping.py \
 	psp.py \
 	queues.py \
+	ring_reconfig.py \
 	shaper.py \
 	stats.py \
 	xdp.py \
diff --git a/tools/testing/selftests/drivers/net/netdevsim/Makefile b/tools/testing/selftests/drivers/net/netdevsim/Makefile
index daf51113c827..833abd8e6fdc 100644
--- a/tools/testing/selftests/drivers/net/netdevsim/Makefile
+++ b/tools/testing/selftests/drivers/net/netdevsim/Makefile
@@ -8,7 +8,6 @@ TEST_PROGS := \
 	ethtool-features.sh \
 	ethtool-fec.sh \
 	ethtool-pause.sh \
-	ethtool-ring.sh \
 	fib.sh \
 	fib_notifications.sh \
 	hw_stats_l3.sh \
diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-ring.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-ring.sh
deleted file mode 100755
index c969559ffa7a..000000000000
--- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-ring.sh
+++ /dev/null
@@ -1,85 +0,0 @@
-#!/bin/bash
-# SPDX-License-Identifier: GPL-2.0-only
-
-source ethtool-common.sh
-
-function get_value {
-    local query="${SETTINGS_MAP[$1]}"
-
-    echo $(ethtool -g $NSIM_NETDEV | \
-        tail -n +$CURR_SETT_LINE | \
-        awk -F':' -v pattern="$query:" '$0 ~ pattern {gsub(/[\t ]/, "", $2); print $2}')
-}
-
-function update_current_settings {
-    for key in ${!SETTINGS_MAP[@]}; do
-        CURRENT_SETTINGS[$key]=$(get_value $key)
-    done
-    echo ${CURRENT_SETTINGS[@]}
-}
-
-if ! ethtool -h | grep -q set-ring >/dev/null; then
-    echo "SKIP: No --set-ring support in ethtool"
-    exit 4
-fi
-
-NSIM_NETDEV=$(make_netdev)
-
-set -o pipefail
-
-declare -A SETTINGS_MAP=(
-    ["rx"]="RX"
-    ["rx-mini"]="RX Mini"
-    ["rx-jumbo"]="RX Jumbo"
-    ["tx"]="TX"
-)
-
-declare -A EXPECTED_SETTINGS=(
-    ["rx"]=""
-    ["rx-mini"]=""
-    ["rx-jumbo"]=""
-    ["tx"]=""
-)
-
-declare -A CURRENT_SETTINGS=(
-    ["rx"]=""
-    ["rx-mini"]=""
-    ["rx-jumbo"]=""
-    ["tx"]=""
-)
-
-MAX_VALUE=$((RANDOM % $((2**32-1))))
-RING_MAX_LIST=$(ls $NSIM_DEV_DFS/ethtool/ring/)
-
-for ring_max_entry in $RING_MAX_LIST; do
-    echo $MAX_VALUE > $NSIM_DEV_DFS/ethtool/ring/$ring_max_entry
-done
-
-CURR_SETT_LINE=$(ethtool -g $NSIM_NETDEV | grep -i -m1 -n 'Current hardware settings' | cut -f1 -d:)
-
-# populate the expected settings map
-for key in ${!SETTINGS_MAP[@]}; do
-    EXPECTED_SETTINGS[$key]=$(get_value $key)
-done
-
-# test
-for key in ${!SETTINGS_MAP[@]}; do
-    value=$((RANDOM % $MAX_VALUE))
-
-    ethtool -G $NSIM_NETDEV "$key" "$value"
-
-    EXPECTED_SETTINGS[$key]="$value"
-    expected=${EXPECTED_SETTINGS[@]}
-    current=$(update_current_settings)
-
-    check $? "$current" "$expected"
-    set +x
-done
-
-if [ $num_errors -eq 0 ]; then
-    echo "PASSED all $((num_passes)) checks"
-    exit 0
-else
-    echo "FAILED $num_errors/$((num_errors+num_passes)) checks"
-    exit 1
-fi
diff --git a/tools/testing/selftests/drivers/net/ring_reconfig.py b/tools/testing/selftests/drivers/net/ring_reconfig.py
new file mode 100755
index 000000000000..2251efe63014
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/ring_reconfig.py
@@ -0,0 +1,167 @@
+#!/usr/bin/env python3
+# SPDX-License-Identifier: GPL-2.0
+
+"""
+Test channel and ring size configuration via ethtool (-L / -G).
+"""
+
+from lib.py import ksft_run, ksft_exit, ksft_pr
+from lib.py import ksft_eq
+from lib.py import NetDrvEpEnv, EthtoolFamily, GenerateTraffic
+from lib.py import defer, NlError
+
+
+def channels(cfg) -> None:
+    """
+    Twiddle channel counts in various combinations of parameters.
+    We're only looking for driver adhering to the requested config
+    if the config is accepted and crashes.
+    """
+    ehdr = {'header':{'dev-index': cfg.ifindex}}
+    chans = cfg.eth.channels_get(ehdr)
+
+    all_keys = ["rx", "tx", "combined"]
+    mixes = [{"combined"}, {"rx", "tx"}, {"rx", "combined"}, {"tx", "combined"},
+             {"rx", "tx", "combined"},]
+
+    # Get the set of keys that device actually supports
+    restore = {}
+    supported = set()
+    for key in all_keys:
+        if key + "-max" in chans:
+            supported.add(key)
+            restore |= {key + "-count": chans[key + "-count"]}
+
+    defer(cfg.eth.channels_set, ehdr | restore)
+
+    def test_config(config):
+        try:
+            cfg.eth.channels_set(ehdr | config)
+            get = cfg.eth.channels_get(ehdr)
+            for k, v in config.items():
+                ksft_eq(get.get(k, 0), v)
+        except NlError as e:
+            failed.append(mix)
+            ksft_pr("Can't set", config, e)
+        else:
+            ksft_pr("Okay", config)
+
+    failed = []
+    for mix in mixes:
+        if not mix.issubset(supported):
+            continue
+
+        # Set all the values in the mix to 1, other supported to 0
+        config = {}
+        for key in all_keys:
+            config[key + "-count"] = 1 if key in mix else 0
+        test_config(config)
+
+    for mix in mixes:
+        if not mix.issubset(supported):
+            continue
+        if mix in failed:
+            continue
+
+        # Set all the values in the mix to max, other supported to 0
+        config = {}
+        for key in all_keys:
+            config[key + "-count"] = chans[key + '-max'] if key in mix else 0
+        test_config(config)
+
+
+def _configure_min_ring_cnt(cfg) -> None:
+    """ Try to configure a single Rx/Tx ring. """
+    ehdr = {'header':{'dev-index': cfg.ifindex}}
+    chans = cfg.eth.channels_get(ehdr)
+
+    all_keys = ["rx-count", "tx-count", "combined-count"]
+    restore = {}
+    config = {}
+    for key in all_keys:
+        if key in chans:
+            restore[key] = chans[key]
+            config[key] = 0
+
+    if chans.get('combined-count', 0) > 1:
+        config['combined-count'] = 1
+    elif chans.get('rx-count', 0) > 1 and chans.get('tx-count', 0) > 1:
+        config['tx-count'] = 1
+        config['rx-count'] = 1
+    else:
+        # looks like we're already on 1 channel
+        return
+
+    cfg.eth.channels_set(ehdr | config)
+    defer(cfg.eth.channels_set, ehdr | restore)
+
+
+def ringparam(cfg) -> None:
+    """
+    Tweak the ringparam configuration. Try to run some traffic over min
+    ring size to make sure it actually functions.
+    """
+    ehdr = {'header':{'dev-index': cfg.ifindex}}
+    rings = cfg.eth.rings_get(ehdr)
+
+    restore = {}
+    maxes = {}
+    params = set()
+    for key in rings.keys():
+        if 'max' in key:
+            param = key[:-4]
+            maxes[param] = rings[key]
+            params.add(param)
+            restore[param] = rings[param]
+
+    defer(cfg.eth.rings_set, ehdr | restore)
+
+    # Speed up the reconfig by configuring just one ring
+    _configure_min_ring_cnt(cfg)
+
+    # Try to reach min on all settings
+    for param in params:
+        val = rings[param]
+        while True:
+            try:
+                cfg.eth.rings_set({'header':{'dev-index': cfg.ifindex},
+                                   param: val // 2})
+                val //= 2
+                if val <= 1:
+                    break
+            except NlError:
+                break
+
+        get = cfg.eth.rings_get(ehdr)
+        ksft_eq(get[param], val)
+
+        ksft_pr(f"Reached min for '{param}' at {val} (max {rings[param]})")
+
+    GenerateTraffic(cfg).wait_pkts_and_stop(50000)
+
+    # Try max across all params, if the driver supports large rings
+    # this may OOM so we ignore errors
+    try:
+        ksft_pr("Applying max settings")
+        config = {p: maxes[p] for p in params}
+        cfg.eth.rings_set(ehdr | config)
+    except NlError as e:
+        ksft_pr("Can't set max params", config, e)
+    else:
+        GenerateTraffic(cfg).wait_pkts_and_stop(50000)
+
+
+def main() -> None:
+    """ Ksft boiler plate main """
+
+    with NetDrvEpEnv(__file__) as cfg:
+        cfg.eth = EthtoolFamily()
+
+        ksft_run([channels,
+                  ringparam],
+                 args=(cfg, ))
+    ksft_exit()
+
+
+if __name__ == "__main__":
+    main()
-- 
2.51.0

Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[Andrew Lunn]

> +    def test_config(config):
> +        try:
> +            cfg.eth.channels_set(ehdr | config)
> +            get = cfg.eth.channels_get(ehdr)
> +            for k, v in config.items():
> +                ksft_eq(get.get(k, 0), v)
> +        except NlError as e:
> +            failed.append(mix)
> +            ksft_pr("Can't set", config, e)
> +        else:
> +            ksft_pr("Okay", config)

We expect failure to leave the configuration unchanged. So i would
actually do:

try:
	before = get()
	set()
except:
	after = get()
	fail(after != before)

Also, does nlError contain the error code?

        fail(e.errcode not in (EINVAL, EOPNOTSUPP))

It would be good to detect and fail ENOTSUPP, which does appear every
so often, when it should not.

> +    # Try to reach min on all settings
> +    for param in params:
> +        val = rings[param]
> +        while True:
> +            try:
> +                cfg.eth.rings_set({'header':{'dev-index': cfg.ifindex},
> +                                   param: val // 2})
> +                val //= 2
> +                if val <= 1:
> +                    break
> +            except NlError:
> +                break

Is 0 ever valid? I would actually test 0 and make sure it fails with
EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
it is good to test them. The happy days cases are boring because
developers tend to test those, so they are hardly worth testings. Its
the edge cases which should be tested.

	Andrew

Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[Jakub Kicinski]

On Mon, 27 Oct 2025 20:46:00 +0100 Andrew Lunn wrote:
> > +    def test_config(config):
> > +        try:
> > +            cfg.eth.channels_set(ehdr | config)
> > +            get = cfg.eth.channels_get(ehdr)
> > +            for k, v in config.items():
> > +                ksft_eq(get.get(k, 0), v)
> > +        except NlError as e:
> > +            failed.append(mix)
> > +            ksft_pr("Can't set", config, e)
> > +        else:
> > +            ksft_pr("Okay", config)  
> 
> We expect failure to leave the configuration unchanged. So i would
> actually do:
> 
> try:
> 	before = get()
> 	set()
> except:
> 	after = get()
> 	fail(after != before)

Please allow me to introduce you to the magic of defer() ;)
This registers a command to run after the test completely exits:

+    defer(cfg.eth.channels_set, ehdr | restore)

> Also, does nlError contain the error code?
> 
>         fail(e.errcode not in (EINVAL, EOPNOTSUPP))
> 
> It would be good to detect and fail ENOTSUPP, which does appear every
> so often, when it should not.

Dunno, checkpatch warns about ENOTSUPP. I don't that think checking 
for funny error codes in every test scales :(

> > +    # Try to reach min on all settings
> > +    for param in params:
> > +        val = rings[param]
> > +        while True:
> > +            try:
> > +                cfg.eth.rings_set({'header':{'dev-index': cfg.ifindex},
> > +                                   param: val // 2})
> > +                val //= 2
> > +                if val <= 1:
> > +                    break
> > +            except NlError:
> > +                break  
> 
> Is 0 ever valid? I would actually test 0 and make sure it fails with
> EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
> it is good to test them. The happy days cases are boring because
> developers tend to test those, so they are hardly worth testings. Its
> the edge cases which should be tested.

I believe that 0 is a valid settings. I don't have much experience with
devices which support it. But presumably using 0 to disable mini/jumbo
rings would make sense for example? And max validation is done by the
core so nothing interesting to explore there at the driver level :(

Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[Andrew Lunn]

On Mon, Oct 27, 2025 at 05:15:39PM -0700, Jakub Kicinski wrote:
> On Mon, 27 Oct 2025 20:46:00 +0100 Andrew Lunn wrote:
> > > +    def test_config(config):
> > > +        try:
> > > +            cfg.eth.channels_set(ehdr | config)
> > > +            get = cfg.eth.channels_get(ehdr)
> > > +            for k, v in config.items():
> > > +                ksft_eq(get.get(k, 0), v)
> > > +        except NlError as e:
> > > +            failed.append(mix)
> > > +            ksft_pr("Can't set", config, e)
> > > +        else:
> > > +            ksft_pr("Okay", config)  
> > 
> > We expect failure to leave the configuration unchanged. So i would
> > actually do:
> > 
> > try:
> > 	before = get()
> > 	set()
> > except:
> > 	after = get()
> > 	fail(after != before)
> 
> Please allow me to introduce you to the magic of defer() ;)

That is why i don't like magic, especially in tests which have no
documentation of the expected results. For me, tests should be dumb,
often boringly repetitive, and at least 50% comments, explaining what
is being tested, what the expected outcome is, and most importantly,
why that is the expected outcome.

> This registers a command to run after the test completely exits:
> 
> +    defer(cfg.eth.channels_set, ehdr | restore)
> 
> > Also, does nlError contain the error code?
> > 
> >         fail(e.errcode not in (EINVAL, EOPNOTSUPP))
> > 
> > It would be good to detect and fail ENOTSUPP, which does appear every
> > so often, when it should not.
> 
> Dunno, checkpatch warns about ENOTSUPP. I don't that think checking 
> for funny error codes in every test scales :(

How about in the nlError constructor? That gives you a single
location, and you can accept EINVAL, EOPNOTSUPP, ENODEV, ENOMEM, maybe
ETOOBIG. Cause the test to fail for everything else. If anybody
reports test failures with other errno values, the list can be
expanded, if they are sensible.

> > > +    # Try to reach min on all settings
> > > +    for param in params:
> > > +        val = rings[param]
> > > +        while True:
> > > +            try:
> > > +                cfg.eth.rings_set({'header':{'dev-index': cfg.ifindex},
> > > +                                   param: val // 2})
> > > +                val //= 2
> > > +                if val <= 1:
> > > +                    break
> > > +            except NlError:
> > > +                break  
> > 
> > Is 0 ever valid? I would actually test 0 and make sure it fails with
> > EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
> > it is good to test them. The happy days cases are boring because
> > developers tend to test those, so they are hardly worth testings. Its
> > the edge cases which should be tested.
> 
> I believe that 0 is a valid settings. I don't have much experience with
> devices which support it. But presumably using 0 to disable mini/jumbo
> rings would make sense for example? And max validation is done by the
> core so nothing interesting to explore there at the driver level :(

Looking at the code, it seems to cost nothing to actually test 0, if
you say it could be valid. That might also find an off-by-one error,
if it causes something to go negative/large positive.

	Andrew

Re: [PATCH net-next v2] selftests: drv-net: replace the nsim ring test with a drv-net one

[Jakub Kicinski]

On Tue, 28 Oct 2025 03:05:03 +0100 Andrew Lunn wrote:
> On Mon, Oct 27, 2025 at 05:15:39PM -0700, Jakub Kicinski wrote:
> > On Mon, 27 Oct 2025 20:46:00 +0100 Andrew Lunn wrote:  
> > > We expect failure to leave the configuration unchanged. So i would
> > > actually do:
> > > 
> > > try:
> > > 	before = get()
> > > 	set()
> > > except:
> > > 	after = get()
> > > 	fail(after != before)  
> > 
> > Please allow me to introduce you to the magic of defer() ;)  
> 
> That is why i don't like magic, especially in tests which have no
> documentation of the expected results. For me, tests should be dumb,
> often boringly repetitive, and at least 50% comments, explaining what
> is being tested, what the expected outcome is, and most importantly,
> why that is the expected outcome.

We actively avoid creating framework-y stuff.

defer() is one of the few things we added. Single config change is fine
but undoing a sequence of actions quickly becomes gnarly. And defer()
itself is very straightforward.. once you know about it ;)

https://github.com/linux-netdev/nipa/wiki/Guidance-for-test-authors

> > This registers a command to run after the test completely exits:
> > 
> > +    defer(cfg.eth.channels_set, ehdr | restore)
> >   
> > > Also, does nlError contain the error code?
> > > 
> > >         fail(e.errcode not in (EINVAL, EOPNOTSUPP))
> > > 
> > > It would be good to detect and fail ENOTSUPP, which does appear every
> > > so often, when it should not.  
> > 
> > Dunno, checkpatch warns about ENOTSUPP. I don't that think checking 
> > for funny error codes in every test scales :(  
> 
> How about in the nlError constructor? That gives you a single
> location, and you can accept EINVAL, EOPNOTSUPP, ENODEV, ENOMEM, maybe
> ETOOBIG. Cause the test to fail for everything else. If anybody
> reports test failures with other errno values, the list can be
> expanded, if they are sensible.

Maybe it's just my subjective preference but I think we need to be
judicious about what we test. I see no value in checking errno here.
This is mostly a "crash test", IOW must common issues it will find
will be crashes, WARN()s or broken device. Nothing subtle.

> > > Is 0 ever valid? I would actually test 0 and make sure it fails with
> > > EINVAL, or EOPNOTSUPP. Getting range checks wrong is a typical bug, so
> > > it is good to test them. The happy days cases are boring because
> > > developers tend to test those, so they are hardly worth testings. Its
> > > the edge cases which should be tested.  
> > 
> > I believe that 0 is a valid settings. I don't have much experience with
> > devices which support it. But presumably using 0 to disable mini/jumbo
> > rings would make sense for example? And max validation is done by the
> > core so nothing interesting to explore there at the driver level :(  
> 
> Looking at the code, it seems to cost nothing to actually test 0, if
> you say it could be valid. That might also find an off-by-one error,
> if it causes something to go negative/large positive.

Okay. But for the record I'd strongly prefer it the level of nit
picking was correlated with reviewer's authorship of tests :/