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()
- 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 //= 2if val <= 1:breakexcept 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
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 //= 2if val <= 1:breakexcept NlError:breakIs 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 :(
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 //= 2if val <= 1:breakexcept NlError:breakIs 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
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 :/
linux-kselftest-mirror@lists.linaro.org