ofproto-dpif: Hide struct rule_dpif internally.
authorEthan Jackson <ethan@nicira.com>
Tue, 27 Aug 2013 20:17:11 +0000 (13:17 -0700)
committerEthan Jackson <ethan@nicira.com>
Wed, 4 Sep 2013 22:57:06 +0000 (15:57 -0700)
By hiding struct rule_dpif inside ofproto-dpif, it becomes very clear
which attributes are accessed by multiple threads and need to be
protected by locks.

Signed-off-by: Ethan Jackson <ethan@nicira.com>
Acked-by: Ben Pfaff <blp@nicira.com>
ofproto/ofproto-dpif-upcall.c
ofproto/ofproto-dpif-xlate.c
ofproto/ofproto-dpif-xlate.h
ofproto/ofproto-dpif.c
ofproto/ofproto-dpif.h

index 3294d3b..54f441b 100644 (file)
@@ -684,7 +684,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
 
     flow_wildcards_init_catchall(&wc);
     rule_dpif_lookup(ofproto, &miss->flow, &wc, &rule);
-    rule_credit_stats(rule, &miss->stats);
+    rule_dpif_credit_stats(rule, &miss->stats);
     xlate_in_init(&xin, ofproto, &miss->flow, rule, miss->stats.tcp_flags,
                   NULL);
     xin.may_learn = true;
@@ -692,7 +692,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
     xlate_actions(&xin, &miss->xout);
     flow_wildcards_or(&miss->xout.wc, &miss->xout.wc, &wc);
 
-    if (rule->up.cr.priority == FAIL_OPEN_PRIORITY) {
+    if (rule_dpif_fail_open(rule)) {
         LIST_FOR_EACH (packet, list_node, &miss->packets) {
             struct ofputil_packet_in *pin;
 
@@ -725,7 +725,7 @@ execute_flow_miss(struct flow_miss *miss, struct dpif_op *ops, size_t *n_ops)
             xlate_actions_for_side_effects(&xin);
         }
     }
-    rule_release(rule);
+    rule_dpif_release(rule);
 
     if (miss->xout.odp_actions.size) {
         LIST_FOR_EACH (packet, list_node, &miss->packets) {
index feee71c..6a78c6d 100644 (file)
@@ -1668,21 +1668,24 @@ compose_output_action(struct xlate_ctx *ctx, ofp_port_t ofp_port)
 
 static void
 xlate_recursively(struct xlate_ctx *ctx, struct rule_dpif *rule)
-    OVS_RELEASES(rule->up.evict)
+    OVS_RELEASES(rule)
 {
     struct rule_dpif *old_rule = ctx->rule;
+    const struct ofpact *ofpacts;
+    size_t ofpacts_len;
 
     if (ctx->xin->resubmit_stats) {
-        rule_credit_stats(rule, ctx->xin->resubmit_stats);
+        rule_dpif_credit_stats(rule, ctx->xin->resubmit_stats);
     }
 
     ctx->recurse++;
     ctx->rule = rule;
-    do_xlate_actions(rule->up.ofpacts, rule->up.ofpacts_len, ctx);
+    rule_dpif_get_actions(rule, &ofpacts, &ofpacts_len);
+    do_xlate_actions(ofpacts, ofpacts_len, ctx);
     ctx->rule = old_rule;
     ctx->recurse--;
 
-    rule_release(rule);
+    rule_dpif_release(rule);
 }
 
 static void
@@ -1722,10 +1725,9 @@ xlate_table_action(struct xlate_ctx *ctx,
              * OFPTC_TABLE_MISS_DROP
              * When OF1.0, OFPTC_TABLE_MISS_CONTINUE is used. What to do? */
             xport = get_ofp_port(ctx->xbridge, ctx->xin->flow.in_port.ofp_port);
-            rule = choose_miss_rule(xport ? xport->config : 0,
-                                    ctx->xbridge->miss_rule,
-                                    ctx->xbridge->no_packet_in_rule);
-            ovs_rwlock_rdlock(&rule->up.evict);
+            choose_miss_rule(xport ? xport->config : 0,
+                             ctx->xbridge->miss_rule,
+                             ctx->xbridge->no_packet_in_rule, &rule);
             xlate_recursively(ctx, rule);
         }
 
@@ -1811,7 +1813,7 @@ execute_controller_action(struct xlate_ctx *ctx, int len,
     pin->reason = reason;
     pin->controller_id = controller_id;
     pin->table_id = ctx->table_id;
-    pin->cookie = ctx->rule ? ctx->rule->up.flow_cookie : 0;
+    pin->cookie = ctx->rule ? rule_dpif_get_flow_cookie(ctx->rule) : 0;
 
     pin->send_len = len;
     flow_get_metadata(&ctx->xin->flow, &pin->fmd);
@@ -2119,8 +2121,8 @@ xlate_fin_timeout(struct xlate_ctx *ctx,
                   const struct ofpact_fin_timeout *oft)
 {
     if (ctx->xin->tcp_flags & (TCP_FIN | TCP_RST) && ctx->rule) {
-        ofproto_rule_reduce_timeouts(&ctx->rule->up, oft->fin_idle_timeout,
-                                     oft->fin_hard_timeout);
+        rule_dpif_reduce_timeouts(ctx->rule, oft->fin_idle_timeout,
+                                  oft->fin_hard_timeout);
     }
 }
 
@@ -2598,8 +2600,7 @@ xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
         ofpacts = xin->ofpacts;
         ofpacts_len = xin->ofpacts_len;
     } else if (xin->rule) {
-        ofpacts = xin->rule->up.ofpacts;
-        ofpacts_len = xin->rule->up.ofpacts_len;
+        rule_dpif_get_actions(xin->rule, &ofpacts, &ofpacts_len);
     } else {
         NOT_REACHED();
     }
index ba24e92..a54a9e4 100644 (file)
@@ -21,6 +21,8 @@
 #include "ofpbuf.h"
 #include "ofproto-dpif-mirror.h"
 #include "ofproto-dpif.h"
+#include "ofproto.h"
+#include "stp.h"
 
 struct bfd;
 struct bond;
index ed14600..cab7bc3 100644 (file)
@@ -83,7 +83,29 @@ BUILD_ASSERT_DECL(N_TABLES >= 2 && N_TABLES <= 255);
 struct flow_miss;
 struct facet;
 
+struct rule_dpif {
+    struct rule up;
+
+    /* These statistics:
+     *
+     *   - Do include packets and bytes from facets that have been deleted or
+     *     whose own statistics have been folded into the rule.
+     *
+     *   - Do include packets and bytes sent "by hand" that were accounted to
+     *     the rule without any facet being involved (this is a rare corner
+     *     case in rule_execute()).
+     *
+     *   - Do not include packet or bytes that can be obtained from any facet's
+     *     packet_count or byte_count member or that can be obtained from the
+     *     datapath by, e.g., dpif_flow_get() for any subfacet.
+     */
+    struct ovs_mutex stats_mutex;
+    uint64_t packet_count OVS_GUARDED;  /* Number of packets received. */
+    uint64_t byte_count OVS_GUARDED;    /* Number of bytes received. */
+};
+
 static void rule_get_stats(struct rule *, uint64_t *packets, uint64_t *bytes);
+static struct rule_dpif *rule_dpif_cast(const struct rule *);
 
 struct ofbundle {
     struct hmap_node hmap_node; /* In struct ofproto's "bundles" hmap. */
@@ -1359,7 +1381,7 @@ add_internal_flow(struct ofproto_dpif *ofproto, int id,
 
     if (rule_dpif_lookup_in_table(ofproto, &fm.match.flow, NULL, TBL_INTERNAL,
                                   rulep)) {
-        ovs_rwlock_unlock(&(*rulep)->up.evict);
+        rule_dpif_release(*rulep);
     } else {
         NOT_REACHED();
     }
@@ -4190,7 +4212,7 @@ facet_is_controller_flow(struct facet *facet)
         is_controller = ofpacts_len > 0
             && ofpacts->type == OFPACT_CONTROLLER
             && ofpact_next(ofpacts) >= ofpact_end(ofpacts, ofpacts_len);
-        rule_release(rule);
+        rule_dpif_release(rule);
         return is_controller;
     }
     return false;
@@ -4284,7 +4306,7 @@ facet_check_consistency(struct facet *facet)
     rule_dpif_lookup(facet->ofproto, &facet->flow, NULL, &rule);
     xlate_in_init(&xin, facet->ofproto, &facet->flow, rule, 0, NULL);
     xlate_actions(&xin, &xout);
-    rule_release(rule);
+    rule_dpif_release(rule);
 
     ok = ofpbuf_equal(&facet->xout.odp_actions, &xout.odp_actions)
         && facet->xout.slow == xout.slow;
@@ -4382,7 +4404,7 @@ facet_revalidate(struct facet *facet)
         || memcmp(&facet->xout.wc, &xout.wc, sizeof xout.wc)) {
         facet_remove(facet);
         xlate_out_uninit(&xout);
-        rule_release(new_rule);
+        rule_dpif_release(new_rule);
         return false;
     }
 
@@ -4414,7 +4436,7 @@ facet_revalidate(struct facet *facet)
     facet->used = MAX(facet->used, new_rule->up.created);
 
     xlate_out_uninit(&xout);
-    rule_release(new_rule);
+    rule_dpif_release(new_rule);
     return true;
 }
 
@@ -4442,12 +4464,12 @@ flow_push_stats(struct ofproto_dpif *ofproto, struct flow *flow,
     }
 
     rule_dpif_lookup(ofproto, flow, NULL, &rule);
-    rule_credit_stats(rule, stats);
+    rule_dpif_credit_stats(rule, stats);
     xlate_in_init(&xin, ofproto, flow, rule, stats->tcp_flags, NULL);
     xin.resubmit_stats = stats;
     xin.may_learn = may_learn;
     xlate_actions_for_side_effects(&xin);
-    rule_release(rule);
+    rule_dpif_release(rule);
 }
 
 static void
@@ -4513,7 +4535,8 @@ push_all_stats(void)
 }
 
 void
-rule_credit_stats(struct rule_dpif *rule, const struct dpif_flow_stats *stats)
+rule_dpif_credit_stats(struct rule_dpif *rule,
+                       const struct dpif_flow_stats *stats)
 {
     ovs_mutex_lock(&rule->stats_mutex);
     rule->packet_count += stats->n_packets;
@@ -4521,6 +4544,33 @@ rule_credit_stats(struct rule_dpif *rule, const struct dpif_flow_stats *stats)
     ofproto_rule_update_used(&rule->up, stats->used);
     ovs_mutex_unlock(&rule->stats_mutex);
 }
+
+bool
+rule_dpif_fail_open(const struct rule_dpif *rule)
+{
+    return rule->up.cr.priority == FAIL_OPEN_PRIORITY;
+}
+
+ovs_be64
+rule_dpif_get_flow_cookie(const struct rule_dpif *rule)
+{
+    return rule->up.flow_cookie;
+}
+
+void
+rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
+                     uint16_t hard_timeout)
+{
+    ofproto_rule_reduce_timeouts(&rule->up, idle_timeout, hard_timeout);
+}
+
+void
+rule_dpif_get_actions(const struct rule_dpif *rule,
+                      const struct ofpact **ofpacts, size_t *ofpacts_len)
+{
+    *ofpacts = rule->up.ofpacts;
+    *ofpacts_len = rule->up.ofpacts_len;
+}
 \f
 /* Subfacets. */
 
@@ -4789,9 +4839,8 @@ rule_dpif_lookup(struct ofproto_dpif *ofproto, const struct flow *flow,
                      flow->in_port.ofp_port);
     }
 
-    *rule = choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule,
-                             ofproto->no_packet_in_rule);
-    ovs_rwlock_rdlock(&(*rule)->up.evict);
+    choose_miss_rule(port ? port->up.pp.config : 0, ofproto->miss_rule,
+                     ofproto->no_packet_in_rule, rule);
 }
 
 bool
@@ -4846,15 +4895,17 @@ rule_dpif_lookup_in_table(struct ofproto_dpif *ofproto,
 /* Given a port configuration (specified as zero if there's no port), chooses
  * which of 'miss_rule' and 'no_packet_in_rule' should be used in case of a
  * flow table miss. */
-struct rule_dpif *
+void
 choose_miss_rule(enum ofputil_port_config config, struct rule_dpif *miss_rule,
-                 struct rule_dpif *no_packet_in_rule)
+                 struct rule_dpif *no_packet_in_rule, struct rule_dpif **rule)
+    OVS_NO_THREAD_SAFETY_ANALYSIS
 {
-    return config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule;
+    *rule = config & OFPUTIL_PC_NO_PACKET_IN ? no_packet_in_rule : miss_rule;
+    ovs_rwlock_rdlock(&(*rule)->up.evict);
 }
 
 void
-rule_release(struct rule_dpif *rule)
+rule_dpif_release(struct rule_dpif *rule)
     OVS_NO_THREAD_SAFETY_ANALYSIS
 {
     if (rule) {
@@ -4877,6 +4928,11 @@ complete_operation(struct rule_dpif *rule)
     }
 }
 
+static struct rule_dpif *rule_dpif_cast(const struct rule *rule)
+{
+    return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL;
+}
+
 static struct rule *
 rule_alloc(void)
 {
@@ -4953,7 +5009,7 @@ rule_dpif_execute(struct rule_dpif *rule, const struct flow *flow,
     struct xlate_in xin;
 
     dpif_flow_stats_extract(flow, packet, time_msec(), &stats);
-    rule_credit_stats(rule, &stats);
+    rule_dpif_credit_stats(rule, &stats);
 
     xlate_in_init(&xin, ofproto, flow, rule, stats.tcp_flags, packet);
     xin.resubmit_stats = &stats;
@@ -5574,7 +5630,7 @@ ofproto_trace(struct ofproto_dpif *ofproto, const struct flow *flow,
         xlate_out_uninit(&trace.xout);
     }
 
-    rule_release(rule);
+    rule_dpif_release(rule);
 }
 
 static void
index 15e58e9..eb724d5 100644 (file)
 
 #include "hmapx.h"
 #include "odp-util.h"
-#include "ofproto/ofproto-provider.h"
+#include "ofp-util.h"
 #include "ovs-thread.h"
 #include "timer.h"
 #include "util.h"
 #include "ovs-thread.h"
 
 union user_action_cookie;
+struct dpif_flow_stats;
 struct ofproto_dpif;
 struct ofport_dpif;
 struct dpif_backer;
+struct OVS_LOCKABLE rule_dpif;
 
 /* Ofproto-dpif -- DPIF based ofproto implementation.
  *
@@ -58,48 +60,36 @@ struct dpif_backer;
  *   Ofproto-dpif-xlate is responsible for translating translating OpenFlow
  *   actions into datapath actions. */
 
-struct rule_dpif {
-    struct rule up;
-
-    /* These statistics:
-     *
-     *   - Do include packets and bytes from facets that have been deleted or
-     *     whose own statistics have been folded into the rule.
-     *
-     *   - Do include packets and bytes sent "by hand" that were accounted to
-     *     the rule without any facet being involved (this is a rare corner
-     *     case in rule_execute()).
-     *
-     *   - Do not include packet or bytes that can be obtained from any facet's
-     *     packet_count or byte_count member or that can be obtained from the
-     *     datapath by, e.g., dpif_flow_get() for any subfacet.
-     */
-    struct ovs_mutex stats_mutex;
-    uint64_t packet_count OVS_GUARDED;  /* Number of packets received. */
-    uint64_t byte_count OVS_GUARDED;    /* Number of bytes received. */
-};
-
-static inline struct rule_dpif *rule_dpif_cast(const struct rule *rule)
-{
-    return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL;
-}
-
 void rule_dpif_lookup(struct ofproto_dpif *, const struct flow *,
                       struct flow_wildcards *, struct rule_dpif **rule)
-    OVS_ACQ_RDLOCK((*rule)->up.evict);
+    OVS_ACQ_RDLOCK(*rule);
 
 bool rule_dpif_lookup_in_table(struct ofproto_dpif *, const struct flow *,
                                struct flow_wildcards *, uint8_t table_id,
                                struct rule_dpif **rule)
-    OVS_TRY_RDLOCK(true, (*rule)->up.evict);
+    OVS_TRY_RDLOCK(true, *rule);
+
+    void rule_dpif_release(struct rule_dpif *rule) OVS_RELEASES(rule);
+
+    void rule_dpif_credit_stats(struct rule_dpif *rule ,
+                                const struct dpif_flow_stats *);
+
+bool rule_dpif_fail_open(const struct rule_dpif *rule);
+
+void rule_dpif_get_actions(const struct rule_dpif *rule,
+                           const struct ofpact **ofpacts,
+                           size_t *ofpacts_len);
 
-void rule_release(struct rule_dpif *rule) OVS_RELEASES(rule->up.evict);
+ovs_be64 rule_dpif_get_flow_cookie(const struct rule_dpif *rule);
 
-void rule_credit_stats(struct rule_dpif *, const struct dpif_flow_stats *);
+void rule_dpif_reduce_timeouts(struct rule_dpif *rule, uint16_t idle_timeout,
+                               uint16_t hard_timeout);
 
-struct rule_dpif *choose_miss_rule(enum ofputil_port_config,
-                                   struct rule_dpif *miss_rule,
-                                   struct rule_dpif *no_packet_in_rule);
+void choose_miss_rule(enum ofputil_port_config,
+                      struct rule_dpif *miss_rule,
+                      struct rule_dpif *no_packet_in_rule,
+                      struct rule_dpif **rule)
+    OVS_ACQ_RDLOCK(*rule);
 
 bool ofproto_has_vlan_splinters(const struct ofproto_dpif *);
 ofp_port_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,