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

Ryan Moats rmoats at us.ibm.com
Wed Feb 17 18:42:38 UTC 2016


This is a prerequisite for incremental processing.

Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
---
 ovn/controller/ofctrl.c         |  118 +++++++++++++++++++++++++++------------
 ovn/controller/ofctrl.h         |    2 +
 ovn/controller/ovn-controller.c |    4 +-
 3 files changed, 87 insertions(+), 37 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 3297fb3..7280c8b 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -484,16 +484,53 @@ ofctrl_add_flow(struct hmap *desired_flows,
     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);
-        }
+    /* loop through all the flows to see if there is an old flow to be
+     * removed - do so if the old flow has the same priority, table, and match
+     * but a different action or if the old flow has the same priority, table
+     * and action, but the new match is either a superset or subset of the
+     * old match */
+
+    struct ovn_flow *d, *next;
+    HMAP_FOR_EACH_SAFE (d, next, hmap_node, desired_flows) {
+        if (f->table_id == d->table_id && f->priority == d->priority) {
+            if ((match_equal(&f->match, &d->match)
+                 && !ofpacts_equal(f->ofpacts, f->ofpacts_len,
+                                   d->ofpacts, d->ofpacts_len))
+                || (ofpacts_equal(f->ofpacts, f->ofpacts_len,
+                                  d->ofpacts, d->ofpacts_len)
+                    && ((flow_wildcards_has_extra(&f->match.wc,&d->match.wc)
+                         && flow_equal_except(&f->match.flow, &d->match.flow,
+                                              &f->match.wc))
+                        || (flow_wildcards_has_extra(&d->match.wc,&f->match.wc)
+                            && flow_equal_except(&d->match.flow,
+                                                 &f->match.flow,
+                                                 &d->match.wc))))) {
+                static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
+                if (!VLOG_DROP_INFO(&rl)) {
+                    char *s = ovn_flow_to_string(d);
+                    VLOG_INFO("removing superceded flow: %s", s);
+                    free(s);
+                }
 
-        ovn_flow_destroy(f);
-        return;
+                hmap_remove(desired_flows, &d->hmap_node);
+                ovn_flow_destroy(d);
+            }
+
+            /* if this is a complete duplicate, remove the new flow */
+            if (match_equal(&f->match, &d->match)
+                && ofpacts_equal(f->ofpacts, f->ofpacts_len,
+                                 d->ofpacts, d->ofpacts_len)) {
+                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);
+                }
+
+                ovn_flow_destroy(f);
+                return;
+            }
+        }
     }
 
     hmap_insert(desired_flows, &f->hmap_node, f->hmap_node.hash);
@@ -501,6 +538,20 @@ ofctrl_add_flow(struct hmap *desired_flows,
 
 /* ovn_flow. */
 
+/* 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->hmap_node.hash = ovn_flow_hash(answer);
+    return answer;
+}
+
 /* Returns a hash of the key in 'f'. */
 static uint32_t
 ovn_flow_hash(const struct ovn_flow *f)
@@ -595,19 +646,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)
 {
     /* 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;
     }
 
@@ -653,31 +701,31 @@ ofctrl_put(struct hmap *flow_table)
                 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);
+        struct ovn_flow *i = ovn_flow_lookup(&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");
+
+            /* Copy 'd' from 'flow_table' to installed_flows. */
+            struct ovn_flow *new_node = ofctrl_dup_flow(d);
+            hmap_insert(&installed_flows, &new_node->hmap_node,
+                        new_node->hmap_node.hash);
+        }
     }
 }
diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h
index 93ef8ea..d3fabc0 100644
--- a/ovn/controller/ofctrl.h
+++ b/ovn/controller/ofctrl.h
@@ -34,6 +34,8 @@ void ofctrl_put(struct hmap *flows);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 
+struct ovn_flow *ofctrl_dup_flow(struct ovn_flow *source);
+
 /* 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);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 3638342..5a4174e 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -205,6 +205,8 @@ main(int argc, char *argv[])
     bool exiting;
     int retval;
 
+    struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
+
     ovs_cmdl_proctitle_init(argc, argv);
     set_program_name(argv[0]);
     service_start(&argc, &argv);
@@ -299,14 +301,12 @@ 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);
             if (chassis_id) {
                 physical_run(&ctx, mff_ovn_geneve,
                              br_int, chassis_id, &ct_zones, &flow_table);
             }
             ofctrl_put(&flow_table);
-            hmap_destroy(&flow_table);
         }
 
         /* local_datapaths contains bare hmap_node instances.
-- 
1.7.1




More information about the dev mailing list