[PATCH net-next v4 0/5] netdevsim: link and forward skbs between ports

[PATCH net-next v4 0/5] netdevsim: link and forward skbs between ports

[David Wei]

This patchset adds the ability to link two netdevsim ports together and
forward skbs between them, similar to veth. The goal is to use netdevsim
for testing features e.g. zero copy Rx using io_uring.

This feature was tested locally on QEMU, and a selftest is included.

---
v3->v4:
- maintain a mutex protected list of probed nsim_devs instead of using
  nsim_bus_dev
- fixed synchronization issues by taking rtnl_lock
- track tx_dropped skbs

v2->v3:
- take lock when traversing nsim_bus_dev_list
- take device ref when getting a nsim_bus_dev
- return 0 if nsim_dev_peer_read cannot find the port
- address code formatting
- do not hard code values in selftests
- add Makefile for selftests

v1->v2:
- renamed debugfs file from "link" to "peer"
- replaced strstep() with sscanf() for consistency
- increased char[] buf sz to 22 for copying id + port from user
- added err msg w/ expected fmt when linking as a hint to user
- prevent linking port to itself
- protect peer ptr using RCU

David Wei (5):
  netdevsim: maintain a list of probed netdevsims
  netdevsim: allow two netdevsim ports to be connected
  netdevsim: forward skbs from one connected port to another
  netdevsim: add selftest for forwarding skb between connected ports
  netdevsim: add Makefile for selftests

 MAINTAINERS                                   |   1 +
 drivers/net/netdevsim/dev.c                   | 144 ++++++++++++++++--
 drivers/net/netdevsim/netdev.c                |  31 +++-
 drivers/net/netdevsim/netdevsim.h             |   3 +
 .../selftests/drivers/net/netdevsim/Makefile  |  18 +++
 .../selftests/drivers/net/netdevsim/peer.sh   | 123 +++++++++++++++
 6 files changed, 304 insertions(+), 16 deletions(-)
 create mode 100644 tools/testing/selftests/drivers/net/netdevsim/Makefile
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/peer.sh

-- 
2.39.3

[PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[David Wei]

This patch adds a linked list nsim_dev_list of probed netdevsims, added
during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
nsim_dev_list_lock protects the list.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |  1 +
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index b4d3b9cde8bd..e30a12130e07 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -35,6 +35,9 @@
 
 #include "netdevsim.h"
 
+static LIST_HEAD(nsim_dev_list);
+static DEFINE_MUTEX(nsim_dev_list_lock);
+
 static unsigned int
 nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
 {
@@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
 	if (!devlink)
 		return -ENOMEM;
+	mutex_lock(&nsim_dev_list_lock);
 	devl_lock(devlink);
 	nsim_dev = devlink_priv(devlink);
 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
@@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 	spin_lock_init(&nsim_dev->fa_cookie_lock);
 
 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
+	list_add(&nsim_dev->list, &nsim_dev_list);
 
 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
 				      sizeof(struct nsim_vf_config),
@@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
 
 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
 	devl_unlock(devlink);
+	mutex_unlock(&nsim_dev_list_lock);
 	return 0;
 
 err_hwstats_exit:
@@ -1668,8 +1674,18 @@ void nsim_drv_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);
+	struct nsim_dev *pos, *tmp;
 
+	mutex_lock(&nsim_dev_list_lock);
 	devl_lock(devlink);
+
+	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
+		if (pos == nsim_dev) {
+			list_del(&nsim_dev->list);
+			break;
+		}
+	}
+
 	nsim_dev_reload_destroy(nsim_dev);
 
 	nsim_bpf_dev_exit(nsim_dev);
@@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 	kfree(nsim_dev->vfconfigs);
 	kfree(nsim_dev->fa_cookie);
 	devl_unlock(devlink);
+	mutex_unlock(&nsim_dev_list_lock);
 	devlink_free(devlink);
 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
 }
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 028c825b86db..babb61d7790b 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -277,6 +277,7 @@ struct nsim_vf_config {
 
 struct nsim_dev {
 	struct nsim_bus_dev *nsim_bus_dev;
+	struct list_head list;
 	struct nsim_fib_data *fib_data;
 	struct nsim_trap_data *trap_data;
 	struct dentry *ddir;
-- 
2.39.3

[PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected

[David Wei]

Add a debugfs file in
/sys/kernel/debug/netdevsim/netdevsimN/ports/A/peer

Writing "M B" to this file will link port A of netdevsim N with port B
of netdevsim M. Reading this file will return the linked netdevsim id
and port, if any.

nsim_dev_list_lock and rtnl_lock are held during read/write of peer to
prevent concurrent modification of netdevsims or their ports.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/dev.c       | 127 +++++++++++++++++++++++++++---
 drivers/net/netdevsim/netdev.c    |   6 ++
 drivers/net/netdevsim/netdevsim.h |   1 +
 3 files changed, 121 insertions(+), 13 deletions(-)

diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
index e30a12130e07..e4621861c70b 100644
--- a/drivers/net/netdevsim/dev.c
+++ b/drivers/net/netdevsim/dev.c
@@ -391,6 +391,117 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
 	.owner = THIS_MODULE,
 };
 
+static struct nsim_dev *nsim_dev_find_by_id(unsigned int id)
+{
+	struct nsim_dev *dev;
+
+	list_for_each_entry(dev, &nsim_dev_list, list)
+		if (dev->nsim_bus_dev->dev.id == id)
+			return dev;
+
+	return NULL;
+}
+
+static struct nsim_dev_port *
+__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
+		       unsigned int port_index)
+{
+	struct nsim_dev_port *nsim_dev_port;
+
+	port_index = nsim_dev_port_index(type, port_index);
+	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
+		if (nsim_dev_port->port_index == port_index)
+			return nsim_dev_port;
+	return NULL;
+}
+
+static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
+				  size_t count, loff_t *ppos)
+{
+	struct nsim_dev_port *nsim_dev_port;
+	struct netdevsim *peer;
+	unsigned int id, port;
+	char buf[23];
+	ssize_t ret;
+
+	mutex_lock(&nsim_dev_list_lock);
+	rtnl_lock();
+	nsim_dev_port = file->private_data;
+	peer = rtnl_dereference(nsim_dev_port->ns->peer);
+	if (!peer)
+		goto out;
+
+	id = peer->nsim_bus_dev->dev.id;
+	port = peer->nsim_dev_port->port_index;
+	ret = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
+	ret = simple_read_from_buffer(data, count, ppos, buf, ret);
+
+out:
+	rtnl_unlock();
+	mutex_unlock(&nsim_dev_list_lock);
+
+	return ret;
+}
+
+static ssize_t nsim_dev_peer_write(struct file *file,
+				   const char __user *data,
+				   size_t count, loff_t *ppos)
+{
+	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
+	struct nsim_dev *peer_dev;
+	unsigned int id, port;
+	char buf[22];
+	ssize_t ret;
+
+	if (count >= sizeof(buf))
+		return -ENOSPC;
+
+	ret = copy_from_user(buf, data, count);
+	if (ret)
+		return -EFAULT;
+	buf[count] = '\0';
+
+	ret = sscanf(buf, "%u %u", &id, &port);
+	if (ret != 2) {
+		pr_err("Format for adding a peer is \"id port\" (uint uint)");
+		return -EINVAL;
+	}
+
+	ret = -EINVAL;
+	mutex_lock(&nsim_dev_list_lock);
+	rtnl_lock();
+	peer_dev = nsim_dev_find_by_id(id);
+	if (!peer_dev)
+		goto out;
+
+	peer_dev_port = __nsim_dev_port_lookup(peer_dev, NSIM_DEV_PORT_TYPE_PF,
+					       port);
+	if (!peer_dev_port)
+		goto out;
+
+	nsim_dev_port = file->private_data;
+	if (nsim_dev_port == peer_dev_port)
+		goto out;
+
+	rcu_assign_pointer(nsim_dev_port->ns->peer, peer_dev_port->ns);
+	rcu_assign_pointer(peer_dev_port->ns->peer, nsim_dev_port->ns);
+	ret = count;
+
+out:
+	rtnl_unlock();
+	mutex_unlock(&nsim_dev_list_lock);
+
+	return ret;
+}
+
+static const struct file_operations nsim_dev_peer_fops = {
+	.open = simple_open,
+	.read = nsim_dev_peer_read,
+	.write = nsim_dev_peer_write,
+	.llseek = generic_file_llseek,
+	.owner = THIS_MODULE,
+};
+
 static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
 				      struct nsim_dev_port *nsim_dev_port)
 {
@@ -421,6 +532,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
 	}
 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
 
+	debugfs_create_file("peer", 0600, nsim_dev_port->ddir,
+			    nsim_dev_port, &nsim_dev_peer_fops);
+
 	return 0;
 }
 
@@ -1702,19 +1816,6 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
 }
 
-static struct nsim_dev_port *
-__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
-		       unsigned int port_index)
-{
-	struct nsim_dev_port *nsim_dev_port;
-
-	port_index = nsim_dev_port_index(type, port_index);
-	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
-		if (nsim_dev_port->port_index == port_index)
-			return nsim_dev_port;
-	return NULL;
-}
-
 int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
 		      unsigned int port_index)
 {
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index aecaf5f44374..434322f6a565 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	ns->nsim_dev = nsim_dev;
 	ns->nsim_dev_port = nsim_dev_port;
 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
+	RCU_INIT_POINTER(ns->peer, NULL);
 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
 	nsim_ethtool_init(ns);
@@ -407,8 +408,13 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 void nsim_destroy(struct netdevsim *ns)
 {
 	struct net_device *dev = ns->netdev;
+	struct netdevsim *peer;
 
 	rtnl_lock();
+	peer = rtnl_dereference(ns->peer);
+	if (peer)
+		RCU_INIT_POINTER(peer->peer, NULL);
+	RCU_INIT_POINTER(ns->peer, NULL);
 	unregister_netdevice(dev);
 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
 		nsim_macsec_teardown(ns);
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index babb61d7790b..24fc3fbda791 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -125,6 +125,7 @@ struct netdevsim {
 	} udp_ports;
 
 	struct nsim_ethtool ethtool;
+	struct netdevsim __rcu *peer;
 };
 
 struct netdevsim *
-- 
2.39.3

[PATCH net-next v4 3/5] netdevsim: forward skbs from one connected port to another

[David Wei]

Forward skbs sent from one netdevsim port to its connected netdevsim
port using dev_forward_skb, in a spirit similar to veth.

Add a tx_dropped variable to struct netdevsim, tracking the number of
skbs that could not be forwarded using dev_forward_skb().

The xmit() function accessing the peer ptr is protected by an RCU read
critical section. The rcu_read_lock() is functionally redundant as since
v5.0 all softirqs are implicitly RCU read critical sections; but it is
useful for human readers.

If another CPU is concurrently in nsim_destroy(), then it will first set
the peer ptr to NULL. This does not affect any existing readers that
dereferenced a non-NULL peer. Then, in unregister_netdevice(), there is
a synchronize_rcu() before the netdev is actually unregistered and
freed. This ensures that any readers i.e. xmit() that got a non-NULL
peer will complete before the netdev is freed.

Any readers after the RCU_INIT_POINTER() but before synchronize_rcu()
will dereference NULL, making it safe.

The codepath to nsim_destroy() and nsim_create() takes both the newly
added nsim_dev_list_lock and rtnl_lock. This makes it safe with
concurrent calls to linking two netdevsims together.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/netdev.c    | 25 ++++++++++++++++++++++---
 drivers/net/netdevsim/netdevsim.h |  1 +
 2 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 434322f6a565..00ab3098eb9f 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -29,6 +29,8 @@
 static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 {
 	struct netdevsim *ns = netdev_priv(dev);
+	struct netdevsim *peer_ns;
+	int ret = NETDEV_TX_OK;
 
 	if (!nsim_ipsec_tx(ns, skb))
 		goto out;
@@ -36,12 +38,29 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	u64_stats_update_begin(&ns->syncp);
 	ns->tx_packets++;
 	ns->tx_bytes += skb->len;
+
+	rcu_read_lock();
+	peer_ns = rcu_dereference(ns->peer);
+	if (!peer_ns)
+		goto out_stats;
+
+	skb_tx_timestamp(skb);
+	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP)) {
+		ret = NET_XMIT_DROP;
+		ns->tx_dropped++;
+	}
+
+	rcu_read_unlock();
 	u64_stats_update_end(&ns->syncp);
 
+	return ret;
+
+out_stats:
+	rcu_read_unlock();
+	u64_stats_update_end(&ns->syncp);
 out:
 	dev_kfree_skb(skb);
-
-	return NETDEV_TX_OK;
+	return ret;
 }
 
 static void nsim_set_rx_mode(struct net_device *dev)
@@ -70,6 +89,7 @@ nsim_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 		start = u64_stats_fetch_begin(&ns->syncp);
 		stats->tx_bytes = ns->tx_bytes;
 		stats->tx_packets = ns->tx_packets;
+		stats->tx_dropped = ns->tx_dropped;
 	} while (u64_stats_fetch_retry(&ns->syncp, start));
 }
 
@@ -302,7 +322,6 @@ static void nsim_setup(struct net_device *dev)
 	eth_hw_addr_random(dev);
 
 	dev->tx_queue_len = 0;
-	dev->flags |= IFF_NOARP;
 	dev->flags &= ~IFF_MULTICAST;
 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
 			   IFF_NO_QUEUE;
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 24fc3fbda791..083b1ee7a1a2 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -98,6 +98,7 @@ struct netdevsim {
 
 	u64 tx_packets;
 	u64 tx_bytes;
+	u64 tx_dropped;
 	struct u64_stats_sync syncp;
 
 	struct nsim_bus_dev *nsim_bus_dev;
-- 
2.39.3

[PATCH net-next v4 4/5] netdevsim: add selftest for forwarding skb between connected ports

[David Wei]

Connect two netdevsim ports in different namespaces together, then send
packets between them using socat.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 .../selftests/drivers/net/netdevsim/peer.sh   | 123 ++++++++++++++++++
 1 file changed, 123 insertions(+)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/peer.sh

diff --git a/tools/testing/selftests/drivers/net/netdevsim/peer.sh b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
new file mode 100755
index 000000000000..942526640d59
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
@@ -0,0 +1,123 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+
+NSIM_DEV_1_ID=$((RANDOM % 1024))
+NSIM_DEV_1_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_1_ID
+NSIM_DEV_1_DFS=/sys/kernel/debug/netdevsim/netdevsim$NSIM_DEV_1_ID
+NSIM_DEV_2_ID=$((RANDOM % 1024))
+NSIM_DEV_2_SYS=/sys/bus/netdevsim/devices/netdevsim$NSIM_DEV_2_ID
+NSIM_DEV_2_DFS=/sys/kernel/debug/netdevsim/netdevsim$NSIM_DEV_2_ID
+
+NSIM_DEV_SYS_NEW=/sys/bus/netdevsim/new_device
+NSIM_DEV_SYS_DEL=/sys/bus/netdevsim/del_device
+
+socat_check()
+{
+	if [ ! -x "$(command -v socat)" ]; then
+		echo "socat command not found. Skipping test"
+		return 1
+	fi
+
+	return 0
+}
+
+setup_ns()
+{
+	set -e
+	ip netns add nssv
+	ip netns add nscl
+
+	NSIM_DEV_1_NAME=$(find $NSIM_DEV_1_SYS/net -maxdepth 1 -type d ! \
+		-path $NSIM_DEV_1_SYS/net -exec basename {} \;)
+	NSIM_DEV_2_NAME=$(find $NSIM_DEV_2_SYS/net -maxdepth 1 -type d ! \
+		-path $NSIM_DEV_2_SYS/net -exec basename {} \;)
+
+	ip link set $NSIM_DEV_1_NAME netns nssv
+	ip link set $NSIM_DEV_2_NAME netns nscl
+
+	ip netns exec nssv ip addr add '192.168.1.1/24' dev $NSIM_DEV_1_NAME
+	ip netns exec nscl ip addr add '192.168.1.2/24' dev $NSIM_DEV_2_NAME
+
+	ip netns exec nssv ip link set dev $NSIM_DEV_1_NAME up
+	ip netns exec nscl ip link set dev $NSIM_DEV_2_NAME up
+	set +e
+}
+
+cleanup_ns()
+{
+	ip netns del nscl
+	ip netns del nssv
+}
+
+###
+### Code start
+###
+
+modprobe netdevsim
+
+# linking
+
+echo $NSIM_DEV_1_ID > $NSIM_DEV_SYS_NEW
+
+echo "$NSIM_DEV_2_ID 0" > ${NSIM_DEV_1_DFS}/ports/0/peer 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent netdevsim should fail"
+	exit 1
+fi
+
+echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_NEW
+
+echo "$NSIM_DEV_2_ID 0" > ${NSIM_DEV_1_DFS}/ports/0/peer
+if [ $? -ne 0 ]; then
+	echo "linking netdevsim1 port0 with netdevsim2 port0 should succeed"
+	exit 1
+fi
+
+# argument error checking
+
+echo "$NSIM_DEV_2_ID 1" > ${NSIM_DEV_1_DFS}/ports/0/peer 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent port in a netdevsim should fail"
+	exit 1
+fi
+
+echo "$NSIM_DEV_1_ID 0" > ${NSIM_DEV_1_DFS}/ports/0/peer 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with self should fail"
+	exit 1
+fi
+
+echo "$NSIM_DEV_2_ID a" > ${NSIM_DEV_1_DFS}/ports/0/peer 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "invalid arg should fail"
+	exit 1
+fi
+
+# send/recv packets
+
+socat_check || exit 4
+
+setup_ns
+
+tmp_file=$(mktemp)
+ip netns exec nssv socat TCP-LISTEN:1234,fork $tmp_file &
+pid=$!
+
+echo "HI" | ip netns exec nscl socat STDIN TCP:192.168.1.1:1234
+
+count=$(cat $tmp_file | wc -c)
+if [[ $count -ne 3 ]]; then
+	echo "expected 3 bytes, got $count"
+	exit 1
+fi
+
+echo $NSIM_DEV_2_ID > $NSIM_DEV_SYS_DEL
+
+kill $pid
+echo $NSIM_DEV_1_ID > $NSIM_DEV_SYS_DEL
+
+cleanup_ns
+
+modprobe -r netdevsim
+
+exit 0
-- 
2.39.3

[PATCH net-next v4 5/5] netdevsim: add Makefile for selftests

[David Wei]

Add a Makefile for netdevsim selftests and add selftests path to
MAINTAINERS

Signed-off-by: David Wei <dw@davidwei.uk>
---
 MAINTAINERS                                    |  1 +
 .../selftests/drivers/net/netdevsim/Makefile   | 18 ++++++++++++++++++
 2 files changed, 19 insertions(+)
 create mode 100644 tools/testing/selftests/drivers/net/netdevsim/Makefile

diff --git a/MAINTAINERS b/MAINTAINERS
index dda78b4ce707..d086e49eac57 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -14853,6 +14853,7 @@ NETDEVSIM
 M:	Jakub Kicinski <kuba@kernel.org>
 S:	Maintained
 F:	drivers/net/netdevsim/*
+F:	tools/testing/selftests/drivers/net/netdevsim/*
 
 NETEM NETWORK EMULATOR
 M:	Stephen Hemminger <stephen@networkplumber.org>
diff --git a/tools/testing/selftests/drivers/net/netdevsim/Makefile b/tools/testing/selftests/drivers/net/netdevsim/Makefile
new file mode 100644
index 000000000000..5bace0b7fb57
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/Makefile
@@ -0,0 +1,18 @@
+# SPDX-License-Identifier: GPL-2.0+ OR MIT
+
+TEST_PROGS = devlink.sh \
+	devlink_in_netns.sh \
+	devlink_trap.sh \
+	ethtool-coalesce.sh \
+	ethtool-fec.sh \
+	ethtool-pause.sh \
+	ethtool-ring.sh \
+	fib.sh \
+	hw_stats_l3.sh \
+	nexthop.sh \
+	peer.sh \
+	psample.sh \
+	tc-mq-visibility.sh \
+	udp_tunnel_nic.sh \
+
+include ../../../lib.mk
-- 
2.39.3

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[Jiri Pirko]

Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote:
>This patch adds a linked list nsim_dev_list of probed netdevsims, added
>during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>nsim_dev_list_lock protects the list.

In the commit message, you should use imperative mood, command
the codebase what to do:
https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes


>
>Signed-off-by: David Wei <dw@davidwei.uk>
>---
> drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
> drivers/net/netdevsim/netdevsim.h |  1 +
> 2 files changed, 18 insertions(+)
>
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index b4d3b9cde8bd..e30a12130e07 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -35,6 +35,9 @@
> 
> #include "netdevsim.h"
> 
>+static LIST_HEAD(nsim_dev_list);
>+static DEFINE_MUTEX(nsim_dev_list_lock);
>+
> static unsigned int
> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
> {
>@@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
> 	if (!devlink)
> 		return -ENOMEM;
>+	mutex_lock(&nsim_dev_list_lock);

I don't follow. You claim you use this mutex to protect the list.
a) why don't you use spin-lock?
b) why don't don't you take the lock just for list manipulation?


> 	devl_lock(devlink);
> 	nsim_dev = devlink_priv(devlink);
> 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>@@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> 	spin_lock_init(&nsim_dev->fa_cookie_lock);
> 
> 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>+	list_add(&nsim_dev->list, &nsim_dev_list);
> 
> 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
> 				      sizeof(struct nsim_vf_config),
>@@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
> 
> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
> 	devl_unlock(devlink);
>+	mutex_unlock(&nsim_dev_list_lock);
> 	return 0;
> 
> err_hwstats_exit:
>@@ -1668,8 +1674,18 @@ void nsim_drv_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);
>+	struct nsim_dev *pos, *tmp;
> 
>+	mutex_lock(&nsim_dev_list_lock);
> 	devl_lock(devlink);
>+
>+	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
>+		if (pos == nsim_dev) {
>+			list_del(&nsim_dev->list);
>+			break;
>+		}
>+	}
>+
> 	nsim_dev_reload_destroy(nsim_dev);
> 
> 	nsim_bpf_dev_exit(nsim_dev);
>@@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
> 	kfree(nsim_dev->vfconfigs);
> 	kfree(nsim_dev->fa_cookie);
> 	devl_unlock(devlink);
>+	mutex_unlock(&nsim_dev_list_lock);
> 	devlink_free(devlink);
> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
> }
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index 028c825b86db..babb61d7790b 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -277,6 +277,7 @@ struct nsim_vf_config {
> 
> struct nsim_dev {
> 	struct nsim_bus_dev *nsim_bus_dev;
>+	struct list_head list;
> 	struct nsim_fib_data *fib_data;
> 	struct nsim_trap_data *trap_data;
> 	struct dentry *ddir;
>-- 
>2.39.3
>

Re: [PATCH net-next v4 3/5] netdevsim: forward skbs from one connected port to another

[Jiri Pirko]

Wed, Dec 20, 2023 at 02:47:45AM CET, dw@davidwei.uk wrote:
>Forward skbs sent from one netdevsim port to its connected netdevsim
>port using dev_forward_skb, in a spirit similar to veth.
>
>Add a tx_dropped variable to struct netdevsim, tracking the number of
>skbs that could not be forwarded using dev_forward_skb().
>
>The xmit() function accessing the peer ptr is protected by an RCU read
>critical section. The rcu_read_lock() is functionally redundant as since
>v5.0 all softirqs are implicitly RCU read critical sections; but it is
>useful for human readers.
>
>If another CPU is concurrently in nsim_destroy(), then it will first set
>the peer ptr to NULL. This does not affect any existing readers that
>dereferenced a non-NULL peer. Then, in unregister_netdevice(), there is
>a synchronize_rcu() before the netdev is actually unregistered and
>freed. This ensures that any readers i.e. xmit() that got a non-NULL
>peer will complete before the netdev is freed.
>
>Any readers after the RCU_INIT_POINTER() but before synchronize_rcu()
>will dereference NULL, making it safe.
>
>The codepath to nsim_destroy() and nsim_create() takes both the newly
>added nsim_dev_list_lock and rtnl_lock. This makes it safe with
>concurrent calls to linking two netdevsims together.
>
>Signed-off-by: David Wei <dw@davidwei.uk>
>---
> drivers/net/netdevsim/netdev.c    | 25 ++++++++++++++++++++++---
> drivers/net/netdevsim/netdevsim.h |  1 +
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index 434322f6a565..00ab3098eb9f 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -29,6 +29,8 @@
> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> 	struct netdevsim *ns = netdev_priv(dev);
>+	struct netdevsim *peer_ns;
>+	int ret = NETDEV_TX_OK;
> 
> 	if (!nsim_ipsec_tx(ns, skb))
> 		goto out;
>@@ -36,12 +38,29 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
> 	u64_stats_update_begin(&ns->syncp);
> 	ns->tx_packets++;
> 	ns->tx_bytes += skb->len;
>+
>+	rcu_read_lock();
>+	peer_ns = rcu_dereference(ns->peer);
>+	if (!peer_ns)
>+		goto out_stats;
>+
>+	skb_tx_timestamp(skb);
>+	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP)) {
>+		ret = NET_XMIT_DROP;
>+		ns->tx_dropped++;

Idk, does not look fine to me to be in u64_stats_update section while
calling dev_forward_skb()


>+	}
>+
>+	rcu_read_unlock();
> 	u64_stats_update_end(&ns->syncp);
> 
>+	return ret;
>+
>+out_stats:
>+	rcu_read_unlock();
>+	u64_stats_update_end(&ns->syncp);
> out:
> 	dev_kfree_skb(skb);
>-
>-	return NETDEV_TX_OK;
>+	return ret;
> }
> 
> static void nsim_set_rx_mode(struct net_device *dev)
>@@ -70,6 +89,7 @@ nsim_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
> 		start = u64_stats_fetch_begin(&ns->syncp);
> 		stats->tx_bytes = ns->tx_bytes;
> 		stats->tx_packets = ns->tx_packets;
>+		stats->tx_dropped = ns->tx_dropped;
> 	} while (u64_stats_fetch_retry(&ns->syncp, start));
> }
> 
>@@ -302,7 +322,6 @@ static void nsim_setup(struct net_device *dev)
> 	eth_hw_addr_random(dev);
> 
> 	dev->tx_queue_len = 0;
>-	dev->flags |= IFF_NOARP;
> 	dev->flags &= ~IFF_MULTICAST;
> 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
> 			   IFF_NO_QUEUE;
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index 24fc3fbda791..083b1ee7a1a2 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -98,6 +98,7 @@ struct netdevsim {
> 
> 	u64 tx_packets;
> 	u64 tx_bytes;
>+	u64 tx_dropped;
> 	struct u64_stats_sync syncp;
> 
> 	struct nsim_bus_dev *nsim_bus_dev;
>-- 
>2.39.3
>

Re: [PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected

[Jiri Pirko]

Wed, Dec 20, 2023 at 02:47:44AM CET, dw@davidwei.uk wrote:
>Add a debugfs file in
>/sys/kernel/debug/netdevsim/netdevsimN/ports/A/peer
>
>Writing "M B" to this file will link port A of netdevsim N with port B
>of netdevsim M. Reading this file will return the linked netdevsim id
>and port, if any.
>
>nsim_dev_list_lock and rtnl_lock are held during read/write of peer to
>prevent concurrent modification of netdevsims or their ports.
>
>Signed-off-by: David Wei <dw@davidwei.uk>
>---
> drivers/net/netdevsim/dev.c       | 127 +++++++++++++++++++++++++++---
> drivers/net/netdevsim/netdev.c    |   6 ++
> drivers/net/netdevsim/netdevsim.h |   1 +
> 3 files changed, 121 insertions(+), 13 deletions(-)
>
>diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>index e30a12130e07..e4621861c70b 100644
>--- a/drivers/net/netdevsim/dev.c
>+++ b/drivers/net/netdevsim/dev.c
>@@ -391,6 +391,117 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
> 	.owner = THIS_MODULE,
> };
> 
>+static struct nsim_dev *nsim_dev_find_by_id(unsigned int id)
>+{
>+	struct nsim_dev *dev;
>+
>+	list_for_each_entry(dev, &nsim_dev_list, list)
>+		if (dev->nsim_bus_dev->dev.id == id)
>+			return dev;
>+
>+	return NULL;
>+}
>+
>+static struct nsim_dev_port *
>+__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>+		       unsigned int port_index)
>+{
>+	struct nsim_dev_port *nsim_dev_port;
>+
>+	port_index = nsim_dev_port_index(type, port_index);
>+	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>+		if (nsim_dev_port->port_index == port_index)
>+			return nsim_dev_port;
>+	return NULL;
>+}
>+
>+static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
>+				  size_t count, loff_t *ppos)
>+{
>+	struct nsim_dev_port *nsim_dev_port;
>+	struct netdevsim *peer;
>+	unsigned int id, port;
>+	char buf[23];
>+	ssize_t ret;
>+
>+	mutex_lock(&nsim_dev_list_lock);
>+	rtnl_lock();
>+	nsim_dev_port = file->private_data;
>+	peer = rtnl_dereference(nsim_dev_port->ns->peer);
>+	if (!peer)
>+		goto out;
>+
>+	id = peer->nsim_bus_dev->dev.id;
>+	port = peer->nsim_dev_port->port_index;
>+	ret = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>+	ret = simple_read_from_buffer(data, count, ppos, buf, ret);
>+
>+out:
>+	rtnl_unlock();
>+	mutex_unlock(&nsim_dev_list_lock);
>+
>+	return ret;
>+}
>+
>+static ssize_t nsim_dev_peer_write(struct file *file,
>+				   const char __user *data,
>+				   size_t count, loff_t *ppos)
>+{
>+	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>+	struct nsim_dev *peer_dev;
>+	unsigned int id, port;
>+	char buf[22];
>+	ssize_t ret;
>+
>+	if (count >= sizeof(buf))
>+		return -ENOSPC;
>+
>+	ret = copy_from_user(buf, data, count);
>+	if (ret)
>+		return -EFAULT;
>+	buf[count] = '\0';
>+
>+	ret = sscanf(buf, "%u %u", &id, &port);
>+	if (ret != 2) {
>+		pr_err("Format for adding a peer is \"id port\" (uint uint)");
>+		return -EINVAL;
>+	}
>+
>+	ret = -EINVAL;
>+	mutex_lock(&nsim_dev_list_lock);
>+	rtnl_lock();
>+	peer_dev = nsim_dev_find_by_id(id);
>+	if (!peer_dev)
>+		goto out;

Tell the user what's wrong please.

>+
>+	peer_dev_port = __nsim_dev_port_lookup(peer_dev, NSIM_DEV_PORT_TYPE_PF,
>+					       port);
>+	if (!peer_dev_port)
>+		goto out;

Tell the user what's wrong please.


>+
>+	nsim_dev_port = file->private_data;
>+	if (nsim_dev_port == peer_dev_port)

Why fail here? IDK, success sounds better to me.


>+		goto out;
>+
>+	rcu_assign_pointer(nsim_dev_port->ns->peer, peer_dev_port->ns);
>+	rcu_assign_pointer(peer_dev_port->ns->peer, nsim_dev_port->ns);
>+	ret = count;
>+
>+out:
>+	rtnl_unlock();
>+	mutex_unlock(&nsim_dev_list_lock);
>+
>+	return ret;
>+}
>+
>+static const struct file_operations nsim_dev_peer_fops = {
>+	.open = simple_open,
>+	.read = nsim_dev_peer_read,
>+	.write = nsim_dev_peer_write,
>+	.llseek = generic_file_llseek,
>+	.owner = THIS_MODULE,
>+};
>+
> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
> 				      struct nsim_dev_port *nsim_dev_port)
> {
>@@ -421,6 +532,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
> 	}
> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
> 
>+	debugfs_create_file("peer", 0600, nsim_dev_port->ddir,
>+			    nsim_dev_port, &nsim_dev_peer_fops);
>+
> 	return 0;
> }
> 
>@@ -1702,19 +1816,6 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
> }
> 
>-static struct nsim_dev_port *
>-__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>-		       unsigned int port_index)
>-{
>-	struct nsim_dev_port *nsim_dev_port;
>-
>-	port_index = nsim_dev_port_index(type, port_index);
>-	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>-		if (nsim_dev_port->port_index == port_index)
>-			return nsim_dev_port;
>-	return NULL;
>-}
>-
> int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
> 		      unsigned int port_index)
> {
>diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>index aecaf5f44374..434322f6a565 100644
>--- a/drivers/net/netdevsim/netdev.c
>+++ b/drivers/net/netdevsim/netdev.c
>@@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
> 	ns->nsim_dev = nsim_dev;
> 	ns->nsim_dev_port = nsim_dev_port;
> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>+	RCU_INIT_POINTER(ns->peer, NULL);
> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
> 	nsim_ethtool_init(ns);
>@@ -407,8 +408,13 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
> void nsim_destroy(struct netdevsim *ns)
> {
> 	struct net_device *dev = ns->netdev;
>+	struct netdevsim *peer;
> 
> 	rtnl_lock();
>+	peer = rtnl_dereference(ns->peer);
>+	if (peer)
>+		RCU_INIT_POINTER(peer->peer, NULL);
>+	RCU_INIT_POINTER(ns->peer, NULL);
> 	unregister_netdevice(dev);
> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
> 		nsim_macsec_teardown(ns);
>diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>index babb61d7790b..24fc3fbda791 100644
>--- a/drivers/net/netdevsim/netdevsim.h
>+++ b/drivers/net/netdevsim/netdevsim.h
>@@ -125,6 +125,7 @@ struct netdevsim {
> 	} udp_ports;
> 
> 	struct nsim_ethtool ethtool;
>+	struct netdevsim __rcu *peer;
> };
> 
> struct netdevsim *
>-- 
>2.39.3
>

Re: [PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected

[kernel test robot]

Hi David,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/David-Wei/netdevsim-maintain-a-list-of-probed-netdevsims/20231220-095238
base:   net-next/main
patch link:    https://lore.kernel.org/r/20231220014747.1508581-3-dw%40davidwei.uk
patch subject: [PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected
config: arm64-allyesconfig (https://download.01.org/0day-ci/archive/20231220/202312202036.QxZ1fdee-lkp@intel.com/config)
compiler: clang version 18.0.0git (https://github.com/llvm/llvm-project 10056c821a56a19cef732129e4e0c5883ae1ee49)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20231220/202312202036.QxZ1fdee-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202312202036.QxZ1fdee-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/net/netdevsim/dev.c:431:6: warning: variable 'ret' is used uninitialized whenever 'if' condition is true [-Wsometimes-uninitialized]
     431 |         if (!peer)
         |             ^~~~~
   drivers/net/netdevsim/dev.c:443:9: note: uninitialized use occurs here
     443 |         return ret;
         |                ^~~
   drivers/net/netdevsim/dev.c:431:2: note: remove the 'if' if its condition is always false
     431 |         if (!peer)
         |         ^~~~~~~~~~
     432 |                 goto out;
         |                 ~~~~~~~~
   drivers/net/netdevsim/dev.c:425:13: note: initialize the variable 'ret' to silence this warning
     425 |         ssize_t ret;
         |                    ^
         |                     = 0
   1 warning generated.


vim +431 drivers/net/netdevsim/dev.c

   417	
   418	static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
   419					  size_t count, loff_t *ppos)
   420	{
   421		struct nsim_dev_port *nsim_dev_port;
   422		struct netdevsim *peer;
   423		unsigned int id, port;
   424		char buf[23];
   425		ssize_t ret;
   426	
   427		mutex_lock(&nsim_dev_list_lock);
   428		rtnl_lock();
   429		nsim_dev_port = file->private_data;
   430		peer = rtnl_dereference(nsim_dev_port->ns->peer);
 > 431		if (!peer)
   432			goto out;
   433	
   434		id = peer->nsim_bus_dev->dev.id;
   435		port = peer->nsim_dev_port->port_index;
   436		ret = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
   437		ret = simple_read_from_buffer(data, count, ppos, buf, ret);
   438	
   439	out:
   440		rtnl_unlock();
   441		mutex_unlock(&nsim_dev_list_lock);
   442	
   443		return ret;
   444	}
   445	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[Simon Horman]

On Tue, Dec 19, 2023 at 05:47:43PM -0800, David Wei wrote:
> This patch adds a linked list nsim_dev_list of probed netdevsims, added
> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
> nsim_dev_list_lock protects the list.
> 
> Signed-off-by: David Wei <dw@davidwei.uk>
> ---
>  drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>  drivers/net/netdevsim/netdevsim.h |  1 +
>  2 files changed, 18 insertions(+)
> 
> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
> index b4d3b9cde8bd..e30a12130e07 100644
> --- a/drivers/net/netdevsim/dev.c
> +++ b/drivers/net/netdevsim/dev.c
> @@ -35,6 +35,9 @@
>  
>  #include "netdevsim.h"
>  
> +static LIST_HEAD(nsim_dev_list);
> +static DEFINE_MUTEX(nsim_dev_list_lock);
> +
>  static unsigned int
>  nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>  {
> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>  	if (!devlink)
>  		return -ENOMEM;
> +	mutex_lock(&nsim_dev_list_lock);
>  	devl_lock(devlink);
>  	nsim_dev = devlink_priv(devlink);
>  	nsim_dev->nsim_bus_dev = nsim_bus_dev;
> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  	spin_lock_init(&nsim_dev->fa_cookie_lock);
>  
>  	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
> +	list_add(&nsim_dev->list, &nsim_dev_list);
>  
>  	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>  				      sizeof(struct nsim_vf_config),
> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>  
>  	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>  	devl_unlock(devlink);
> +	mutex_unlock(&nsim_dev_list_lock);
>  	return 0;
>  

Hi David,

I see Jiri has asked about the scope and type of this lock.
And updates to address those questions may obviate my observation.
But it is that mutex_unlock(&nsim_dev_list_lock); needs to
be added to the unwind ladder:

	...
err_devlink_unlock:
	devl_unlock(devlink);
	mutex_unlock(&nsim_dev_list_lock);
	...

...

Flagged by Smatch.

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[David Wei]

On 2023-12-20 00:57, Jiri Pirko wrote:
> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote:
>> This patch adds a linked list nsim_dev_list of probed netdevsims, added
>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>> nsim_dev_list_lock protects the list.
> 
> In the commit message, you should use imperative mood, command
> the codebase what to do:
> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes

Thanks, I didn't know about this. Will edit the commit messages.

> 
> 
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>> drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>> drivers/net/netdevsim/netdevsim.h |  1 +
>> 2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index b4d3b9cde8bd..e30a12130e07 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -35,6 +35,9 @@
>>
>> #include "netdevsim.h"
>>
>> +static LIST_HEAD(nsim_dev_list);
>> +static DEFINE_MUTEX(nsim_dev_list_lock);
>> +
>> static unsigned int
>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>> {
>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>> 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>> 	if (!devlink)
>> 		return -ENOMEM;
>> +	mutex_lock(&nsim_dev_list_lock);
> 
> I don't follow. You claim you use this mutex to protect the list.
> a) why don't you use spin-lock?

I'm using a mutex unless I know (or someone else who knows better point
out) that a spinlock is better. It is simple, there are fewer gotchas,
and I anticipate actual contention here to be near 0. The
nsim_bus_dev_list is also protected by a mutex.

Is a spinlock better here and if so why?

> b) why don't don't you take the lock just for list manipulation?

Many code paths interact here, touching drivers and netdevs. There is an
ordering of locks being taken:

1. nsim_bus_dev->dev.mutex
2. devlink->lock
3. rtnl_lock

I was careful to avoid deadlocking by acquiring locks in the same order.
But looking at it again, I can reduce the critical section by acquiring
nsim_dev_list_lock after devlink->lock, thanks.

> 
> 
>> 	devl_lock(devlink);
>> 	nsim_dev = devlink_priv(devlink);
>> 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>> 	spin_lock_init(&nsim_dev->fa_cookie_lock);
>>
>> 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>> +	list_add(&nsim_dev->list, &nsim_dev_list);
>>
>> 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>> 				      sizeof(struct nsim_vf_config),
>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>
>> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>> 	devl_unlock(devlink);
>> +	mutex_unlock(&nsim_dev_list_lock);
>> 	return 0;
>>
>> err_hwstats_exit:
>> @@ -1668,8 +1674,18 @@ void nsim_drv_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);
>> +	struct nsim_dev *pos, *tmp;
>>
>> +	mutex_lock(&nsim_dev_list_lock);
>> 	devl_lock(devlink);
>> +
>> +	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
>> +		if (pos == nsim_dev) {
>> +			list_del(&nsim_dev->list);
>> +			break;
>> +		}
>> +	}
>> +
>> 	nsim_dev_reload_destroy(nsim_dev);
>>
>> 	nsim_bpf_dev_exit(nsim_dev);
>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>> 	kfree(nsim_dev->vfconfigs);
>> 	kfree(nsim_dev->fa_cookie);
>> 	devl_unlock(devlink);
>> +	mutex_unlock(&nsim_dev_list_lock);
>> 	devlink_free(devlink);
>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>> }
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 028c825b86db..babb61d7790b 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -277,6 +277,7 @@ struct nsim_vf_config {
>>
>> struct nsim_dev {
>> 	struct nsim_bus_dev *nsim_bus_dev;
>> +	struct list_head list;
>> 	struct nsim_fib_data *fib_data;
>> 	struct nsim_trap_data *trap_data;
>> 	struct dentry *ddir;
>> -- 
>> 2.39.3
>>

Re: [PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected

[David Wei]

On 2023-12-20 01:09, Jiri Pirko wrote:
> Wed, Dec 20, 2023 at 02:47:44AM CET, dw@davidwei.uk wrote:
>> Add a debugfs file in
>> /sys/kernel/debug/netdevsim/netdevsimN/ports/A/peer
>>
>> Writing "M B" to this file will link port A of netdevsim N with port B
>> of netdevsim M. Reading this file will return the linked netdevsim id
>> and port, if any.
>>
>> nsim_dev_list_lock and rtnl_lock are held during read/write of peer to
>> prevent concurrent modification of netdevsims or their ports.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>> drivers/net/netdevsim/dev.c       | 127 +++++++++++++++++++++++++++---
>> drivers/net/netdevsim/netdev.c    |   6 ++
>> drivers/net/netdevsim/netdevsim.h |   1 +
>> 3 files changed, 121 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index e30a12130e07..e4621861c70b 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -391,6 +391,117 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
>> 	.owner = THIS_MODULE,
>> };
>>
>> +static struct nsim_dev *nsim_dev_find_by_id(unsigned int id)
>> +{
>> +	struct nsim_dev *dev;
>> +
>> +	list_for_each_entry(dev, &nsim_dev_list, list)
>> +		if (dev->nsim_bus_dev->dev.id == id)
>> +			return dev;
>> +
>> +	return NULL;
>> +}
>> +
>> +static struct nsim_dev_port *
>> +__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>> +		       unsigned int port_index)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port;
>> +
>> +	port_index = nsim_dev_port_index(type, port_index);
>> +	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>> +		if (nsim_dev_port->port_index == port_index)
>> +			return nsim_dev_port;
>> +	return NULL;
>> +}
>> +
>> +static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
>> +				  size_t count, loff_t *ppos)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port;
>> +	struct netdevsim *peer;
>> +	unsigned int id, port;
>> +	char buf[23];
>> +	ssize_t ret;
>> +
>> +	mutex_lock(&nsim_dev_list_lock);
>> +	rtnl_lock();
>> +	nsim_dev_port = file->private_data;
>> +	peer = rtnl_dereference(nsim_dev_port->ns->peer);
>> +	if (!peer)
>> +		goto out;
>> +
>> +	id = peer->nsim_bus_dev->dev.id;
>> +	port = peer->nsim_dev_port->port_index;
>> +	ret = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>> +	ret = simple_read_from_buffer(data, count, ppos, buf, ret);
>> +
>> +out:
>> +	rtnl_unlock();
>> +	mutex_unlock(&nsim_dev_list_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static ssize_t nsim_dev_peer_write(struct file *file,
>> +				   const char __user *data,
>> +				   size_t count, loff_t *ppos)
>> +{
>> +	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>> +	struct nsim_dev *peer_dev;
>> +	unsigned int id, port;
>> +	char buf[22];
>> +	ssize_t ret;
>> +
>> +	if (count >= sizeof(buf))
>> +		return -ENOSPC;
>> +
>> +	ret = copy_from_user(buf, data, count);
>> +	if (ret)
>> +		return -EFAULT;
>> +	buf[count] = '\0';
>> +
>> +	ret = sscanf(buf, "%u %u", &id, &port);
>> +	if (ret != 2) {
>> +		pr_err("Format for adding a peer is \"id port\" (uint uint)");
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = -EINVAL;
>> +	mutex_lock(&nsim_dev_list_lock);
>> +	rtnl_lock();
>> +	peer_dev = nsim_dev_find_by_id(id);
>> +	if (!peer_dev)
>> +		goto out;
> 
> Tell the user what's wrong please.

Okay.

> 
>> +
>> +	peer_dev_port = __nsim_dev_port_lookup(peer_dev, NSIM_DEV_PORT_TYPE_PF,
>> +					       port);
>> +	if (!peer_dev_port)
>> +		goto out;
> 
> Tell the user what's wrong please.

Ditto.

> 
> 
>> +
>> +	nsim_dev_port = file->private_data;
>> +	if (nsim_dev_port == peer_dev_port)
> 
> Why fail here? IDK, success sounds better to me.

I don't want to link a port to itself. In my mental model a port is e.g.
a port on a switch. It doesn't make sense to connect it to itself.

> 
> 
>> +		goto out;
>> +
>> +	rcu_assign_pointer(nsim_dev_port->ns->peer, peer_dev_port->ns);
>> +	rcu_assign_pointer(peer_dev_port->ns->peer, nsim_dev_port->ns);
>> +	ret = count;
>> +
>> +out:
>> +	rtnl_unlock();
>> +	mutex_unlock(&nsim_dev_list_lock);
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct file_operations nsim_dev_peer_fops = {
>> +	.open = simple_open,
>> +	.read = nsim_dev_peer_read,
>> +	.write = nsim_dev_peer_write,
>> +	.llseek = generic_file_llseek,
>> +	.owner = THIS_MODULE,
>> +};
>> +
>> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>> 				      struct nsim_dev_port *nsim_dev_port)
>> {
>> @@ -421,6 +532,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>> 	}
>> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
>>
>> +	debugfs_create_file("peer", 0600, nsim_dev_port->ddir,
>> +			    nsim_dev_port, &nsim_dev_peer_fops);
>> +
>> 	return 0;
>> }
>>
>> @@ -1702,19 +1816,6 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>> }
>>
>> -static struct nsim_dev_port *
>> -__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>> -		       unsigned int port_index)
>> -{
>> -	struct nsim_dev_port *nsim_dev_port;
>> -
>> -	port_index = nsim_dev_port_index(type, port_index);
>> -	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>> -		if (nsim_dev_port->port_index == port_index)
>> -			return nsim_dev_port;
>> -	return NULL;
>> -}
>> -
>> int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
>> 		      unsigned int port_index)
>> {
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index aecaf5f44374..434322f6a565 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>> 	ns->nsim_dev = nsim_dev;
>> 	ns->nsim_dev_port = nsim_dev_port;
>> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>> +	RCU_INIT_POINTER(ns->peer, NULL);
>> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
>> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
>> 	nsim_ethtool_init(ns);
>> @@ -407,8 +408,13 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>> void nsim_destroy(struct netdevsim *ns)
>> {
>> 	struct net_device *dev = ns->netdev;
>> +	struct netdevsim *peer;
>>
>> 	rtnl_lock();
>> +	peer = rtnl_dereference(ns->peer);
>> +	if (peer)
>> +		RCU_INIT_POINTER(peer->peer, NULL);
>> +	RCU_INIT_POINTER(ns->peer, NULL);
>> 	unregister_netdevice(dev);
>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
>> 		nsim_macsec_teardown(ns);
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index babb61d7790b..24fc3fbda791 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -125,6 +125,7 @@ struct netdevsim {
>> 	} udp_ports;
>>
>> 	struct nsim_ethtool ethtool;
>> +	struct netdevsim __rcu *peer;
>> };
>>
>> struct netdevsim *
>> -- 
>> 2.39.3
>>

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[David Wei]

On 2023-12-20 08:40, Simon Horman wrote:
> On Tue, Dec 19, 2023 at 05:47:43PM -0800, David Wei wrote:
>> This patch adds a linked list nsim_dev_list of probed netdevsims, added
>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>> nsim_dev_list_lock protects the list.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>>  drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>>  drivers/net/netdevsim/netdevsim.h |  1 +
>>  2 files changed, 18 insertions(+)
>>
>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>> index b4d3b9cde8bd..e30a12130e07 100644
>> --- a/drivers/net/netdevsim/dev.c
>> +++ b/drivers/net/netdevsim/dev.c
>> @@ -35,6 +35,9 @@
>>  
>>  #include "netdevsim.h"
>>  
>> +static LIST_HEAD(nsim_dev_list);
>> +static DEFINE_MUTEX(nsim_dev_list_lock);
>> +
>>  static unsigned int
>>  nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>>  {
>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>  				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>>  	if (!devlink)
>>  		return -ENOMEM;
>> +	mutex_lock(&nsim_dev_list_lock);
>>  	devl_lock(devlink);
>>  	nsim_dev = devlink_priv(devlink);
>>  	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>  	spin_lock_init(&nsim_dev->fa_cookie_lock);
>>  
>>  	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>> +	list_add(&nsim_dev->list, &nsim_dev_list);
>>  
>>  	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>>  				      sizeof(struct nsim_vf_config),
>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>  
>>  	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>>  	devl_unlock(devlink);
>> +	mutex_unlock(&nsim_dev_list_lock);
>>  	return 0;
>>  
> 
> Hi David,
> 
> I see Jiri has asked about the scope and type of this lock.
> And updates to address those questions may obviate my observation.
> But it is that mutex_unlock(&nsim_dev_list_lock); needs to
> be added to the unwind ladder:
> 
> 	...
> err_devlink_unlock:
> 	devl_unlock(devlink);
> 	mutex_unlock(&nsim_dev_list_lock);
> 	...
> 
> ...
> 
> Flagged by Smatch.

Hi Simon, thanks for flagging this and I will address it in the next
version.

Re: [PATCH net-next v4 3/5] netdevsim: forward skbs from one connected port to another

[David Wei]

On 2023-12-20 01:04, Jiri Pirko wrote:
> Wed, Dec 20, 2023 at 02:47:45AM CET, dw@davidwei.uk wrote:
>> Forward skbs sent from one netdevsim port to its connected netdevsim
>> port using dev_forward_skb, in a spirit similar to veth.
>>
>> Add a tx_dropped variable to struct netdevsim, tracking the number of
>> skbs that could not be forwarded using dev_forward_skb().
>>
>> The xmit() function accessing the peer ptr is protected by an RCU read
>> critical section. The rcu_read_lock() is functionally redundant as since
>> v5.0 all softirqs are implicitly RCU read critical sections; but it is
>> useful for human readers.
>>
>> If another CPU is concurrently in nsim_destroy(), then it will first set
>> the peer ptr to NULL. This does not affect any existing readers that
>> dereferenced a non-NULL peer. Then, in unregister_netdevice(), there is
>> a synchronize_rcu() before the netdev is actually unregistered and
>> freed. This ensures that any readers i.e. xmit() that got a non-NULL
>> peer will complete before the netdev is freed.
>>
>> Any readers after the RCU_INIT_POINTER() but before synchronize_rcu()
>> will dereference NULL, making it safe.
>>
>> The codepath to nsim_destroy() and nsim_create() takes both the newly
>> added nsim_dev_list_lock and rtnl_lock. This makes it safe with
>> concurrent calls to linking two netdevsims together.
>>
>> Signed-off-by: David Wei <dw@davidwei.uk>
>> ---
>> drivers/net/netdevsim/netdev.c    | 25 ++++++++++++++++++++++---
>> drivers/net/netdevsim/netdevsim.h |  1 +
>> 2 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>> index 434322f6a565..00ab3098eb9f 100644
>> --- a/drivers/net/netdevsim/netdev.c
>> +++ b/drivers/net/netdevsim/netdev.c
>> @@ -29,6 +29,8 @@
>> static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> {
>> 	struct netdevsim *ns = netdev_priv(dev);
>> +	struct netdevsim *peer_ns;
>> +	int ret = NETDEV_TX_OK;
>>
>> 	if (!nsim_ipsec_tx(ns, skb))
>> 		goto out;
>> @@ -36,12 +38,29 @@ static netdev_tx_t nsim_start_xmit(struct sk_buff *skb, struct net_device *dev)
>> 	u64_stats_update_begin(&ns->syncp);
>> 	ns->tx_packets++;
>> 	ns->tx_bytes += skb->len;
>> +
>> +	rcu_read_lock();
>> +	peer_ns = rcu_dereference(ns->peer);
>> +	if (!peer_ns)
>> +		goto out_stats;
>> +
>> +	skb_tx_timestamp(skb);
>> +	if (unlikely(dev_forward_skb(peer_ns->netdev, skb) == NET_RX_DROP)) {
>> +		ret = NET_XMIT_DROP;
>> +		ns->tx_dropped++;
> 
> Idk, does not look fine to me to be in u64_stats_update section while
> calling dev_forward_skb()

I see, it's a type of spinlock. I will fix this.

> 
> 
>> +	}
>> +
>> +	rcu_read_unlock();
>> 	u64_stats_update_end(&ns->syncp);
>>
>> +	return ret;
>> +
>> +out_stats:
>> +	rcu_read_unlock();
>> +	u64_stats_update_end(&ns->syncp);
>> out:
>> 	dev_kfree_skb(skb);
>> -
>> -	return NETDEV_TX_OK;
>> +	return ret;
>> }
>>
>> static void nsim_set_rx_mode(struct net_device *dev)
>> @@ -70,6 +89,7 @@ nsim_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
>> 		start = u64_stats_fetch_begin(&ns->syncp);
>> 		stats->tx_bytes = ns->tx_bytes;
>> 		stats->tx_packets = ns->tx_packets;
>> +		stats->tx_dropped = ns->tx_dropped;
>> 	} while (u64_stats_fetch_retry(&ns->syncp, start));
>> }
>>
>> @@ -302,7 +322,6 @@ static void nsim_setup(struct net_device *dev)
>> 	eth_hw_addr_random(dev);
>>
>> 	dev->tx_queue_len = 0;
>> -	dev->flags |= IFF_NOARP;
>> 	dev->flags &= ~IFF_MULTICAST;
>> 	dev->priv_flags |= IFF_LIVE_ADDR_CHANGE |
>> 			   IFF_NO_QUEUE;
>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>> index 24fc3fbda791..083b1ee7a1a2 100644
>> --- a/drivers/net/netdevsim/netdevsim.h
>> +++ b/drivers/net/netdevsim/netdevsim.h
>> @@ -98,6 +98,7 @@ struct netdevsim {
>>
>> 	u64 tx_packets;
>> 	u64 tx_bytes;
>> +	u64 tx_dropped;
>> 	struct u64_stats_sync syncp;
>>
>> 	struct nsim_bus_dev *nsim_bus_dev;
>> -- 
>> 2.39.3
>>

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[David Wei]

On 2023-12-21 16:45, David Wei wrote:
> On 2023-12-20 00:57, Jiri Pirko wrote:
>> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote:
>>> This patch adds a linked list nsim_dev_list of probed netdevsims, added
>>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>>> nsim_dev_list_lock protects the list.
>>
>> In the commit message, you should use imperative mood, command
>> the codebase what to do:
>> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes
> 
> Thanks, I didn't know about this. Will edit the commit messages.
> 
>>
>>
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>> drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>>> drivers/net/netdevsim/netdevsim.h |  1 +
>>> 2 files changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>> index b4d3b9cde8bd..e30a12130e07 100644
>>> --- a/drivers/net/netdevsim/dev.c
>>> +++ b/drivers/net/netdevsim/dev.c
>>> @@ -35,6 +35,9 @@
>>>
>>> #include "netdevsim.h"
>>>
>>> +static LIST_HEAD(nsim_dev_list);
>>> +static DEFINE_MUTEX(nsim_dev_list_lock);
>>> +
>>> static unsigned int
>>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>>> {
>>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>>> 	if (!devlink)
>>> 		return -ENOMEM;
>>> +	mutex_lock(&nsim_dev_list_lock);
>>
>> I don't follow. You claim you use this mutex to protect the list.
>> a) why don't you use spin-lock?
> 
> I'm using a mutex unless I know (or someone else who knows better point
> out) that a spinlock is better. It is simple, there are fewer gotchas,
> and I anticipate actual contention here to be near 0. The
> nsim_bus_dev_list is also protected by a mutex.
> 
> Is a spinlock better here and if so why?
> 
>> b) why don't don't you take the lock just for list manipulation?
> 
> Many code paths interact here, touching drivers and netdevs. There is an
> ordering of locks being taken:
> 
> 1. nsim_bus_dev->dev.mutex
> 2. devlink->lock
> 3. rtnl_lock
> 
> I was careful to avoid deadlocking by acquiring locks in the same order.
> But looking at it again, I can reduce the critical section by acquiring
> nsim_dev_list_lock after devlink->lock, thanks.

Looking at this again, I need to prevent concurrent nsim_dev
modifications _and_ nsim_dev_port modifications. This is because
nsim_dev_peer_write() needs to traverse both of those lists to link up
two netdevsims.

I cannot use the existing devlink->lock for this, because to take it in
nsim_dev_peer_write() I need to first safely get a nsim_dev. That's why
in the patch I take nsim_dev_list_lock early with a seemingly large
critical section.

I think the following locking scheme would work:

In nsim_drv_probe():

1. Take nsim_dev_list_lock
2. Take devlink->lock
3. Construct nsim_dev
4. Construct all nsim_dev_ports
  a. Take rtnl_lock for each netdevsim port
5. Add to nsim_dev_list
6. Release devlink->lock
7. Release nsim_dev_list_lock

Maybe 5 and 6 can be swapped, but I don't think it matters.

In nsim_drv_remove():

1. Take nsim_dev_list_lock
2. Take devlink->lock
3. Remove from nsim_dev_list
4. Destroy nsim_dev
5. Destroy all nsim_dev_ports
  a. During which, take rtnl_lock for each netdevsim
6. Release devlink->lock
7. Release nsim_dev_list_lock

Similarly, maybe 2 and 3 can be swapped.

In nsim_drv_port_add():

1. Take devlink->lock
2. Take rtnl_lock and create netdevsim
3. Add to port_list
4. Release devlink->lock

In nsim_dev_port_del():

1. Take devlink->lock
2. Remove from port_list
3. Take rtnl_lock and destroy netdevsim
4. Release devlink->lock

In nsim_dev_peer_write():

1. Take nsim_dev_list_lock
   No concurrent modifications to nsim_dev_list, get peer nsim_dev
2. Take devlink->lock
   No concurrent modifications to port_list, get peer port and check
   current port in private_data still exists
3. Do the linking
4. Release devlink->lock
5. Release nsim_dev_list_lock

In v4 I am taking rtnl_lock which may be a mistake. I don't know if
there are other code paths that can modify a netdevsim's underlying
net_device without taking devlink lock. If so, then I'd also need to
take rtnl_lock after devlink->lock.

> 
>>
>>
>>> 	devl_lock(devlink);
>>> 	nsim_dev = devlink_priv(devlink);
>>> 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 	spin_lock_init(&nsim_dev->fa_cookie_lock);
>>>
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>>> +	list_add(&nsim_dev->list, &nsim_dev_list);
>>>
>>> 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>>> 				      sizeof(struct nsim_vf_config),
>>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>>
>>> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	return 0;
>>>
>>> err_hwstats_exit:
>>> @@ -1668,8 +1674,18 @@ void nsim_drv_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);
>>> +	struct nsim_dev *pos, *tmp;
>>>
>>> +	mutex_lock(&nsim_dev_list_lock);
>>> 	devl_lock(devlink);
>>> +
>>> +	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
>>> +		if (pos == nsim_dev) {
>>> +			list_del(&nsim_dev->list);
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> 	nsim_dev_reload_destroy(nsim_dev);
>>>
>>> 	nsim_bpf_dev_exit(nsim_dev);
>>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>>> 	kfree(nsim_dev->vfconfigs);
>>> 	kfree(nsim_dev->fa_cookie);
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	devlink_free(devlink);
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>>> }
>>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>> index 028c825b86db..babb61d7790b 100644
>>> --- a/drivers/net/netdevsim/netdevsim.h
>>> +++ b/drivers/net/netdevsim/netdevsim.h
>>> @@ -277,6 +277,7 @@ struct nsim_vf_config {
>>>
>>> struct nsim_dev {
>>> 	struct nsim_bus_dev *nsim_bus_dev;
>>> +	struct list_head list;
>>> 	struct nsim_fib_data *fib_data;
>>> 	struct nsim_trap_data *trap_data;
>>> 	struct dentry *ddir;
>>> -- 
>>> 2.39.3
>>>

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[Jiri Pirko]

Fri, Dec 22, 2023 at 01:45:58AM CET, dw@davidwei.uk wrote:
>On 2023-12-20 00:57, Jiri Pirko wrote:
>> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote:
>>> This patch adds a linked list nsim_dev_list of probed netdevsims, added
>>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>>> nsim_dev_list_lock protects the list.
>> 
>> In the commit message, you should use imperative mood, command
>> the codebase what to do:
>> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes
>
>Thanks, I didn't know about this. Will edit the commit messages.
>
>> 
>> 
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>> drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>>> drivers/net/netdevsim/netdevsim.h |  1 +
>>> 2 files changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>> index b4d3b9cde8bd..e30a12130e07 100644
>>> --- a/drivers/net/netdevsim/dev.c
>>> +++ b/drivers/net/netdevsim/dev.c
>>> @@ -35,6 +35,9 @@
>>>
>>> #include "netdevsim.h"
>>>
>>> +static LIST_HEAD(nsim_dev_list);
>>> +static DEFINE_MUTEX(nsim_dev_list_lock);
>>> +
>>> static unsigned int
>>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>>> {
>>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>>> 	if (!devlink)
>>> 		return -ENOMEM;
>>> +	mutex_lock(&nsim_dev_list_lock);
>> 
>> I don't follow. You claim you use this mutex to protect the list.
>> a) why don't you use spin-lock?
>
>I'm using a mutex unless I know (or someone else who knows better point
>out) that a spinlock is better. It is simple, there are fewer gotchas,
>and I anticipate actual contention here to be near 0. The
>nsim_bus_dev_list is also protected by a mutex.
>
>Is a spinlock better here and if so why?

spinlock has lower overhead. If you don't need to sleep with the lock,
spinlock is probably better for you.


>
>> b) why don't don't you take the lock just for list manipulation?
>
>Many code paths interact here, touching drivers and netdevs. There is an
>ordering of locks being taken:
>
>1. nsim_bus_dev->dev.mutex
>2. devlink->lock
>3. rtnl_lock
>
>I was careful to avoid deadlocking by acquiring locks in the same order.
>But looking at it again, I can reduce the critical section by acquiring
>nsim_dev_list_lock after devlink->lock, thanks.
>
>> 
>> 
>>> 	devl_lock(devlink);
>>> 	nsim_dev = devlink_priv(devlink);
>>> 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 	spin_lock_init(&nsim_dev->fa_cookie_lock);
>>>
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>>> +	list_add(&nsim_dev->list, &nsim_dev_list);
>>>
>>> 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>>> 				      sizeof(struct nsim_vf_config),
>>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>>
>>> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	return 0;
>>>
>>> err_hwstats_exit:
>>> @@ -1668,8 +1674,18 @@ void nsim_drv_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);
>>> +	struct nsim_dev *pos, *tmp;
>>>
>>> +	mutex_lock(&nsim_dev_list_lock);
>>> 	devl_lock(devlink);
>>> +
>>> +	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
>>> +		if (pos == nsim_dev) {
>>> +			list_del(&nsim_dev->list);
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> 	nsim_dev_reload_destroy(nsim_dev);
>>>
>>> 	nsim_bpf_dev_exit(nsim_dev);
>>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>>> 	kfree(nsim_dev->vfconfigs);
>>> 	kfree(nsim_dev->fa_cookie);
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	devlink_free(devlink);
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>>> }
>>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>> index 028c825b86db..babb61d7790b 100644
>>> --- a/drivers/net/netdevsim/netdevsim.h
>>> +++ b/drivers/net/netdevsim/netdevsim.h
>>> @@ -277,6 +277,7 @@ struct nsim_vf_config {
>>>
>>> struct nsim_dev {
>>> 	struct nsim_bus_dev *nsim_bus_dev;
>>> +	struct list_head list;
>>> 	struct nsim_fib_data *fib_data;
>>> 	struct nsim_trap_data *trap_data;
>>> 	struct dentry *ddir;
>>> -- 
>>> 2.39.3
>>>

Re: [PATCH net-next v4 2/5] netdevsim: allow two netdevsim ports to be connected

[Jiri Pirko]

Fri, Dec 22, 2023 at 01:47:59AM CET, dw@davidwei.uk wrote:
>On 2023-12-20 01:09, Jiri Pirko wrote:
>> Wed, Dec 20, 2023 at 02:47:44AM CET, dw@davidwei.uk wrote:
>>> Add a debugfs file in
>>> /sys/kernel/debug/netdevsim/netdevsimN/ports/A/peer
>>>
>>> Writing "M B" to this file will link port A of netdevsim N with port B
>>> of netdevsim M. Reading this file will return the linked netdevsim id
>>> and port, if any.
>>>
>>> nsim_dev_list_lock and rtnl_lock are held during read/write of peer to
>>> prevent concurrent modification of netdevsims or their ports.
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>> drivers/net/netdevsim/dev.c       | 127 +++++++++++++++++++++++++++---
>>> drivers/net/netdevsim/netdev.c    |   6 ++
>>> drivers/net/netdevsim/netdevsim.h |   1 +
>>> 3 files changed, 121 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>> index e30a12130e07..e4621861c70b 100644
>>> --- a/drivers/net/netdevsim/dev.c
>>> +++ b/drivers/net/netdevsim/dev.c
>>> @@ -391,6 +391,117 @@ static const struct file_operations nsim_dev_rate_parent_fops = {
>>> 	.owner = THIS_MODULE,
>>> };
>>>
>>> +static struct nsim_dev *nsim_dev_find_by_id(unsigned int id)
>>> +{
>>> +	struct nsim_dev *dev;
>>> +
>>> +	list_for_each_entry(dev, &nsim_dev_list, list)
>>> +		if (dev->nsim_bus_dev->dev.id == id)
>>> +			return dev;
>>> +
>>> +	return NULL;
>>> +}
>>> +
>>> +static struct nsim_dev_port *
>>> +__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>>> +		       unsigned int port_index)
>>> +{
>>> +	struct nsim_dev_port *nsim_dev_port;
>>> +
>>> +	port_index = nsim_dev_port_index(type, port_index);
>>> +	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>>> +		if (nsim_dev_port->port_index == port_index)
>>> +			return nsim_dev_port;
>>> +	return NULL;
>>> +}
>>> +
>>> +static ssize_t nsim_dev_peer_read(struct file *file, char __user *data,
>>> +				  size_t count, loff_t *ppos)
>>> +{
>>> +	struct nsim_dev_port *nsim_dev_port;
>>> +	struct netdevsim *peer;
>>> +	unsigned int id, port;
>>> +	char buf[23];
>>> +	ssize_t ret;
>>> +
>>> +	mutex_lock(&nsim_dev_list_lock);
>>> +	rtnl_lock();
>>> +	nsim_dev_port = file->private_data;
>>> +	peer = rtnl_dereference(nsim_dev_port->ns->peer);
>>> +	if (!peer)
>>> +		goto out;
>>> +
>>> +	id = peer->nsim_bus_dev->dev.id;
>>> +	port = peer->nsim_dev_port->port_index;
>>> +	ret = scnprintf(buf, sizeof(buf), "%u %u\n", id, port);
>>> +	ret = simple_read_from_buffer(data, count, ppos, buf, ret);
>>> +
>>> +out:
>>> +	rtnl_unlock();
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static ssize_t nsim_dev_peer_write(struct file *file,
>>> +				   const char __user *data,
>>> +				   size_t count, loff_t *ppos)
>>> +{
>>> +	struct nsim_dev_port *nsim_dev_port, *peer_dev_port;
>>> +	struct nsim_dev *peer_dev;
>>> +	unsigned int id, port;
>>> +	char buf[22];
>>> +	ssize_t ret;
>>> +
>>> +	if (count >= sizeof(buf))
>>> +		return -ENOSPC;
>>> +
>>> +	ret = copy_from_user(buf, data, count);
>>> +	if (ret)
>>> +		return -EFAULT;
>>> +	buf[count] = '\0';
>>> +
>>> +	ret = sscanf(buf, "%u %u", &id, &port);
>>> +	if (ret != 2) {
>>> +		pr_err("Format for adding a peer is \"id port\" (uint uint)");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	ret = -EINVAL;
>>> +	mutex_lock(&nsim_dev_list_lock);
>>> +	rtnl_lock();
>>> +	peer_dev = nsim_dev_find_by_id(id);
>>> +	if (!peer_dev)
>>> +		goto out;
>> 
>> Tell the user what's wrong please.
>
>Okay.
>
>> 
>>> +
>>> +	peer_dev_port = __nsim_dev_port_lookup(peer_dev, NSIM_DEV_PORT_TYPE_PF,
>>> +					       port);
>>> +	if (!peer_dev_port)
>>> +		goto out;
>> 
>> Tell the user what's wrong please.
>
>Ditto.
>
>> 
>> 
>>> +
>>> +	nsim_dev_port = file->private_data;
>>> +	if (nsim_dev_port == peer_dev_port)
>> 
>> Why fail here? IDK, success sounds better to me.
>
>I don't want to link a port to itself. In my mental model a port is e.g.
>a port on a switch. It doesn't make sense to connect it to itself.

Okay, I misread.


>
>> 
>> 
>>> +		goto out;
>>> +
>>> +	rcu_assign_pointer(nsim_dev_port->ns->peer, peer_dev_port->ns);
>>> +	rcu_assign_pointer(peer_dev_port->ns->peer, nsim_dev_port->ns);
>>> +	ret = count;
>>> +
>>> +out:
>>> +	rtnl_unlock();
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> +
>>> +	return ret;
>>> +}
>>> +
>>> +static const struct file_operations nsim_dev_peer_fops = {
>>> +	.open = simple_open,
>>> +	.read = nsim_dev_peer_read,
>>> +	.write = nsim_dev_peer_write,
>>> +	.llseek = generic_file_llseek,
>>> +	.owner = THIS_MODULE,
>>> +};
>>> +
>>> static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>>> 				      struct nsim_dev_port *nsim_dev_port)
>>> {
>>> @@ -421,6 +532,9 @@ static int nsim_dev_port_debugfs_init(struct nsim_dev *nsim_dev,
>>> 	}
>>> 	debugfs_create_symlink("dev", nsim_dev_port->ddir, dev_link_name);
>>>
>>> +	debugfs_create_file("peer", 0600, nsim_dev_port->ddir,
>>> +			    nsim_dev_port, &nsim_dev_peer_fops);
>>> +
>>> 	return 0;
>>> }
>>>
>>> @@ -1702,19 +1816,6 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>>> }
>>>
>>> -static struct nsim_dev_port *
>>> -__nsim_dev_port_lookup(struct nsim_dev *nsim_dev, enum nsim_dev_port_type type,
>>> -		       unsigned int port_index)
>>> -{
>>> -	struct nsim_dev_port *nsim_dev_port;
>>> -
>>> -	port_index = nsim_dev_port_index(type, port_index);
>>> -	list_for_each_entry(nsim_dev_port, &nsim_dev->port_list, list)
>>> -		if (nsim_dev_port->port_index == port_index)
>>> -			return nsim_dev_port;
>>> -	return NULL;
>>> -}
>>> -
>>> int nsim_drv_port_add(struct nsim_bus_dev *nsim_bus_dev, enum nsim_dev_port_type type,
>>> 		      unsigned int port_index)
>>> {
>>> diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
>>> index aecaf5f44374..434322f6a565 100644
>>> --- a/drivers/net/netdevsim/netdev.c
>>> +++ b/drivers/net/netdevsim/netdev.c
>>> @@ -388,6 +388,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>>> 	ns->nsim_dev = nsim_dev;
>>> 	ns->nsim_dev_port = nsim_dev_port;
>>> 	ns->nsim_bus_dev = nsim_dev->nsim_bus_dev;
>>> +	RCU_INIT_POINTER(ns->peer, NULL);
>>> 	SET_NETDEV_DEV(dev, &ns->nsim_bus_dev->dev);
>>> 	SET_NETDEV_DEVLINK_PORT(dev, &nsim_dev_port->devlink_port);
>>> 	nsim_ethtool_init(ns);
>>> @@ -407,8 +408,13 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
>>> void nsim_destroy(struct netdevsim *ns)
>>> {
>>> 	struct net_device *dev = ns->netdev;
>>> +	struct netdevsim *peer;
>>>
>>> 	rtnl_lock();
>>> +	peer = rtnl_dereference(ns->peer);
>>> +	if (peer)
>>> +		RCU_INIT_POINTER(peer->peer, NULL);
>>> +	RCU_INIT_POINTER(ns->peer, NULL);
>>> 	unregister_netdevice(dev);
>>> 	if (nsim_dev_port_is_pf(ns->nsim_dev_port)) {
>>> 		nsim_macsec_teardown(ns);
>>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>> index babb61d7790b..24fc3fbda791 100644
>>> --- a/drivers/net/netdevsim/netdevsim.h
>>> +++ b/drivers/net/netdevsim/netdevsim.h
>>> @@ -125,6 +125,7 @@ struct netdevsim {
>>> 	} udp_ports;
>>>
>>> 	struct nsim_ethtool ethtool;
>>> +	struct netdevsim __rcu *peer;
>>> };
>>>
>>> struct netdevsim *
>>> -- 
>>> 2.39.3
>>>

Re: [PATCH net-next v4 1/5] netdevsim: maintain a list of probed netdevsims

[Jiri Pirko]

Fri, Dec 22, 2023 at 01:45:58AM CET, dw@davidwei.uk wrote:
>On 2023-12-20 00:57, Jiri Pirko wrote:
>> Wed, Dec 20, 2023 at 02:47:43AM CET, dw@davidwei.uk wrote:
>>> This patch adds a linked list nsim_dev_list of probed netdevsims, added
>>> during nsim_drv_probe() and removed during nsim_drv_remove(). A mutex
>>> nsim_dev_list_lock protects the list.
>> 
>> In the commit message, you should use imperative mood, command
>> the codebase what to do:
>> https://www.kernel.org/doc/html/v6.6/process/submitting-patches.html#describe-your-changes
>
>Thanks, I didn't know about this. Will edit the commit messages.
>
>> 
>> 
>>>
>>> Signed-off-by: David Wei <dw@davidwei.uk>
>>> ---
>>> drivers/net/netdevsim/dev.c       | 17 +++++++++++++++++
>>> drivers/net/netdevsim/netdevsim.h |  1 +
>>> 2 files changed, 18 insertions(+)
>>>
>>> diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c
>>> index b4d3b9cde8bd..e30a12130e07 100644
>>> --- a/drivers/net/netdevsim/dev.c
>>> +++ b/drivers/net/netdevsim/dev.c
>>> @@ -35,6 +35,9 @@
>>>
>>> #include "netdevsim.h"
>>>
>>> +static LIST_HEAD(nsim_dev_list);
>>> +static DEFINE_MUTEX(nsim_dev_list_lock);
>>> +
>>> static unsigned int
>>> nsim_dev_port_index(enum nsim_dev_port_type type, unsigned int port_index)
>>> {
>>> @@ -1531,6 +1534,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 				 nsim_bus_dev->initial_net, &nsim_bus_dev->dev);
>>> 	if (!devlink)
>>> 		return -ENOMEM;
>>> +	mutex_lock(&nsim_dev_list_lock);
>> 
>> I don't follow. You claim you use this mutex to protect the list.
>> a) why don't you use spin-lock?
>
>I'm using a mutex unless I know (or someone else who knows better point
>out) that a spinlock is better. It is simple, there are fewer gotchas,
>and I anticipate actual contention here to be near 0. The
>nsim_bus_dev_list is also protected by a mutex.
>
>Is a spinlock better here and if so why?
>
>> b) why don't don't you take the lock just for list manipulation?
>
>Many code paths interact here, touching drivers and netdevs. There is an
>ordering of locks being taken:
>
>1. nsim_bus_dev->dev.mutex
>2. devlink->lock
>3. rtnl_lock
>
>I was careful to avoid deadlocking by acquiring locks in the same order.
>But looking at it again, I can reduce the critical section by acquiring
>nsim_dev_list_lock after devlink->lock, thanks.

Again, what is the purpose of the lock? I was under impression, that you
just need to maintain consistency of the list. Or do you need it for
anything else?


>
>> 
>> 
>>> 	devl_lock(devlink);
>>> 	nsim_dev = devlink_priv(devlink);
>>> 	nsim_dev->nsim_bus_dev = nsim_bus_dev;
>>> @@ -1544,6 +1548,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>> 	spin_lock_init(&nsim_dev->fa_cookie_lock);
>>>
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, nsim_dev);
>>> +	list_add(&nsim_dev->list, &nsim_dev_list);
>>>
>>> 	nsim_dev->vfconfigs = kcalloc(nsim_bus_dev->max_vfs,
>>> 				      sizeof(struct nsim_vf_config),
>>> @@ -1607,6 +1612,7 @@ int nsim_drv_probe(struct nsim_bus_dev *nsim_bus_dev)
>>>
>>> 	nsim_dev->esw_mode = DEVLINK_ESWITCH_MODE_LEGACY;
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	return 0;
>>>
>>> err_hwstats_exit:
>>> @@ -1668,8 +1674,18 @@ void nsim_drv_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);
>>> +	struct nsim_dev *pos, *tmp;
>>>
>>> +	mutex_lock(&nsim_dev_list_lock);
>>> 	devl_lock(devlink);
>>> +
>>> +	list_for_each_entry_safe(pos, tmp, &nsim_dev_list, list) {
>>> +		if (pos == nsim_dev) {
>>> +			list_del(&nsim_dev->list);
>>> +			break;
>>> +		}
>>> +	}
>>> +
>>> 	nsim_dev_reload_destroy(nsim_dev);
>>>
>>> 	nsim_bpf_dev_exit(nsim_dev);
>>> @@ -1681,6 +1697,7 @@ void nsim_drv_remove(struct nsim_bus_dev *nsim_bus_dev)
>>> 	kfree(nsim_dev->vfconfigs);
>>> 	kfree(nsim_dev->fa_cookie);
>>> 	devl_unlock(devlink);
>>> +	mutex_unlock(&nsim_dev_list_lock);
>>> 	devlink_free(devlink);
>>> 	dev_set_drvdata(&nsim_bus_dev->dev, NULL);
>>> }
>>> diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
>>> index 028c825b86db..babb61d7790b 100644
>>> --- a/drivers/net/netdevsim/netdevsim.h
>>> +++ b/drivers/net/netdevsim/netdevsim.h
>>> @@ -277,6 +277,7 @@ struct nsim_vf_config {
>>>
>>> struct nsim_dev {
>>> 	struct nsim_bus_dev *nsim_bus_dev;
>>> +	struct list_head list;
>>> 	struct nsim_fib_data *fib_data;
>>> 	struct nsim_trap_data *trap_data;
>>> 	struct dentry *ddir;
>>> -- 
>>> 2.39.3
>>>