[PATCH net 0/5] netdev: fix repeated netlink messages in queue dumps

[PATCH net 0/5] netdev: fix repeated netlink messages in queue dumps

[Jakub Kicinski]

Fix dump continuation for queues and queue stats in the netdev family.
Because we used post-increment when saving id of dumped queue next
skb would re-dump the already dumped queue.

Jakub Kicinski (5):
  netdev: fix repeated netlink messages in queue dump
  netdev: fix repeated netlink messages in queue stats
  selftests: net: support setting recv_size in YNL
  selftests: net-drv: queues: sanity check netlink dumps
  selftests: net-drv: stats: sanity check netlink dumps

 net/core/netdev-genl.c                        | 15 +++++-------
 tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
 tools/testing/selftests/drivers/net/stats.py  | 19 ++++++++++++++-
 tools/testing/selftests/net/lib/py/ynl.py     | 16 ++++++-------
 4 files changed, 45 insertions(+), 28 deletions(-)

-- 
2.47.1

[PATCH net 1/5] netdev: fix repeated netlink messages in queue dump

[Jakub Kicinski]

The context is supposed to record the next queue do dump,
not last dumped. If the dump doesn't fit we will restart
from the already-dumped queue, duplicating the message.

Before this fix and with the selftest improvements later
in this series we see:

  # ./run_kselftest.sh -t drivers/net:queues.py
  timeout set to 45
  selftests: drivers/net: queues.py
  KTAP version 1
  1..2
  # Check| At /root/ksft-net-drv/drivers/net/./queues.py, line 32, in get_queues:
  # Check|     ksft_eq(queues, expected)
  # Check failed 102 != 100
  # Check| At /root/ksft-net-drv/drivers/net/./queues.py, line 32, in get_queues:
  # Check|     ksft_eq(queues, expected)
  # Check failed 101 != 100
  not ok 1 queues.get_queues
  ok 2 queues.addremove_queues
  # Totals: pass:1 fail:1 xfail:0 xpass:0 skip:0 error:0
  not ok 1 selftests: drivers/net: queues.py # exit=1

With the fix:

  # ./ksft-net-drv/run_kselftest.sh -t drivers/net:queues.py
  timeout set to 45
  selftests: drivers/net: queues.py
  KTAP version 1
  1..2
  ok 1 queues.get_queues
  ok 2 queues.addremove_queues
  # Totals: pass:2 fail:0 xfail:0 xpass:0 skip:0 error:0

Fixes: 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jdamato@fastly.com
CC: almasrymina@google.com
CC: amritha.nambiar@intel.com
CC: sridhar.samudrala@intel.com
---
 net/core/netdev-genl.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 9527dd46e4dc..9f086b190619 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -488,24 +488,21 @@ netdev_nl_queue_dump_one(struct net_device *netdev, struct sk_buff *rsp,
 			 struct netdev_nl_dump_ctx *ctx)
 {
 	int err = 0;
-	int i;
 
 	if (!(netdev->flags & IFF_UP))
 		return err;
 
-	for (i = ctx->rxq_idx; i < netdev->real_num_rx_queues;) {
-		err = netdev_nl_queue_fill_one(rsp, netdev, i,
+	for (; ctx->rxq_idx < netdev->real_num_rx_queues; ctx->rxq_idx++) {
+		err = netdev_nl_queue_fill_one(rsp, netdev, ctx->rxq_idx,
 					       NETDEV_QUEUE_TYPE_RX, info);
 		if (err)
 			return err;
-		ctx->rxq_idx = i++;
 	}
-	for (i = ctx->txq_idx; i < netdev->real_num_tx_queues;) {
-		err = netdev_nl_queue_fill_one(rsp, netdev, i,
+	for (; ctx->txq_idx < netdev->real_num_tx_queues; ctx->txq_idx++) {
+		err = netdev_nl_queue_fill_one(rsp, netdev, ctx->txq_idx,
 					       NETDEV_QUEUE_TYPE_TX, info);
 		if (err)
 			return err;
-		ctx->txq_idx = i++;
 	}
 
 	return err;
-- 
2.47.1

Re: [PATCH net 1/5] netdev: fix repeated netlink messages in queue dump

[Joe Damato]

On Fri, Dec 13, 2024 at 07:22:40AM -0800, Jakub Kicinski wrote:
> The context is supposed to record the next queue do dump,

extremely minor nit: "next queue to dump" ?

[...]
 
> Fixes: 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jdamato@fastly.com
> CC: almasrymina@google.com
> CC: amritha.nambiar@intel.com
> CC: sridhar.samudrala@intel.com
> ---
>  net/core/netdev-genl.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 9527dd46e4dc..9f086b190619 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -488,24 +488,21 @@ netdev_nl_queue_dump_one(struct net_device *netdev, struct sk_buff *rsp,
>  			 struct netdev_nl_dump_ctx *ctx)
>  {
>  	int err = 0;
> -	int i;
>  
>  	if (!(netdev->flags & IFF_UP))
>  		return err;
>  
> -	for (i = ctx->rxq_idx; i < netdev->real_num_rx_queues;) {
> -		err = netdev_nl_queue_fill_one(rsp, netdev, i,
> +	for (; ctx->rxq_idx < netdev->real_num_rx_queues; ctx->rxq_idx++) {
> +		err = netdev_nl_queue_fill_one(rsp, netdev, ctx->rxq_idx,
>  					       NETDEV_QUEUE_TYPE_RX, info);
>  		if (err)
>  			return err;
> -		ctx->rxq_idx = i++;
>  	}
> -	for (i = ctx->txq_idx; i < netdev->real_num_tx_queues;) {
> -		err = netdev_nl_queue_fill_one(rsp, netdev, i,
> +	for (; ctx->txq_idx < netdev->real_num_tx_queues; ctx->txq_idx++) {
> +		err = netdev_nl_queue_fill_one(rsp, netdev, ctx->txq_idx,
>  					       NETDEV_QUEUE_TYPE_TX, info);
>  		if (err)
>  			return err;
> -		ctx->txq_idx = i++;
>  	}
>  
>  	return err;

Reviewed-by: Joe Damato <jdamato@fastly.com>

[PATCH net 2/5] netdev: fix repeated netlink messages in queue stats

[Jakub Kicinski]

The context is supposed to record the next queue do dump,
not last dumped. If the dump doesn't fit we will restart
from the already-dumped queue, duplicating the message.

Before this fix and with the selftest improvements later
in this series we see:

  # ./run_kselftest.sh -t drivers/net:stats.py
  timeout set to 45
  selftests: drivers/net: stats.py
  KTAP version 1
  1..5
  ok 1 stats.check_pause
  ok 2 stats.check_fec
  ok 3 stats.pkt_byte_sum
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 125, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
  # Check failed 45 != 44 repeated queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 127, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
  # Check failed 45 != 44 missing queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 125, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
  # Check failed 45 != 44 repeated queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 127, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
  # Check failed 45 != 44 missing queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 125, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
  # Check failed 103 != 100 repeated queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 127, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
  # Check failed 103 != 100 missing queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 125, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
  # Check failed 102 != 100 repeated queue keys
  # Check| At /root/ksft-net-drv/drivers/net/./stats.py, line 127, in qstat_by_ifindex:
  # Check|     ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
  # Check failed 102 != 100 missing queue keys
  not ok 4 stats.qstat_by_ifindex
  ok 5 stats.check_down
  # Totals: pass:4 fail:1 xfail:0 xpass:0 skip:0 error:0

With the fix:

  # ./ksft-net-drv/run_kselftest.sh -t drivers/net:stats.py
  timeout set to 45
  selftests: drivers/net: stats.py
  KTAP version 1
  1..5
  ok 1 stats.check_pause
  ok 2 stats.check_fec
  ok 3 stats.pkt_byte_sum
  ok 4 stats.qstat_by_ifindex
  ok 5 stats.check_down
  # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0

Fixes: ab63a2387cb9 ("netdev: add per-queue statistics")
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: jdamato@fastly.com
CC: almasrymina@google.com
CC: amritha.nambiar@intel.com
CC: xuanzhuo@linux.alibaba.com
CC: sdf@fomichev.me
---
 net/core/netdev-genl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
index 9f086b190619..1be8c7c21d19 100644
--- a/net/core/netdev-genl.c
+++ b/net/core/netdev-genl.c
@@ -668,7 +668,7 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
 					    i, info);
 		if (err)
 			return err;
-		ctx->rxq_idx = i++;
+		ctx->rxq_idx = ++i;
 	}
 	i = ctx->txq_idx;
 	while (ops->get_queue_stats_tx && i < netdev->real_num_tx_queues) {
@@ -676,7 +676,7 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
 					    i, info);
 		if (err)
 			return err;
-		ctx->txq_idx = i++;
+		ctx->txq_idx = ++i;
 	}
 
 	ctx->rxq_idx = 0;
-- 
2.47.1

Re: [PATCH net 2/5] netdev: fix repeated netlink messages in queue stats

[Joe Damato]

On Fri, Dec 13, 2024 at 07:22:41AM -0800, Jakub Kicinski wrote:
> The context is supposed to record the next queue do dump,

Same extremely minor nit as in previous patch: "next queue to dump"
?

[...]

> Fixes: ab63a2387cb9 ("netdev: add per-queue statistics")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: jdamato@fastly.com
> CC: almasrymina@google.com
> CC: amritha.nambiar@intel.com
> CC: xuanzhuo@linux.alibaba.com
> CC: sdf@fomichev.me
> ---
>  net/core/netdev-genl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/net/core/netdev-genl.c b/net/core/netdev-genl.c
> index 9f086b190619..1be8c7c21d19 100644
> --- a/net/core/netdev-genl.c
> +++ b/net/core/netdev-genl.c
> @@ -668,7 +668,7 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
>  					    i, info);
>  		if (err)
>  			return err;
> -		ctx->rxq_idx = i++;
> +		ctx->rxq_idx = ++i;
>  	}
>  	i = ctx->txq_idx;
>  	while (ops->get_queue_stats_tx && i < netdev->real_num_tx_queues) {
> @@ -676,7 +676,7 @@ netdev_nl_stats_by_queue(struct net_device *netdev, struct sk_buff *rsp,
>  					    i, info);
>  		if (err)
>  			return err;
> -		ctx->txq_idx = i++;
> +		ctx->txq_idx = ++i;
>  	}
>  
>  	ctx->rxq_idx = 0;

Reviewed-by: Joe Damato <jdamato@fastly.com>

[PATCH net 3/5] selftests: net: support setting recv_size in YNL

[Jakub Kicinski]

recv_size parameter allows constraining the buffer size for dumps.
It's useful in testing kernel handling of dump continuation,
IOW testing dumps which span multiple skbs.

Let the tests set this parameter when initializing the YNL family.
Keep the normal default, we don't want tests to unintentionally
behave very differently than normal code.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: jiri@resnulli.us
CC: petrm@nvidia.com
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/net/lib/py/ynl.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/ynl.py b/tools/testing/selftests/net/lib/py/ynl.py
index a0d689d58c57..076a7e8dc3eb 100644
--- a/tools/testing/selftests/net/lib/py/ynl.py
+++ b/tools/testing/selftests/net/lib/py/ynl.py
@@ -32,23 +32,23 @@ from .ksft import ksft_pr, ktap_result
 # Set schema='' to avoid jsonschema validation, it's slow
 #
 class EthtoolFamily(YnlFamily):
-    def __init__(self):
+    def __init__(self, recv_size=0):
         super().__init__((SPEC_PATH / Path('ethtool.yaml')).as_posix(),
-                         schema='')
+                         schema='', recv_size=recv_size)
 
 
 class RtnlFamily(YnlFamily):
-    def __init__(self):
+    def __init__(self, recv_size=0):
         super().__init__((SPEC_PATH / Path('rt_link.yaml')).as_posix(),
-                         schema='')
+                         schema='', recv_size=recv_size)
 
 
 class NetdevFamily(YnlFamily):
-    def __init__(self):
+    def __init__(self, recv_size=0):
         super().__init__((SPEC_PATH / Path('netdev.yaml')).as_posix(),
-                         schema='')
+                         schema='', recv_size=recv_size)
 
 class NetshaperFamily(YnlFamily):
-    def __init__(self):
+    def __init__(self, recv_size=0):
         super().__init__((SPEC_PATH / Path('net_shaper.yaml')).as_posix(),
-                         schema='')
+                         schema='', recv_size=recv_size)
-- 
2.47.1

Re: [PATCH net 3/5] selftests: net: support setting recv_size in YNL

[Joe Damato]

On Fri, Dec 13, 2024 at 07:22:42AM -0800, Jakub Kicinski wrote:
> recv_size parameter allows constraining the buffer size for dumps.
> It's useful in testing kernel handling of dump continuation,
> IOW testing dumps which span multiple skbs.
> 
> Let the tests set this parameter when initializing the YNL family.
> Keep the normal default, we don't want tests to unintentionally
> behave very differently than normal code.
> 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: jiri@resnulli.us
> CC: petrm@nvidia.com
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/net/lib/py/ynl.py | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/net/lib/py/ynl.py b/tools/testing/selftests/net/lib/py/ynl.py
> index a0d689d58c57..076a7e8dc3eb 100644
> --- a/tools/testing/selftests/net/lib/py/ynl.py
> +++ b/tools/testing/selftests/net/lib/py/ynl.py
> @@ -32,23 +32,23 @@ from .ksft import ksft_pr, ktap_result
>  # Set schema='' to avoid jsonschema validation, it's slow
>  #
>  class EthtoolFamily(YnlFamily):
> -    def __init__(self):
> +    def __init__(self, recv_size=0):
>          super().__init__((SPEC_PATH / Path('ethtool.yaml')).as_posix(),
> -                         schema='')
> +                         schema='', recv_size=recv_size)
>  
>  
>  class RtnlFamily(YnlFamily):
> -    def __init__(self):
> +    def __init__(self, recv_size=0):
>          super().__init__((SPEC_PATH / Path('rt_link.yaml')).as_posix(),
> -                         schema='')
> +                         schema='', recv_size=recv_size)
>  
>  
>  class NetdevFamily(YnlFamily):
> -    def __init__(self):
> +    def __init__(self, recv_size=0):
>          super().__init__((SPEC_PATH / Path('netdev.yaml')).as_posix(),
> -                         schema='')
> +                         schema='', recv_size=recv_size)
>  
>  class NetshaperFamily(YnlFamily):
> -    def __init__(self):
> +    def __init__(self, recv_size=0):
>          super().__init__((SPEC_PATH / Path('net_shaper.yaml')).as_posix(),
> -                         schema='')
> +                         schema='', recv_size=recv_size)

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net 3/5] selftests: net: support setting recv_size in YNL

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> recv_size parameter allows constraining the buffer size for dumps.
> It's useful in testing kernel handling of dump continuation,
> IOW testing dumps which span multiple skbs.
>
> Let the tests set this parameter when initializing the YNL family.
> Keep the normal default, we don't want tests to unintentionally
> behave very differently than normal code.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Petr Machata <petrm@nvidia.com>

[PATCH net 4/5] selftests: net-drv: queues: sanity check netlink dumps

[Jakub Kicinski]

This test already catches a netlink bug fixed by this series,
but only when running on HW with many queues. Make sure the
netdevsim instance created has a lot of queues, and constrain
the size of the recv_buffer used by netlink.

While at it test both rx and tx queues.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
index 30f29096e27c..9c5473abbd78 100755
--- a/tools/testing/selftests/drivers/net/queues.py
+++ b/tools/testing/selftests/drivers/net/queues.py
@@ -8,25 +8,28 @@ from lib.py import cmd
 import glob
 
 
-def sys_get_queues(ifname) -> int:
-    folders = glob.glob(f'/sys/class/net/{ifname}/queues/rx-*')
+def sys_get_queues(ifname, qtype='rx') -> int:
+    folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
     return len(folders)
 
 
-def nl_get_queues(cfg, nl):
+def nl_get_queues(cfg, nl, qtype='rx'):
     queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
     if queues:
-        return len([q for q in queues if q['type'] == 'rx'])
+        return len([q for q in queues if q['type'] == qtype])
     return None
 
 
 def get_queues(cfg, nl) -> None:
-    queues = nl_get_queues(cfg, nl)
-    if not queues:
-        raise KsftSkipEx('queue-get not supported by device')
+    snl = NetdevFamily(recv_size=4096)
 
-    expected = sys_get_queues(cfg.dev['ifname'])
-    ksft_eq(queues, expected)
+    for qtype in ['rx', 'tx']:
+        queues = nl_get_queues(cfg, snl, qtype)
+        if not queues:
+            raise KsftSkipEx('queue-get not supported by device')
+
+        expected = sys_get_queues(cfg.dev['ifname'], qtype)
+        ksft_eq(queues, expected)
 
 
 def addremove_queues(cfg, nl) -> None:
@@ -57,7 +60,7 @@ import glob
 
 
 def main() -> None:
-    with NetDrvEnv(__file__, queue_count=3) as cfg:
+    with NetDrvEnv(__file__, queue_count=100) as cfg:
         ksft_run([get_queues, addremove_queues], args=(cfg, NetdevFamily()))
     ksft_exit()
 
-- 
2.47.1

Re: [PATCH net 4/5] selftests: net-drv: queues: sanity check netlink dumps

[Joe Damato]

On Fri, Dec 13, 2024 at 07:22:43AM -0800, Jakub Kicinski wrote:
> This test already catches a netlink bug fixed by this series,
> but only when running on HW with many queues. Make sure the
> netdevsim instance created has a lot of queues, and constrain
> the size of the recv_buffer used by netlink.
> 
> While at it test both rx and tx queues.

Thanks for expanding the test to cover TX, as well.
 
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> index 30f29096e27c..9c5473abbd78 100755
> --- a/tools/testing/selftests/drivers/net/queues.py
> +++ b/tools/testing/selftests/drivers/net/queues.py
> @@ -8,25 +8,28 @@ from lib.py import cmd
>  import glob
>  
>  
> -def sys_get_queues(ifname) -> int:
> -    folders = glob.glob(f'/sys/class/net/{ifname}/queues/rx-*')
> +def sys_get_queues(ifname, qtype='rx') -> int:
> +    folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
>      return len(folders)
>  
>  
> -def nl_get_queues(cfg, nl):
> +def nl_get_queues(cfg, nl, qtype='rx'):
>      queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
>      if queues:
> -        return len([q for q in queues if q['type'] == 'rx'])
> +        return len([q for q in queues if q['type'] == qtype])
>      return None
>  
>  
>  def get_queues(cfg, nl) -> None:
> -    queues = nl_get_queues(cfg, nl)
> -    if not queues:
> -        raise KsftSkipEx('queue-get not supported by device')
> +    snl = NetdevFamily(recv_size=4096)
>  
> -    expected = sys_get_queues(cfg.dev['ifname'])
> -    ksft_eq(queues, expected)
> +    for qtype in ['rx', 'tx']:
> +        queues = nl_get_queues(cfg, snl, qtype)
> +        if not queues:
> +            raise KsftSkipEx('queue-get not supported by device')
> +
> +        expected = sys_get_queues(cfg.dev['ifname'], qtype)
> +        ksft_eq(queues, expected)
>  
>  
>  def addremove_queues(cfg, nl) -> None:
> @@ -57,7 +60,7 @@ import glob
>  
>  
>  def main() -> None:
> -    with NetDrvEnv(__file__, queue_count=3) as cfg:
> +    with NetDrvEnv(__file__, queue_count=100) as cfg:
>          ksft_run([get_queues, addremove_queues], args=(cfg, NetdevFamily()))
>      ksft_exit()
>  

Reviewed-by: Joe Damato <jdamato@fastly.com>

Re: [PATCH net 4/5] selftests: net-drv: queues: sanity check netlink dumps

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> This test already catches a netlink bug fixed by this series,
> but only when running on HW with many queues. Make sure the
> netdevsim instance created has a lot of queues, and constrain
> the size of the recv_buffer used by netlink.
>
> While at it test both rx and tx queues.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
>  1 file changed, 13 insertions(+), 10 deletions(-)
>
> diff --git a/tools/testing/selftests/drivers/net/queues.py b/tools/testing/selftests/drivers/net/queues.py
> index 30f29096e27c..9c5473abbd78 100755
> --- a/tools/testing/selftests/drivers/net/queues.py
> +++ b/tools/testing/selftests/drivers/net/queues.py
> @@ -8,25 +8,28 @@ from lib.py import cmd
>  import glob
>  
>  
> -def sys_get_queues(ifname) -> int:
> -    folders = glob.glob(f'/sys/class/net/{ifname}/queues/rx-*')
> +def sys_get_queues(ifname, qtype='rx') -> int:
> +    folders = glob.glob(f'/sys/class/net/{ifname}/queues/{qtype}-*')
>      return len(folders)
>  
>  
> -def nl_get_queues(cfg, nl):
> +def nl_get_queues(cfg, nl, qtype='rx'):
>      queues = nl.queue_get({'ifindex': cfg.ifindex}, dump=True)
>      if queues:
> -        return len([q for q in queues if q['type'] == 'rx'])
> +        return len([q for q in queues if q['type'] == qtype])
>      return None
>  
>  
>  def get_queues(cfg, nl) -> None:
> -    queues = nl_get_queues(cfg, nl)
> -    if not queues:
> -        raise KsftSkipEx('queue-get not supported by device')
> +    snl = NetdevFamily(recv_size=4096)
>  
> -    expected = sys_get_queues(cfg.dev['ifname'])
> -    ksft_eq(queues, expected)
> +    for qtype in ['rx', 'tx']:
> +        queues = nl_get_queues(cfg, snl, qtype)
> +        if not queues:
> +            raise KsftSkipEx('queue-get not supported by device')

Passing qtype in the message might be helpful here.

> +
> +        expected = sys_get_queues(cfg.dev['ifname'], qtype)
> +        ksft_eq(queues, expected)
>  
>  
>  def addremove_queues(cfg, nl) -> None:
> @@ -57,7 +60,7 @@ import glob
>  
>  
>  def main() -> None:
> -    with NetDrvEnv(__file__, queue_count=3) as cfg:
> +    with NetDrvEnv(__file__, queue_count=100) as cfg:
>          ksft_run([get_queues, addremove_queues], args=(cfg, NetdevFamily()))
>      ksft_exit()

Hmm, get_queues() doesn't use the passed-in NetdevFamily(), but that
gets created anyway for addremove_queues(), so whatever.

Reviewed-by: Petr Machata <petrm@nvidia.com>

[PATCH net 5/5] selftests: net-drv: stats: sanity check netlink dumps

[Jakub Kicinski]

Sanity check netlink dumps, to make sure dumps don't have
repeated entries or gaps in IDs.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: shuah@kernel.org
CC: linux-kselftest@vger.kernel.org
---
 tools/testing/selftests/drivers/net/stats.py | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
index 63e3c045a3b2..031ac9def6c0 100755
--- a/tools/testing/selftests/drivers/net/stats.py
+++ b/tools/testing/selftests/drivers/net/stats.py
@@ -110,6 +110,23 @@ rtnl = RtnlFamily()
             ksft_ge(triple[1][key], triple[0][key], comment="bad key: " + key)
             ksft_ge(triple[2][key], triple[1][key], comment="bad key: " + key)
 
+    # Sanity check the dumps
+    queues = NetdevFamily(recv_size=4096).qstats_get({"scope": "queue"}, dump=True)
+    # Reformat the output into {ifindex: {rx: [id, id, ...], tx: [id, id, ...]}}
+    parsed = {}
+    for entry in queues:
+        ifindex = entry["ifindex"]
+        if ifindex not in parsed:
+            parsed[ifindex] = {"rx":[], "tx": []}
+        parsed[ifindex][entry["queue-type"]].append(entry['queue-id'])
+    # Now, validate
+    for ifindex, queues in parsed.items():
+        for qtype in ['rx', 'tx']:
+            ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
+                    comment="repeated queue keys")
+            ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
+                    comment="missing queue keys")
+
     # Test invalid dumps
     # 0 is invalid
     with ksft_raises(NlError) as cm:
@@ -158,7 +175,7 @@ rtnl = RtnlFamily()
 
 
 def main() -> None:
-    with NetDrvEnv(__file__) as cfg:
+    with NetDrvEnv(__file__, queue_count=100) as cfg:
         ksft_run([check_pause, check_fec, pkt_byte_sum, qstat_by_ifindex,
                   check_down],
                  args=(cfg, ))
-- 
2.47.1

Re: [PATCH net 5/5] selftests: net-drv: stats: sanity check netlink dumps

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Sanity check netlink dumps, to make sure dumps don't have
> repeated entries or gaps in IDs.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: shuah@kernel.org
> CC: linux-kselftest@vger.kernel.org
> ---
>  tools/testing/selftests/drivers/net/stats.py | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/drivers/net/stats.py b/tools/testing/selftests/drivers/net/stats.py
> index 63e3c045a3b2..031ac9def6c0 100755
> --- a/tools/testing/selftests/drivers/net/stats.py
> +++ b/tools/testing/selftests/drivers/net/stats.py
> @@ -110,6 +110,23 @@ rtnl = RtnlFamily()
>              ksft_ge(triple[1][key], triple[0][key], comment="bad key: " + key)
>              ksft_ge(triple[2][key], triple[1][key], comment="bad key: " + key)
>  
> +    # Sanity check the dumps
> +    queues = NetdevFamily(recv_size=4096).qstats_get({"scope": "queue"}, dump=True)
> +    # Reformat the output into {ifindex: {rx: [id, id, ...], tx: [id, id, ...]}}
> +    parsed = {}
> +    for entry in queues:
> +        ifindex = entry["ifindex"]
> +        if ifindex not in parsed:
> +            parsed[ifindex] = {"rx":[], "tx": []}
> +        parsed[ifindex][entry["queue-type"]].append(entry['queue-id'])

BTW setdefault() exists for exactly these add-unless-already-exists
scenarios:

        parsed_entry = parsed.setdefault(ifindex, {"rx":[], "tx": []})
        parsed_entry[entry["queue-type"]].append(entry['queue-id'])

Sometimes this can be used to inline the whole expression, such as
mydict.setdefault(key, []).append(value), but that would be unwieldy here.

Anyway, consider rewriting, but it's a nit, it's readable just fine as is.

Reviewed-by: Petr Machata <petrm@nvidia.com>

> +    # Now, validate
> +    for ifindex, queues in parsed.items():
> +        for qtype in ['rx', 'tx']:
> +            ksft_eq(len(queues[qtype]), len(set(queues[qtype])),
> +                    comment="repeated queue keys")
> +            ksft_eq(len(queues[qtype]), max(queues[qtype]) + 1,
> +                    comment="missing queue keys")
> +
>      # Test invalid dumps
>      # 0 is invalid
>      with ksft_raises(NlError) as cm:
> @@ -158,7 +175,7 @@ rtnl = RtnlFamily()
>  
>  
>  def main() -> None:
> -    with NetDrvEnv(__file__) as cfg:
> +    with NetDrvEnv(__file__, queue_count=100) as cfg:
>          ksft_run([check_pause, check_fec, pkt_byte_sum, qstat_by_ifindex,
>                    check_down],
>                   args=(cfg, ))

Re: [PATCH net 5/5] selftests: net-drv: stats: sanity check netlink dumps

[Jakub Kicinski]

On Mon, 16 Dec 2024 11:53:58 +0100 Petr Machata wrote:
> > +        if ifindex not in parsed:
> > +            parsed[ifindex] = {"rx":[], "tx": []}
> > +        parsed[ifindex][entry["queue-type"]].append(entry['queue-id'])  
> 
> BTW setdefault() exists for exactly these add-unless-already-exists
> scenarios:
> 
>         parsed_entry = parsed.setdefault(ifindex, {"rx":[], "tx": []})
>         parsed_entry[entry["queue-type"]].append(entry['queue-id'])
> 
> Sometimes this can be used to inline the whole expression, such as
> mydict.setdefault(key, []).append(value), but that would be unwieldy here.

Ack, I used setdefault() initially but it made the line too
incomprehensible. Too many things get indexed at once..

Re: [PATCH net 0/5] netdev: fix repeated netlink messages in queue dumps

[Joe Damato]

On Fri, Dec 13, 2024 at 07:22:39AM -0800, Jakub Kicinski wrote:
> Fix dump continuation for queues and queue stats in the netdev family.
> Because we used post-increment when saving id of dumped queue next
> skb would re-dump the already dumped queue.
> 
> Jakub Kicinski (5):
>   netdev: fix repeated netlink messages in queue dump
>   netdev: fix repeated netlink messages in queue stats
>   selftests: net: support setting recv_size in YNL
>   selftests: net-drv: queues: sanity check netlink dumps
>   selftests: net-drv: stats: sanity check netlink dumps

Thanks for the work and improvements.

Patch 1 and 2 definitely seem to be "fixes" and are against net
which seems appropriate to me.

Patches 3, 4, and 5 seem like new features, though. Should those
three be a separate series against net-next, instead?

In the event that you do decide to separate out 3, 4, and 5 you can
feel free to carry along my Reviewed-bys.

Re: [PATCH net 0/5] netdev: fix repeated netlink messages in queue dumps

[Jakub Kicinski]

Thanks for the reviews!

On Fri, 13 Dec 2024 13:52:11 -0800 Joe Damato wrote:
> Patches 3, 4, and 5 seem like new features, though. Should those
> three be a separate series against net-next, instead?

Right, this case is a bit disputable. Docs and selftests can't really
break the current release. And someone who backports the fixes should 
be able to easily test them.

Also, thinking selfishly as a maintainer, having them in one series
saves me mental tracking of who promised to send tests via another
tree and hasn't posted them yet.

Re: [PATCH net 0/5] netdev: fix repeated netlink messages in queue dumps

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Fri, 13 Dec 2024 07:22:39 -0800 you wrote:
> Fix dump continuation for queues and queue stats in the netdev family.
> Because we used post-increment when saving id of dumped queue next
> skb would re-dump the already dumped queue.
> 
> Jakub Kicinski (5):
>   netdev: fix repeated netlink messages in queue dump
>   netdev: fix repeated netlink messages in queue stats
>   selftests: net: support setting recv_size in YNL
>   selftests: net-drv: queues: sanity check netlink dumps
>   selftests: net-drv: stats: sanity check netlink dumps
> 
> [...]

Here is the summary with links:
  - [net,1/5] netdev: fix repeated netlink messages in queue dump
    https://git.kernel.org/netdev/net/c/b1f3a2f5a742
  - [net,2/5] netdev: fix repeated netlink messages in queue stats
    https://git.kernel.org/netdev/net/c/ecc391a54157
  - [net,3/5] selftests: net: support setting recv_size in YNL
    https://git.kernel.org/netdev/net/c/0518863407b8
  - [net,4/5] selftests: net-drv: queues: sanity check netlink dumps
    https://git.kernel.org/netdev/net/c/1234810b1649
  - [net,5/5] selftests: net-drv: stats: sanity check netlink dumps
    https://git.kernel.org/netdev/net/c/5712e323d4c3

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html