[v1] selftests: drv-net: Check if combined-count exists

[PATCH net] selftests: drv-net: Check if combined-count exists

Posted by Joe Damato 11 months, 2 weeks ago

Some drivers, like tg3, do not set combined-count:

$ ethtool -l enp4s0f1
Channel parameters for enp4s0f1:
Pre-set maximums:
RX:		4
TX:		4
Other:		n/a
Combined:	n/a
Current hardware settings:
RX:		4
TX:		1
Other:		n/a
Combined:	n/a

In the case where combined-count is not set, the ethtool netlink code
in the kernel elides the value and the code in the test:

  netnl.channels_get(...)

With a tg3 device, the returned dictionary looks like:

{'header': {'dev-index': 3, 'dev-name': 'enp4s0f1'},
 'rx-max': 4,
 'rx-count': 4,
 'tx-max': 4,
 'tx-count': 1}

Note that the key 'combined-count' is missing. As a result of this
missing key the test raises an exception:

 # Exception|     if channels['combined-count'] == 0:
 # Exception|        ~~~~~~~~^^^^^^^^^^^^^^^^^^
 # Exception| KeyError: 'combined-count'

Change the test to check if 'combined-count' is a key in the dictionary
first and if not assume that this means the driver has separate RX and
TX queues.

With this change, the test now passes successfully on tg3 and mlx5
(which does have a 'combined-count').

Fixes: 1cf270424218 ("net: selftest: add test for netdev netlink queue-get API")
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 tools/testing/selftests/drivers/net/queues.py | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 38303da957ee..baa8845d9f64 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -45,10 +45,13 @@ def addremove_queues(cfg, nl) -> None:
 
     netnl = EthtoolFamily()
     channels = netnl.channels_get({'header': {'dev-index': cfg.ifindex}})
-    if channels['combined-count'] == 0:
-        rx_type = 'rx'
+    if 'combined-count' in channels:
+        if channels['combined-count'] == 0:
+            rx_type = 'rx'
+        else:
+            rx_type = 'combined'
     else:
-        rx_type = 'combined'
+        rx_type = 'rx'
 
     expected = curr_queues - 1
     cmd(f"ethtool -L {cfg.dev['ifname']} {rx_type} {expected}", timeout=10)

base-commit: bc50682128bde778a1ddc457a02d92a637c20c6f
-- 
2.43.0

Re: [PATCH net] selftests: drv-net: Check if combined-count exists

Posted by David Wei 11 months, 2 weeks ago

On 2025-02-25 10:14, Joe Damato wrote:
> Some drivers, like tg3, do not set combined-count:
> 
> $ ethtool -l enp4s0f1
> Channel parameters for enp4s0f1:
> Pre-set maximums:
> RX:		4
> TX:		4
> Other:		n/a
> Combined:	n/a
> Current hardware settings:
> RX:		4
> TX:		1
> Other:		n/a
> Combined:	n/a
> 
> In the case where combined-count is not set, the ethtool netlink code
> in the kernel elides the value and the code in the test:
> 
>   netnl.channels_get(...)
> 
> With a tg3 device, the returned dictionary looks like:
> 
> {'header': {'dev-index': 3, 'dev-name': 'enp4s0f1'},
>  'rx-max': 4,
>  'rx-count': 4,
>  'tx-max': 4,
>  'tx-count': 1}
> 
> Note that the key 'combined-count' is missing. As a result of this
> missing key the test raises an exception:
> 
>  # Exception|     if channels['combined-count'] == 0:
>  # Exception|        ~~~~~~~~^^^^^^^^^^^^^^^^^^
>  # Exception| KeyError: 'combined-count'
> 
> Change the test to check if 'combined-count' is a key in the dictionary
> first and if not assume that this means the driver has separate RX and
> TX queues.
> 
> With this change, the test now passes successfully on tg3 and mlx5
> (which does have a 'combined-count').
> 
> Fixes: 1cf270424218 ("net: selftest: add test for netdev netlink queue-get API")
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  tools/testing/selftests/drivers/net/queues.py | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> index 38303da957ee..baa8845d9f64 100755
> --- a/tools/testing/selftests/drivers/net/queues.py
> +++ b/tools/testing/selftests/drivers/net/queues.py
> @@ -45,10 +45,13 @@ def addremove_queues(cfg, nl) -> None:
>  
>      netnl = EthtoolFamily()
>      channels = netnl.channels_get({'header': {'dev-index': cfg.ifindex}})
> -    if channels['combined-count'] == 0:
> -        rx_type = 'rx'
> +    if 'combined-count' in channels:
> +        if channels['combined-count'] == 0:
> +            rx_type = 'rx'
> +        else:
> +            rx_type = 'combined'
>      else:
> -        rx_type = 'combined'
> +        rx_type = 'rx'

Logic is good but minor nit in reducing nestiness:

rx_type = 'rx'
if channels.get('combined-count', 0) > 0:
	rx_type = 'combined'

>  
>      expected = curr_queues - 1
>      cmd(f"ethtool -L {cfg.dev['ifname']} {rx_type} {expected}", timeout=10)
> 
> base-commit: bc50682128bde778a1ddc457a02d92a637c20c6f

Re: [PATCH net] selftests: drv-net: Check if combined-count exists

Posted by Joe Damato 11 months, 2 weeks ago

On Tue, Feb 25, 2025 at 03:11:24PM -0800, David Wei wrote:
> On 2025-02-25 10:14, Joe Damato wrote:
[...]
> > diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> > index 38303da957ee..baa8845d9f64 100755
> > --- a/tools/testing/selftests/drivers/net/queues.py
> > +++ b/tools/testing/selftests/drivers/net/queues.py
> > @@ -45,10 +45,13 @@ def addremove_queues(cfg, nl) -> None:
> >  
> >      netnl = EthtoolFamily()
> >      channels = netnl.channels_get({'header': {'dev-index': cfg.ifindex}})
> > -    if channels['combined-count'] == 0:
> > -        rx_type = 'rx'
> > +    if 'combined-count' in channels:
> > +        if channels['combined-count'] == 0:
> > +            rx_type = 'rx'
> > +        else:
> > +            rx_type = 'combined'
> >      else:
> > -        rx_type = 'combined'
> > +        rx_type = 'rx'
> 
> Logic is good but minor nit in reducing nestiness:
> 
> rx_type = 'rx'
> if channels.get('combined-count', 0) > 0:
> 	rx_type = 'combined'

Thanks; will fix in the v2.

---
pw-bot: cr

Re: [PATCH net] selftests: drv-net: Check if combined-count exists

Posted by Gerhard Engleder 11 months, 2 weeks ago

On 25.02.25 19:14, Joe Damato wrote:
> Some drivers, like tg3, do not set combined-count:
> 
> $ ethtool -l enp4s0f1
> Channel parameters for enp4s0f1:
> Pre-set maximums:
> RX:		4
> TX:		4
> Other:		n/a
> Combined:	n/a
> Current hardware settings:
> RX:		4
> TX:		1
> Other:		n/a
> Combined:	n/a
> 
> In the case where combined-count is not set, the ethtool netlink code
> in the kernel elides the value and the code in the test:
> 
>    netnl.channels_get(...)
> 
> With a tg3 device, the returned dictionary looks like:
> 
> {'header': {'dev-index': 3, 'dev-name': 'enp4s0f1'},
>   'rx-max': 4,
>   'rx-count': 4,
>   'tx-max': 4,
>   'tx-count': 1}
> 
> Note that the key 'combined-count' is missing. As a result of this
> missing key the test raises an exception:
> 
>   # Exception|     if channels['combined-count'] == 0:
>   # Exception|        ~~~~~~~~^^^^^^^^^^^^^^^^^^
>   # Exception| KeyError: 'combined-count'
> 
> Change the test to check if 'combined-count' is a key in the dictionary
> first and if not assume that this means the driver has separate RX and
> TX queues.
> 
> With this change, the test now passes successfully on tg3 and mlx5
> (which does have a 'combined-count').
> 
> Fixes: 1cf270424218 ("net: selftest: add test for netdev netlink queue-get API")
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>   tools/testing/selftests/drivers/net/queues.py | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> index 38303da957ee..baa8845d9f64 100755
> --- a/tools/testing/selftests/drivers/net/queues.py
> +++ b/tools/testing/selftests/drivers/net/queues.py
> @@ -45,10 +45,13 @@ def addremove_queues(cfg, nl) -> None:
>   
>       netnl = EthtoolFamily()
>       channels = netnl.channels_get({'header': {'dev-index': cfg.ifindex}})
> -    if channels['combined-count'] == 0:
> -        rx_type = 'rx'
> +    if 'combined-count' in channels:
> +        if channels['combined-count'] == 0:
> +            rx_type = 'rx'
> +        else:
> +            rx_type = 'combined'
>       else:
> -        rx_type = 'combined'
> +        rx_type = 'rx'
>   
>       expected = curr_queues - 1
>       cmd(f"ethtool -L {cfg.dev['ifname']} {rx_type} {expected}", timeout=10)
> 
> base-commit: bc50682128bde778a1ddc457a02d92a637c20c6f

Reviewed-by: Gerhard Engleder <gerhard@engleder-embedded.com>