ovn-controller: Explicitly pass the flow table from function to funct…

…ion.

As I was working in ovn-controller, I found it hard to tell what code
produced and what code consumed the OpenFlow flow table, because it was
all implicit.  This commit makes the data structure an explicit variable
in the main loop, which makes it easier to see.

Signed-off-by: Ben Pfaff <[email protected]>
Acked-by: Russell Bryant <[email protected]>

ovn/controller/ofctrl.c

@@ -35,7 +35,7 @@ VLOG_DEFINE_THIS_MODULE(ofctrl);
 /* An OpenFlow flow. */
 struct ovn_flow {
     /* Key. */
-    struct hmap_node hmap_node; /* In 'desired_flows' or 'installed_flows'. */
+    struct hmap_node hmap_node;
     uint8_t table_id;
     uint16_t priority;
     struct match match;
@@ -64,48 +64,48 @@ static unsigned int seqno;
  * zero, to avoid unbounded buffering. */
 static struct rconn_packet_counter *tx_counter;
 
-/* Flow tables.  Each holds "struct ovn_flow"s.
- *
- * 'desired_flows' is the flow table that we want the switch to have.
- * 'installed_flows' is the flow table currently installed in the switch. */
-static struct hmap desired_flows;
+/* Flow table of "struct ovn_flow"s, that holds the flow table currently
+ * installed in the switch. */
 static struct hmap installed_flows;
 
 static void ovn_flow_table_clear(struct hmap *flow_table);
 static void ovn_flow_table_destroy(struct hmap *flow_table);
 
-static void ofctrl_update_flows(void);
+static void ofctrl_update_flows(struct hmap *desired_flows);
 static void ofctrl_recv(const struct ofpbuf *msg);
 
 void
 ofctrl_init(void)
 {
     swconn = rconn_create(5, 0, DSCP_DEFAULT, 1 << OFP13_VERSION);
     tx_counter = rconn_packet_counter_create();
-    hmap_init(&desired_flows);
     hmap_init(&installed_flows);
 }
 
-/* This function should be called in the main loop after anything that updates
- * the flow table (e.g. after calls to ofctrl_clear_flows() and
- * ofctrl_add_flow()). */
+/* Attempts to update the OpenFlow flows in bridge 'br_int_name' to those in
+ * 'flow_table'.  Removes all of the flows from 'flow_table' and frees them.
+ *
+ * The flow table will only be updated if we've got an OpenFlow connection to
+ * 'br_int_name' and it's not backlogged.  Otherwise, it'll have to wait until
+ * the next iteration. */
 void
-ofctrl_run(struct controller_ctx *ctx)
+ofctrl_run(struct controller_ctx *ctx, struct hmap *flow_table)
 {
     char *target;
     target = xasprintf("unix:%s/%s.mgmt", ovs_rundir(), ctx->br_int_name);
     if (strcmp(target, rconn_get_target(swconn))) {
+        VLOG_INFO("%s: connecting to switch", target);
         rconn_connect(swconn, target, target);
     }
     free(target);
 
     rconn_run(swconn);
 
     if (!rconn_is_connected(swconn)) {
-        return;
+        goto exit;
     }
     if (!rconn_packet_counter_n_packets(tx_counter)) {
-        ofctrl_update_flows();
+        ofctrl_update_flows(flow_table);
     }
 
     for (int i = 0; i < 50; i++) {
@@ -117,6 +117,9 @@ ofctrl_run(struct controller_ctx *ctx)
         ofctrl_recv(msg);
         ofpbuf_delete(msg);
     }
+
+exit:
+    ovn_flow_table_clear(flow_table);
 }
 
 void
@@ -131,7 +134,6 @@ ofctrl_destroy(void)
 {
     rconn_destroy(swconn);
     ovn_flow_table_destroy(&installed_flows);
-    ovn_flow_table_destroy(&desired_flows);
     rconn_packet_counter_destroy(tx_counter);
 }
 
@@ -246,22 +248,17 @@ ofctrl_recv(const struct ofpbuf *msg)
 
 /* Flow table interface to the rest of ovn-controller. */
 
-/* Clears the table of flows desired to be in the switch.  Call this before
- * adding the desired flows (with ofctrl_add_flow()). */
-void
-ofctrl_clear_flows(void)
-{
-    ovn_flow_table_clear(&desired_flows);
-}
-
-/* Adds a flow 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'.
+/* Adds a flow to 'desired_flows' 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
- * sent to the switch until a later call to ofctrl_run(). */
+ * sent to the switch until a later call to ofctrl_run().
+ *
+ * The caller should initialize its own hmap to hold the flows. */
 void
-ofctrl_add_flow(uint8_t table_id, uint16_t priority,
+ofctrl_add_flow(struct hmap *desired_flows,
+                uint8_t table_id, uint16_t priority,
                 const struct match *match, const struct ofpbuf *actions)
 {
     struct ovn_flow *f = xmalloc(sizeof *f);
@@ -271,7 +268,7 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority,
     f->ofpacts = xmemdup(actions->data, actions->size);
     f->ofpacts_len = actions->size;
 
-    if (ovn_flow_lookup(&desired_flows, 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);
@@ -283,7 +280,7 @@ ofctrl_add_flow(uint8_t table_id, uint16_t priority,
         return;
     }
 
-    hmap_insert(&desired_flows, &f->hmap_node, ovn_flow_hash(f));
+    hmap_insert(desired_flows, &f->hmap_node, ovn_flow_hash(f));
 }
 
 /* ovn_flow. */
@@ -376,7 +373,7 @@ queue_flow_mod(struct ofputil_flow_mod *fm)
 }
 
 static void
-ofctrl_update_flows(void)
+ofctrl_update_flows(struct hmap *desired_flows)
 {
     /* If we've (re)connected, don't make any assumptions about the flows in
      * the switch: delete all of them.  (We'll immediately repopulate it
@@ -402,7 +399,7 @@ ofctrl_update_flows(void)
      * 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(&desired_flows, i);
+        struct ovn_flow *d = ovn_flow_lookup(desired_flows, i);
         if (!d) {
             /* Installed flow is no longer desirable.  Delete it from the
              * switch and from installed_flows. */
@@ -440,16 +437,16 @@ ofctrl_update_flows(void)
                 d->ofpacts_len = 0;
             }
 
-            hmap_remove(&desired_flows, &d->hmap_node);
+            hmap_remove(desired_flows, &d->hmap_node);
             ovn_flow_destroy(d);
         }
     }
 
-    /* The previous loop removed from desired_flows all of the flows that are
-     * already installed.  Thus, any flows remaining in desired_flows need to
+    /* The previous loop removed from 'desired_flows' all of the flows that are
+     * already installed.  Thus, any flows remaining in 'desired_flows' need to
      * be added to the flow table. */
     struct ovn_flow *d;
-    HMAP_FOR_EACH_SAFE (d, next, hmap_node, &desired_flows) {
+    HMAP_FOR_EACH_SAFE (d, next, hmap_node, desired_flows) {
         /* Send flow_mod to add flow. */
         struct ofputil_flow_mod fm = {
             .match = d->match,
@@ -462,8 +459,8 @@ ofctrl_update_flows(void)
         queue_flow_mod(&fm);
         ovn_flow_log(d, "adding");
 
-        /* Move 'd' from desired_flows to installed_flows. */
-        hmap_remove(&desired_flows, &d->hmap_node);
+        /* Move 'd' from 'desired_flows' to installed_flows. */
+        hmap_remove(desired_flows, &d->hmap_node);
         hmap_insert(&installed_flows, &d->hmap_node, d->hmap_node.hash);
     }
 }

ovn/controller/ofctrl.h

@@ -20,18 +20,19 @@
 #include <stdint.h>
 
 struct controller_ctx;
+struct hmap;
 struct match;
 struct ofpbuf;
 
 /* Interface for OVN main loop. */
 void ofctrl_init(void);
-void ofctrl_run(struct controller_ctx *);
+void ofctrl_run(struct controller_ctx *, struct hmap *flow_table);
 void ofctrl_wait(void);
 void ofctrl_destroy(void);
 
 /* Flow table interface to the rest of ovn-controller. */
 void ofctrl_clear_flows(void);
-void ofctrl_add_flow(uint8_t table_id, uint16_t priority,
+void ofctrl_add_flow(struct hmap *flows, uint8_t table_id, uint16_t priority,
                      const struct match *, const struct ofpbuf *ofpacts);
 
 #endif /* ovn/ofctrl.h */

ovn/controller/ovn-controller.c

@@ -286,13 +286,15 @@ main(int argc, char *argv[])
             goto exit;
         }
 
-        ofctrl_clear_flows();
-
         chassis_run(&ctx);
         binding_run(&ctx);
-        pipeline_run(&ctx);
-        physical_run(&ctx);
-        ofctrl_run(&ctx);
+
+        struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
+        pipeline_run(&ctx, &flow_table);
+        physical_run(&ctx, &flow_table);
+        ofctrl_run(&ctx, &flow_table);
+        hmap_destroy(&flow_table);
+
         unixctl_server_run(unixctl);
 
         unixctl_server_wait(unixctl);

ovn/controller/physical.c

@@ -43,7 +43,7 @@ physical_init(struct controller_ctx *ctx)
 }
 
 void
-physical_run(struct controller_ctx *ctx)
+physical_run(struct controller_ctx *ctx, struct hmap *flow_table)
 {
     struct simap lport_to_ofport = SIMAP_INITIALIZER(&lport_to_ofport);
     struct simap chassis_to_ofport = SIMAP_INITIALIZER(&chassis_to_ofport);
@@ -177,7 +177,7 @@ physical_run(struct controller_ctx *ctx)
             struct ofpact_resubmit *resubmit = ofpact_put_RESUBMIT(&ofpacts);
             resubmit->in_port = OFPP_IN_PORT;
             resubmit->table_id = 16;
-            ofctrl_add_flow(0, tag ? 150 : 100, &match, &ofpacts);
+            ofctrl_add_flow(flow_table, 0, tag ? 150 : 100, &match, &ofpacts);
 
             /* Table 0, Priority 50.
              * =====================
@@ -195,7 +195,7 @@ physical_run(struct controller_ctx *ctx)
                 vlan_vid->push_vlan_if_needed = true;
             }
             ofpact_put_OUTPUT(&ofpacts)->port = ofport;
-            ofctrl_add_flow(0, 50, &match, &ofpacts);
+            ofctrl_add_flow(flow_table, 0, 50, &match, &ofpacts);
         }
 
         /* Table 64, Priority 100.
@@ -206,7 +206,7 @@ physical_run(struct controller_ctx *ctx)
         ofpbuf_clear(&ofpacts);
         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(64, 100, &match, &ofpacts);
+        ofctrl_add_flow(flow_table, 64, 100, &match, &ofpacts);
 
         /* Table 64, Priority 50.
          * ======================
@@ -241,7 +241,7 @@ physical_run(struct controller_ctx *ctx)
             sf->mask.be16 = OVS_BE16_MAX;
         }
         ofpact_put_OUTPUT(&ofpacts)->port = ofport;
-        ofctrl_add_flow(64, 50, &match, &ofpacts);
+        ofctrl_add_flow(flow_table, 64, 50, &match, &ofpacts);
     }
 
     ofpbuf_uninit(&ofpacts);

ovn/controller/physical.h

@@ -25,8 +25,9 @@
  */
 
 struct controller_ctx;
+struct hmap;
 
 void physical_init(struct controller_ctx *);
-void physical_run(struct controller_ctx *);
+void physical_run(struct controller_ctx *, struct hmap *flow_table);
 
 #endif /* ovn/physical.h */

ovn/controller/pipeline.c

@@ -250,11 +250,11 @@ pipeline_init(void)
 }
 
 /* Translates logical flows in the Pipeline table in the OVN_SB database
- * into OpenFlow flows.
+ * into OpenFlow flows, adding the OpenFlow flows to 'flow_table'.
  *
  * We put the Pipeline flows into OpenFlow tables 16 through 47 (inclusive). */
 void
-pipeline_run(struct controller_ctx *ctx)
+pipeline_run(struct controller_ctx *ctx, struct hmap *flow_table)
 {
     struct hmap flows = HMAP_INITIALIZER(&flows);
     uint32_t conj_id_ofs = 1;
@@ -327,8 +327,8 @@ pipeline_run(struct controller_ctx *ctx)
                 m->match.flow.conj_id += conj_id_ofs;
             }
             if (!m->n) {
-                ofctrl_add_flow(pipeline->table_id + 16, pipeline->priority,
-                                &m->match, &ofpacts);
+                ofctrl_add_flow(flow_table, pipeline->table_id + 16,
+                                pipeline->priority, &m->match, &ofpacts);
             } else {
                 uint64_t conj_stubs[64 / 8];
                 struct ofpbuf conj;
@@ -343,8 +343,8 @@ pipeline_run(struct controller_ctx *ctx)
                     dst->clause = src->clause;
                     dst->n_clauses = src->n_clauses;
                 }
-                ofctrl_add_flow(pipeline->table_id + 16, pipeline->priority,
-                                &m->match, &conj);
+                ofctrl_add_flow(flow_table, pipeline->table_id + 16,
+                                pipeline->priority, &m->match, &conj);
                 ofpbuf_uninit(&conj);
             }
         }

ovn/controller/pipeline.h

@@ -34,14 +34,15 @@
 #include <stdint.h>
 
 struct controller_ctx;
+struct hmap;
 struct uuid;
 
 /* Logical ports. */
 #define MFF_LOG_INPORT  MFF_REG6 /* Logical input port. */
 #define MFF_LOG_OUTPORT MFF_REG7 /* Logical output port. */
 
 void pipeline_init(void);
-void pipeline_run(struct controller_ctx *);
+void pipeline_run(struct controller_ctx *, struct hmap *flow_table);
 void pipeline_destroy(struct controller_ctx *);
 
 uint32_t ldp_to_integer(const struct uuid *logical_datapath);