ofproto-dpif-xlate: Do initial rule lookup for callers.

None of the functions available in ofproto-dpif.h are thread safe
unless holding the xlate_rwlock because one can't know that an ofproto
or ofport used as argument will survive during the function call.  For
this reason, ofproto-dpif-upcall's invocation of rule_dpif_lookup()
is unsafe because the ofproto could be destroyed during the call.

This patch fixes the problem by optionally doing the initial rule
lookup in xlate_actions() so that it can be done while holding the
xlate_rwlock.  This has the nice side benefit of removing a bunch of
boilerplate.

Note that this only partially solves the problem, there's still
vsp_realdev_to_vlandev() and ofproto_dpif_send_packet_in() which
aren't thread safe for the same reason.

Signed-off-by: Ethan Jackson <ethan@nicira.com>

diff --git a/manpages.mk b/manpages.mk
index 811d2f9..2a34f04 100644 (file)
--- a/manpages.mk
+++ b/manpages.mk
@@ -116,6 +116,10 @@ lib/vconn-active.man:
 lib/vconn-passive.man:
 lib/vlog.man:
 
+utilities/ovs-dpctl-top.8: \
+       utilities/ovs-dpctl-top.8.in
+utilities/ovs-dpctl-top.8.in:
+
 utilities/ovs-dpctl.8: \
        utilities/ovs-dpctl.8.in \
        lib/common.man \
@@ -124,10 +128,6 @@ utilities/ovs-dpctl.8.in:
 lib/common.man:
 lib/vlog.man:
 
-utilities/ovs-dpctl-top.8: \
-       utilities/ovs-dpctl-top.8.in
-utilities/ovs-dpctl-top.8.in:
-
 utilities/ovs-l3ping.8: \
        utilities/ovs-l3ping.8.in \
        lib/common-syn.man \

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index bc1e884..9ed10c0 100644 (file)
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -604,8 +604,6 @@ static void
 execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
 {
     struct ofproto_dpif *ofproto = miss->ofproto;
-    struct flow_wildcards wc;
-    struct rule_dpif *rule;
     struct ofpbuf *packet;
     struct xlate_in xin;
 
@@ -617,17 +615,13 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
         miss->stats.n_packets++;
     }
 
-    flow_wildcards_init_catchall(&wc);
-    rule_dpif_lookup(ofproto, &miss->flow, &wc, &rule);
-    rule_dpif_credit_stats(rule, &miss->stats);
-    xlate_in_init(&xin, ofproto, &miss->flow, rule, miss->stats.tcp_flags,
+    xlate_in_init(&xin, ofproto, &miss->flow, NULL, miss->stats.tcp_flags,
                   NULL);
     xin.may_learn = true;
     xin.resubmit_stats = &miss->stats;
     xlate_actions(&xin, &miss->xout);
-    flow_wildcards_or(&miss->xout.wc, &miss->xout.wc, &wc);
 
-    if (rule_dpif_fail_open(rule)) {
+    if (miss->xout.fail_open) {
         LIST_FOR_EACH (packet, list_node, &miss->packets) {
             struct ofputil_packet_in *pin;
 
@@ -656,11 +650,10 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
         LIST_FOR_EACH (packet, list_node, &miss->packets) {
             struct xlate_in xin;
 
-            xlate_in_init(&xin, miss->ofproto, &miss->flow, rule, 0, packet);
+            xlate_in_init(&xin, miss->ofproto, &miss->flow, NULL, 0, packet);
             xlate_actions_for_side_effects(&xin);
         }
     }
-    rule_dpif_unref(rule);
 
     if (miss->xout.odp_actions.size) {
         LIST_FOR_EACH (packet, list_node, &miss->packets) {

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 05c7041..74dddd6 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2521,6 +2521,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
 {
     struct flow_wildcards *wc = &xout->wc;
     struct flow *flow = &xin->flow;
+    struct rule_dpif *rule = NULL;
 
     struct rule_actions *actions = NULL;
     enum slow_path_reason special;
@@ -2595,11 +2596,20 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
     ctx.table_id = 0;
     ctx.exit = false;
 
+    if (!xin->ofpacts && !ctx.rule) {
+        rule_dpif_lookup(ctx.xbridge->ofproto, flow, wc, &rule);
+        if (ctx.xin->resubmit_stats) {
+            rule_dpif_credit_stats(rule, ctx.xin->resubmit_stats);
+        }
+        ctx.rule = rule;
+    }
+    xout->fail_open = ctx.rule && rule_dpif_fail_open(ctx.rule);
+
     if (xin->ofpacts) {
         ofpacts = xin->ofpacts;
         ofpacts_len = xin->ofpacts_len;
-    } else if (xin->rule) {
-        actions = rule_dpif_get_actions(xin->rule);
+    } else if (ctx.rule) {
+        actions = rule_dpif_get_actions(ctx.rule);
         ofpacts = actions->ofpacts;
         ofpacts_len = actions->ofpacts_len;
     } else {
@@ -2688,5 +2698,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
 out:
     ovs_rwlock_unlock(&xlate_rwlock);
 
+    rule_dpif_unref(rule);
     rule_actions_unref(actions);
 }

diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index a54a9e4..48e58bc 100644 (file)
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -43,6 +43,7 @@ struct xlate_out {
     struct flow_wildcards wc;
 
     enum slow_path_reason slow; /* 0 if fast path may be used. */
+    bool fail_open;             /* Initial rule is fail open? */
     bool has_learn;             /* Actions include NXAST_LEARN? */
     bool has_normal;            /* Actions output to OFPP_NORMAL? */
     bool has_fin_timeout;       /* Actions include NXAST_FIN_TIMEOUT? */
@@ -72,7 +73,8 @@ struct xlate_in {
      * not if we are just revalidating. */
     bool may_learn;
 
-    /* The rule initiating translation or NULL. */
+    /* The rule initiating translation or NULL. If both 'rule' and 'ofpacts'
+     * are NULL, xlate_actions() will do the initial rule lookup itself. */
     struct rule_dpif *rule;
 
     /* The actions to translate.  If 'rule' is not NULL, these may be NULL. */

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8945b00..92f3262 100644 (file)
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4198,15 +4198,11 @@ facet_check_consistency(struct facet *facet)
 
     struct xlate_out xout;
     struct xlate_in xin;
-
-    struct rule_dpif *rule;
     bool ok;
 
     /* Check the datapath actions for consistency. */
-    rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule);
-    xlate_in_init(&xin, facet->ofproto, &facet->flow, rule, 0, NULL);
+    xlate_in_init(&xin, facet->ofproto, &facet->flow, NULL, 0, NULL);
     xlate_actions(&xin, &xout);
-    rule_dpif_unref(rule);
 
     ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions)
         && facet->xout.slow == xout.slow;
@@ -4358,7 +4354,6 @@ flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow,
                 struct dpif_flow_stats *stats, bool may_learn)
 {
     struct ofport_dpif *in_port;
-    struct rule_dpif *rule;
     struct xlate_in xin;
 
     in_port = get_ofp_port(ofproto, flow->in_port.ofp_port);
@@ -4366,13 +4361,10 @@ flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow,
         netdev_vport_inc_rx(in_port->up.netdev, stats);
     }
 
-    rule_dpif_lookup(ofproto, flow, NULL, &rule);
-    rule_dpif_credit_stats(rule, stats);
-    xlate_in_init(&xin, ofproto, flow, rule, stats->tcp_flags, NULL);
+    xlate_in_init(&xin, ofproto, flow, NULL, stats->tcp_flags, NULL);
     xin.resubmit_stats = stats;
     xin.may_learn = may_learn;
     xlate_actions_for_side_effects(&xin);
-    rule_dpif_unref(rule);
 }
 
 static void