From: Ben Pfaff <blp@nicira.com>
Date: Fri, 1 May 2009 17:20:35 +0000 (-0700)
Subject: datapath: Eliminate synchronize_rcu() in port group update.
X-Git-Url: https://pintos-os.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=1369fd47d27e6a4e8b3d1470bd982409875161d0;p=openvswitch

datapath: Eliminate synchronize_rcu() in port group update.

We found out some time ago that synchronize_rcu() can block for multiple
seconds in some cases, so it's a good idea to eliminate as many of them
as we can.

This removes such a call in set_port_group().  This requires adding an
rcu_head to the data structure for port groups; since until now we've just
used the same "struct odp_port_group" exported to userspace, this means
that we need to introduce a new "struct dp_port_group" for internal use,
which in turn causes a fair bit of code motion.

Bug #1233.
---

diff --git a/datapath/actions.c b/datapath/actions.c
index bd511e92..cce1e5c0 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -374,7 +374,7 @@ error:
 static int output_group(struct datapath *dp, __u16 group,
 			struct sk_buff *skb, gfp_t gfp)
 {
-	struct odp_port_group *g = rcu_dereference(dp->groups[group]);
+	struct dp_port_group *g = rcu_dereference(dp->groups[group]);
 	int prev_port = -1;
 	int i;
 
diff --git a/datapath/datapath.c b/datapath/datapath.c
index f6ad8a69..899ed495 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -307,13 +307,8 @@ static void do_destroy_dp(struct datapath *dp)
 	dp_table_destroy(dp->table, 1);
 	for (i = 0; i < DP_N_QUEUES; i++)
 		skb_queue_purge(&dp->queues[i]);
-	for (i = 0; i < DP_MAX_GROUPS; i++) {
-		struct odp_port_group *pg = dp->groups[i];
-		if (pg) {
-			kfree(dp->groups[i]->ports);
-			kfree(dp->groups[i]);
-		}
-	}
+	for (i = 0; i < DP_MAX_GROUPS; i++)
+		kfree(dp->groups[i]);
 	free_percpu(dp->stats_percpu);
 	kfree(dp);
 	module_put(THIS_MODULE);
@@ -1285,11 +1280,18 @@ list_ports(struct datapath *dp, struct odp_portvec __user *pvp)
 	return put_user(idx, &pvp->n_ports);
 }
 
+/* RCU callback for freeing a dp_port_group */
+static void free_port_group(struct rcu_head *rcu)
+{
+	struct dp_port_group *g = container_of(rcu, struct dp_port_group, rcu);
+	kfree(g);
+}
+
 static int
 set_port_group(struct datapath *dp, const struct odp_port_group __user *upg)
 {
 	struct odp_port_group pg;
-	struct odp_port_group *pgp, *old_pg;
+	struct dp_port_group *new_group, *old_group;
 	int error;
 
 	error = -EFAULT;
@@ -1301,31 +1303,25 @@ set_port_group(struct datapath *dp, const struct odp_port_group __user *upg)
 		goto error;
 
 	error = -ENOMEM;
-	pgp = kmalloc(sizeof *pgp, GFP_KERNEL);
-	if (!pgp)
+	new_group = kmalloc(sizeof *new_group + sizeof(u16) * pg.n_ports,
+			    GFP_KERNEL);
+	if (!new_group)
 		goto error;
-	pgp->ports = kmalloc(sizeof(u16) * pg.n_ports, GFP_KERNEL);
-	if (!pgp->ports)
-		goto error_free_pgp;
 
-	pgp->n_ports = pg.n_ports;
+	new_group->n_ports = pg.n_ports;
 	error = -EFAULT;
-	if (copy_from_user(pgp->ports, pg.ports, sizeof(u16) * pg.n_ports))
-		goto error_free_pgp_ports;
-
-	old_pg = rcu_dereference(dp->groups[pg.group]);
-	rcu_assign_pointer(dp->groups[pg.group], pgp);
-	if (old_pg) {
-		synchronize_rcu();	/* XXX expensive! */
-		kfree(old_pg->ports);
-		kfree(old_pg);
-	}
+	if (copy_from_user(new_group->ports, pg.ports,
+			   sizeof(u16) * pg.n_ports))
+		goto error_free;
+
+	old_group = rcu_dereference(dp->groups[pg.group]);
+	rcu_assign_pointer(dp->groups[pg.group], new_group);
+	if (old_group)
+		call_rcu(&old_group->rcu, free_port_group);
 	return 0;
 
-error_free_pgp_ports:
-	kfree(pgp->ports);
-error_free_pgp:
-	kfree(pgp);
+error_free:
+	kfree(new_group);
 error:
 	return error;
 }
@@ -1333,7 +1329,8 @@ error:
 static int
 get_port_group(struct datapath *dp, struct odp_port_group *upg)
 {
-	struct odp_port_group pg, *g;
+	struct odp_port_group pg;
+	struct dp_port_group *g;
 	u16 n_copy;
 
 	if (copy_from_user(&pg, upg, sizeof pg))
@@ -1343,7 +1340,7 @@ get_port_group(struct datapath *dp, struct odp_port_group *upg)
 		return -EINVAL;
 
 	g = dp->groups[pg.group];
-	n_copy = g ? min(g->n_ports, pg.n_ports) : 0;
+	n_copy = g ? min_t(int, g->n_ports, pg.n_ports) : 0;
 	if (n_copy && copy_to_user(pg.ports, g->ports, n_copy * sizeof(u16)))
 		return -EFAULT;
 
diff --git a/datapath/datapath.h b/datapath/datapath.h
index dbb02c7d..43914263 100644
--- a/datapath/datapath.h
+++ b/datapath/datapath.h
@@ -46,6 +46,12 @@ struct dp_stats_percpu {
 	u64 n_lost;
 };
 
+struct dp_port_group {
+	struct rcu_head rcu;
+	int n_ports;
+	u16 ports[];
+};
+
 struct datapath {
 	struct mutex mutex;
 	int dp_idx;
@@ -69,7 +75,7 @@ struct datapath {
 	struct dp_table *table;
 
 	/* Port groups. */
-	struct odp_port_group *groups[DP_MAX_GROUPS];
+	struct dp_port_group *groups[DP_MAX_GROUPS];
 
 	/* Switch ports. */
 	unsigned int n_ports;