ofproto-dpif: Always maintain subfacet key.

Due to flow based tunneling, we can no longer assume that it's
possible to reconstruct a subfacet's key from its facet's flow.
The flow's in_port may be stale due to tunnel configuration
changes.

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

1 files changed, 16 insertions, 67 deletions

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index c8acdafe..aca77a6c 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -343,11 +343,6 @@ struct subfacet {
     struct list list_node;      /* In struct facet's 'facets' list. */
     struct facet *facet;        /* Owning facet. */
 
-    /* Key.
-     *
-     * To save memory in the common case, 'key' is NULL if 'key_fitness' is
-     * ODP_FIT_PERFECT, that is, odp_flow_key_from_flow() can accurately
-     * regenerate the ODP flow key from ->facet->flow. */
     enum odp_key_fitness key_fitness;
     struct nlattr *key;
     int key_len;
@@ -386,14 +381,11 @@ static struct subfacet *subfacet_create(struct facet *, struct flow_miss *miss,
                                         long long int now);
 static struct subfacet *subfacet_find(struct ofproto_dpif *,
                                       const struct nlattr *key, size_t key_len,
-                                      uint32_t key_hash,
-                                      const struct flow *flow);
+                                      uint32_t key_hash);
 static void subfacet_destroy(struct subfacet *);
 static void subfacet_destroy__(struct subfacet *);
 static void subfacet_destroy_batch(struct ofproto_dpif *,
                                    struct subfacet **, int n);
-static void subfacet_get_key(struct subfacet *, struct odputil_keybuf *,
-                             struct ofpbuf *key);
 static void subfacet_reset_dp_stats(struct subfacet *,
                                     struct dpif_flow_stats *);
 static void subfacet_update_time(struct subfacet *, long long int used);
@@ -4151,7 +4143,7 @@ update_stats(struct dpif_backer *backer)
         }
 
         key_hash = odp_flow_key_hash(key, key_len);
-        subfacet = subfacet_find(ofproto, key, key_len, key_hash, &flow);
+        subfacet = subfacet_find(ofproto, key, key_len, key_hash);
         switch (subfacet ? subfacet->path : SF_NOT_INSTALLED) {
         case SF_FAST_PATH:
             update_subfacet_stats(subfacet, stats);
@@ -4691,9 +4683,7 @@ facet_check_consistency(struct facet *facet)
     ofpbuf_use_stub(&odp_actions, odp_actions_stub, sizeof odp_actions_stub);
     LIST_FOR_EACH (subfacet, list_node, &facet->subfacets) {
         enum subfacet_path want_path;
-        struct odputil_keybuf keybuf;
         struct action_xlate_ctx ctx;
-        struct ofpbuf key;
         struct ds s;
 
         action_xlate_ctx_init(&ctx, ofproto, &facet->flow,
@@ -4729,8 +4719,7 @@ facet_check_consistency(struct facet *facet)
         }
 
         ds_init(&s);
-        subfacet_get_key(subfacet, &keybuf, &key);
-        odp_flow_key_format(key.data, key.size, &s);
+        odp_flow_key_format(subfacet->key, subfacet->key_len, &s);
 
         ds_put_cstr(&s, ": inconsistency in subfacet");
         if (want_path != subfacet->path) {
@@ -4946,17 +4935,14 @@ flow_push_stats(struct rule_dpif *rule,
 
 static struct subfacet *
 subfacet_find(struct ofproto_dpif *ofproto,
-              const struct nlattr *key, size_t key_len, uint32_t key_hash,
-              const struct flow *flow)
+              const struct nlattr *key, size_t key_len, uint32_t key_hash)
 {
     struct subfacet *subfacet;
 
     HMAP_FOR_EACH_WITH_HASH (subfacet, hmap_node, key_hash,
                              &ofproto->subfacets) {
-        if (subfacet->key
-            ? (subfacet->key_len == key_len
-               && !memcmp(key, subfacet->key, key_len))
-            : flow_equal(flow, &subfacet->facet->flow)) {
+        if (subfacet->key_len == key_len
+            && !memcmp(key, subfacet->key, key_len)) {
             return subfacet;
         }
     }
@@ -4988,8 +4974,7 @@ subfacet_create(struct facet *facet, struct flow_miss *miss,
     if (list_is_empty(&facet->subfacets)) {
         subfacet = &facet->one_subfacet;
     } else {
-        subfacet = subfacet_find(ofproto, key, key_len, key_hash,
-                                 &facet->flow);
+        subfacet = subfacet_find(ofproto, key, key_len, key_hash);
         if (subfacet) {
             if (subfacet->facet == facet) {
                 return subfacet;
@@ -5007,13 +4992,8 @@ subfacet_create(struct facet *facet, struct flow_miss *miss,
     list_push_back(&facet->subfacets, &subfacet->list_node);
     subfacet->facet = facet;
     subfacet->key_fitness = key_fitness;
-    if (key_fitness != ODP_FIT_PERFECT) {
-        subfacet->key = xmemdup(key, key_len);
-        subfacet->key_len = key_len;
-    } else {
-        subfacet->key = NULL;
-        subfacet->key_len = 0;
-    }
+    subfacet->key = xmemdup(key, key_len);
+    subfacet->key_len = key_len;
     subfacet->used = now;
     subfacet->dp_packet_count = 0;
     subfacet->dp_byte_count = 0;
@@ -5066,18 +5046,15 @@ static void
 subfacet_destroy_batch(struct ofproto_dpif *ofproto,
                        struct subfacet **subfacets, int n)
 {
-    struct odputil_keybuf keybufs[SUBFACET_DESTROY_MAX_BATCH];
     struct dpif_op ops[SUBFACET_DESTROY_MAX_BATCH];
     struct dpif_op *opsp[SUBFACET_DESTROY_MAX_BATCH];
-    struct ofpbuf keys[SUBFACET_DESTROY_MAX_BATCH];
     struct dpif_flow_stats stats[SUBFACET_DESTROY_MAX_BATCH];
     int i;
 
     for (i = 0; i < n; i++) {
         ops[i].type = DPIF_OP_FLOW_DEL;
-        subfacet_get_key(subfacets[i], &keybufs[i], &keys[i]);
-        ops[i].u.flow_del.key = keys[i].data;
-        ops[i].u.flow_del.key_len = keys[i].size;
+        ops[i].u.flow_del.key = subfacets[i]->key;
+        ops[i].u.flow_del.key_len = subfacets[i]->key_len;
         ops[i].u.flow_del.stats = &stats[i];
         opsp[i] = &ops[i];
     }
@@ -5090,24 +5067,6 @@ subfacet_destroy_batch(struct ofproto_dpif *ofproto,
     }
 }
 
-/* Initializes 'key' with the sequence of OVS_KEY_ATTR_* Netlink attributes
- * that can be used to refer to 'subfacet'.  The caller must provide 'keybuf'
- * for use as temporary storage. */
-static void
-subfacet_get_key(struct subfacet *subfacet, struct odputil_keybuf *keybuf,
-                 struct ofpbuf *key)
-{
-
-    if (!subfacet->key) {
-        struct flow *flow = &subfacet->facet->flow;
-
-        ofpbuf_use_stack(key, keybuf, sizeof *keybuf);
-        odp_flow_key_from_flow(key, flow, subfacet->odp_in_port);
-    } else {
-        ofpbuf_use_const(key, subfacet->key, subfacet->key_len);
-    }
-}
-
 /* Composes the datapath actions for 'subfacet' based on its rule's actions.
  * Translates the actions into 'odp_actions', which the caller must have
  * initialized and is responsible for uninitializing. */
@@ -5156,9 +5115,7 @@ subfacet_install(struct subfacet *subfacet,
     struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
     enum subfacet_path path = subfacet_want_path(slow);
     uint64_t slow_path_stub[128 / 8];
-    struct odputil_keybuf keybuf;
     enum dpif_flow_put_flags flags;
-    struct ofpbuf key;
     int ret;
 
     flags = DPIF_FP_CREATE | DPIF_FP_MODIFY;
@@ -5172,9 +5129,8 @@ subfacet_install(struct subfacet *subfacet,
                           &actions, &actions_len);
     }
 
-    subfacet_get_key(subfacet, &keybuf, &key);
-    ret = dpif_flow_put(ofproto->backer->dpif, flags, key.data, key.size,
-                        actions, actions_len, stats);
+    ret = dpif_flow_put(ofproto->backer->dpif, flags, subfacet->key,
+                        subfacet->key_len, actions, actions_len, stats);
 
     if (stats) {
         subfacet_reset_dp_stats(subfacet, stats);
@@ -5200,14 +5156,11 @@ subfacet_uninstall(struct subfacet *subfacet)
     if (subfacet->path != SF_NOT_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(ofproto->backer->dpif,
-                              key.data, key.size, &stats);
+        error = dpif_flow_del(ofproto->backer->dpif, subfacet->key,
+                              subfacet->key_len, &stats);
         subfacet_reset_dp_stats(subfacet, &stats);
         if (!error) {
             subfacet_update_stats(subfacet, &stats);
@@ -8034,11 +7987,7 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn,
     update_stats(ofproto->backer);
 
     HMAP_FOR_EACH (subfacet, hmap_node, &ofproto->subfacets) {
-        struct odputil_keybuf keybuf;
-        struct ofpbuf key;
-
-        subfacet_get_key(subfacet, &keybuf, &key);
-        odp_flow_key_format(key.data, key.size, &ds);
+        odp_flow_key_format(subfacet->key, subfacet->key_len, &ds);
 
         ds_put_format(&ds, ", packets:%"PRIu64", bytes:%"PRIu64", used:",
                       subfacet->dp_packet_count, subfacet->dp_byte_count);