All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports
@ 2024-01-26  1:23 David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected David Wei
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: David Wei @ 2024-01-26  1:23 UTC (permalink / raw)
  To: Jakub Kicinski, Jiri Pirko, Sabrina Dubroca, netdev
  Cc: David S. Miller, Eric Dumazet, Paolo Abeni

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.

---
v5->v6:
- reworked to link two netdevsims using sysfs attribute on the bus
  device instead of debugfs due to deadlock possibility if a netdevsim
  is removed during linking
- removed unnecessary patch maintaining a list of probed nsim_devs
- updated selftest

v4->v5:
- reduce nsim_dev_list_lock critical section
- fixed missing mutex unlock during unwind ladder
- rework nsim_dev_peer_write synchronization to take devlink lock as
  well as rtnl_lock
- return err msgs to user during linking if port doesn't exist or
  linking to self
- update tx stats outside of RCU lock

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 (4):
  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/bus.c                   |  72 ++++++++++
 drivers/net/netdevsim/netdev.c                |  35 ++++-
 drivers/net/netdevsim/netdevsim.h             |   3 +
 .../selftests/drivers/net/netdevsim/Makefile  |  18 +++
 .../selftests/drivers/net/netdevsim/peer.sh   | 124 ++++++++++++++++++
 6 files changed, 249 insertions(+), 4 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


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected
  2024-01-26  1:23 [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports David Wei
@ 2024-01-26  1:23 ` David Wei
  2024-01-26  2:24   ` Jakub Kicinski
  2024-01-26  1:23 ` [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another David Wei
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: David Wei @ 2024-01-26  1:23 UTC (permalink / raw)
  To: Jakub Kicinski, Jiri Pirko, Sabrina Dubroca, netdev
  Cc: David S. Miller, Eric Dumazet, Paolo Abeni

Add a netdevsim bus attribute to sysfs:
/sys/bus/netdevsim/link_device

Writing "A M B N" to this file will link netdevsim M in netnsid A with
netdevsim N in netnsid B.

rtnl_lock is taken to ensure nothing changes during the linking.

Signed-off-by: David Wei <dw@davidwei.uk>
---
 drivers/net/netdevsim/bus.c       | 72 +++++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdev.c    | 11 +++++
 drivers/net/netdevsim/netdevsim.h |  2 +
 3 files changed, 85 insertions(+)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index bcbc1e19edde..be8ac2e60c69 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c
@@ -232,9 +232,81 @@ del_device_store(const struct bus_type *bus, const char *buf, size_t count)
 }
 static BUS_ATTR_WO(del_device);
 
+static ssize_t link_device_store(const struct bus_type *bus, const char *buf, size_t count)
+{
+	unsigned int netnsid_a, netnsid_b, ifidx_a, ifidx_b;
+	struct netdevsim *nsim_a, *nsim_b;
+	struct net_device *dev_a, *dev_b;
+	struct net *ns_a, *ns_b;
+	int err;
+
+	err = sscanf(buf, "%u %u %u %u", &netnsid_a, &ifidx_a, &netnsid_b, &ifidx_b);
+	if (err != 4) {
+		pr_err("Format for linking two devices is \"netnsid_a ifidx_a netnsid_b ifidx_b\" (uint uint unit uint).\n");
+		return -EINVAL;
+	}
+
+	err = -EINVAL;
+	rtnl_lock();
+	ns_a = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_a);
+	if (!ns_a) {
+		pr_err("Could not find netns with id: %d\n", netnsid_a);
+		goto out_unlock_rtnl;
+	}
+
+	dev_a = dev_get_by_index(ns_a, ifidx_a);
+	if (!dev_a) {
+		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_a, netnsid_a);
+		goto out_put_netns_a;
+	}
+
+	if (!netdev_is_nsim(dev_a)) {
+		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_a, netnsid_a);
+		goto out_put_dev_a;
+	}
+
+	ns_b = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_b);
+	if (!ns_b) {
+		pr_err("Could not find netns with id: %d\n", netnsid_b);
+		goto out_put_dev_a;
+	}
+
+	dev_b = dev_get_by_index(ns_b, ifidx_b);
+	if (!dev_b) {
+		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_b, netnsid_b);
+		goto out_put_netns_b;
+	}
+
+	if (!netdev_is_nsim(dev_b)) {
+		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_b, netnsid_b);
+		goto out_put_dev_b;
+	}
+
+	err = 0;
+	nsim_a = netdev_priv(dev_a);
+	nsim_b = netdev_priv(dev_b);
+	rcu_assign_pointer(nsim_a->peer, nsim_b);
+	rcu_assign_pointer(nsim_b->peer, nsim_a);
+
+out_put_dev_b:
+	dev_put(dev_b);
+out_put_netns_b:
+	put_net(ns_b);
+out_put_dev_a:
+	dev_put(dev_a);
+out_put_netns_a:
+	put_net(ns_a);
+out_unlock_rtnl:
+	rtnl_unlock();
+
+	return !err ? count : err;
+}
+static BUS_ATTR_WO(link_device);
+
 static struct attribute *nsim_bus_attrs[] = {
 	&bus_attr_new_device.attr,
 	&bus_attr_del_device.attr,
+	&bus_attr_link_device.attr,
 	NULL
 };
 ATTRIBUTE_GROUPS(nsim_bus);
diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 77e8250282a5..969248ffeca8 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -394,6 +394,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);
@@ -413,8 +414,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);
@@ -427,6 +433,11 @@ void nsim_destroy(struct netdevsim *ns)
 	free_netdev(dev);
 }
 
+bool netdev_is_nsim(struct net_device *dev)
+{
+	return dev->netdev_ops == &nsim_netdev_ops;
+}
+
 static int nsim_validate(struct nlattr *tb[], struct nlattr *data[],
 			 struct netlink_ext_ack *extack)
 {
diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 028c825b86db..c8b45b0d955e 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h
@@ -125,11 +125,13 @@ struct netdevsim {
 	} udp_ports;
 
 	struct nsim_ethtool ethtool;
+	struct netdevsim __rcu *peer;
 };
 
 struct netdevsim *
 nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port);
 void nsim_destroy(struct netdevsim *ns);
+bool netdev_is_nsim(struct net_device *dev);
 
 void nsim_ethtool_init(struct netdevsim *ns);
 
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another
  2024-01-26  1:23 [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected David Wei
@ 2024-01-26  1:23 ` David Wei
  2024-01-26  2:26   ` Jakub Kicinski
  2024-01-26  1:23 ` [PATCH net-next v6 3/4] netdevsim: add selftest for forwarding skb between connected ports David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 4/4] netdevsim: add Makefile for selftests David Wei
  3 siblings, 1 reply; 10+ messages in thread
From: David Wei @ 2024-01-26  1:23 UTC (permalink / raw)
  To: Jakub Kicinski, Jiri Pirko, Sabrina Dubroca, netdev
  Cc: David S. Miller, Eric Dumazet, Paolo Abeni

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 rtnl_lock, making
it safe with concurrent calls to linking two netdevsims together.

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

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 969248ffeca8..978c34334018 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c
@@ -29,19 +29,35 @@
 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;
+	unsigned int len = skb->len;
+	int ret = NETDEV_TX_OK;
 
 	if (!nsim_ipsec_tx(ns, skb))
 		goto out;
 
+	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;
+
+out_stats:
+	rcu_read_unlock();
 	u64_stats_update_begin(&ns->syncp);
 	ns->tx_packets++;
-	ns->tx_bytes += skb->len;
+	ns->tx_bytes += len;
+	if (ret == NET_XMIT_DROP)
+		ns->tx_dropped++;
 	u64_stats_update_end(&ns->syncp);
+	return ret;
 
 out:
 	dev_kfree_skb(skb);
-
-	return NETDEV_TX_OK;
+	return ret;
 }
 
 static void nsim_set_rx_mode(struct net_device *dev)
@@ -70,6 +86,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 +319,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 c8b45b0d955e..553c4b9b4f63 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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v6 3/4] netdevsim: add selftest for forwarding skb between connected ports
  2024-01-26  1:23 [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another David Wei
@ 2024-01-26  1:23 ` David Wei
  2024-01-26  1:23 ` [PATCH net-next v6 4/4] netdevsim: add Makefile for selftests David Wei
  3 siblings, 0 replies; 10+ messages in thread
From: David Wei @ 2024-01-26  1:23 UTC (permalink / raw)
  To: Jakub Kicinski, Jiri Pirko, Sabrina Dubroca, netdev
  Cc: David S. Miller, Eric Dumazet, Paolo Abeni

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   | 124 ++++++++++++++++++
 1 file changed, 124 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..4fdb43fec044
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/peer.sh
@@ -0,0 +1,124 @@
+#!/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
+NSIM_DEV_SYS_LINK=/sys/bus/netdevsim/link_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 > $NSIM_DEV_SYS_NEW
+
+setup_ns
+
+NSIM_DEV_1_NETNSID=$(ip netns list-id | grep nssv | awk '{print $2}')
+NSIM_DEV_1_IFIDX=$(ip netns exec nssv cat /sys/class/net/$NSIM_DEV_1_NAME/ifindex)
+
+NSIM_DEV_2_NETNSID=$(ip netns list-id | grep nscl | awk '{print $2}')
+NSIM_DEV_2_IFIDX=$(ip netns exec nscl cat /sys/class/net/$NSIM_DEV_2_NAME/ifindex)
+
+echo "$NSIM_DEV_1_NETNSID $NSIM_DEV_1_IFIDX $NSIM_DEV_2_NETNSID 20" > $NSIM_DEV_SYS_LINK 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent netdevsim should fail"
+	exit 1
+fi
+
+echo "$NSIM_DEV_1_NETNSID $NSIM_DEV_1_IFIDX 20 $NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "linking with non-existent netnsid should fail"
+	exit 1
+fi
+
+echo "$NSIM_DEV_1_NETNSID $NSIM_DEV_1_IFIDX $NSIM_DEV_2_NETNSID $NSIM_DEV_2_IFIDX" > $NSIM_DEV_SYS_LINK
+if [ $? -ne 0 ]; then
+	echo "linking netdevsim1 with netdevsim2 should succeed"
+	exit 1
+fi
+
+# argument error checking
+
+echo "$NSIM_DEV_1_NETNSID $NSIM_DEV_1_IFIDX $NSIM_DEV_2_NETNSID a" > $NSIM_DEV_SYS_LINK 2>/dev/null
+if [ $? -eq 0 ]; then
+	echo "invalid arg should fail"
+	exit 1
+fi
+
+# send/recv packets
+
+socat_check || exit 4
+
+tmp_file=$(mktemp)
+ip netns exec nssv socat TCP-LISTEN:1234,fork $tmp_file &
+pid=$!
+res=0
+
+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"
+	res=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 $res
-- 
2.39.3


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [PATCH net-next v6 4/4] netdevsim: add Makefile for selftests
  2024-01-26  1:23 [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports David Wei
                   ` (2 preceding siblings ...)
  2024-01-26  1:23 ` [PATCH net-next v6 3/4] netdevsim: add selftest for forwarding skb between connected ports David Wei
@ 2024-01-26  1:23 ` David Wei
  3 siblings, 0 replies; 10+ messages in thread
From: David Wei @ 2024-01-26  1:23 UTC (permalink / raw)
  To: Jakub Kicinski, Jiri Pirko, Sabrina Dubroca, netdev
  Cc: David S. Miller, Eric Dumazet, Paolo Abeni

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 8709c7cd3656..6b3608db5bda 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -15061,6 +15061,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


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected
  2024-01-26  1:23 ` [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected David Wei
@ 2024-01-26  2:24   ` Jakub Kicinski
  2024-01-26 18:54     ` David Wei
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2024-01-26  2:24 UTC (permalink / raw)
  To: David Wei
  Cc: Jiri Pirko, Sabrina Dubroca, netdev, David S. Miller,
	Eric Dumazet, Paolo Abeni

On Thu, 25 Jan 2024 17:23:54 -0800 David Wei wrote:
> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
> index bcbc1e19edde..be8ac2e60c69 100644
> --- a/drivers/net/netdevsim/bus.c
> +++ b/drivers/net/netdevsim/bus.c
> @@ -232,9 +232,81 @@ del_device_store(const struct bus_type *bus, const char *buf, size_t count)
>  }
>  static BUS_ATTR_WO(del_device);
>  
> +static ssize_t link_device_store(const struct bus_type *bus, const char *buf, size_t count)
> +{
> +	unsigned int netnsid_a, netnsid_b, ifidx_a, ifidx_b;
> +	struct netdevsim *nsim_a, *nsim_b;
> +	struct net_device *dev_a, *dev_b;
> +	struct net *ns_a, *ns_b;
> +	int err;
> +
> +	err = sscanf(buf, "%u %u %u %u", &netnsid_a, &ifidx_a, &netnsid_b, &ifidx_b);

I'd go for "%u:%u %u:%u" to make the 'grouping' of netns and ifindex
more obvious. But no strong feelings.

> +	if (err != 4) {
> +		pr_err("Format for linking two devices is \"netnsid_a ifidx_a netnsid_b ifidx_b\" (uint uint unit uint).\n");
> +		return -EINVAL;
> +	}
> +
> +	err = -EINVAL;
> +	rtnl_lock();
> +	ns_a = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_a);
> +	if (!ns_a) {
> +		pr_err("Could not find netns with id: %d\n", netnsid_a);
> +		goto out_unlock_rtnl;
> +	}
> +
> +	dev_a = dev_get_by_index(ns_a, ifidx_a);

since you're under rtnl_lock you can use __get_device_by_index(),
it doesn't increase the refcount so you won't have to worry about
releasing it.

> +	if (!dev_a) {
> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_a, netnsid_a);
> +		goto out_put_netns_a;
> +	}
> +
> +	if (!netdev_is_nsim(dev_a)) {
> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_a, netnsid_a);
> +		goto out_put_dev_a;
> +	}
> +
> +	ns_b = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_b);
> +	if (!ns_b) {
> +		pr_err("Could not find netns with id: %d\n", netnsid_b);
> +		goto out_put_dev_a;
> +	}
> +
> +	dev_b = dev_get_by_index(ns_b, ifidx_b);
> +	if (!dev_b) {
> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_b, netnsid_b);
> +		goto out_put_netns_b;
> +	}
> +
> +	if (!netdev_is_nsim(dev_b)) {
> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_b, netnsid_b);
> +		goto out_put_dev_b;
> +	}
> +
> +	err = 0;
> +	nsim_a = netdev_priv(dev_a);
> +	nsim_b = netdev_priv(dev_b);
> +	rcu_assign_pointer(nsim_a->peer, nsim_b);
> +	rcu_assign_pointer(nsim_b->peer, nsim_a);

Shouldn't we check if peer is NULL? Otherwise we can get into weird
situations where we link A<>B then B<>C and then the pointers look like
this A->B<>C. When B gets freed A's pointer won't get cleared.

> +out_put_dev_b:
> +	dev_put(dev_b);
> +out_put_netns_b:
> +	put_net(ns_b);
> +out_put_dev_a:
> +	dev_put(dev_a);
> +out_put_netns_a:
> +	put_net(ns_a);
> +out_unlock_rtnl:
> +	rtnl_unlock();
> +
> +	return !err ? count : err;
> +}
> +static BUS_ATTR_WO(link_device);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another
  2024-01-26  1:23 ` [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another David Wei
@ 2024-01-26  2:26   ` Jakub Kicinski
  2024-01-26 18:55     ` David Wei
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Kicinski @ 2024-01-26  2:26 UTC (permalink / raw)
  To: David Wei
  Cc: Jiri Pirko, Sabrina Dubroca, netdev, David S. Miller,
	Eric Dumazet, Paolo Abeni

On Thu, 25 Jan 2024 17:23:55 -0800 David Wei wrote:
>  	struct netdevsim *ns = netdev_priv(dev);
> +	struct netdevsim *peer_ns;
> +	unsigned int len = skb->len;
> +	int ret = NETDEV_TX_OK;

nit: order variables longest to shortest

>  	if (!nsim_ipsec_tx(ns, skb))
>  		goto out;
>  
> +	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;
> +
> +out_stats:
> +	rcu_read_unlock();
>  	u64_stats_update_begin(&ns->syncp);
>  	ns->tx_packets++;
> -	ns->tx_bytes += skb->len;
> +	ns->tx_bytes += len;
> +	if (ret == NET_XMIT_DROP)
> +		ns->tx_dropped++;

drops should not be counted as Tx

>  	u64_stats_update_end(&ns->syncp);
> +	return ret;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected
  2024-01-26  2:24   ` Jakub Kicinski
@ 2024-01-26 18:54     ` David Wei
  2024-01-26 19:17       ` Jakub Kicinski
  0 siblings, 1 reply; 10+ messages in thread
From: David Wei @ 2024-01-26 18:54 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jiri Pirko, Sabrina Dubroca, netdev, David S. Miller,
	Eric Dumazet, Paolo Abeni

On 2024-01-25 18:24, Jakub Kicinski wrote:
> On Thu, 25 Jan 2024 17:23:54 -0800 David Wei wrote:
>> diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
>> index bcbc1e19edde..be8ac2e60c69 100644
>> --- a/drivers/net/netdevsim/bus.c
>> +++ b/drivers/net/netdevsim/bus.c
>> @@ -232,9 +232,81 @@ del_device_store(const struct bus_type *bus, const char *buf, size_t count)
>>  }
>>  static BUS_ATTR_WO(del_device);
>>  
>> +static ssize_t link_device_store(const struct bus_type *bus, const char *buf, size_t count)
>> +{
>> +	unsigned int netnsid_a, netnsid_b, ifidx_a, ifidx_b;
>> +	struct netdevsim *nsim_a, *nsim_b;
>> +	struct net_device *dev_a, *dev_b;
>> +	struct net *ns_a, *ns_b;
>> +	int err;
>> +
>> +	err = sscanf(buf, "%u %u %u %u", &netnsid_a, &ifidx_a, &netnsid_b, &ifidx_b);
> 
> I'd go for "%u:%u %u:%u" to make the 'grouping' of netns and ifindex
> more obvious. But no strong feelings.

Also no strong feelings so I will go with your feelings.

> 
>> +	if (err != 4) {
>> +		pr_err("Format for linking two devices is \"netnsid_a ifidx_a netnsid_b ifidx_b\" (uint uint unit uint).\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	err = -EINVAL;
>> +	rtnl_lock();
>> +	ns_a = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_a);
>> +	if (!ns_a) {
>> +		pr_err("Could not find netns with id: %d\n", netnsid_a);
>> +		goto out_unlock_rtnl;
>> +	}
>> +
>> +	dev_a = dev_get_by_index(ns_a, ifidx_a);
> 
> since you're under rtnl_lock you can use __get_device_by_index(),
> it doesn't increase the refcount so you won't have to worry about
> releasing it.

Ah, I will change this. Is this true in general i.e. if I hold some big
lock then I can use versions of functions that do not modify refcounts?

> 
>> +	if (!dev_a) {
>> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_a, netnsid_a);
>> +		goto out_put_netns_a;
>> +	}
>> +
>> +	if (!netdev_is_nsim(dev_a)) {
>> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_a, netnsid_a);
>> +		goto out_put_dev_a;
>> +	}
>> +
>> +	ns_b = get_net_ns_by_id(current->nsproxy->net_ns, netnsid_b);
>> +	if (!ns_b) {
>> +		pr_err("Could not find netns with id: %d\n", netnsid_b);
>> +		goto out_put_dev_a;
>> +	}
>> +
>> +	dev_b = dev_get_by_index(ns_b, ifidx_b);
>> +	if (!dev_b) {
>> +		pr_err("Could not find device with ifindex %d in netnsid %d\n", ifidx_b, netnsid_b);
>> +		goto out_put_netns_b;
>> +	}
>> +
>> +	if (!netdev_is_nsim(dev_b)) {
>> +		pr_err("Device with ifindex %d in netnsid %d is not a netdevsim\n", ifidx_b, netnsid_b);
>> +		goto out_put_dev_b;
>> +	}
>> +
>> +	err = 0;
>> +	nsim_a = netdev_priv(dev_a);
>> +	nsim_b = netdev_priv(dev_b);
>> +	rcu_assign_pointer(nsim_a->peer, nsim_b);
>> +	rcu_assign_pointer(nsim_b->peer, nsim_a);
> 
> Shouldn't we check if peer is NULL? Otherwise we can get into weird
> situations where we link A<>B then B<>C and then the pointers look like
> this A->B<>C. When B gets freed A's pointer won't get cleared.

Yep, that's an oversight from me. Will address.

> 
>> +out_put_dev_b:
>> +	dev_put(dev_b);
>> +out_put_netns_b:
>> +	put_net(ns_b);
>> +out_put_dev_a:
>> +	dev_put(dev_a);
>> +out_put_netns_a:
>> +	put_net(ns_a);
>> +out_unlock_rtnl:
>> +	rtnl_unlock();
>> +
>> +	return !err ? count : err;
>> +}
>> +static BUS_ATTR_WO(link_device);

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another
  2024-01-26  2:26   ` Jakub Kicinski
@ 2024-01-26 18:55     ` David Wei
  0 siblings, 0 replies; 10+ messages in thread
From: David Wei @ 2024-01-26 18:55 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Jiri Pirko, Sabrina Dubroca, netdev, David S. Miller,
	Eric Dumazet, Paolo Abeni

On 2024-01-25 18:26, Jakub Kicinski wrote:
> On Thu, 25 Jan 2024 17:23:55 -0800 David Wei wrote:
>>  	struct netdevsim *ns = netdev_priv(dev);
>> +	struct netdevsim *peer_ns;
>> +	unsigned int len = skb->len;
>> +	int ret = NETDEV_TX_OK;
> 
> nit: order variables longest to shortest

Thanks, I'll be more mindful about this.

> 
>>  	if (!nsim_ipsec_tx(ns, skb))
>>  		goto out;
>>  
>> +	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;
>> +
>> +out_stats:
>> +	rcu_read_unlock();
>>  	u64_stats_update_begin(&ns->syncp);
>>  	ns->tx_packets++;
>> -	ns->tx_bytes += skb->len;
>> +	ns->tx_bytes += len;
>> +	if (ret == NET_XMIT_DROP)
>> +		ns->tx_dropped++;
> 
> drops should not be counted as Tx

Will address.

> 
>>  	u64_stats_update_end(&ns->syncp);
>> +	return ret;

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected
  2024-01-26 18:54     ` David Wei
@ 2024-01-26 19:17       ` Jakub Kicinski
  0 siblings, 0 replies; 10+ messages in thread
From: Jakub Kicinski @ 2024-01-26 19:17 UTC (permalink / raw)
  To: David Wei
  Cc: Jiri Pirko, Sabrina Dubroca, netdev, David S. Miller,
	Eric Dumazet, Paolo Abeni

On Fri, 26 Jan 2024 10:54:35 -0800 David Wei wrote:
> > since you're under rtnl_lock you can use __get_device_by_index(),
> > it doesn't increase the refcount so you won't have to worry about
> > releasing it.  
> 
> Ah, I will change this. Is this true in general i.e. if I hold some big
> lock then I can use versions of functions that do not modify refcounts?

I don't think so, generally you can ignore refcounts if you're holding
the lock protecting the table in which the object is registered while
it is alive, and you just looked it up in that table... if that makes
sense.

netdev lifetime is a bit unusual in how much the rtnl_lock protects.

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-01-26 19:17 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-01-26  1:23 [PATCH net-next v6 0/4] netdevsim: link and forward skbs between ports David Wei
2024-01-26  1:23 ` [PATCH net-next v6 1/4] netdevsim: allow two netdevsim ports to be connected David Wei
2024-01-26  2:24   ` Jakub Kicinski
2024-01-26 18:54     ` David Wei
2024-01-26 19:17       ` Jakub Kicinski
2024-01-26  1:23 ` [PATCH net-next v6 2/4] netdevsim: forward skbs from one connected port to another David Wei
2024-01-26  2:26   ` Jakub Kicinski
2024-01-26 18:55     ` David Wei
2024-01-26  1:23 ` [PATCH net-next v6 3/4] netdevsim: add selftest for forwarding skb between connected ports David Wei
2024-01-26  1:23 ` [PATCH net-next v6 4/4] netdevsim: add Makefile for selftests David Wei

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.