[ovs-dev] [netlink v3 5/5] datapath: Merge vport "attach" into "create" and "detach" into "destroy".

Ben Pfaff blp at nicira.com
Wed Nov 17 01:11:26 UTC 2010


These steps are sequentially in lockstep, so we might as well combine them.

This expands the region over which the vport_lock is held.  I didn't
carefully verify that this was OK.

This also eliminates the synchronize_rcu() call from destruction of tunnel
vports, since they didn't appear to me to need it.

It should be possible to eliminate the synchronize_rcu() from the netdev,
patch, and internal_dev vports, but this commit does not do that.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 datapath/datapath.c           |   19 +---------------
 datapath/vport-internal_dev.c |   33 ++++++++---------------------
 datapath/vport-netdev.c       |   43 ++++++++++++-------------------------
 datapath/vport-patch.c        |   40 ++++++++++++-----------------------
 datapath/vport.c              |   46 ++++------------------------------------
 datapath/vport.h              |   13 +----------
 6 files changed, 45 insertions(+), 149 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index a3e7872..6c8177c 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -339,7 +339,6 @@ static int new_vport(struct datapath *dp, struct odp_port *odp_port, int port_no
 {
 	struct vport_parms parms;
 	struct vport *vport;
-	int err;
 
 	parms.name = odp_port->devname;
 	parms.type = odp_port->type;
@@ -354,12 +353,6 @@ static int new_vport(struct datapath *dp, struct odp_port *odp_port, int port_no
 	if (IS_ERR(vport))
 		return PTR_ERR(vport);
 
-	err = vport_attach(vport);
-	if (err) {
-		vport_del(vport);
-		return err;
-	}
-
 	rcu_assign_pointer(dp->ports[port_no], vport);
 	list_add_rcu(&vport->node, &dp->port_list);
 	dp->n_ports++;
@@ -414,8 +407,6 @@ out:
 
 int dp_detach_port(struct vport *p)
 {
-	int err;
-
 	ASSERT_RTNL();
 
 	if (p->port_no != ODPP_LOCAL)
@@ -427,15 +418,9 @@ int dp_detach_port(struct vport *p)
 	list_del_rcu(&p->node);
 	rcu_assign_pointer(p->dp->ports[p->port_no], NULL);
 
-	err = vport_detach(p);
-	if (err)
-		return err;
-
-	/* Then wait until no one is still using it, and destroy it. */
-	synchronize_rcu();
-
+	/* Then destroy it. */
 	vport_lock();
-	vport_del(p);
+	vport_del(p);		/* XXX handle error */
 	vport_unlock();
 
 	return 0;
diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
index f6348be..09bd55b 100644
--- a/datapath/vport-internal_dev.c
+++ b/datapath/vport-internal_dev.c
@@ -214,6 +214,10 @@ static struct vport *internal_dev_create(const struct vport_parms *parms)
 	if (err)
 		goto error_free_netdev;
 
+	rcu_assign_pointer(internal_dev->attached_vport, internal_dev->vport);
+	dev_set_promiscuity(netdev_vport->dev, 1);
+	netif_start_queue(netdev_vport->dev);
+
 	return vport;
 
 error_free_netdev:
@@ -227,34 +231,17 @@ error:
 static int internal_dev_destroy(struct vport *vport)
 {
 	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
-
-	unregister_netdevice(netdev_vport->dev);
-	vport_free(vport);
-
-	return 0;
-}
-
-static int internal_dev_attach(struct vport *vport)
-{
-	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
-	struct internal_dev *internal_dev = internal_dev_priv(netdev_vport->dev);
-
-	rcu_assign_pointer(internal_dev->attached_vport, internal_dev->vport);
-	dev_set_promiscuity(netdev_vport->dev, 1);
-	netif_start_queue(netdev_vport->dev);
-
-	return 0;
-}
-
-static int internal_dev_detach(struct vport *vport)
-{
-	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
 	struct internal_dev *internal_dev = internal_dev_priv(netdev_vport->dev);
 
 	netif_stop_queue(netdev_vport->dev);
 	dev_set_promiscuity(netdev_vport->dev, -1);
 	rcu_assign_pointer(internal_dev->attached_vport, NULL);
 
+	synchronize_rcu();
+
+	unregister_netdevice(netdev_vport->dev);
+	vport_free(vport);
+
 	return 0;
 }
 
@@ -282,8 +269,6 @@ struct vport_ops internal_vport_ops = {
 	.flags		= VPORT_F_REQUIRED | VPORT_F_GEN_STATS | VPORT_F_FLOW,
 	.create		= internal_dev_create,
 	.destroy	= internal_dev_destroy,
-	.attach		= internal_dev_attach,
-	.detach		= internal_dev_detach,
 	.set_mtu	= netdev_set_mtu,
 	.set_addr	= netdev_set_addr,
 	.get_name	= netdev_get_name,
diff --git a/datapath/vport-netdev.c b/datapath/vport-netdev.c
index d4536a4..a5096df 100644
--- a/datapath/vport-netdev.c
+++ b/datapath/vport-netdev.c
@@ -125,6 +125,15 @@ static struct vport *netdev_create(const struct vport_parms *parms)
 			vport_set_stats(vport, &stats);
 	}
 
+	err = netdev_rx_handler_register(netdev_vport->dev, netdev_frame_hook,
+					 vport);
+	if (err)
+		goto error_put;
+
+	dev_set_promiscuity(netdev_vport->dev, 1);
+	dev_disable_lro(netdev_vport->dev);
+	netdev_vport->dev->priv_flags |= IFF_OVS_DATAPATH;
+
 	return vport;
 
 error_put:
@@ -139,37 +148,15 @@ static int netdev_destroy(struct vport *vport)
 {
 	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
 
-	dev_put(netdev_vport->dev);
-	vport_free(vport);
-
-	return 0;
-}
-
-static int netdev_attach(struct vport *vport)
-{
-	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
-	int err;
-
-	err = netdev_rx_handler_register(netdev_vport->dev, netdev_frame_hook,
-					 vport);
-	if (err)
-		return err;
-
-	dev_set_promiscuity(netdev_vport->dev, 1);
-	dev_disable_lro(netdev_vport->dev);
-	netdev_vport->dev->priv_flags |= IFF_OVS_DATAPATH;
-
-	return 0;
-}
-
-static int netdev_detach(struct vport *vport)
-{
-	struct netdev_vport *netdev_vport = netdev_vport_priv(vport);
-
 	netdev_vport->dev->priv_flags &= ~IFF_OVS_DATAPATH;
 	netdev_rx_handler_unregister(netdev_vport->dev);
 	dev_set_promiscuity(netdev_vport->dev, -1);
 
+	synchronize_rcu();
+
+	dev_put(netdev_vport->dev);
+	vport_free(vport);
+
 	return 0;
 }
 
@@ -309,8 +296,6 @@ struct vport_ops netdev_vport_ops = {
 	.exit		= netdev_exit,
 	.create		= netdev_create,
 	.destroy	= netdev_destroy,
-	.attach		= netdev_attach,
-	.detach		= netdev_detach,
 	.set_mtu	= netdev_set_mtu,
 	.set_addr	= netdev_set_addr,
 	.get_name	= netdev_get_name,
diff --git a/datapath/vport-patch.c b/datapath/vport-patch.c
index 52d3da1..a0dd36d 100644
--- a/datapath/vport-patch.c
+++ b/datapath/vport-patch.c
@@ -39,6 +39,8 @@ struct patch_vport {
 static struct hlist_head *peer_table;
 #define PEER_HASH_BUCKETS 256
 
+static void update_peers(const char *name, struct vport *);
+
 static inline struct patch_vport *patch_vport_priv(const struct vport *vport)
 {
 	return vport_priv(vport);
@@ -130,6 +132,11 @@ static struct vport *patch_create(const struct vport_parms *parms)
      * bottleneck on systems using jumbo frames. */
 	patch_vport->devconf->mtu = 65535;
 
+	hlist_add_head(&patch_vport->hash_node, hash_bucket(patch_vport->peer_name));
+
+	rcu_assign_pointer(patch_vport->peer, vport_locate(patch_vport->peer_name));
+	update_peers(patch_vport->name, vport);
+
 	return vport;
 
 error_free_vport:
@@ -155,6 +162,13 @@ static int patch_destroy(struct vport *vport)
 {
 	struct patch_vport *patch_vport = patch_vport_priv(vport);
 
+	update_peers(patch_vport->name, NULL);
+	rcu_assign_pointer(patch_vport->peer, NULL);
+
+	hlist_del(&patch_vport->hash_node);
+
+	synchronize_rcu();
+
 	kfree(patch_vport->devconf);
 	vport_free(vport);
 
@@ -172,30 +186,6 @@ static void update_peers(const char *name, struct vport *vport)
 			rcu_assign_pointer(peer_vport->peer, vport);
 }
 
-static int patch_attach(struct vport *vport)
-{
-	struct patch_vport *patch_vport = patch_vport_priv(vport);
-
-	hlist_add_head(&patch_vport->hash_node, hash_bucket(patch_vport->peer_name));
-
-	rcu_assign_pointer(patch_vport->peer, vport_locate(patch_vport->peer_name));
-	update_peers(patch_vport->name, vport);
-
-	return 0;
-}
-
-static int patch_detach(struct vport *vport)
-{
-	struct patch_vport *patch_vport = patch_vport_priv(vport);
-
-	update_peers(patch_vport->name, NULL);
-	rcu_assign_pointer(patch_vport->peer, NULL);
-
-	hlist_del(&patch_vport->hash_node);
-
-	return 0;
-}
-
 static int patch_set_mtu(struct vport *vport, int mtu)
 {
 	struct patch_vport *patch_vport = patch_vport_priv(vport);
@@ -270,8 +260,6 @@ struct vport_ops patch_vport_ops = {
 	.create		= patch_create,
 	.modify		= patch_modify,
 	.destroy	= patch_destroy,
-	.attach		= patch_attach,
-	.detach		= patch_detach,
 	.set_mtu	= patch_set_mtu,
 	.set_addr	= patch_set_addr,
 	.get_name	= patch_get_name,
diff --git a/datapath/vport.c b/datapath/vport.c
index b9334ee..cddb891 100644
--- a/datapath/vport.c
+++ b/datapath/vport.c
@@ -587,8 +587,9 @@ void vport_free(struct vport *vport)
  *
  * @parms: Information about new vport.
  *
- * Creates a new vport with the specified configuration (which is dependent
- * on device type).  Both RTNL and vport locks must be held.
+ * Creates a new vport with the specified configuration (which is dependent on
+ * device type) and attaches it to a datapath.  Both RTNL and vport locks must
+ * be held.
  */
 struct vport *vport_add(const struct vport_parms *parms)
 {
@@ -643,9 +644,8 @@ int vport_mod(struct vport *vport, struct odp_port *port)
  *
  * @vport: vport to delete.
  *
- * Deletes the specified device.  The device must not be currently attached to
- * a datapath.  It is possible to fail for reasons such as lack of memory.
- * Both RTNL and vport locks must be held.
+ * Detaches @vport from its datapath and destroys it.  It is possible to fail
+ * for reasons such as lack of memory.  Both RTNL and vport locks must be held.
  */
 int vport_del(struct vport *vport)
 {
@@ -658,42 +658,6 @@ int vport_del(struct vport *vport)
 }
 
 /**
- *	vport_attach - notify a vport that it has been attached to a datapath
- *
- * @vport: vport to attach.
- *
- * Performs vport-specific actions so that packets may be exchanged.  RTNL lock
- * and the appropriate DP mutex must be held.
- */
-int vport_attach(struct vport *vport)
-{
-	ASSERT_RTNL();
-
-	if (vport->ops->attach)
-		return vport->ops->attach(vport);
-
-	return 0;
-}
-
-/**
- *	vport_detach - detach a vport from a datapath
- *
- * @vport: vport to detach.
- *
- * Performs vport-specific actions before a vport is detached from a datapath.
- * May fail for a variety of reasons, including lack of memory.  RTNL lock and
- * the appropriate DP mutex must be held.
- */
-int vport_detach(struct vport *vport)
-{
-	ASSERT_RTNL();
-
-	if (vport->ops->detach)
-		return vport->ops->detach(vport);
-	return 0;
-}
-
-/**
  *	vport_set_mtu - set device MTU (for kernel callers)
  *
  * @vport: vport on which to set MTU.
diff --git a/datapath/vport.h b/datapath/vport.h
index 13772c7..f53291d 100644
--- a/datapath/vport.h
+++ b/datapath/vport.h
@@ -45,9 +45,6 @@ int vport_del(struct vport *);
 
 struct vport *vport_locate(const char *name);
 
-int vport_attach(struct vport *);
-int vport_detach(struct vport *);
-
 int vport_set_mtu(struct vport *, int mtu);
 int vport_set_addr(struct vport *, const unsigned char *);
 int vport_set_stats(struct vport *, struct rtnl_link_stats64 *);
@@ -170,12 +167,7 @@ struct vport_parms {
  * a new vport allocated with vport_alloc(), otherwise an ERR_PTR() value.
  * @modify: Modify the configuration of an existing vport.  May be null if
  * modification is not supported.
- * @destroy: Destroy and free a vport using vport_free().  Prior to destruction
- * @detach will be called followed by synchronize_rcu().
- * @attach: Attach a previously created vport to a datapath.  After attachment
- * packets may be sent and received.  Prior to attachment any packets may be
- * silently discarded.  May be null if not needed.
- * @detach: Detach a vport from a datapath.  May be null if not needed.
+ * @destroy: Detach and destroy a vport.
  * @set_mtu: Set the device's MTU.  May be null if not supported.
  * @set_addr: Set the device's MAC address.  May be null if not supported.
  * @set_stats: Provides stats as an offset to be added to the device stats.
@@ -211,9 +203,6 @@ struct vport_ops {
 	int (*modify)(struct vport *, struct odp_port *);
 	int (*destroy)(struct vport *);
 
-	int (*attach)(struct vport *);
-	int (*detach)(struct vport *);
-
 	int (*set_mtu)(struct vport *, int mtu);
 	int (*set_addr)(struct vport *, const unsigned char *);
 	int (*set_stats)(const struct vport *, struct rtnl_link_stats64 *);
-- 
1.7.1





More information about the dev mailing list