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 bd511e929c1edec685bc9ffac9d8894b2a3deaf6..cce1e5c03dfc0747dd7e02962e5c04e9c285409b 100644 (file)
--- 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 f6ad8a697a6ca85592a607e299f53b70deaebc6a..899ed495bf03fcb445ad46c4692e12274630c6f3 100644 (file)
--- 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 dbb02c7d7f7dff9abbe27e35309e5d57a336ef82..43914263fdf8b5313d5a368c05e3f7027ad773ee 100644 (file)
--- 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;