[PATCH net-next 0/2] netdevsim: fix tests and netdevsim

[PATCH net-next 0/2] netdevsim: fix tests and netdevsim

[Jakub Kicinski]

Hi!

The first patch fixes a merge which brought back some dead
code. Next a tiny re-write of the main test using netdevsim
aims to ease debugging.

Jakub Kicinski (2):
  netdevsim: drop code duplicated by a merge
  selftests: bpf: log direct file writes

 drivers/net/netdevsim/dev.c                 | 47 ++++-----------------
 tools/testing/selftests/bpf/test_offload.py | 20 ++++++---
 2 files changed, 22 insertions(+), 45 deletions(-)

-- 
2.23.0

[PATCH net-next 1/2] netdevsim: drop code duplicated by a merge

[Jakub Kicinski]

Looks like the port adding loop makes a re-appearance on net-next
after net was merged back into it (even though it doesn't feature
in the merge diff).

The ports are already added in nsim_dev_create() so when we try
to add them again get EEXIST, and see:

netdevsim: probe of netdevsim0 failed with error -17

in the logs. When we remove the loop again the nsim_dev_probe()
and nsim_dev_remove() become a wrapper of nsim_dev_create() and
nsim_dev_destroy(). Remove this layer of indirection.

Fixes: d31e95585ca6 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 drivers/net/netdevsim/dev.c | 47 +++++++------------------------------
 1 file changed, 8 insertions(+), 39 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e59a8826f36d..3da96c7e8265 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -753,7 +753,7 @@ static int nsim_dev_reload_create(struct nsim_dev *nsim_dev,
 	return err;
 }
 
-static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
+int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
 {
 	struct nsim_dev *nsim_dev;
 	struct devlink *devlink;
@@ -761,7 +761,7 @@ static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 
 	devlink = devlink_alloc(&nsim_dev_devlink_ops, sizeof(*nsim_dev));
 	if (!devlink)
-		return ERR_PTR(-ENOMEM);
+		return -ENOMEM;
 	devlink_net_set(devlink, nsim_bus_dev->initial_net);
 	nsim_dev = devlink_priv(devlink);
 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
@@ -773,6 +773,8 @@ static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 	nsim_dev->max_macs = NSIM_DEV_MAX_MACS_DEFAULT;
 	nsim_dev->test1 = NSIM_DEV_TEST1_DEFAULT;
 
+	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
+
 	err = nsim_dev_resources_register(devlink);
 	if (err)
 		goto err_devlink_free;
@@ -818,7 +820,7 @@ static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 		goto err_bpf_dev_exit;
 
 	devlink_params_publish(devlink);
-	return nsim_dev;
+	return 0;
 
 err_bpf_dev_exit:
 	nsim_bpf_dev_exit(nsim_dev);
@@ -841,7 +843,7 @@ static struct nsim_dev *nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev)
 	devlink_resources_unregister(devlink, NULL);
 err_devlink_free:
 	devlink_free(devlink);
-	return ERR_PTR(err);
+	return err;
 }
 
 static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
@@ -858,8 +860,9 @@ static void nsim_dev_reload_destroy(struct nsim_dev *nsim_dev)
 	nsim_fib_destroy(devlink, nsim_dev->fib_data);
 }
 
-static void nsim_dev_destroy(struct nsim_dev *nsim_dev)
+void nsim_dev_remove(struct nsim_bus_dev *nsim_bus_dev)
 {
+	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
 	struct devlink *devlink = priv_to_devlink(nsim_dev);
 
 	nsim_dev_reload_destroy(nsim_dev);
@@ -873,40 +876,6 @@ static void nsim_dev_destroy(struct nsim_dev *nsim_dev)
 	devlink_free(devlink);
 }
 
-int nsim_dev_probe(struct nsim_bus_dev *nsim_bus_dev)
-{
-	struct nsim_dev *nsim_dev;
-	int i;
-	int err;
-
-	nsim_dev = nsim_dev_create(nsim_bus_dev);
-	if (IS_ERR(nsim_dev))
-		return PTR_ERR(nsim_dev);
-	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
-
-	mutex_lock(&nsim_dev->port_list_lock);
-	for (i = 0; i < nsim_bus_dev->port_count; i++) {
-		err = __nsim_dev_port_add(nsim_dev, i);
-		if (err)
-			goto err_port_del_all;
-	}
-	mutex_unlock(&nsim_dev->port_list_lock);
-	return 0;
-
-err_port_del_all:
-	mutex_unlock(&nsim_dev->port_list_lock);
-	nsim_dev_port_del_all(nsim_dev);
-	nsim_dev_destroy(nsim_dev);
-	return err;
-}
-
-void nsim_dev_remove(struct nsim_bus_dev *nsim_bus_dev)
-{
-	struct nsim_dev *nsim_dev = dev_get_drvdata(&nsim_bus_dev->dev);
-
-	nsim_dev_destroy(nsim_dev);
-}
-
 static struct nsim_dev_port *
 __nsim_dev_port_lookup(struct nsim_dev *nsim_dev, unsigned int port_index)
 {
-- 
2.23.0

[PATCH net-next 2/2] selftests: bpf: log direct file writes

[Jakub Kicinski]

Recent changes to netdevsim moved creating and destroying
devices from netlink to sysfs. The sysfs writes have been
implemented as direct writes, without shelling out. This
is faster, but leaves no trace in the logs. Add explicit
logs to make debugging possible.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 tools/testing/selftests/bpf/test_offload.py | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/bpf/test_offload.py b/tools/testing/selftests/bpf/test_offload.py
index 1afa22c88e42..8294ae3ffb3c 100755
--- a/tools/testing/selftests/bpf/test_offload.py
+++ b/tools/testing/selftests/bpf/test_offload.py
@@ -335,13 +335,22 @@ def bpftool_prog_load(sample, file_name, maps=[], prog_type="xdp", dev=None,
     """
     Class for netdevsim bus device and its attributes.
     """
+    @staticmethod
+    def ctrl_write(path, val):
+        fullpath = os.path.join("/sys/bus/netdevsim/", path)
+        try:
+            with open(fullpath, "w") as f:
+                f.write(val)
+        except OSError as e:
+            log("WRITE %s: %r" % (fullpath, val), -e.errno)
+            raise e
+        log("WRITE %s: %r" % (fullpath, val), 0)
 
     def __init__(self, port_count=1):
         addr = 0
         while True:
             try:
-                with open("/sys/bus/netdevsim/new_device", "w") as f:
-                    f.write("%u %u" % (addr, port_count))
+                self.ctrl_write("new_device", "%u %u" % (addr, port_count))
             except OSError as e:
                 if e.errno == errno.ENOSPC:
                     addr += 1
@@ -403,14 +412,13 @@ def bpftool_prog_load(sample, file_name, maps=[], prog_type="xdp", dev=None,
         return progs
 
     def remove(self):
-        with open("/sys/bus/netdevsim/del_device", "w") as f:
-            f.write("%u" % self.addr)
+        self.ctrl_write("del_device", "%u" % (self.addr, ))
         devs.remove(self)
 
     def remove_nsim(self, nsim):
         self.nsims.remove(nsim)
-        with open("/sys/bus/netdevsim/devices/netdevsim%u/del_port" % self.addr ,"w") as f:
-            f.write("%u" % nsim.port_index)
+        self.ctrl_write("devices/netdevsim%u/del_port" % (self.addr, ),
+                        "%u" % (nsim.port_index, ))
 
 class NetdevSim:
     """
-- 
2.23.0

Re: [PATCH net-next 1/2] netdevsim: drop code duplicated by a merge

[Jiri Pirko]

Tue, Nov 05, 2019 at 10:26:11PM CET, jakub.kicinski@netronome.com wrote:
>Looks like the port adding loop makes a re-appearance on net-next
>after net was merged back into it (even though it doesn't feature
>in the merge diff).
>
>The ports are already added in nsim_dev_create() so when we try
>to add them again get EEXIST, and see:
>
>netdevsim: probe of netdevsim0 failed with error -17
>
>in the logs. When we remove the loop again the nsim_dev_probe()
>and nsim_dev_remove() become a wrapper of nsim_dev_create() and
>nsim_dev_destroy(). Remove this layer of indirection.
>
>Fixes: d31e95585ca6 ("Merge git://git.kernel.org/pub/scm/linux/kernel/git/netdev/net")
>Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

lgtm
Acked-by: Jiri Pirko <jiri@mellanox.com>

Re: [PATCH net-next 2/2] selftests: bpf: log direct file writes

[Daniel Borkmann]

On 11/5/19 10:26 PM, Jakub Kicinski wrote:
> Recent changes to netdevsim moved creating and destroying
> devices from netlink to sysfs. The sysfs writes have been
> implemented as direct writes, without shelling out. This
> is faster, but leaves no trace in the logs. Add explicit
> logs to make debugging possible.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>

Assuming this goes directly to net-next, so:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Thanks!

Re: [PATCH net-next 0/2] netdevsim: fix tests and netdevsim

[David Miller]

From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Tue,  5 Nov 2019 13:26:10 -0800

> The first patch fixes a merge which brought back some dead
> code. Next a tiny re-write of the main test using netdevsim
> aims to ease debugging.

Series applied, thanks Jakub.