[ovs-dev] [PATCHv2] datapath: Return vport configuration when queried.

Justin Pettit jpettit at nicira.com
Thu Dec 23 22:06:14 UTC 2010


On Dec 23, 2010, at 1:41 AM, Jesse Gross wrote:

> On Wed, Dec 22, 2010 at 8:32 PM, Justin Pettit <jpettit at nicira.com> wrote:
>> @@ -95,7 +95,7 @@ static int set_config(struct vport *vport, const void *config)
>>        if (!strcmp(patch_vport->name, peer_name))
>>                return -EINVAL;
>> 
>> -       strcpy(patch_vport->peer_name, peer_name);
>> +       strcpy(patch_vport->devconf->peer_name, peer_name);
> 
> You can't do in-place updates of RCU protected data.  You'll have to
> allocate a new copy, make the change, and use assign_config_rcu(),
> similar to patch_set_mtu().
> 
>> @@ -150,10 +154,12 @@ static int patch_modify(struct vport *vport, struct odp_port *port)
>>        int err = set_config(vport, port->config);
>>        if (!err) {
>>                struct patch_vport *patch_vport = patch_vport_priv(vport);
>> -
>> +               const char *peer_name = patch_vport->devconf->peer_name;
> 
> In the places where you've added accesses to devconf, you need to use
> rtnl_dereference(), since all of these places hold RTNL lock.


Thanks for the feedback.  I've had more experience using traditional locking mechanisms, and I wasn't in the right mindset for read/write locks.  How does the revised patch at the bottom of this message look?

--Justin


-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-

From d37f6905c12a6e86de7efeecc1d7896cd468e339 Mon Sep 17 00:00:00 2001
From: Justin Pettit <jpettit at nicira.com>
Date: Sat, 18 Dec 2010 01:04:37 -0800
Subject: [PATCH] datapath: Return vport configuration when queried.

Additional configuration is passed down to the kernel in the "config"
array of an odp_port when a vport is created.  This information is not
returned when a vport is queried, though.  This information is useful
for debugging, since it may be used to distinguish ports based on
additional data, such as the peer in tunnels.  In a forthcoming patch, it
will be essential to distinguish between plain GRE and GRE over IPsec.

Signed-off-by: Justin Pettit <jpettit at nicira.com>
---
 datapath/datapath.c     |    1 +
 datapath/tunnel.c       |    9 ++++++
 datapath/tunnel.h       |    1 +
 datapath/vport-capwap.c |    1 +
 datapath/vport-gre.c    |    1 +
 datapath/vport-netdev.h |    1 +
 datapath/vport-patch.c  |   71 +++++++++++++++++++++++++++++++++++-----------
 datapath/vport.c        |   16 ++++++++++
 datapath/vport.h        |    3 ++
 9 files changed, 87 insertions(+), 17 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index b95c8f2..f919887 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -1263,6 +1263,7 @@ static int put_port(const struct vport *p, struct odp_port __user *uop)
 	rcu_read_lock();
 	strncpy(op.devname, vport_get_name(p), sizeof op.devname);
 	strncpy(op.type, vport_get_type(p), sizeof op.type);
+	vport_get_config(p, op.config);
 	rcu_read_unlock();
 
 	op.port = p->port_no;
diff --git a/datapath/tunnel.c b/datapath/tunnel.c
index eac3fa3..f741e20 100644
--- a/datapath/tunnel.c
+++ b/datapath/tunnel.c
@@ -1524,6 +1524,15 @@ const unsigned char *tnl_get_addr(const struct vport *vport)
 	return rcu_dereference_rtnl(tnl_vport->mutable)->eth_addr;
 }
 
+void tnl_get_config(const struct vport *vport, void *config)
+{
+	const struct tnl_vport *tnl_vport = tnl_vport_priv(vport);
+	struct tnl_port_config *port_config;
+	
+	port_config = &rcu_dereference_rtnl(tnl_vport->mutable)->port_config;
+	memcpy(config, port_config, sizeof(*port_config));
+}
+
 int tnl_get_mtu(const struct vport *vport)
 {
 	const struct tnl_vport *tnl_vport = tnl_vport_priv(vport);
diff --git a/datapath/tunnel.h b/datapath/tunnel.h
index aa859f6..29f5922 100644
--- a/datapath/tunnel.h
+++ b/datapath/tunnel.h
@@ -192,6 +192,7 @@ int tnl_set_mtu(struct vport *vport, int mtu);
 int tnl_set_addr(struct vport *vport, const unsigned char *addr);
 const char *tnl_get_name(const struct vport *vport);
 const unsigned char *tnl_get_addr(const struct vport *vport);
+void tnl_get_config(const struct vport *vport, void *config);
 int tnl_get_mtu(const struct vport *vport);
 int tnl_send(struct vport *vport, struct sk_buff *skb);
 void tnl_rcv(struct vport *vport, struct sk_buff *skb);
diff --git a/datapath/vport-capwap.c b/datapath/vport-capwap.c
index e17d85f..fc2ea8e 100644
--- a/datapath/vport-capwap.c
+++ b/datapath/vport-capwap.c
@@ -656,6 +656,7 @@ const struct vport_ops capwap_vport_ops = {
 	.set_addr	= tnl_set_addr,
 	.get_name	= tnl_get_name,
 	.get_addr	= tnl_get_addr,
+	.get_config	= tnl_get_config,
 	.get_dev_flags	= vport_gen_get_dev_flags,
 	.is_running	= vport_gen_is_running,
 	.get_operstate	= vport_gen_get_operstate,
diff --git a/datapath/vport-gre.c b/datapath/vport-gre.c
index 191fd06..cddfa59 100644
--- a/datapath/vport-gre.c
+++ b/datapath/vport-gre.c
@@ -398,6 +398,7 @@ const struct vport_ops gre_vport_ops = {
 	.set_addr	= tnl_set_addr,
 	.get_name	= tnl_get_name,
 	.get_addr	= tnl_get_addr,
+	.get_config = tnl_get_config,
 	.get_dev_flags	= vport_gen_get_dev_flags,
 	.is_running	= vport_gen_is_running,
 	.get_operstate	= vport_gen_get_operstate,
diff --git a/datapath/vport-netdev.h b/datapath/vport-netdev.h
index 542ddfd..88a5f90 100644
--- a/datapath/vport-netdev.h
+++ b/datapath/vport-netdev.h
@@ -29,6 +29,7 @@ int netdev_set_mtu(struct vport *, int mtu);
 int netdev_set_addr(struct vport *, const unsigned char *addr);
 const char *netdev_get_name(const struct vport *);
 const unsigned char *netdev_get_addr(const struct vport *);
+const char *netdev_get_config(const struct vport *);
 struct kobject *netdev_get_kobj(const struct vport *);
 int netdev_get_stats(const struct vport *, struct rtnl_link_stats64 *);
 unsigned netdev_get_dev_flags(const struct vport *);
diff --git a/datapath/vport-patch.c b/datapath/vport-patch.c
index 4fdbcf5..b03c3aa 100644
--- a/datapath/vport-patch.c
+++ b/datapath/vport-patch.c
@@ -18,6 +18,7 @@
 struct device_config {
 	struct rcu_head rcu;
 
+	char peer_name[IFNAMSIZ];
 	unsigned char eth_addr[ETH_ALEN];
 	unsigned int mtu;
 };
@@ -28,7 +29,6 @@ struct patch_vport {
 	char name[IFNAMSIZ];
 
 	/* Protected by RTNL lock. */
-	char peer_name[IFNAMSIZ];
 	struct hlist_node hash_node;
 
 	struct vport __rcu *peer;
@@ -85,7 +85,8 @@ static void patch_exit(void)
 	kfree(peer_table);
 }
 
-static int set_config(struct vport *vport, const void *config)
+static int set_config(struct vport *vport, const void *config, 
+			struct device_config *devconf)
 {
 	struct patch_vport *patch_vport = patch_vport_priv(vport);
 	char peer_name[IFNAMSIZ];
@@ -95,7 +96,7 @@ static int set_config(struct vport *vport, const void *config)
 	if (!strcmp(patch_vport->name, peer_name))
 		return -EINVAL;
 
-	strcpy(patch_vport->peer_name, peer_name);
+	strcpy(devconf->peer_name, peer_name);
 
 	return 0;
 }
@@ -104,6 +105,7 @@ static struct vport *patch_create(const struct vport_parms *parms)
 {
 	struct vport *vport;
 	struct patch_vport *patch_vport;
+	const char *peer_name;
 	int err;
 
 	vport = vport_alloc(sizeof(struct patch_vport), &patch_vport_ops, parms);
@@ -116,29 +118,32 @@ static struct vport *patch_create(const struct vport_parms *parms)
 
 	strcpy(patch_vport->name, parms->name);
 
-	err = set_config(vport, parms->config);
-	if (err)
-		goto error_free_vport;
-
 	patch_vport->devconf = kmalloc(sizeof(struct device_config), GFP_KERNEL);
 	if (!patch_vport->devconf) {
 		err = -ENOMEM;
 		goto error_free_vport;
 	}
 
+	err = set_config(vport, parms->config, patch_vport->devconf);
+	if (err)
+		goto error_free_devconf;
+
 	vport_gen_rand_ether_addr(patch_vport->devconf->eth_addr);
 
 	/* Make the default MTU fairly large so that it doesn't become the
 	 * bottleneck on systems using jumbo frames. */
 	patch_vport->devconf->mtu = 65535;
 
-	hlist_add_head(&patch_vport->hash_node, hash_bucket(patch_vport->peer_name));
+	peer_name = patch_vport->devconf->peer_name;
+	hlist_add_head(&patch_vport->hash_node, hash_bucket(peer_name));
 
-	rcu_assign_pointer(patch_vport->peer, vport_locate(patch_vport->peer_name));
+	rcu_assign_pointer(patch_vport->peer, vport_locate(peer_name));
 	update_peers(patch_vport->name, vport);
 
 	return vport;
 
+error_free_devconf:
+	kfree(patch_vport->devconf);
 error_free_vport:
 	vport_free(vport);
 error:
@@ -147,14 +152,32 @@ error:
 
 static int patch_modify(struct vport *vport, struct odp_port *port)
 {
-	int err = set_config(vport, port->config);
-	if (!err) {
-		struct patch_vport *patch_vport = patch_vport_priv(vport);
+	struct patch_vport *patch_vport = patch_vport_priv(vport);
+	struct device_config *devconf;
+	int err;
 
-		hlist_del(&patch_vport->hash_node);
-		rcu_assign_pointer(patch_vport->peer, vport_locate(patch_vport->peer_name));
-		hlist_add_head(&patch_vport->hash_node, hash_bucket(patch_vport->peer_name));
+	devconf = kmemdup(patch_vport->devconf, sizeof(struct device_config), GFP_KERNEL);
+	if (!devconf) {
+		err = -ENOMEM;
+		goto error;
 	}
+
+	err = set_config(vport, port->config, devconf);
+	if (err)
+		goto error_free;
+
+	assign_config_rcu(vport, devconf);
+	
+	hlist_del(&patch_vport->hash_node);
+
+	rcu_assign_pointer(patch_vport->peer, vport_locate(devconf->peer_name));
+	hlist_add_head(&patch_vport->hash_node, hash_bucket(devconf->peer_name));
+
+	return 0;
+
+error_free:
+	kfree(devconf);
+error:
 	return err;
 }
 
@@ -184,9 +207,13 @@ static void update_peers(const char *name, struct vport *vport)
 	struct patch_vport *peer_vport;
 	struct hlist_node *node;
 
-	hlist_for_each_entry(peer_vport, node, bucket, hash_node)
-		if (!strcmp(peer_vport->peer_name, name))
+	hlist_for_each_entry(peer_vport, node, bucket, hash_node) {
+		const char *peer_name;
+
+		peer_name = rtnl_dereference(peer_vport->devconf)->peer_name;
+		if (!strcmp(peer_name, name))
 			rcu_assign_pointer(peer_vport->peer, vport);
+	}
 }
 
 static int patch_set_mtu(struct vport *vport, int mtu)
@@ -232,6 +259,15 @@ static const unsigned char *patch_get_addr(const struct vport *vport)
 	return rcu_dereference_rtnl(patch_vport->devconf)->eth_addr;
 }
 
+static void patch_get_config(const struct vport *vport, void *config)
+{
+	const struct patch_vport *patch_vport = patch_vport_priv(vport);
+	const char *peer_name;
+
+	peer_name = rcu_dereference_rtnl(patch_vport->devconf)->peer_name;
+	strlcpy(config, peer_name, VPORT_CONFIG_SIZE);
+}
+
 static int patch_get_mtu(const struct vport *vport)
 {
 	const struct patch_vport *patch_vport = patch_vport_priv(vport);
@@ -267,6 +303,7 @@ const struct vport_ops patch_vport_ops = {
 	.set_addr	= patch_set_addr,
 	.get_name	= patch_get_name,
 	.get_addr	= patch_get_addr,
+	.get_config	= patch_get_config,
 	.get_dev_flags	= vport_gen_get_dev_flags,
 	.is_running	= vport_gen_is_running,
 	.get_operstate	= vport_gen_get_operstate,
diff --git a/datapath/vport.c b/datapath/vport.c
index e699f8c..9e847e5 100644
--- a/datapath/vport.c
+++ b/datapath/vport.c
@@ -978,6 +978,22 @@ int vport_get_mtu(const struct vport *vport)
 }
 
 /**
+ *	vport_get_config - retrieve device configuration
+ *
+ * @vport: vport from which to retrieve the configuration.
+ * @config: buffer to store config, which must be at least the length
+ *          of VPORT_CONFIG_SIZE.
+ *
+ * Retrieves the configuration of the given device.  Either RTNL lock or
+ * rcu_read_lock must be held.
+ */
+void vport_get_config(const struct vport *vport, void *config)
+{
+	if (vport->ops->get_config)
+		vport->ops->get_config(vport, config);
+}
+
+/**
  *	vport_receive - pass up received packet to the datapath for processing
  *
  * @vport: vport that received the packet
diff --git a/datapath/vport.h b/datapath/vport.h
index fb3a3e3..6ba7f2f 100644
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -63,6 +63,7 @@ int vport_get_ifindex(const struct vport *);
 int vport_get_iflink(const struct vport *);
 
 int vport_get_mtu(const struct vport *);
+void vport_get_config(const struct vport *, void *);
 
 int vport_send(struct vport *, struct sk_buff *);
 
@@ -169,6 +170,7 @@ struct vport_parms {
  * May be null if not supported.
  * @get_name: Get the device's name.
  * @get_addr: Get the device's MAC address.
+ * @get_config: Get the device's configuration.
  * @get_kobj: Get the kobj associated with the device (may return null).
  * @get_stats: Fill in the transmit/receive stats.  May be null if stats are
  * not supported or if generic stats are in use.  If defined and
@@ -205,6 +207,7 @@ struct vport_ops {
 	/* Called with rcu_read_lock or RTNL lock. */
 	const char *(*get_name)(const struct vport *);
 	const unsigned char *(*get_addr)(const struct vport *);
+	void (*get_config)(const struct vport *, void *);
 	struct kobject *(*get_kobj)(const struct vport *);
 	int (*get_stats)(const struct vport *, struct rtnl_link_stats64 *);
 
-- 
1.7.1







More information about the dev mailing list