diff mbox

[ovs-dev,v9,03/10] Make flow table persistent in ovn controller

Message ID 1457730385-28923-4-git-send-email-rmoats@us.ibm.com
State Changes Requested
Headers show

Commit Message

Ryan Moats March 11, 2016, 9:06 p.m. UTC
From: RYAN D. MOATS <rmoats@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@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(-)

Comments

Ben Pfaff March 22, 2016, 9:54 p.m. UTC | #1
On Fri, Mar 11, 2016 at 03:06:18PM -0600, Ryan Moats wrote:
> From: RYAN D. MOATS <rmoats@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@us.ibm.com>

This had a number of patch rejects so I'm reviewing it on the basis of
what I see, without trying to build or test it.  Therefore it's a pretty
superficial review; I'll look at it more carefully when it is rebased.

There's a number of instances of code similar to this, but I really
haven't got a clue what it really does or where the number 4 comes from:
> +        // this offset is to protect the hard coded rules in physical.c
> +        ins_seqno += 4;

It looks like ofpacts_hash() could just be a wrapper around
hash_bytes().

It seems like the main change here is to make lflow_run() iterate over
only the logical flows that have changed.  But I don't see how that can
work as-is, because OpenFlow flows are not a pure function of the
logical flows; rather, they have other inputs, such as the mapping from
logical ports to OpenFlow port numbers.  I don't see anything here that
makes sure that logical flows are revisited if this mapping changes.
Maybe this is in a later patch in the series, but we ordinarily require
each commit to be correct as a self-contained unit.
Ryan Moats March 23, 2016, 1:40 p.m. UTC | #2
Ben Pfaff <blp@ovn.org> wrote on 03/22/2016 04:54:16 PM:

> From: Ben Pfaff <blp@ovn.org>
> To: Ryan Moats/Omaha/IBM@IBMUS
> Cc: dev@openvswitch.org
> Date: 03/22/2016 04:54 PM
> Subject: Re: [ovs-dev,v9,03/10] Make flow table persistent in ovn
controller
>
> On Fri, Mar 11, 2016 at 03:06:18PM -0600, Ryan Moats wrote:
> > From: RYAN D. MOATS <rmoats@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@us.ibm.com>
>
> This had a number of patch rejects so I'm reviewing it on the basis of
> what I see, without trying to build or test it.  Therefore it's a pretty
> superficial review; I'll look at it more carefully when it is rebased.
>
> There's a number of instances of code similar to this, but I really
> haven't got a clue what it really does or where the number 4 comes from:
> > +        // this offset is to protect the hard coded rules in
physical.c
> > +        ins_seqno += 4;
>
> It looks like ofpacts_hash() could just be a wrapper around
> hash_bytes().
>
> It seems like the main change here is to make lflow_run() iterate over
> only the logical flows that have changed.  But I don't see how that can
> work as-is, because OpenFlow flows are not a pure function of the
> logical flows; rather, they have other inputs, such as the mapping from
> logical ports to OpenFlow port numbers.  I don't see anything here that
> makes sure that logical flows are revisited if this mapping changes.
> Maybe this is in a later patch in the series, but we ordinarily require
> each commit to be correct as a self-contained unit.

This is the first patch in the series that needs major rework as part of
the rebase and it was self-consistent when first submitted (in the sense
that it passed all of the unit tests) and it will be again before I send
an update.

As to the += 4 offset, this comes from the fact that when _TRACKED returns
a deleted row, you can't trust what is in the returned row as the memory
has already been freed). Therefore, all you have is the seqnos to go by.
Fortunately, the insert seqno is unique and constant, so we can use this
as a key for looking up the flow(s) that need to be removed - this is the
reason for all of the extra "seqno" hashmaps and related lookups.  However,
there are four OF flows added in physical.c that aren't driven by any ovnsb
rows (I refer to them as "hard-coded").  To avoid collisions, I assigned
these flows to insert seqno's 1, 2, 3, and 4 and then offset each row's
insert seqno received for a row by 4 to avoid collisions.

As to ofpacts_hash(), I wanted to use hash_bytes(), but I just couldn't
make it work reliably.  Since I'm rebasing this anyway, I'll give it
another pass.

Finally, this patch set doesn't iterate over only logical flows that
have changed - it still iterates over all of the logical flows during each
pass, so each flow is still revisited and updated as other inputs change.

Ryan (regXboi)
Ben Pfaff March 23, 2016, 4:21 p.m. UTC | #3
On Wed, Mar 23, 2016 at 07:40:08AM -0600, Ryan Moats wrote:
> Ben Pfaff <blp@ovn.org> wrote on 03/22/2016 04:54:16 PM:
> > There's a number of instances of code similar to this, but I really
> > haven't got a clue what it really does or where the number 4 comes from:
> > > +        // this offset is to protect the hard coded rules in
> physical.c
> > > +        ins_seqno += 4;
> >
> As to the += 4 offset, this comes from the fact that when _TRACKED returns
> a deleted row, you can't trust what is in the returned row as the memory
> has already been freed). Therefore, all you have is the seqnos to go by.
> Fortunately, the insert seqno is unique and constant, so we can use this
> as a key for looking up the flow(s) that need to be removed - this is the
> reason for all of the extra "seqno" hashmaps and related lookups.  However,
> there are four OF flows added in physical.c that aren't driven by any ovnsb
> rows (I refer to them as "hard-coded").  To avoid collisions, I assigned
> these flows to insert seqno's 1, 2, 3, and 4 and then offset each row's
> insert seqno received for a row by 4 to avoid collisions.

This sort of magic really needs in-code documentation.
diff mbox

Patch

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 702575d..36d80d0 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 */