netdev: fix repeated netlink messages in queue dumps

The context is supposed to record the next queue do dump,

Fixes 6b6171db7fc8 ("netdev-genl: Add netlink framework functions for queue")

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 net/core/netdev-genl.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

net/core/netdev-genl.c +2 -5

index 9527dd46e4dc..9f086b190619 100644
@@ -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;) {

The context is supposed to record the next queue do dump,

Fixes ab63a2387cb9 ("netdev: add per-queue statistics")

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 net/core/netdev-genl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

net/core/netdev-genl.c +1 -1

index 9f086b190619..1be8c7c21d19 100644
@@ -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) {

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 <[email protected]>

---
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: [email protected]

---
 tools/testing/selftests/net/lib/py/ynl.py | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

tools/testing/selftests/net/lib/py/ynl.py +5 -5

index a0d689d58c57..076a7e8dc3eb 100644
@@ -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):

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 <[email protected]>

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 <[email protected]>

---
CC: [email protected]
CC: [email protected]

---
 tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

tools/testing/selftests/drivers/net/queues.py +12 -9

index 30f29096e27c..9c5473abbd78 100755
@@ -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:

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 <[email protected]>

---
CC: [email protected]
CC: [email protected]

---
 tools/testing/selftests/drivers/net/queues.py | 23 +++++++++++--------
 1 file changed, 13 insertions(+), 10 deletions(-)

tools/testing/selftests/drivers/net/queues.py +4 -5

index 30f29096e27c..9c5473abbd78 100755
@@ -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)

tools/testing/selftests/drivers/net/queues.py +3 -0

+
+        expected = sys_get_queues(cfg.dev['ifname'], qtype)
+        ksft_eq(queues, expected)
 
 
 def addremove_queues(cfg, nl) -> None:

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

Signed-off-by Jakub Kicinski <[email protected]>

---
CC: [email protected]
CC: [email protected]

---
 tools/testing/selftests/drivers/net/stats.py | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

tools/testing/selftests/drivers/net/stats.py +0 -0

index 63e3c045a3b2..031ac9def6c0 100755
@@ -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)

tools/testing/selftests/drivers/net/stats.py +9 -1

+    # 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, ))

tools/testing/selftests/drivers/net/stats.py rtnl = RtnlFamily() · line 121

Quoted 3 lines

tools/testing/selftests/drivers/net/stats.py +3 -0

+        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.

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

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