[ovs-dev] [PATCH v8 3/3] Make flow table persistent in ovn controller

Ryan Moats us.ibm.com at oc7146733065.ibm.com
Wed Mar 9 19:44:27 UTC 2016


From: RYAN D. MOATS <rmoats at us.ibm.com>

This is a prerequisite for incremental processing.

Side effects:

1. Table rows are now tracked so that removed rows are correctly
   handled.
2. Hash by table id+priority+action added to help detect superseded
   flows.
3. Hash by insert seqno added to help find deleted flows.

Signed-off-by: RYAN D. MOATS <rmoats at us.ibm.com>
---
 lib/ofp-actions.c               |   12 ++
 lib/ofp-actions.h               |    2 +
 ovn/controller/lflow.c          |   30 +++-
 ovn/controller/lflow.h          |    3 +-
 ovn/controller/ofctrl.c         |  316 +++++++++++++++++++++++++++++---------
 ovn/controller/ofctrl.h         |   13 +-
 ovn/controller/ovn-controller.c |   12 +-
 ovn/controller/physical.c       |  105 ++++++++++---
 ovn/controller/physical.h       |    2 +-
 9 files changed, 377 insertions(+), 118 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index ae961f6..30cec2a 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -7309,6 +7309,18 @@ ofpacts_equal(const struct ofpact *a, size_t a_len,
     return a_len == b_len && !memcmp(a, b, a_len);
 }
 
+uint32_t
+ofpacts_hash(const struct ofpact *a, size_t a_len, uint32_t basis)
+{
+    size_t i;
+    uint32_t interim = basis;
+    for (i = 0; i < a_len; i += 4) {
+         uint32_t *term = (uint32_t *) ((uint8_t *)a+i);
+         interim = hash_add(*term, interim);
+    }
+    return hash_finish(interim, a_len);
+}
+
 /* Finds the OFPACT_METER action, if any, in the 'ofpacts_len' bytes of
  * 'ofpacts'.  If found, returns its meter ID; if not, returns 0.
  *
diff --git a/lib/ofp-actions.h b/lib/ofp-actions.h
index 24143d3..400ee48 100644
--- a/lib/ofp-actions.h
+++ b/lib/ofp-actions.h
@@ -885,6 +885,8 @@ bool ofpacts_output_to_group(const struct ofpact[], size_t ofpacts_len,
                              uint32_t group_id);
 bool ofpacts_equal(const struct ofpact a[], size_t a_len,
                    const struct ofpact b[], size_t b_len);
+uint32_t ofpacts_hash(const struct ofpact a[], size_t a_len, uint32_t basis);
+
 const struct mf_field *ofpact_get_mf_dst(const struct ofpact *ofpact);
 uint32_t ofpacts_get_meter(const struct ofpact[], size_t ofpacts_len);
 
diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c
index 33dca9b..a66dcd0 100644
--- a/ovn/controller/lflow.c
+++ b/ovn/controller/lflow.c
@@ -276,7 +276,7 @@ lflow_init(void)
 /* Translates logical flows in the Logical_Flow table in the OVN_SB database
  * into OpenFlow flows.  See ovn-architecture(7) for more information. */
 void
-lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
+lflow_run(struct controller_ctx *ctx,
           const struct simap *ct_zones,
           struct hmap *local_datapaths)
 {
@@ -286,7 +286,25 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
     ldp_run(ctx);
 
     const struct sbrec_logical_flow *lflow;
-    SBREC_LOGICAL_FLOW_FOR_EACH (lflow, ctx->ovnsb_idl) {
+    SBREC_LOGICAL_FLOW_FOR_EACH_TRACKED (lflow, ctx->ovnsb_idl) {
+        unsigned int del_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+            OVSDB_IDL_CHANGE_DELETE);
+        unsigned int mod_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+            OVSDB_IDL_CHANGE_MODIFY);
+        unsigned int ins_seqno = sbrec_logical_flow_row_get_seqno(lflow,
+            OVSDB_IDL_CHANGE_INSERT);
+        // this offset is to protect the hard coded rules in physical.c
+        ins_seqno += 4;
+
+        /* if the row has a del_seqno > 0, then trying to process the
+         * row isn't going to work (as it has already been freed).
+         * Therefore all we can do is to pass the ins_seqno to 
+         * ofctrl_remove_flow() to remove the flow */
+        if (del_seqno > 0) {
+            ofctrl_remove_flow(ins_seqno);
+            continue;
+        }
+
         /* Find the "struct logical_datapath" associated with this
          * Logical_Flow row.  If there's no such struct, that must be because
          * no logical ports are bound to that logical datapath, so there's no
@@ -400,8 +418,8 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
                 m->match.flow.conj_id += conj_id_ofs;
             }
             if (!m->n) {
-                ofctrl_add_flow(flow_table, ptable, lflow->priority,
-                                &m->match, &ofpacts);
+                ofctrl_add_flow(ptable, lflow->priority, &m->match, &ofpacts,
+                                ins_seqno, mod_seqno);
             } else {
                 uint64_t conj_stubs[64 / 8];
                 struct ofpbuf conj;
@@ -416,8 +434,8 @@ lflow_run(struct controller_ctx *ctx, struct hmap *flow_table,
                     dst->clause = src->clause;
                     dst->n_clauses = src->n_clauses;
                 }
-                ofctrl_add_flow(flow_table, ptable, lflow->priority,
-                                &m->match, &conj);
+                ofctrl_add_flow(ptable, lflow->priority, &m->match, &conj,
+                                ins_seqno, mod_seqno);
                 ofpbuf_uninit(&conj);
             }
         }
diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h
index ccbad30..e0e902c 100644
--- a/ovn/controller/lflow.h
+++ b/ovn/controller/lflow.h
@@ -56,8 +56,7 @@ struct uuid;
 #define LOG_PIPELINE_LEN 16
 
 void lflow_init(void);
-void lflow_run(struct controller_ctx *, struct hmap *flow_table,
-               const struct simap *ct_zones,
+void lflow_run(struct controller_ctx *, const struct simap *ct_zones,
                struct hmap *local_datapaths);
 void lflow_destroy(void);
 
diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 3297fb3..2479ca1 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -37,19 +37,28 @@ VLOG_DEFINE_THIS_MODULE(ofctrl);
 /* An OpenFlow flow. */
 struct ovn_flow {
     /* Key. */
-    struct hmap_node hmap_node;
+    struct hmap_node match_hmap_node; /* for match based hashing */
+    struct hmap_node action_hmap_node; /* for action based hashing */
+    struct hmap_node seqno_hmap_node; /* for seqno based hashing */
     uint8_t table_id;
     uint16_t priority;
-    struct match match;
+    unsigned int ins_seqno;
 
     /* Data. */
+    struct match match;
     struct ofpact *ofpacts;
     size_t ofpacts_len;
 };
 
-static uint32_t ovn_flow_hash(const struct ovn_flow *);
-static struct ovn_flow *ovn_flow_lookup(struct hmap *flow_table,
-                                        const struct ovn_flow *target);
+static uint32_t ovn_flow_match_hash(const struct ovn_flow *);
+static uint32_t ovn_flow_action_hash(const struct ovn_flow *);
+static uint32_t ovn_flow_seqno_hash(const struct ovn_flow *);
+static struct ovn_flow *ovn_flow_lookup_by_action(struct hmap *,
+    const struct ovn_flow *target);
+static struct ovn_flow *ovn_flow_lookup_by_match(struct hmap *,
+    const struct ovn_flow *target);
+static struct ovn_flow *ovn_flow_lookup_by_seqno(struct hmap *,
+    const struct ovn_flow *target);
 static char *ovn_flow_to_string(const struct ovn_flow *);
 static void ovn_flow_log(const struct ovn_flow *, const char *action);
 static void ovn_flow_destroy(struct ovn_flow *);
@@ -97,11 +106,15 @@ static struct hmap installed_flows;
  * S_CLEAR_FLOWS or S_UPDATE_FLOWS, this is really the option we have. */
 static enum mf_field_id mff_ovn_geneve;
 
-static void ovn_flow_table_clear(struct hmap *flow_table);
-static void ovn_flow_table_destroy(struct hmap *flow_table);
+static void ovn_flow_table_clear(void);
+static void ovn_flow_table_destroy(void);
 
 static void ofctrl_recv(const struct ofp_header *, enum ofptype);
 
+struct hmap match_flow_table = HMAP_INITIALIZER(&match_flow_table);
+struct hmap action_flow_table = HMAP_INITIALIZER(&action_flow_table);
+struct hmap seqno_flow_table = HMAP_INITIALIZER(&seqno_flow_table);
+
 void
 ofctrl_init(void)
 {
@@ -310,7 +323,7 @@ run_S_CLEAR_FLOWS(void)
     VLOG_DBG("clearing all flows");
 
     /* Clear installed_flows, to match the state of the switch. */
-    ovn_flow_table_clear(&installed_flows);
+    ovn_flow_table_clear();
 
     state = S_UPDATE_FLOWS;
 }
@@ -428,7 +441,7 @@ void
 ofctrl_destroy(void)
 {
     rconn_destroy(swconn);
-    ovn_flow_table_destroy(&installed_flows);
+    ovn_flow_table_destroy();
     rconn_packet_counter_destroy(tx_counter);
 }
 
@@ -461,63 +474,168 @@ ofctrl_recv(const struct ofp_header *oh, enum ofptype type)
     }
 }
 
-/* Flow table interface to the rest of ovn-controller. */
+/* Flow table interfaces to the rest of ovn-controller. */
 
-/* Adds a flow to 'desired_flows' with the specified 'match' and 'actions' to
+/* Adds a flow to flow tables with the specified 'match' and 'actions' to
  * the OpenFlow table numbered 'table_id' with the given 'priority'.  The
  * caller retains ownership of 'match' and 'actions'.
  *
- * This just assembles the desired flow table in memory.  Nothing is actually
+ * Because it is possible for both actions and matches to change on a rule,
+ * and because the hmap struct only supports a single hash, this method
+ * uses two hash maps - one that uses table_id+priority+matches for its hash
+ * and the other that uses table_id+priority+actions.
+ *
+ * This just assembles the desired flow tables in memory.  Nothing is actually
  * sent to the switch until a later call to ofctrl_run().
  *
- * The caller should initialize its own hmap to hold the flows. */
+ * The caller should initialize its own hmaps to hold the flows. */
 void
-ofctrl_add_flow(struct hmap *desired_flows,
-                uint8_t table_id, uint16_t priority,
-                const struct match *match, const struct ofpbuf *actions)
+ofctrl_add_flow(uint8_t table_id, uint16_t priority,
+                const struct match *match, const struct ofpbuf *actions,
+                unsigned int ins_seqno, unsigned int mod_seqno)
 {
+    // structure that uses table_id+priority+various things as hashes
     struct ovn_flow *f = xmalloc(sizeof *f);
     f->table_id = table_id;
     f->priority = priority;
     f->match = *match;
     f->ofpacts = xmemdup(actions->data, actions->size);
     f->ofpacts_len = actions->size;
-    f->hmap_node.hash = ovn_flow_hash(f);
-
-    if (ovn_flow_lookup(desired_flows, f)) {
-        static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
-        if (!VLOG_DROP_INFO(&rl)) {
-            char *s = ovn_flow_to_string(f);
-            VLOG_INFO("dropping duplicate flow: %s", s);
-            free(s);
+    f->ins_seqno = ins_seqno;
+    f->match_hmap_node.hash = ovn_flow_match_hash(f);
+    f->action_hmap_node.hash = ovn_flow_action_hash(f);
+    f->seqno_hmap_node.hash = ovn_flow_seqno_hash(f);
+
+    /* if mod_seqno > 0 then this is a modify operation, so look up
+     * the old flow via the match hash.  If you can't find it,
+     * then look up via the action hash. */
+   
+    if (mod_seqno > 0) {
+        struct ovn_flow *d = ovn_flow_lookup_by_match(&match_flow_table, f);
+        if (!d) {
+            d = ovn_flow_lookup_by_action(&action_flow_table, f);
         }
 
-        ovn_flow_destroy(f);
-        return;
+        if (d) {
+            hmap_remove(&match_flow_table, &d->match_hmap_node);
+            hmap_remove(&action_flow_table, &d->action_hmap_node);
+            hmap_remove(&seqno_flow_table, &d->seqno_hmap_node);
+            ovn_flow_destroy(d);
+        }
+    } else {
+        /* this is an insert operation, so check to see if this 
+         * is a duplicate via the match hash.  If so, then 
+         * check if the actions have changed.  If it is a complete
+         * duplicate (i.e. the actions are the same) drop the new
+         * flow. If not, then drop the old flow as superseded.
+         * If the new rule is not a duplicate, check the action
+         * hash to see if this flow is superseding a previous
+         * flow and if so, drop the old flow and insert the
+         * new one */
+
+        struct ovn_flow *d = ovn_flow_lookup_by_match(&match_flow_table, f);
+
+        if (d) {
+            if (ofpacts_equal(f->ofpacts, f->ofpacts_len,
+                              d->ofpacts, d->ofpacts_len)) {
+                ovn_flow_destroy(f);
+                return;
+            }
+            hmap_remove(&match_flow_table, &d->match_hmap_node);
+            hmap_remove(&action_flow_table, &d->action_hmap_node);
+            hmap_remove(&seqno_flow_table, &d->seqno_hmap_node);
+            ovn_flow_destroy(d);
+        } else {
+            d = ovn_flow_lookup_by_action(&action_flow_table, f);
+            if (d) {
+                hmap_remove(&match_flow_table, &d->match_hmap_node);
+                hmap_remove(&action_flow_table, &d->action_hmap_node);
+                hmap_remove(&seqno_flow_table, &d->seqno_hmap_node);
+                ovn_flow_destroy(d);
+            }
+        }
     }
+    hmap_insert(&match_flow_table, &f->match_hmap_node,
+                f->match_hmap_node.hash);
+    hmap_insert(&action_flow_table, &f->action_hmap_node,
+                f->action_hmap_node.hash);
+    hmap_insert(&seqno_flow_table, &f->seqno_hmap_node,
+                f->seqno_hmap_node.hash);
+}
+
+/* removes a flow from the flow table */
 
-    hmap_insert(desired_flows, &f->hmap_node, f->hmap_node.hash);
+void
+ofctrl_remove_flow(unsigned int ins_seqno)
+{
+    // structure that uses table_id+priority+various things as hashes
+    struct ovn_flow *f = xmalloc(sizeof *f);
+    f->ins_seqno = ins_seqno;
+    f->ofpacts = NULL;
+    f->seqno_hmap_node.hash = ovn_flow_seqno_hash(f);
+
+    struct ovn_flow *d = ovn_flow_lookup_by_seqno(&seqno_flow_table, f);
+    if (d) {
+        hmap_remove(&match_flow_table, &d->match_hmap_node);
+        hmap_remove(&action_flow_table, &d->action_hmap_node);
+        hmap_remove(&seqno_flow_table, &d->seqno_hmap_node);
+        ovn_flow_destroy(d);
+    }
+    ovn_flow_destroy(f);
 }
+
 
 /* ovn_flow. */
 
-/* Returns a hash of the key in 'f'. */
+/* duplicate an ovn_flow structure */
+struct ovn_flow *
+ofctrl_dup_flow(struct ovn_flow *source)
+{
+    struct ovn_flow *answer = xmalloc(sizeof *answer);
+    answer->table_id = source->table_id;
+    answer->priority = source->priority;
+    answer->match = source->match;
+    answer->ofpacts = xmemdup(source->ofpacts, source->ofpacts_len);
+    answer->ofpacts_len = source->ofpacts_len;
+    answer->ins_seqno = source->ins_seqno;
+    answer->match_hmap_node.hash = ovn_flow_match_hash(answer);
+    answer->action_hmap_node.hash = ovn_flow_action_hash(answer);
+    answer->seqno_hmap_node.hash = ovn_flow_seqno_hash(answer);
+    return answer;
+}
+
+/* Returns a hash of the match key in 'f'. */
 static uint32_t
-ovn_flow_hash(const struct ovn_flow *f)
+ovn_flow_match_hash(const struct ovn_flow *f)
 {
     return hash_2words((f->table_id << 16) | f->priority,
                        match_hash(&f->match, 0));
+}
+
+/* Returns a hash of the action key in 'f'. */
+static uint32_t
+ovn_flow_action_hash(const struct ovn_flow *f)
+{
+    return hash_2words((f->table_id << 16) | f->priority,
+                       ofpacts_hash(f->ofpacts, f->ofpacts_len, 0));
+}
 
+/* Returns a hash of the seqno key in 'f'. */
+static uint32_t
+ovn_flow_seqno_hash(const struct ovn_flow *f)
+{
+  return hash_int(f->ins_seqno, 8);
 }
 
 /* Finds and returns an ovn_flow in 'flow_table' whose key is identical to
- * 'target''s key, or NULL if there is none. */
+ * 'target''s key, or NULL if there is none, using the match hashmap. */
 static struct ovn_flow *
-ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target)
+ovn_flow_lookup_by_match(struct hmap* flow_table,
+                         const struct ovn_flow *target)
 {
     struct ovn_flow *f;
 
-    HMAP_FOR_EACH_WITH_HASH (f, hmap_node, target->hmap_node.hash,
+    HMAP_FOR_EACH_WITH_HASH (f, match_hmap_node, target->match_hmap_node.hash,
                              flow_table) {
         if (f->table_id == target->table_id
             && f->priority == target->priority
@@ -528,6 +646,53 @@ ovn_flow_lookup(struct hmap *flow_table, const struct ovn_flow *target)
     return NULL;
 }
 
+/* Finds and returns an ovn_flow in 'flow_table' whose key is identical to
+ * 'target''s key, or NULL if there is none, using the seqno hashmap. */
+static struct ovn_flow *
+ovn_flow_lookup_by_seqno(struct hmap* flow_table,
+                         const struct ovn_flow *target)
+{
+    struct ovn_flow *f;
+
+    HMAP_FOR_EACH_WITH_HASH (f, seqno_hmap_node, target->seqno_hmap_node.hash,
+                             flow_table) {
+        if (f->ins_seqno == target->ins_seqno) {
+            return f;
+        }
+    }
+    return NULL;
+}
+
+/* Finds and returns an ovn_flow in 'flow_table' whose key is identical to
+ * 'target''s key, or NULL if there is none, using the action hashmap. 
+ * Bescaue this hashmap is fairly coarse, we look for an */
+static struct ovn_flow *
+ovn_flow_lookup_by_action(struct hmap* flow_table,
+                          const struct ovn_flow *target)
+{
+    struct ovn_flow *f;
+
+    HMAP_FOR_EACH_WITH_HASH (f, action_hmap_node,
+                             target->action_hmap_node.hash, flow_table) {
+        if (f->table_id == target->table_id
+            && f->priority == target->priority
+            && (match_equal(&f->match, &target->match)
+                || ((flow_wildcards_has_extra(&f->match.wc,
+                                              &target->match.wc)
+                     && flow_equal_except(&f->match.flow,
+                                          &target->match.flow,
+                                          &f->match.wc))
+                    || (flow_wildcards_has_extra(&target->match.wc,
+                                                 &f->match.wc)
+                        && flow_equal_except(&target->match.flow,
+                                             &f->match.flow,
+                                             &target->match.wc))))) {
+            return f;
+        }
+    }
+    return NULL;
+}
+
 static char *
 ovn_flow_to_string(const struct ovn_flow *f)
 {
@@ -554,7 +719,9 @@ static void
 ovn_flow_destroy(struct ovn_flow *f)
 {
     if (f) {
-        free(f->ofpacts);
+        if (f->ofpacts) {
+            free(f->ofpacts);
+        }
         free(f);
     }
 }
@@ -562,20 +729,24 @@ ovn_flow_destroy(struct ovn_flow *f)
 /* Flow tables of struct ovn_flow. */
 
 static void
-ovn_flow_table_clear(struct hmap *flow_table)
+ovn_flow_table_clear(void)
 {
     struct ovn_flow *f, *next;
-    HMAP_FOR_EACH_SAFE (f, next, hmap_node, flow_table) {
-        hmap_remove(flow_table, &f->hmap_node);
+    HMAP_FOR_EACH_SAFE (f, next, match_hmap_node, &match_flow_table) {
+        hmap_remove(&match_flow_table, &f->match_hmap_node);
+        hmap_remove(&action_flow_table, &f->action_hmap_node);
+        hmap_remove(&seqno_flow_table, &f->seqno_hmap_node);
         ovn_flow_destroy(f);
     }
 }
 
 static void
-ovn_flow_table_destroy(struct hmap *flow_table)
+ovn_flow_table_destroy(void)
 {
-    ovn_flow_table_clear(flow_table);
-    hmap_destroy(flow_table);
+    ovn_flow_table_clear();
+    hmap_destroy(&match_flow_table);
+    hmap_destroy(&action_flow_table);
+    hmap_destroy(&seqno_flow_table);
 }
 
 /* Flow table update. */
@@ -595,19 +766,16 @@ queue_flow_mod(struct ofputil_flow_mod *fm)
  * flows from 'flow_table' and frees them.  (The hmap itself isn't
  * destroyed.)
  *
- * This called be called be ofctrl_run() within the main loop. */
+ * This can be called by ofctrl_run() within the main loop. */
 void
-ofctrl_put(struct hmap *flow_table)
+ofctrl_put(void)
 {
     /* The flow table can be updated if the connection to the switch is up and
      * in the correct state and not backlogged with existing flow_mods.  (Our
      * criteria for being backlogged appear very conservative, but the socket
-     * between ovn-controller and OVS provides some buffering.)  Otherwise,
-     * discard the flows.  A solution to either of those problems will cause us
-     * to wake up and retry. */
+     * between ovn-controller and OVS provides some buffering.) */
     if (state != S_UPDATE_FLOWS
         || rconn_packet_counter_n_packets(tx_counter)) {
-        ovn_flow_table_clear(flow_table);
         return;
     }
 
@@ -615,8 +783,8 @@ ofctrl_put(struct hmap *flow_table)
      * longer desired, delete them; if any of them should have different
      * actions, update them. */
     struct ovn_flow *i, *next;
-    HMAP_FOR_EACH_SAFE (i, next, hmap_node, &installed_flows) {
-        struct ovn_flow *d = ovn_flow_lookup(flow_table, i);
+    HMAP_FOR_EACH_SAFE (i, next, match_hmap_node, &installed_flows) {
+        struct ovn_flow *d = ovn_flow_lookup_by_match(&match_flow_table, i);
         if (!d) {
             /* Installed flow is no longer desirable.  Delete it from the
              * switch and from installed_flows. */
@@ -627,9 +795,9 @@ ofctrl_put(struct hmap *flow_table)
                 .command = OFPFC_DELETE_STRICT,
             };
             queue_flow_mod(&fm);
-            ovn_flow_log(i, "removing");
+            ovn_flow_log(i, "removing installed");
 
-            hmap_remove(&installed_flows, &i->hmap_node);
+            hmap_remove(&installed_flows, &i->match_hmap_node);
             ovn_flow_destroy(i);
         } else {
             if (!ofpacts_equal(i->ofpacts, i->ofpacts_len,
@@ -644,40 +812,38 @@ ofctrl_put(struct hmap *flow_table)
                     .command = OFPFC_MODIFY_STRICT,
                 };
                 queue_flow_mod(&fm);
-                ovn_flow_log(i, "updating");
+                ovn_flow_log(i, "updating installed");
 
                 /* Replace 'i''s actions by 'd''s. */
                 free(i->ofpacts);
-                i->ofpacts = d->ofpacts;
+                i->ofpacts = xmemdup(d->ofpacts, d->ofpacts_len);
                 i->ofpacts_len = d->ofpacts_len;
-                d->ofpacts = NULL;
-                d->ofpacts_len = 0;
             }
-
-            hmap_remove(flow_table, &d->hmap_node);
-            ovn_flow_destroy(d);
         }
     }
 
-    /* The previous loop removed from 'flow_table' all of the flows that are
-     * already installed.  Thus, any flows remaining in 'flow_table' need to
-     * be added to the flow table. */
+    /* Iterate through the new flows and add those that aren't found
+     * in the installed flow table */
     struct ovn_flow *d;
-    HMAP_FOR_EACH_SAFE (d, next, hmap_node, flow_table) {
-        /* Send flow_mod to add flow. */
-        struct ofputil_flow_mod fm = {
-            .match = d->match,
-            .priority = d->priority,
-            .table_id = d->table_id,
-            .ofpacts = d->ofpacts,
-            .ofpacts_len = d->ofpacts_len,
-            .command = OFPFC_ADD,
-        };
-        queue_flow_mod(&fm);
-        ovn_flow_log(d, "adding");
-
-        /* Move 'd' from 'flow_table' to installed_flows. */
-        hmap_remove(flow_table, &d->hmap_node);
-        hmap_insert(&installed_flows, &d->hmap_node, d->hmap_node.hash);
+    HMAP_FOR_EACH_SAFE (d, next, match_hmap_node, &match_flow_table) {
+        struct ovn_flow *i = ovn_flow_lookup_by_match(&installed_flows, d);
+        if (!i) {
+            /* Send flow_mod to add flow. */
+            struct ofputil_flow_mod fm = {
+                .match = d->match,
+                .priority = d->priority,
+                .table_id = d->table_id,
+                .ofpacts = d->ofpacts,
+                .ofpacts_len = d->ofpacts_len,
+                .command = OFPFC_ADD,
+            };
+            queue_flow_mod(&fm);
+            ovn_flow_log(d, "adding installed");
+
+            /* Copy 'd' from 'flow_table' to installed_flows. */
+            struct ovn_flow *new_node = ofctrl_dup_flow(d);
+            hmap_insert(&installed_flows, &new_node->match_hmap_node,
+                        new_node->match_hmap_node.hash);
+        }
     }
 }
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index 93ef8ea..4ae0d42 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -30,12 +30,17 @@ struct ovsrec_bridge;
 /* Interface for OVN main loop. */
 void ofctrl_init(void);
 enum mf_field_id ofctrl_run(const struct ovsrec_bridge *br_int);
-void ofctrl_put(struct hmap *flows);
+void ofctrl_put(void);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 
-/* Flow table interface to the rest of ovn-controller. */
-void ofctrl_add_flow(struct hmap *flows, uint8_t table_id, uint16_t priority,
-                     const struct match *, const struct ofpbuf *ofpacts);
+struct ovn_flow *ofctrl_dup_flow(struct ovn_flow *source);
+
+/* Flow table interfaces to the rest of ovn-controller. */
+void ofctrl_add_flow(uint8_t table_id, uint16_t priority,
+                     const struct match *, const struct ofpbuf *ofpacts,
+                     unsigned int ins_seqno, unsigned int mod_seqno);
+
+void ofctrl_remove_flow(unsigned int ins_seqno);
 
 #endif /* ovn/ofctrl.h */
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index f5769b5..8f3873d 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -259,6 +259,10 @@ main(int argc, char *argv[])
     char *ovnsb_remote = get_ovnsb_remote(ovs_idl_loop.idl);
     struct ovsdb_idl_loop ovnsb_idl_loop = OVSDB_IDL_LOOP_INITIALIZER(
         ovsdb_idl_create(ovnsb_remote, &sbrec_idl_class, true, true));
+
+    /* track the southbound idl */
+    ovsdb_idl_track_add_all(ovnsb_idl_loop.idl);
+
     ovsdb_idl_get_initial_snapshot(ovnsb_idl_loop.idl);
 
     /* Initialize connection tracking zones. */
@@ -299,15 +303,13 @@ main(int argc, char *argv[])
 
             pinctrl_run(&ctx, br_int);
 
-            struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
-            lflow_run(&ctx, &flow_table, &ct_zones, &local_datapaths);
+            lflow_run(&ctx, &ct_zones, &local_datapaths);
             if (chassis_id) {
                 physical_run(&ctx, mff_ovn_geneve,
-                             br_int, chassis_id, &ct_zones, &flow_table,
+                             br_int, chassis_id, &ct_zones, 
                              &local_datapaths);
             }
-            ofctrl_put(&flow_table);
-            hmap_destroy(&flow_table);
+            ofctrl_put();
         }
 
         struct local_datapath *cur_node, *next_node;
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 657c3e2..f86e2f5 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -147,7 +147,7 @@ get_localnet_port(struct hmap *local_datapaths, int64_t tunnel_key)
 void
 physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
              const struct ovsrec_bridge *br_int, const char *this_chassis_id,
-             const struct simap *ct_zones, struct hmap *flow_table,
+             const struct simap *ct_zones,
              struct hmap *local_datapaths)
 {
     struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport);
@@ -231,7 +231,25 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     /* Set up flows in table 0 for physical-to-logical translation and in table
      * 64 for logical-to-physical translation. */
     const struct sbrec_port_binding *binding;
-    SBREC_PORT_BINDING_FOR_EACH (binding, ctx->ovnsb_idl) {
+    SBREC_PORT_BINDING_FOR_EACH_TRACKED (binding, ctx->ovnsb_idl) {
+        unsigned int del_seqno = sbrec_port_binding_row_get_seqno(binding,
+            OVSDB_IDL_CHANGE_DELETE);
+        unsigned int mod_seqno = sbrec_port_binding_row_get_seqno(binding,
+            OVSDB_IDL_CHANGE_MODIFY);
+        unsigned int ins_seqno = sbrec_port_binding_row_get_seqno(binding,
+            OVSDB_IDL_CHANGE_INSERT);
+        // this offset is to protect the hard coded rules below
+        ins_seqno += 4;
+
+        /* if the row has a del_seqno > 0, then trying to process the
+         * row isn't going to work (as it has already been freed).
+         * Therefore all we can do is to pass the ins_seqno to 
+         * ofctrl_remove_flow() to remove the flow */
+        if (del_seqno > 0) {
+            ofctrl_remove_flow(ins_seqno);
+            continue;
+        }
+
         /* Find the OpenFlow port for the logical port, as 'ofport'.  This is
          * one of:
          *
@@ -347,8 +365,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
             /* Resubmit to first logical ingress pipeline table. */
             put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
-            ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG,
-                            tag ? 150 : 100, &match, &ofpacts);
+            ofctrl_add_flow(OFTABLE_PHY_TO_LOG,
+                            tag ? 150 : 100, &match, &ofpacts,
+                            ins_seqno, mod_seqno);
 
             if (!tag && !strcmp(binding->type, "localnet")) {
                 /* Add a second flow for frames that lack any 802.1Q
@@ -356,7 +375,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                  * action. */
                 ofpbuf_pull(&ofpacts, ofpacts_orig_size);
                 match_set_dl_tci_masked(&match, 0, htons(VLAN_CFI));
-                ofctrl_add_flow(flow_table, 0, 100, &match, &ofpacts);
+                ofctrl_add_flow(0, 100, &match, &ofpacts,
+                                ins_seqno, mod_seqno);
             }
 
             /* Table 33, priority 100.
@@ -381,8 +401,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
             /* Resubmit to table 34. */
             put_resubmit(OFTABLE_DROP_LOOPBACK, &ofpacts);
-            ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, &match,
-                            &ofpacts);
+            ofctrl_add_flow(OFTABLE_LOCAL_OUTPUT,
+                            100, &match, &ofpacts,
+                            ins_seqno, mod_seqno);
 
             /* Table 64, Priority 100.
              * =======================
@@ -417,8 +438,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 ofpact_put_STRIP_VLAN(&ofpacts);
                 put_stack(MFF_IN_PORT, ofpact_put_STACK_POP(&ofpacts));
             }
-            ofctrl_add_flow(flow_table, OFTABLE_LOG_TO_PHY, 100,
-                            &match, &ofpacts);
+            ofctrl_add_flow(OFTABLE_LOG_TO_PHY, 100,
+                            &match, &ofpacts, ins_seqno, mod_seqno);
         } else if (!tun) {
             /* Remote port connected by localnet port */
             /* Table 33, priority 100.
@@ -441,8 +462,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
             /* Resubmit to table 33. */
             put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
-            ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100, &match,
-                            &ofpacts);
+            ofctrl_add_flow(OFTABLE_LOCAL_OUTPUT, 100, &match,
+                            &ofpacts, ins_seqno, mod_seqno);
         } else {
             /* Remote port connected by tunnel */
             /* Table 32, priority 100.
@@ -466,8 +487,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
             /* Output to tunnel. */
             ofpact_put_OUTPUT(&ofpacts)->port = ofport;
-            ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
-                            &match, &ofpacts);
+            ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT,
+                            100, &match, &ofpacts,
+                            ins_seqno, mod_seqno);
         }
 
         /* Table 34, Priority 100.
@@ -479,15 +501,34 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         match_set_metadata(&match, htonll(binding->datapath->tunnel_key));
         match_set_reg(&match, MFF_LOG_INPORT - MFF_REG0, binding->tunnel_key);
         match_set_reg(&match, MFF_LOG_OUTPORT - MFF_REG0, binding->tunnel_key);
-        ofctrl_add_flow(flow_table, OFTABLE_DROP_LOOPBACK, 100,
-                        &match, &ofpacts);
+        ofctrl_add_flow(OFTABLE_DROP_LOOPBACK, 100,
+                        &match, &ofpacts,
+                        ins_seqno, mod_seqno);
     }
 
     /* Handle output to multicast groups, in tables 32 and 33. */
     const struct sbrec_multicast_group *mc;
     struct ofpbuf remote_ofpacts;
     ofpbuf_init(&remote_ofpacts, 0);
-    SBREC_MULTICAST_GROUP_FOR_EACH (mc, ctx->ovnsb_idl) {
+    SBREC_MULTICAST_GROUP_FOR_EACH_TRACKED (mc, ctx->ovnsb_idl) {
+        unsigned int del_seqno = sbrec_multicast_group_row_get_seqno(mc,
+            OVSDB_IDL_CHANGE_DELETE);
+        unsigned int mod_seqno = sbrec_multicast_group_row_get_seqno(mc,
+            OVSDB_IDL_CHANGE_MODIFY);
+        unsigned int ins_seqno = sbrec_multicast_group_row_get_seqno(mc,
+            OVSDB_IDL_CHANGE_INSERT);
+        // this offset is to protect the hard coded rules below
+        ins_seqno += 4;
+
+        /* if the row has a del_seqno > 0, then trying to process the
+         * row isn't going to work (as it has already been freed).
+         * Therefore all we can do is to pass the ins_seqno to 
+         * ofctrl_remove_flow() to remove the flow */
+        if (del_seqno > 0) {
+            ofctrl_remove_flow(ins_seqno);
+            continue;
+        }
+
         struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
         struct match match;
 
@@ -554,8 +595,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
              * group as the logical output port. */
             put_load(mc->tunnel_key, MFF_LOG_OUTPORT, 0, 32, &ofpacts);
 
-            ofctrl_add_flow(flow_table, OFTABLE_LOCAL_OUTPUT, 100,
-                            &match, &ofpacts);
+            ofctrl_add_flow(OFTABLE_LOCAL_OUTPUT,
+                            100, &match, &ofpacts,
+                            ins_seqno, mod_seqno);
         }
 
         /* Table 32, priority 100.
@@ -592,8 +634,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 if (local_ports) {
                     put_resubmit(OFTABLE_LOCAL_OUTPUT, &remote_ofpacts);
                 }
-                ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 100,
-                                &match, &remote_ofpacts);
+                ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 100,
+                                &match, &remote_ofpacts,
+                                ins_seqno, mod_seqno);
             }
         }
         sset_destroy(&remote_chassis);
@@ -636,7 +679,10 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
         put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
 
-        ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts);
+        /* note: we hardcode the insert sequence number to 1 to 
+         * avoid collisions */
+        ofctrl_add_flow(OFTABLE_PHY_TO_LOG, 100, &match, &ofpacts,
+                        1, 0);
     }
 
     /* Add flows for VXLAN encapsulations.  Due to the limited amount of
@@ -669,8 +715,11 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
             put_load(binding->tunnel_key, MFF_LOG_INPORT, 0, 15, &ofpacts);
             put_resubmit(OFTABLE_LOG_INGRESS_PIPELINE, &ofpacts);
 
-            ofctrl_add_flow(flow_table, OFTABLE_PHY_TO_LOG, 100, &match,
-                    &ofpacts);
+            /* note: we hardcode the insert sequence number to 2 to 
+             * avoid collisions */
+            ofctrl_add_flow(OFTABLE_PHY_TO_LOG, 100,
+                            &match, &ofpacts,
+                            2, 0);
         }
     }
 
@@ -683,7 +732,10 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     match_init_catchall(&match);
     ofpbuf_clear(&ofpacts);
     put_resubmit(OFTABLE_LOCAL_OUTPUT, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts);
+    /* note: we hardcode the insert sequence number to 3 to 
+     * avoid collisions */
+    ofctrl_add_flow(OFTABLE_REMOTE_OUTPUT, 0, &match, &ofpacts,
+                    3, 0);
 
     /* Table 34, Priority 0.
      * =======================
@@ -697,7 +749,10 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     MFF_LOG_REGS;
 #undef MFF_LOG_REGS
     put_resubmit(OFTABLE_LOG_EGRESS_PIPELINE, &ofpacts);
-    ofctrl_add_flow(flow_table, OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts);
+    /* note: we hardcode the insert sequence number to 4 to 
+     * avoid collisions */
+    ofctrl_add_flow(OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts,
+                    4, 0);
 
     ofpbuf_uninit(&ofpacts);
     simap_destroy(&localvif_to_ofport);
diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index 826b99b..1bea6bd 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -43,7 +43,7 @@ struct simap;
 void physical_register_ovs_idl(struct ovsdb_idl *);
 void physical_run(struct controller_ctx *, enum mf_field_id mff_ovn_geneve,
                   const struct ovsrec_bridge *br_int, const char *chassis_id,
-                  const struct simap *ct_zones, struct hmap *flow_table,
+                  const struct simap *ct_zones,
                   struct hmap *local_datapaths);
 
 #endif /* ovn/physical.h */
-- 
1.7.1




More information about the dev mailing list