[ovs-dev] [self-check 2/3] ofproto-dpif: Remove many redundant "struct ofproto_dpif *" parameters.

Ben Pfaff blp at nicira.com
Tue Dec 27 21:28:43 UTC 2011


It's redundant to pass both a facet or subfacet and an ofproto_dpif,
because the latter can be derived from the former.

Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 ofproto/ofproto-dpif.c |  138 +++++++++++++++++++++++++-----------------------
 1 files changed, 71 insertions(+), 67 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1a399c0..f131ebd 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -306,27 +306,25 @@ struct facet {
 };
 
 static struct facet *facet_create(struct rule_dpif *, const struct flow *);
-static void facet_remove(struct ofproto_dpif *, struct facet *);
+static void facet_remove(struct facet *);
 static void facet_free(struct facet *);
 
 static struct facet *facet_find(struct ofproto_dpif *, const struct flow *);
 static struct facet *facet_lookup_valid(struct ofproto_dpif *,
                                         const struct flow *);
-static bool facet_revalidate(struct ofproto_dpif *, struct facet *);
-
+static bool facet_revalidate(struct facet *);
 static bool execute_controller_action(struct ofproto_dpif *,
                                       const struct flow *,
                                       const struct nlattr *odp_actions,
                                       size_t actions_len,
                                       struct ofpbuf *packet, bool clone);
 
-static void facet_flush_stats(struct ofproto_dpif *, struct facet *);
+static void facet_flush_stats(struct facet *);
 
-static void facet_update_time(struct ofproto_dpif *, struct facet *,
-                              long long int used);
+static void facet_update_time(struct facet *, long long int used);
 static void facet_reset_counters(struct facet *);
 static void facet_push_stats(struct facet *);
-static void facet_account(struct ofproto_dpif *, struct facet *);
+static void facet_account(struct facet *);
 
 static bool facet_is_controller_flow(struct facet *);
 
@@ -368,26 +366,24 @@ struct subfacet {
     ovs_be16 initial_tci;       /* Initial VLAN TCI value. */
 };
 
-static struct subfacet *subfacet_create(struct ofproto_dpif *, struct facet *,
-                                        enum odp_key_fitness,
+static struct subfacet *subfacet_create(struct facet *, enum odp_key_fitness,
                                         const struct nlattr *key,
                                         size_t key_len, ovs_be16 initial_tci);
 static struct subfacet *subfacet_find(struct ofproto_dpif *,
                                       const struct nlattr *key, size_t key_len);
-static void subfacet_destroy(struct ofproto_dpif *, struct subfacet *);
-static void subfacet_destroy__(struct ofproto_dpif *, struct subfacet *);
+static void subfacet_destroy(struct subfacet *);
+static void subfacet_destroy__(struct subfacet *);
 static void subfacet_reset_dp_stats(struct subfacet *,
                                     struct dpif_flow_stats *);
-static void subfacet_update_time(struct ofproto_dpif *, struct subfacet *,
-                                 long long int used);
-static void subfacet_update_stats(struct ofproto_dpif *, struct subfacet *,
+static void subfacet_update_time(struct subfacet *, long long int used);
+static void subfacet_update_stats(struct subfacet *,
                                   const struct dpif_flow_stats *);
-static void subfacet_make_actions(struct ofproto_dpif *, struct subfacet *,
+static void subfacet_make_actions(struct subfacet *,
                                   const struct ofpbuf *packet);
-static int subfacet_install(struct ofproto_dpif *, struct subfacet *,
+static int subfacet_install(struct subfacet *,
                             const struct nlattr *actions, size_t actions_len,
                             struct dpif_flow_stats *);
-static void subfacet_uninstall(struct ofproto_dpif *, struct subfacet *);
+static void subfacet_uninstall(struct subfacet *);
 
 struct ofport_dpif {
     struct ofport up;
@@ -813,7 +809,7 @@ run(struct ofproto *ofproto_)
         HMAP_FOR_EACH_SAFE (facet, next, hmap_node, &ofproto->facets) {
             if (revalidate_all
                 || tag_set_intersects(&revalidate_set, facet->tags)) {
-                facet_revalidate(ofproto, facet);
+                facet_revalidate(facet);
             }
         }
     }
@@ -878,7 +874,7 @@ flush(struct ofproto *ofproto_)
             subfacet->dp_packet_count = 0;
             subfacet->dp_byte_count = 0;
         }
-        facet_remove(ofproto, facet);
+        facet_remove(facet);
     }
     dpif_flow_flush(ofproto->dpif);
 }
@@ -2538,7 +2534,7 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
         facet = facet_create(rule, flow);
     }
 
-    subfacet = subfacet_create(ofproto, facet,
+    subfacet = subfacet_create(facet,
                                miss->key_fitness, miss->key, miss->key_len,
                                miss->initial_tci);
 
@@ -2563,13 +2559,13 @@ handle_flow_miss(struct ofproto_dpif *ofproto, struct flow_miss *miss,
         }
 
         if (!facet->may_install || !subfacet->actions) {
-            subfacet_make_actions(ofproto, subfacet, packet);
+            subfacet_make_actions(subfacet, packet);
         }
 
         /* Credit statistics to subfacet for this packet.  We must do this now
          * because execute_controller_action() below may destroy 'packet'. */
         dpif_flow_stats_extract(&facet->flow, packet, &stats);
-        subfacet_update_stats(ofproto, subfacet, &stats);
+        subfacet_update_stats(subfacet, &stats);
 
         if (!execute_controller_action(ofproto, &facet->flow,
                                        subfacet->actions,
@@ -2906,8 +2902,8 @@ update_stats(struct ofproto_dpif *p)
             subfacet->dp_packet_count = stats->n_packets;
             subfacet->dp_byte_count = stats->n_bytes;
 
-            subfacet_update_time(p, subfacet, stats->used);
-            facet_account(p, facet);
+            subfacet_update_time(subfacet, stats->used);
+            facet_account(facet);
             facet_push_stats(facet);
         } else {
             if (!VLOG_DROP_WARN(&rl)) {
@@ -3025,7 +3021,7 @@ expire_subfacets(struct ofproto_dpif *ofproto, int dp_max_idle)
     HMAP_FOR_EACH_SAFE (subfacet, next_subfacet, hmap_node,
                         &ofproto->subfacets) {
         if (subfacet->used < cutoff) {
-            subfacet_destroy(ofproto, subfacet);
+            subfacet_destroy(subfacet);
         }
     }
 }
@@ -3035,7 +3031,6 @@ expire_subfacets(struct ofproto_dpif *ofproto, int dp_max_idle)
 static void
 rule_expire(struct rule_dpif *rule)
 {
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
     struct facet *facet, *next_facet;
     long long int now;
     uint8_t reason;
@@ -3057,7 +3052,7 @@ rule_expire(struct rule_dpif *rule)
     /* Update stats.  (This is a no-op if the rule expired due to an idle
      * timeout, because that only happens when the rule has no facets left.) */
     LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
-        facet_remove(ofproto, facet);
+        facet_remove(facet);
     }
 
     /* Get rid of the rule. */
@@ -3168,24 +3163,26 @@ execute_odp_actions(struct ofproto_dpif *ofproto, const struct flow *flow,
  *   - Removes 'facet' from its rule and from ofproto->facets.
  */
 static void
-facet_remove(struct ofproto_dpif *ofproto, struct facet *facet)
+facet_remove(struct facet *facet)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     struct subfacet *subfacet, *next_subfacet;
 
     LIST_FOR_EACH_SAFE (subfacet, next_subfacet, list_node,
                         &facet->subfacets) {
-        subfacet_destroy__(ofproto, subfacet);
+        subfacet_destroy__(subfacet);
     }
 
-    facet_flush_stats(ofproto, facet);
+    facet_flush_stats(facet);
     hmap_remove(&ofproto->facets, &facet->hmap_node);
     list_remove(&facet->list_node);
     facet_free(facet);
 }
 
 static void
-facet_account(struct ofproto_dpif *ofproto, struct facet *facet)
+facet_account(struct facet *facet)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     uint64_t n_bytes;
     struct subfacet *subfacet;
     const struct nlattr *a;
@@ -3269,8 +3266,9 @@ facet_is_controller_flow(struct facet *facet)
  * 'facet''s statistics in the datapath should have been zeroed and folded into
  * its packet and byte counts before this function is called. */
 static void
-facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
+facet_flush_stats(struct facet *facet)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     struct subfacet *subfacet;
 
     LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
@@ -3279,7 +3277,7 @@ facet_flush_stats(struct ofproto_dpif *ofproto, struct facet *facet)
     }
 
     facet_push_stats(facet);
-    facet_account(ofproto, facet);
+    facet_account(facet);
 
     if (ofproto->netflow && !facet_is_controller_flow(facet)) {
         struct ofexpired expired;
@@ -3334,7 +3332,7 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow)
     if (facet
         && (ofproto->need_revalidate
             || tag_set_intersects(&ofproto->revalidate_set, facet->tags))
-        && !facet_revalidate(ofproto, facet)) {
+        && !facet_revalidate(facet)) {
         COVERAGE_INC(facet_invalidated);
         return NULL;
     }
@@ -3342,7 +3340,7 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow)
     return facet;
 }
 
-/* Re-searches 'ofproto''s classifier for a rule matching 'facet':
+/* Re-searches the classifier for 'facet':
  *
  *   - If the rule found is different from 'facet''s current rule, moves
  *     'facet' to the new rule and recompiles its actions.
@@ -3354,8 +3352,9 @@ facet_lookup_valid(struct ofproto_dpif *ofproto, const struct flow *flow)
  *
  * Returns true if 'facet' still exists, false if it has been destroyed. */
 static bool
-facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
+facet_revalidate(struct facet *facet)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     struct actions {
         struct nlattr *odp_actions;
         size_t actions_len;
@@ -3374,7 +3373,7 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
     new_rule = rule_dpif_lookup(ofproto, &facet->flow, 0);
     if (!new_rule) {
         /* No new rule, so delete the facet. */
-        facet_remove(ofproto, facet);
+        facet_remove(facet);
         return false;
     }
 
@@ -3407,11 +3406,11 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
             if (should_install) {
                 struct dpif_flow_stats stats;
 
-                subfacet_install(ofproto, subfacet,
+                subfacet_install(subfacet,
                                  odp_actions->data, odp_actions->size, &stats);
-                subfacet_update_stats(ofproto, subfacet, &stats);
+                subfacet_update_stats(subfacet, &stats);
             } else {
-                subfacet_uninstall(ofproto, subfacet);
+                subfacet_uninstall(subfacet);
             }
 
             if (!new_actions) {
@@ -3427,7 +3426,7 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
         i++;
     }
     if (new_actions) {
-        facet_flush_stats(ofproto, facet);
+        facet_flush_stats(facet);
     }
 
     /* Update 'facet' now that we've taken care of all the old state. */
@@ -3464,9 +3463,9 @@ facet_revalidate(struct ofproto_dpif *ofproto, struct facet *facet)
 /* Updates 'facet''s used time.  Caller is responsible for calling
  * facet_push_stats() to update the flows which 'facet' resubmits into. */
 static void
-facet_update_time(struct ofproto_dpif *ofproto, struct facet *facet,
-                  long long int used)
+facet_update_time(struct facet *facet, long long int used)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     if (used > facet->used) {
         facet->used = used;
         if (used > facet->rule->used) {
@@ -3580,10 +3579,10 @@ subfacet_find__(struct ofproto_dpif *ofproto,
  * which case the caller must populate the actions with
  * subfacet_make_actions(). */
 static struct subfacet *
-subfacet_create(struct ofproto_dpif *ofproto, struct facet *facet,
-                enum odp_key_fitness key_fitness,
+subfacet_create(struct facet *facet, enum odp_key_fitness key_fitness,
                 const struct nlattr *key, size_t key_len, ovs_be16 initial_tci)
 {
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     uint32_t key_hash = odp_flow_key_hash(key, key_len);
     struct subfacet *subfacet;
 
@@ -3595,7 +3594,7 @@ subfacet_create(struct ofproto_dpif *ofproto, struct facet *facet,
 
         /* This shouldn't happen. */
         VLOG_ERR_RL(&rl, "subfacet with wrong facet");
-        subfacet_destroy(ofproto, subfacet);
+        subfacet_destroy(subfacet);
     }
 
     subfacet = xzalloc(sizeof *subfacet);
@@ -3635,9 +3634,12 @@ subfacet_find(struct ofproto_dpif *ofproto,
 /* Uninstalls 'subfacet' from the datapath, if it is installed, removes it from
  * its facet within 'ofproto', and frees it. */
 static void
-subfacet_destroy__(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
+subfacet_destroy__(struct subfacet *subfacet)
 {
-    subfacet_uninstall(ofproto, subfacet);
+    struct facet *facet = subfacet->facet;
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
+
+    subfacet_uninstall(subfacet);
     hmap_remove(&ofproto->subfacets, &subfacet->hmap_node);
     list_remove(&subfacet->list_node);
     free(subfacet->key);
@@ -3648,13 +3650,13 @@ subfacet_destroy__(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
 /* Destroys 'subfacet', as with subfacet_destroy__(), and then if this was the
  * last remaining subfacet in its facet destroys the facet too. */
 static void
-subfacet_destroy(struct ofproto_dpif *ofproto, struct subfacet *subfacet)
+subfacet_destroy(struct subfacet *subfacet)
 {
     struct facet *facet = subfacet->facet;
 
-    subfacet_destroy__(ofproto, subfacet);
+    subfacet_destroy__(subfacet);
     if (list_is_empty(&facet->subfacets)) {
-        facet_remove(ofproto, facet);
+        facet_remove(facet);
     }
 }
 
@@ -3675,15 +3677,15 @@ subfacet_get_key(struct subfacet *subfacet, struct odputil_keybuf *keybuf,
 
 /* Composes the datapath actions for 'subfacet' based on its rule's actions. */
 static void
-subfacet_make_actions(struct ofproto_dpif *p, struct subfacet *subfacet,
-                      const struct ofpbuf *packet)
+subfacet_make_actions(struct subfacet *subfacet, const struct ofpbuf *packet)
 {
     struct facet *facet = subfacet->facet;
     const struct rule_dpif *rule = facet->rule;
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
     struct ofpbuf *odp_actions;
     struct action_xlate_ctx ctx;
 
-    action_xlate_ctx_init(&ctx, p, &facet->flow, subfacet->initial_tci,
+    action_xlate_ctx_init(&ctx, ofproto, &facet->flow, subfacet->initial_tci,
                           packet);
     odp_actions = xlate_actions(&ctx, rule->up.actions, rule->up.n_actions);
     facet->tags = ctx.tags;
@@ -3710,10 +3712,12 @@ subfacet_make_actions(struct ofproto_dpif *p, struct subfacet *subfacet,
  *
  * Returns 0 if successful, otherwise a positive errno value. */
 static int
-subfacet_install(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
+subfacet_install(struct subfacet *subfacet,
                  const struct nlattr *actions, size_t actions_len,
                  struct dpif_flow_stats *stats)
 {
+    struct facet *facet = subfacet->facet;
+    struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     struct odputil_keybuf keybuf;
     enum dpif_flow_put_flags flags;
     struct ofpbuf key;
@@ -3737,19 +3741,21 @@ subfacet_install(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
 
 /* If 'subfacet' is installed in the datapath, uninstalls it. */
 static void
-subfacet_uninstall(struct ofproto_dpif *p, struct subfacet *subfacet)
+subfacet_uninstall(struct subfacet *subfacet)
 {
     if (subfacet->installed) {
+        struct rule_dpif *rule = subfacet->facet->rule;
+        struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
         struct odputil_keybuf keybuf;
         struct dpif_flow_stats stats;
         struct ofpbuf key;
         int error;
 
         subfacet_get_key(subfacet, &keybuf, &key);
-        error = dpif_flow_del(p->dpif, key.data, key.size, &stats);
+        error = dpif_flow_del(ofproto->dpif, key.data, key.size, &stats);
         subfacet_reset_dp_stats(subfacet, &stats);
         if (!error) {
-            subfacet_update_stats(p, subfacet, &stats);
+            subfacet_update_stats(subfacet, &stats);
         }
         subfacet->installed = false;
     } else {
@@ -3781,12 +3787,11 @@ subfacet_reset_dp_stats(struct subfacet *subfacet,
 /* Updates 'subfacet''s used time.  The caller is responsible for calling
  * facet_push_stats() to update the flows which 'subfacet' resubmits into. */
 static void
-subfacet_update_time(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
-                     long long int used)
+subfacet_update_time(struct subfacet *subfacet, long long int used)
 {
     if (used > subfacet->used) {
         subfacet->used = used;
-        facet_update_time(ofproto, subfacet->facet, used);
+        facet_update_time(subfacet->facet, used);
     }
 }
 
@@ -3797,13 +3802,13 @@ subfacet_update_time(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
  * represents a packet that was sent by hand or if it represents statistics
  * that have been cleared out of the datapath. */
 static void
-subfacet_update_stats(struct ofproto_dpif *ofproto, struct subfacet *subfacet,
+subfacet_update_stats(struct subfacet *subfacet,
                       const struct dpif_flow_stats *stats)
 {
     if (stats->n_packets || stats->used > subfacet->used) {
         struct facet *facet = subfacet->facet;
 
-        subfacet_update_time(ofproto, subfacet, stats->used);
+        subfacet_update_time(subfacet, stats->used);
         facet->packet_count += stats->n_packets;
         facet->byte_count += stats->n_bytes;
         facet_push_stats(facet);
@@ -3922,11 +3927,10 @@ static void
 rule_destruct(struct rule *rule_)
 {
     struct rule_dpif *rule = rule_dpif_cast(rule_);
-    struct ofproto_dpif *ofproto = ofproto_dpif_cast(rule->up.ofproto);
     struct facet *facet, *next_facet;
 
     LIST_FOR_EACH_SAFE (facet, next_facet, list_node, &rule->facets) {
-        facet_revalidate(ofproto, facet);
+        facet_revalidate(facet);
     }
 
     complete_operation(rule);
@@ -5522,9 +5526,9 @@ send_active_timeout(struct ofproto_dpif *ofproto, struct facet *facet)
             if (subfacet->installed) {
                 struct dpif_flow_stats stats;
 
-                subfacet_install(ofproto, subfacet, subfacet->actions,
+                subfacet_install(subfacet, subfacet->actions,
                                  subfacet->actions_len, &stats);
-                subfacet_update_stats(ofproto, subfacet, &stats);
+                subfacet_update_stats(subfacet, &stats);
             }
         }
 
-- 
1.7.2.5




More information about the dev mailing list