[PATCH net-next 0/6] selftests: net: exercise page pool reporting via netlink

[PATCH net-next 0/6] selftests: net: exercise page pool reporting via netlink

[Jakub Kicinski]

Add a basic test for page pool netlink reporting.

Jakub Kicinski (6):
  net: netdevsim: add some fake page pool use
  tools: ynl: don't return None for dumps
  selftests: net: print report check location in python tests
  selftests: net: print full exception on failure
  selftests: net: support use of NetdevSimDev under "with" in python
  selftests: net: exercise page pool reporting via netlink

 drivers/net/netdevsim/netdev.c             | 93 ++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h          |  4 +
 tools/net/ynl/lib/ynl.py                   |  4 +-
 tools/testing/selftests/net/lib/py/ksft.py | 29 ++++---
 tools/testing/selftests/net/lib/py/nsim.py | 22 ++++-
 tools/testing/selftests/net/nl_netdev.py   | 79 +++++++++++++++++-
 6 files changed, 215 insertions(+), 16 deletions(-)

-- 
2.44.0

[PATCH net-next 1/6] net: netdevsim: add some fake page pool use

[Jakub Kicinski]

Add very basic page pool use so that we can exercise
the netlink uAPI in a selftest.

Page pool gets created on open, destroyed on close.
But we control allocating of a single page thru debugfs.
This page may survive past the page pool itself so that
we can test orphaned page pools.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/netdev.c    | 93 +++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |  4 ++
 2 files changed, 97 insertions(+)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index d7ba447db17c..d127856f8f36 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -20,6 +20,7 @@
 #include <linux/netdevice.h>
 #include <linux/slab.h>
 #include <net/netdev_queues.h>
+#include <net/page_pool/helpers.h>
 #include <net/netlink.h>
 #include <net/pkt_cls.h>
 #include <net/rtnetlink.h>
@@ -299,6 +300,29 @@ static int nsim_get_iflink(const struct net_device *dev)
 	return iflink;
 }
 
+static int nsim_open(struct net_device *dev)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+	struct page_pool_params pp = { 0 };
+
+	pp.pool_size = 128;
+	pp.dev = &dev->dev;
+	pp.dma_dir = DMA_BIDIRECTIONAL;
+	pp.netdev = dev;
+
+	ns->pp = page_pool_create(&pp);
+	return PTR_ERR_OR_ZERO(ns->pp);
+}
+
+static int nsim_stop(struct net_device *dev)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	page_pool_destroy(ns->pp);
+
+	return 0;
+}
+
 static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_start_xmit		= nsim_start_xmit,
 	.ndo_set_rx_mode	= nsim_set_rx_mode,
@@ -318,6 +342,8 @@ static const struct net_device_ops nsim_netdev_ops = {
 	.ndo_set_features	= nsim_set_features,
 	.ndo_get_iflink		= nsim_get_iflink,
 	.ndo_bpf		= nsim_bpf,
+	.ndo_open		= nsim_open,
+	.ndo_stop		= nsim_stop,
 };
 
 static const struct net_device_ops nsim_vf_netdev_ops = {
@@ -378,6 +404,60 @@ static const struct netdev_stat_ops nsim_stat_ops = {
 	.get_base_stats		= nsim_get_base_stats,
 };
 
+static ssize_t
+nsim_pp_hold_read(struct file *file, char __user *data,
+		  size_t count, loff_t *ppos)
+{
+	struct netdevsim *ns = file->private_data;
+	char buf[3] = "n\n";
+
+	if (ns->page)
+		buf[0] = 'y';
+
+	return simple_read_from_buffer(data, count, ppos, buf, 2);
+}
+
+static ssize_t
+nsim_pp_hold_write(struct file *file, const char __user *data,
+		   size_t count, loff_t *ppos)
+{
+	struct netdevsim *ns = file->private_data;
+	ssize_t ret;
+	bool val;
+
+	ret = kstrtobool_from_user(data, count, &val);
+	if (ret)
+		return ret;
+
+	rtnl_lock();
+	ret = count;
+	if (val == !!ns->page)
+		goto exit;
+
+	if (!netif_running(ns->netdev) && val) {
+		ret = -ENETDOWN;
+	} else if (val) {
+		ns->page = page_pool_dev_alloc_pages(ns->pp);
+		if (!ns->page)
+			ret = -ENOMEM;
+	} else {
+		page_pool_put_full_page(ns->page->pp, ns->page, false);
+		ns->page = NULL;
+	}
+	rtnl_unlock();
+
+exit:
+	return count;
+}
+
+static const struct file_operations nsim_pp_hold_fops = {
+	.open = simple_open,
+	.read = nsim_pp_hold_read,
+	.write = nsim_pp_hold_write,
+	.llseek = generic_file_llseek,
+	.owner = THIS_MODULE,
+};
+
 static void nsim_setup(struct net_device *dev)
 {
 	ether_setup(dev);
@@ -485,6 +565,10 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 		err = nsim_init_netdevsim_vf(ns);
 	if (err)
 		goto err_free_netdev;
+
+	ns->pp_dfs = debugfs_create_file("pp_hold", 0600, nsim_dev_port->ddir,
+					 ns, &nsim_pp_hold_fops);
+
 	return ns;
 
 err_free_netdev:
@@ -497,6 +581,8 @@ void nsim_destroy(struct netdevsim *ns)
 	struct net_device *dev = ns->netdev;
 	struct netdevsim *peer;
 
+	debugfs_remove(ns->pp_dfs);
+
 	rtnl_lock();
 	peer = rtnl_dereference(ns->peer);
 	if (peer)
@@ -511,6 +597,13 @@ void nsim_destroy(struct netdevsim *ns)
 	rtnl_unlock();
 	if (nsim_dev_port_is_pf(ns->nsim_dev_port))
 		nsim_exit_netdevsim(ns);
+
+	/* Put this intentionally late to exercise the orphaning path */
+	if (ns->page) {
+		page_pool_put_full_page(ns->page->pp, ns->page, false);
+		ns->page = NULL;
+	}
+
 	free_netdev(dev);
 }
 
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 553c4b9b4f63..7664ab823e29 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -125,6 +125,10 @@ struct netdevsim {
 		struct debugfs_u32_array dfs_ports[2];
 	} udp_ports;
 
+	struct page_pool *pp;
+	struct page *page;
+	struct dentry *pp_dfs;
+
 	struct nsim_ethtool ethtool;
 	struct netdevsim __rcu *peer;
 };
-- 
2.44.0

[PATCH net-next 2/6] tools: ynl: don't return None for dumps

[Jakub Kicinski]

YNL currently reports None for empty dump:

 $ cli.py ...netdev.yaml --dump page-pool-get
 None

This doesn't matter for the CLI but when writing YNL based tests
having to deal with either list or None is annoying. Limit the
None conversion to non-dump ops:

 $ cli.py ...netdev.yaml --dump page-pool-get
 []

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
CC: donald.hunter@gmail.com
CC: jiri@resnulli.us
---
 tools/net/ynl/lib/ynl.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
index 0ba5f6fb8747..a67f7b6fef92 100644
--- a/tools/net/ynl/lib/ynl.py
+++ b/tools/net/ynl/lib/ynl.py
@@ -995,9 +995,11 @@ genl_family_name_to_id = None
                     rsp_msg.update(self._decode_struct(decoded.raw, op.fixed_header))
                 rsp.append(rsp_msg)
 
+        if dump:
+            return rsp
         if not rsp:
             return None
-        if not dump and len(rsp) == 1:
+        if len(rsp) == 1:
             return rsp[0]
         return rsp
 
-- 
2.44.0

Re: [PATCH net-next 2/6] tools: ynl: don't return None for dumps

[Donald Hunter]

Jakub Kicinski <kuba@kernel.org> writes:

> YNL currently reports None for empty dump:
>
>  $ cli.py ...netdev.yaml --dump page-pool-get
>  None
>
> This doesn't matter for the CLI but when writing YNL based tests
> having to deal with either list or None is annoying. Limit the
> None conversion to non-dump ops:
>
>  $ cli.py ...netdev.yaml --dump page-pool-get
>  []

Makes sense and I'll need to update my --multi patch to retain this
behaviour.

Reviewed-by: Donald Hunter <donald.hunter@gmail.com>

> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
> CC: donald.hunter@gmail.com
> CC: jiri@resnulli.us
> ---
>  tools/net/ynl/lib/ynl.py | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/tools/net/ynl/lib/ynl.py b/tools/net/ynl/lib/ynl.py
> index 0ba5f6fb8747..a67f7b6fef92 100644
> --- a/tools/net/ynl/lib/ynl.py
> +++ b/tools/net/ynl/lib/ynl.py
> @@ -995,9 +995,11 @@ genl_family_name_to_id = None
>                      rsp_msg.update(self._decode_struct(decoded.raw, op.fixed_header))
>                  rsp.append(rsp_msg)
>  
> +        if dump:
> +            return rsp
>          if not rsp:
>              return None
> -        if not dump and len(rsp) == 1:
> +        if len(rsp) == 1:
>              return rsp[0]
>          return rsp

[PATCH net-next 3/6] selftests: net: print report check location in python tests

[Jakub Kicinski]

Developing Python tests is a bit annoying because when test fails
we only print the fail message and no info about which exact check
led to it. Print the location (the first line of this example is new):

  # At /root/ksft-net-drv/./net/nl_netdev.py line 38:
  # Check failed 0 != 10
  not ok 3 nl_netdev.page_pool_check

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/ksft.py | 25 ++++++++++++----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index c7210525981c..5838aadd95a7 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 
 import builtins
+import inspect
 from .consts import KSFT_MAIN_NAME
 
 KSFT_RESULT = None
@@ -18,32 +19,34 @@ KSFT_RESULT = None
     print("#", *objs, **kwargs)
 
 
+def _fail(*args):
+    global KSFT_RESULT
+    KSFT_RESULT = False
+
+    frame = inspect.stack()[2]
+    ksft_pr("At " + frame.filename + " line " + str(frame.lineno) + ":")
+    ksft_pr(" ".join([str(a) for a in args]))
+
+
 def ksft_eq(a, b, comment=""):
     global KSFT_RESULT
     if a != b:
-        KSFT_RESULT = False
-        ksft_pr("Check failed", a, "!=", b, comment)
+        _fail("Check failed", a, "!=", b, comment)
 
 
 def ksft_true(a, comment=""):
-    global KSFT_RESULT
     if not a:
-        KSFT_RESULT = False
-        ksft_pr("Check failed", a, "does not eval to True", comment)
+        _fail("Check failed", a, "does not eval to True", comment)
 
 
 def ksft_in(a, b, comment=""):
-    global KSFT_RESULT
     if a not in b:
-        KSFT_RESULT = False
-        ksft_pr("Check failed", a, "not in", b, comment)
+        _fail("Check failed", a, "not in", b, comment)
 
 
 def ksft_ge(a, b, comment=""):
-    global KSFT_RESULT
     if a < b:
-        KSFT_RESULT = False
-        ksft_pr("Check failed", a, "<", b, comment)
+        _fail("Check failed", a, "<", b, comment)
 
 
 def ktap_result(ok, cnt=1, case="", comment=""):
-- 
2.44.0

Re: [PATCH net-next 3/6] selftests: net: print report check location in python tests

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Developing Python tests is a bit annoying because when test fails
> we only print the fail message and no info about which exact check
> led to it. Print the location (the first line of this example is new):
>
>   # At /root/ksft-net-drv/./net/nl_netdev.py line 38:
>   # Check failed 0 != 10
>   not ok 3 nl_netdev.page_pool_check
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

> +def _fail(*args):
> +    global KSFT_RESULT
> +    KSFT_RESULT = False
> +
> +    frame = inspect.stack()[2]
> +    ksft_pr("At " + frame.filename + " line " + str(frame.lineno) + ":")
> +    ksft_pr(" ".join([str(a) for a in args]))

I think this could have just been ksft_pr(*args) like before, but
whatever.

> +
>  def ksft_eq(a, b, comment=""):
>      global KSFT_RESULT
>      if a != b:
> -        KSFT_RESULT = False
> -        ksft_pr("Check failed", a, "!=", b, comment)
> +        _fail("Check failed", a, "!=", b, comment)

[PATCH net-next 4/6] selftests: net: print full exception on failure

[Jakub Kicinski]

Instead of a summary line print the full exception.
This makes debugging Python tests much easier.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/ksft.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/lib/py/ksft.py b/tools/testing/selftests/net/lib/py/ksft.py
index 5838aadd95a7..6e1f4685669c 100644
--- a/tools/testing/selftests/net/lib/py/ksft.py
+++ b/tools/testing/selftests/net/lib/py/ksft.py
@@ -2,6 +2,7 @@
 
 import builtins
 import inspect
+import traceback
 from .consts import KSFT_MAIN_NAME
 
 KSFT_RESULT = None
@@ -85,7 +86,8 @@ KSFT_RESULT = None
             totals['xfail'] += 1
             continue
         except Exception as e:
-            for line in str(e).split('\n'):
+            tb = traceback.format_exc()
+            for line in tb.strip().split('\n'):
                 ksft_pr("Exception|", line)
             ktap_result(False, cnt, case)
             totals['fail'] += 1
-- 
2.44.0

Re: [PATCH net-next 4/6] selftests: net: print full exception on failure

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Instead of a summary line print the full exception.
> This makes debugging Python tests much easier.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

> @@ -85,7 +86,8 @@ KSFT_RESULT = None
>              totals['xfail'] += 1
>              continue
>          except Exception as e:
> -            for line in str(e).split('\n'):
> +            tb = traceback.format_exc()
> +            for line in tb.strip().split('\n'):

(The strip is necessary to get rid of trailing newlines.)

>                  ksft_pr("Exception|", line)
>              ktap_result(False, cnt, case)
>              totals['fail'] += 1

[PATCH net-next 5/6] selftests: net: support use of NetdevSimDev under "with" in python

[Jakub Kicinski]

Using "with" on an entire driver test env is supported already,
but it's also useful to use "with" on an individual nsim.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/nsim.py | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/lib/py/nsim.py b/tools/testing/selftests/net/lib/py/nsim.py
index 97457aca7e08..94aa32f59fdb 100644
--- a/tools/testing/selftests/net/lib/py/nsim.py
+++ b/tools/testing/selftests/net/lib/py/nsim.py
@@ -84,6 +84,17 @@ from .utils import cmd, ip
         for port_index in range(port_count):
             self.nsims.append(self._make_port(port_index, ifnames[port_index]))
 
+        self.removed = False
+
+    def __enter__(self):
+        return self
+
+    def __exit__(self, ex_type, ex_value, ex_tb):
+        """
+        __exit__ gets called at the end of a "with" block.
+        """
+        self.remove()
+
     def _make_port(self, port_index, ifname):
         return NetdevSim(self, port_index, ifname, self.ns)
 
@@ -112,7 +123,9 @@ from .utils import cmd, ip
             raise Exception("netdevices did not appear within timeout")
 
     def remove(self):
-        self.ctrl_write("del_device", "%u" % (self.addr, ))
+        if not self.removed:
+            self.ctrl_write("del_device", "%u" % (self.addr, ))
+            self.removed = True
 
     def remove_nsim(self, nsim):
         self.nsims.remove(nsim)
-- 
2.44.0

Re: [PATCH net-next 5/6] selftests: net: support use of NetdevSimDev under "with" in python

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Using "with" on an entire driver test env is supported already,
> but it's also useful to use "with" on an individual nsim.
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

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

[PATCH net-next 6/6] selftests: net: exercise page pool reporting via netlink

[Jakub Kicinski]

Add a Python test for the basic ops.

  # ./net/nl_netdev.py
  KTAP version 1
  1..3
  ok 1 nl_netdev.empty_check
  ok 2 nl_netdev.lo_check
  ok 3 nl_netdev.page_pool_check
  # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 tools/testing/selftests/net/lib/py/nsim.py |  7 ++
 tools/testing/selftests/net/nl_netdev.py   | 79 +++++++++++++++++++++-
 2 files changed, 84 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/net/lib/py/nsim.py b/tools/testing/selftests/net/lib/py/nsim.py
index 94aa32f59fdb..1fd50a308408 100644
--- a/tools/testing/selftests/net/lib/py/nsim.py
+++ b/tools/testing/selftests/net/lib/py/nsim.py
@@ -28,6 +28,13 @@ from .utils import cmd, ip
         self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
         ret = ip("-j link show dev %s" % ifname, ns=ns)
         self.dev = json.loads(ret.stdout)[0]
+        self.ifindex = self.dev["ifindex"]
+
+    def up(self):
+        ip("link set dev {} up".format(self.ifname))
+
+    def down(self):
+        ip("link set dev {} down".format(self.ifname))
 
     def dfs_write(self, path, val):
         self.nsimdev.dfs_write(f'ports/{self.port_index}/' + path, val)
diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
index 2b8b488fb419..afc510c044ce 100755
--- a/tools/testing/selftests/net/nl_netdev.py
+++ b/tools/testing/selftests/net/nl_netdev.py
@@ -1,7 +1,8 @@
 #!/usr/bin/env python3
 # SPDX-License-Identifier: GPL-2.0
 
-from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily
+import time
+from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily, NetdevSimDev, ip
 
 
 def empty_check(nf) -> None:
@@ -15,9 +16,83 @@ from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily
     ksft_eq(len(lo_info['xdp-rx-metadata-features']), 0)
 
 
+def page_pool_check(nf) -> None:
+    with NetdevSimDev() as nsimdev:
+        nsim = nsimdev.nsims[0]
+
+        # No page pools when down
+        nsim.down()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        ksft_eq(len(pp_list), 0)
+
+        # Up, empty page pool appears
+        nsim.up()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        ksft_ge(len(pp_list), 0)
+        refs = sum([pp["inflight"] for pp in pp_list])
+        ksft_eq(refs, 0)
+
+        # Down, it disappears, again
+        nsim.down()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        ksft_eq(len(pp_list), 0)
+
+        # Up, allocate a page
+        nsim.up()
+        nsim.dfs_write("pp_hold", "y")
+        pp_list = nf.page_pool_get({}, dump=True)
+        refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") == nsim.ifindex])
+        ksft_ge(refs, 1)
+
+        # Now let's leak a page
+        nsim.down()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        ksft_eq(len(pp_list), 1)
+        refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") == nsim.ifindex])
+        ksft_eq(refs, 1)
+        undetached = [pp for pp in pp_list if "detach-time" not in pp]
+        ksft_eq(len(undetached), 0)
+
+        # New pp can get created, and we'll have two
+        nsim.up()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        attached = [pp for pp in pp_list if "detach-time" not in pp]
+        undetached = [pp for pp in pp_list if "detach-time" in pp]
+        ksft_eq(len(attached), 1)
+        ksft_eq(len(undetached), 1)
+
+        # Free the old page and the old pp is gone
+        nsim.dfs_write("pp_hold", "n")
+        # Freeing check is once a second so we may need to retry
+        for i in range(50):
+            pp_list = nf.page_pool_get({}, dump=True)
+            pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+            if len(pp_list) == 1:
+                break
+            time.sleep(0.05)
+        ksft_eq(len(pp_list), 1)
+
+        # And down...
+        nsim.down()
+        pp_list = nf.page_pool_get({}, dump=True)
+        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
+        ksft_eq(len(pp_list), 0)
+
+        # Last, leave the page hanging for destroy, nothing to check
+        # we're trying to exercise the orphaning path in the kernel
+        nsim.up()
+        nsim.dfs_write("pp_hold", "y")
+
+
 def main() -> None:
     nf = NetdevFamily()
-    ksft_run([empty_check, lo_check], args=(nf, ))
+    ksft_run([empty_check, lo_check, page_pool_check],
+             args=(nf, ))
 
 
 if __name__ == "__main__":
-- 
2.44.0

Re: [PATCH net-next 6/6] selftests: net: exercise page pool reporting via netlink

[Petr Machata]

Jakub Kicinski <kuba@kernel.org> writes:

> Add a Python test for the basic ops.
>
>   # ./net/nl_netdev.py
>   KTAP version 1
>   1..3
>   ok 1 nl_netdev.empty_check
>   ok 2 nl_netdev.lo_check
>   ok 3 nl_netdev.page_pool_check
>   # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>
> ---
>  tools/testing/selftests/net/lib/py/nsim.py |  7 ++
>  tools/testing/selftests/net/nl_netdev.py   | 79 +++++++++++++++++++++-
>  2 files changed, 84 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/net/lib/py/nsim.py b/tools/testing/selftests/net/lib/py/nsim.py
> index 94aa32f59fdb..1fd50a308408 100644
> --- a/tools/testing/selftests/net/lib/py/nsim.py
> +++ b/tools/testing/selftests/net/lib/py/nsim.py
> @@ -28,6 +28,13 @@ from .utils import cmd, ip
>          self.dfs_dir = "%s/ports/%u/" % (nsimdev.dfs_dir, port_index)
>          ret = ip("-j link show dev %s" % ifname, ns=ns)
>          self.dev = json.loads(ret.stdout)[0]
> +        self.ifindex = self.dev["ifindex"]
> +
> +    def up(self):
> +        ip("link set dev {} up".format(self.ifname))
> +
> +    def down(self):
> +        ip("link set dev {} down".format(self.ifname))

Yeah, what I meant by integration pain with LNST the other day was
basically this. You end up rewriting the iproute2 API in Python.
But it is what it is.

>      def dfs_write(self, path, val):
>          self.nsimdev.dfs_write(f'ports/{self.port_index}/' + path, val)
> diff --git a/tools/testing/selftests/net/nl_netdev.py b/tools/testing/selftests/net/nl_netdev.py
> index 2b8b488fb419..afc510c044ce 100755
> --- a/tools/testing/selftests/net/nl_netdev.py
> +++ b/tools/testing/selftests/net/nl_netdev.py
> @@ -1,7 +1,8 @@
>  #!/usr/bin/env python3
>  # SPDX-License-Identifier: GPL-2.0
>  
> -from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily
> +import time
> +from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily, NetdevSimDev, ip
>  
>  
>  def empty_check(nf) -> None:
> @@ -15,9 +16,83 @@ from lib.py import ksft_run, ksft_pr, ksft_eq, ksft_ge, NetdevFamily
>      ksft_eq(len(lo_info['xdp-rx-metadata-features']), 0)
>  
>  
> +def page_pool_check(nf) -> None:
> +    with NetdevSimDev() as nsimdev:
> +        nsim = nsimdev.nsims[0]
> +
> +        # No page pools when down
> +        nsim.down()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]

This combo of page_pool_get / filter looks like it should be in a helper.

> +        ksft_eq(len(pp_list), 0)
> +
> +        # Up, empty page pool appears
> +        nsim.up()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +        ksft_ge(len(pp_list), 0)
> +        refs = sum([pp["inflight"] for pp in pp_list])
> +        ksft_eq(refs, 0)
> +
> +        # Down, it disappears, again
> +        nsim.down()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +        ksft_eq(len(pp_list), 0)
> +
> +        # Up, allocate a page
> +        nsim.up()
> +        nsim.dfs_write("pp_hold", "y")
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") == nsim.ifindex])
> +        ksft_ge(refs, 1)
> +
> +        # Now let's leak a page
> +        nsim.down()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +        ksft_eq(len(pp_list), 1)
> +        refs = sum([pp["inflight"] for pp in pp_list if pp.get("ifindex") == nsim.ifindex])

The second filtering is unnecessary.

> +        ksft_eq(refs, 1)
> +        undetached = [pp for pp in pp_list if "detach-time" not in pp]

Maybe call this attached to be in sync with the next hunk?

> +        ksft_eq(len(undetached), 0)
> +
> +        # New pp can get created, and we'll have two
> +        nsim.up()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +        attached = [pp for pp in pp_list if "detach-time" not in pp]
> +        undetached = [pp for pp in pp_list if "detach-time" in pp]

detached.

> +        ksft_eq(len(attached), 1)
> +        ksft_eq(len(undetached), 1)
> +
> +        # Free the old page and the old pp is gone
> +        nsim.dfs_write("pp_hold", "n")
> +        # Freeing check is once a second so we may need to retry
> +        for i in range(50):
> +            pp_list = nf.page_pool_get({}, dump=True)
> +            pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +            if len(pp_list) == 1:
> +                break
> +            time.sleep(0.05)
> +        ksft_eq(len(pp_list), 1)

Yeah, I don't know if busywait / slowwait sort of a helper is practical
to write in Python. No idea how to go about it. But the bash experience
shows it would useful fairly often.

> +        # And down...
> +        nsim.down()
> +        pp_list = nf.page_pool_get({}, dump=True)
> +        pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> +        ksft_eq(len(pp_list), 0)
> +
> +        # Last, leave the page hanging for destroy, nothing to check
> +        # we're trying to exercise the orphaning path in the kernel
> +        nsim.up()
> +        nsim.dfs_write("pp_hold", "y")
> +
> +
>  def main() -> None:
>      nf = NetdevFamily()
> -    ksft_run([empty_check, lo_check], args=(nf, ))
> +    ksft_run([empty_check, lo_check, page_pool_check],
> +             args=(nf, ))
>  
>  
>  if __name__ == "__main__":

Re: [PATCH net-next 6/6] selftests: net: exercise page pool reporting via netlink

[Jakub Kicinski]

On Fri, 12 Apr 2024 10:20:47 +0200 Petr Machata wrote:
> > +    def up(self):
> > +        ip("link set dev {} up".format(self.ifname))
> > +
> > +    def down(self):
> > +        ip("link set dev {} down".format(self.ifname))  
> 
> Yeah, what I meant by integration pain with LNST the other day was
> basically this. You end up rewriting the iproute2 API in Python.
> But it is what it is.

Yes, I wasn't sure where to make it a local helper in the test or 
a method of the class. I also don't think we should wrap too much but
I was worried someone will give me feedback in the opposite direction :)

Let me make it a local helper in the test.

I should probably move to format strings, too, not sure now what made 
me use .format().

> > +        ksft_eq(len(attached), 1)
> > +        ksft_eq(len(undetached), 1)
> > +
> > +        # Free the old page and the old pp is gone
> > +        nsim.dfs_write("pp_hold", "n")
> > +        # Freeing check is once a second so we may need to retry
> > +        for i in range(50):
> > +            pp_list = nf.page_pool_get({}, dump=True)
> > +            pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]
> > +            if len(pp_list) == 1:
> > +                break
> > +            time.sleep(0.05)
> > +        ksft_eq(len(pp_list), 1)  
> 
> Yeah, I don't know if busywait / slowwait sort of a helper is practical
> to write in Python. No idea how to go about it. But the bash experience
> shows it would useful fairly often.

If I wrap:

            pp_list = nf.page_pool_get({}, dump=True)
            pp_list = [pp for pp in pp_list if pp.get("ifindex") == nsim.ifindex]

into a helper, I can probably pass the condition in as a lambda 🤔️
Let me try..