ofproto: Move rule_execute() out of ofopgroup_complete().

One goal we're moving toward is to be able to execute "learn" actions
in each thread that handles packets that arrive on an interface just before
we forward those packets.  The planned strategy there is to have a global
lock that protects everything required to modify the flow table.  Generally
this works out well, but rule_execute() is a trouble spot.  That's because
it's called during a flow table modification (when that global lock is
held) which means that a "learn" action triggered by the executing the
packet would try to recursively modify the flow table and reacquire the
global lock.

I can see two ways out of this issue.  One would be to make the global lock
a recursive one, or otherwise deal with handling recursive flow_mods.  The
other is to just queue up flow_mods due to rule_execute(), which itself is
a corner case (it only happens when a flow_mod sent via OpenFlow includes
a buffer_id).  (I guess there could be other more radical solutions, like
just dropping packets that contain "learn" actions if they occur in this
situation.)

This commit implements the second solution because it seems less likely to
be wrong in a way that crashes the switch.

Signed-off-by: Ben Pfaff <blp@nicira.com>
Acked-by: Ethan Jackson <ethan@nicira.com>

diff --git a/ofproto/ofproto-provider.h b/ofproto/ofproto-provider.h
index 59ac3dc..8d0e14f 100644 (file)
--- a/ofproto/ofproto-provider.h
+++ b/ofproto/ofproto-provider.h
@@ -21,6 +21,7 @@
 
 #include "cfm.h"
 #include "classifier.h"
+#include "guarded-list.h"
 #include "heap.h"
 #include "hindex.h"
 #include "list.h"
@@ -98,6 +99,14 @@ struct ofproto {
     unsigned int n_pending;     /* list_size(&pending). */
     struct hmap deletions;      /* All OFOPERATION_DELETE "ofoperation"s. */
 
+    /* Delayed rule executions.
+     *
+     * We delay calls to ->ofproto_class->rule_execute() past releasing
+     * ofproto_mutex during a flow_mod, because otherwise a "learn" action
+     * triggered by the executing the packet would try to recursively modify
+     * the flow table and reacquire the global lock. */
+    struct guarded_list rule_executes;
+
     /* Flow table operation logging. */
     int n_add, n_delete, n_modify; /* Number of unreported ops of each kind. */
     long long int first_op, last_op; /* Range of times for unreported ops. */

diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index 5473c7d..685bfa6 100644 (file)
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -218,6 +218,17 @@ static void rule_criteria_init(struct rule_criteria *, uint8_t table_id,
                                ofp_port_t out_port);
 static void rule_criteria_destroy(struct rule_criteria *);
 
+/* A packet that needs to be passed to rule_execute(). */
+struct rule_execute {
+    struct list list_node;      /* In struct ofproto's "rule_executes" list. */
+    struct rule *rule;          /* Owns a reference to the rule. */
+    ofp_port_t in_port;
+    struct ofpbuf *packet;      /* Owns the packet. */
+};
+
+static void run_rule_executes(struct ofproto *);
+static void destroy_rule_executes(struct ofproto *);
+
 /* ofport. */
 static void ofport_destroy__(struct ofport *);
 static void ofport_destroy(struct ofport *);
@@ -479,6 +490,7 @@ ofproto_create(const char *datapath_name, const char *datapath_type,
     list_init(&ofproto->pending);
     ofproto->n_pending = 0;
     hmap_init(&ofproto->deletions);
+    guarded_list_init(&ofproto->rule_executes);
     ofproto->n_add = ofproto->n_delete = ofproto->n_modify = 0;
     ofproto->first_op = ofproto->last_op = LLONG_MIN;
     ofproto->next_op_report = LLONG_MAX;
@@ -1173,6 +1185,9 @@ ofproto_destroy__(struct ofproto *ofproto)
     ovs_assert(list_is_empty(&ofproto->pending));
     ovs_assert(!ofproto->n_pending);
 
+    destroy_rule_executes(ofproto);
+    guarded_list_destroy(&ofproto->rule_executes);
+
     connmgr_destroy(ofproto->connmgr);
 
     hmap_remove(&all_ofprotos, &ofproto->hmap_node);
@@ -1312,6 +1327,8 @@ ofproto_run(struct ofproto *p)
         VLOG_ERR_RL(&rl, "%s: run failed (%s)", p->name, ovs_strerror(error));
     }
 
+    run_rule_executes(p);
+
     /* Restore the eviction group heap invariant occasionally. */
     if (p->eviction_group_timer < time_msec()) {
         size_t i;
@@ -2430,22 +2447,48 @@ ofoperation_has_out_port(const struct ofoperation *op, ofp_port_t out_port)
     NOT_REACHED();
 }
 
-/* Executes the actions indicated by 'rule' on 'packet' and credits 'rule''s
- * statistics appropriately.
- *
- * 'packet' doesn't necessarily have to match 'rule'.  'rule' will be credited
- * with statistics for 'packet' either way.
- *
- * Takes ownership of 'packet'. */
-static int
-rule_execute(struct rule *rule, ofp_port_t in_port, struct ofpbuf *packet)
+static void
+rule_execute_destroy(struct rule_execute *e)
 {
-    struct flow flow;
-    union flow_in_port in_port_;
+    ofproto_rule_unref(e->rule);
+    list_remove(&e->list_node);
+    free(e);
+}
+
+/* Executes all "rule_execute" operations queued up in ofproto->rule_executes,
+ * by passing them to the ofproto provider. */
+static void
+run_rule_executes(struct ofproto *ofproto)
+{
+    struct rule_execute *e, *next;
+    struct list executes;
+
+    guarded_list_pop_all(&ofproto->rule_executes, &executes);
+    LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
+        union flow_in_port in_port_;
+        struct flow flow;
+
+        in_port_.ofp_port = e->in_port;
+        flow_extract(e->packet, 0, 0, NULL, &in_port_, &flow);
+        ofproto->ofproto_class->rule_execute(e->rule, &flow, e->packet);
 
-    in_port_.ofp_port = in_port;
-    flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
-    return rule->ofproto->ofproto_class->rule_execute(rule, &flow, packet);
+        rule_execute_destroy(e);
+    }
+}
+
+/* Destroys and discards all "rule_execute" operations queued up in
+ * ofproto->rule_executes. */
+static void
+destroy_rule_executes(struct ofproto *ofproto)
+{
+    struct rule_execute *e, *next;
+    struct list executes;
+
+    guarded_list_pop_all(&ofproto->rule_executes, &executes);
+    LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
+        ofpbuf_delete(e->packet);
+        rule_execute_destroy(e);
+    }
 }
 
 /* Returns true if 'rule' should be hidden from the controller.
@@ -4126,35 +4169,46 @@ static enum ofperr
 handle_flow_mod__(struct ofproto *ofproto, struct ofconn *ofconn,
                   struct ofputil_flow_mod *fm, const struct ofp_header *oh)
 {
-    if (ofproto->n_pending >= 50) {
-        ovs_assert(!list_is_empty(&ofproto->pending));
-        return OFPROTO_POSTPONE;
-    }
+    enum ofperr error;
 
-    switch (fm->command) {
-    case OFPFC_ADD:
-        return add_flow(ofproto, ofconn, fm, oh);
+    if (ofproto->n_pending < 50) {
+        switch (fm->command) {
+        case OFPFC_ADD:
+            error = add_flow(ofproto, ofconn, fm, oh);
+            break;
 
-    case OFPFC_MODIFY:
-        return modify_flows_loose(ofproto, ofconn, fm, oh);
+        case OFPFC_MODIFY:
+            error = modify_flows_loose(ofproto, ofconn, fm, oh);
+            break;
 
-    case OFPFC_MODIFY_STRICT:
-        return modify_flow_strict(ofproto, ofconn, fm, oh);
+        case OFPFC_MODIFY_STRICT:
+            error = modify_flow_strict(ofproto, ofconn, fm, oh);
+            break;
 
-    case OFPFC_DELETE:
-        return delete_flows_loose(ofproto, ofconn, fm, oh);
+        case OFPFC_DELETE:
+            error = delete_flows_loose(ofproto, ofconn, fm, oh);
+            break;
 
-    case OFPFC_DELETE_STRICT:
-        return delete_flow_strict(ofproto, ofconn, fm, oh);
+        case OFPFC_DELETE_STRICT:
+            error = delete_flow_strict(ofproto, ofconn, fm, oh);
+            break;
 
-    default:
-        if (fm->command > 0xff) {
-            VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
-                         "flow_mod_table_id extension is not enabled",
-                         ofproto->name);
+        default:
+            if (fm->command > 0xff) {
+                VLOG_WARN_RL(&rl, "%s: flow_mod has explicit table_id but "
+                             "flow_mod_table_id extension is not enabled",
+                             ofproto->name);
+            }
+            error = OFPERR_OFPFMFC_BAD_COMMAND;
+            break;
         }
-        return OFPERR_OFPFMFC_BAD_COMMAND;
+    } else {
+        ovs_assert(!list_is_empty(&ofproto->pending));
+        error = OFPROTO_POSTPONE;
     }
+
+    run_rule_executes(ofproto);
+    return error;
 }
 
 static enum ofperr
@@ -5132,8 +5186,23 @@ ofopgroup_complete(struct ofopgroup *group)
                 error = ofconn_pktbuf_retrieve(group->ofconn, group->buffer_id,
                                                &packet, &in_port);
                 if (packet) {
+                    struct rule_execute *re;
+
                     ovs_assert(!error);
-                    error = rule_execute(op->rule, in_port, packet);
+
+                    ofproto_rule_ref(op->rule);
+
+                    re = xmalloc(sizeof *re);
+                    re->rule = op->rule;
+                    re->in_port = in_port;
+                    re->packet = packet;
+
+                    if (!guarded_list_push_back(&ofproto->rule_executes,
+                                                &re->list_node, 1024)) {
+                        ofproto_rule_unref(op->rule);
+                        ofpbuf_delete(re->packet);
+                        free(re);
+                    }
                 }
                 break;
             }