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

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