[ovs-dev] [PATCH v3] ovn-controller: add quiet mode

Ryan Moats rmoats at us.ibm.com
Wed Oct 5 14:53:59 UTC 2016


As discussed in [1], what the incremental processing code
actually accomplished was that the ovn-controller would
be "quiet" and not burn CPU when things weren't changing.
This patch set recreates this state by calculating whether
changes have occured that would require a full calculation
to be performed.  It does this by persisting a copy of
the localvif_to_ofport and tunnel information in the
controller module, rather than in the physical.c module
as was the case with previous commits.

[1] http://openvswitch.org/pipermail/dev/2016-August/078272.html

Signed-off-by: Ryan Moats <rmoats at us.ibm.com>
---
Note: it may make sense to consider a follow on patch to clean
up all the tests for the existence of ovs_idl_txn into the
main ovn-controller loop...

 ovn/controller/ofctrl.c         |   2 +
 ovn/controller/ovn-controller.c |  71 +++++++++++++++++++---------
 ovn/controller/ovn-controller.h |   1 +
 ovn/controller/patch.c          |   5 ++
 ovn/controller/physical.c       | 100 ++++++++++++++++++++++++----------------
 ovn/controller/physical.h       |   5 ++
 6 files changed, 123 insertions(+), 61 deletions(-)

diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c
index 1d8fbf3..a3f465f 100644
--- a/ovn/controller/ofctrl.c
+++ b/ovn/controller/ofctrl.c
@@ -377,6 +377,8 @@ run_S_CLEAR_FLOWS(void)
     queue_msg(encode_group_mod(&gm));
     ofputil_uninit_group_mod(&gm);
 
+    force_full_process();
+
     /* Clear existing groups, to match the state of the switch. */
     if (groups) {
         ovn_group_table_clear(groups, true);
diff --git a/ovn/controller/ovn-controller.c b/ovn/controller/ovn-controller.c
index 00392ca..3c6b8b3 100644
--- a/ovn/controller/ovn-controller.c
+++ b/ovn/controller/ovn-controller.c
@@ -402,6 +402,23 @@ get_nb_cfg(struct ovsdb_idl *idl)
     return sb ? sb->nb_cfg : 0;
 }
 
+/* Contains mapping of localvifs to OF ports for detecting if the physical
+ * configuration of the switch has changed. */
+static struct simap localvif_to_ofport =
+    SIMAP_INITIALIZER(&localvif_to_ofport);
+
+/* Contains the list of known tunnels. */
+static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
+
+/* The last sequence number seen from the southbound IDL. */
+static unsigned int seqno = 0;
+
+void
+force_full_process(void)
+{
+    seqno = 0;
+}
+
 int
 main(int argc, char *argv[])
 {
@@ -528,42 +545,52 @@ main(int argc, char *argv[])
                         &all_lports);
         }
 
+        bool physical_change = true;
         if (br_int && chassis) {
+            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
+                                                         &pending_ct_zones);
+            physical_change = detect_and_save_physical_changes(
+                &localvif_to_ofport, &tunnels, mff_ovn_geneve, br_int,
+                chassis_id);
+            unsigned int cur_seqno = ovsdb_idl_get_seqno(ovnsb_idl_loop.idl);
+
             patch_run(&ctx, br_int, chassis_id, &local_datapaths,
                       &patched_datapaths);
 
             static struct lport_index lports;
-            static struct mcgroup_index mcgroups;
             lport_index_init(&lports, ctx.ovnsb_idl);
-            mcgroup_index_init(&mcgroups, ctx.ovnsb_idl);
-
-            enum mf_field_id mff_ovn_geneve = ofctrl_run(br_int,
-                                                         &pending_ct_zones);
 
             pinctrl_run(&ctx, &lports, br_int, chassis_id, &local_datapaths);
             update_ct_zones(&all_lports, &patched_datapaths, &ct_zones,
                             ct_zone_bitmap, &pending_ct_zones);
             commit_ct_zones(&ctx, br_int, &pending_ct_zones);
 
-            struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
-            lflow_run(&ctx, &lports, &mcgroups, &local_datapaths,
-                      &patched_datapaths, &group_table, &ct_zones,
-                      &flow_table);
-
-            physical_run(&ctx, mff_ovn_geneve,
-                         br_int, chassis_id, &ct_zones, &flow_table,
-                         &local_datapaths, &patched_datapaths);
-
-            ofctrl_put(&flow_table, &pending_ct_zones,
-                       get_nb_cfg(ctx.ovnsb_idl));
-            hmap_destroy(&flow_table);
-            if (ctx.ovnsb_idl_txn) {
-                int64_t cur_cfg = ofctrl_get_cur_cfg();
-                if (cur_cfg && cur_cfg != chassis->nb_cfg) {
-                    sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+            if (ctx.ovs_idl_txn && (physical_change || seqno < cur_seqno)) {
+                seqno = cur_seqno;
+
+                static struct mcgroup_index mcgroups;
+                mcgroup_index_init(&mcgroups, ctx.ovnsb_idl);
+
+                struct hmap flow_table = HMAP_INITIALIZER(&flow_table);
+                lflow_run(&ctx, &lports, &mcgroups, &local_datapaths,
+                          &patched_datapaths, &group_table, &ct_zones,
+                          &flow_table);
+
+                physical_run(&ctx, mff_ovn_geneve,
+                             br_int, chassis_id, &ct_zones, &flow_table,
+                             &local_datapaths, &patched_datapaths);
+
+                ofctrl_put(&flow_table, &pending_ct_zones,
+                           get_nb_cfg(ctx.ovnsb_idl));
+                hmap_destroy(&flow_table);
+                if (ctx.ovnsb_idl_txn) {
+                    int64_t cur_cfg = ofctrl_get_cur_cfg();
+                    if (cur_cfg && cur_cfg != chassis->nb_cfg) {
+                        sbrec_chassis_set_nb_cfg(chassis, cur_cfg);
+                    }
                 }
+                mcgroup_index_destroy(&mcgroups);
             }
-            mcgroup_index_destroy(&mcgroups);
             lport_index_destroy(&lports);
         }
 
diff --git a/ovn/controller/ovn-controller.h b/ovn/controller/ovn-controller.h
index 4dcf4e5..807b57c 100644
--- a/ovn/controller/ovn-controller.h
+++ b/ovn/controller/ovn-controller.h
@@ -91,5 +91,6 @@ enum chassis_tunnel_type {
 
 uint32_t get_tunnel_type(const char *name);
 
+void force_full_process(void);
 
 #endif /* ovn/ovn-controller.h */
diff --git a/ovn/controller/patch.c b/ovn/controller/patch.c
index c9a5dd9..7ee3da5 100644
--- a/ovn/controller/patch.c
+++ b/ovn/controller/patch.c
@@ -96,6 +96,7 @@ create_patch_port(struct controller_ctx *ctx,
     ovsrec_bridge_set_ports(src, ports, src->n_ports + 1);
 
     free(ports);
+    force_full_process();
 }
 
 static void
@@ -124,6 +125,7 @@ remove_port(struct controller_ctx *ctx,
             ovsrec_bridge_set_ports(bridge, new_ports, bridge->n_ports - 1);
             free(new_ports);
             ovsrec_port_delete(port);
+            force_full_process();
             return;
         }
     }
@@ -269,6 +271,7 @@ add_patched_datapath(struct hmap *patched_datapaths,
     /* stale is set to false. */
     hmap_insert(patched_datapaths, &pd->hmap_node,
                 binding_rec->datapath->tunnel_key);
+    force_full_process();
 }
 
 static void
@@ -294,6 +297,7 @@ add_logical_patch_ports_postprocess(struct hmap *patched_datapaths)
         if (pd_cur_node->stale == true) {
             hmap_remove(patched_datapaths, &pd_cur_node->hmap_node);
             free(pd_cur_node);
+            force_full_process();
         }
     }
 }
@@ -366,6 +370,7 @@ add_logical_patch_ports(struct controller_ctx *ctx,
             if (local_port) {
                 if (binding->chassis != chassis_rec && ctx->ovnsb_idl_txn) {
                     sbrec_port_binding_set_chassis(binding, chassis_rec);
+                    force_full_process();
                 }
             }
         }
diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index 1a91531..4cb72fe 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -55,10 +55,6 @@ physical_register_ovs_idl(struct ovsdb_idl *ovs_idl)
     ovsdb_idl_add_column(ovs_idl, &ovsrec_interface_col_external_ids);
 }
 
-static struct simap localvif_to_ofport =
-    SIMAP_INITIALIZER(&localvif_to_ofport);
-static struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
-
 /* Maps from a chassis to the OpenFlow port number of the tunnel that can be
  * used to reach that chassis. */
 struct chassis_tunnel {
@@ -69,11 +65,11 @@ struct chassis_tunnel {
 };
 
 static struct chassis_tunnel *
-chassis_tunnel_find(const char *chassis_id)
+chassis_tunnel_find(struct hmap *tunnels_p, const char *chassis_id)
 {
     struct chassis_tunnel *tun;
     HMAP_FOR_EACH_WITH_HASH (tun, hmap_node, hash_string(chassis_id, 0),
-                             &tunnels) {
+                             tunnels_p) {
         if (!strcmp(tun->chassis_id, chassis_id)) {
             return tun;
         }
@@ -160,7 +156,9 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
                       struct hmap *patched_datapaths,
                       const struct sbrec_port_binding *binding,
                       struct ofpbuf *ofpacts_p,
-                      struct hmap *flow_table)
+                      struct hmap *flow_table,
+                      struct simap *localvif_to_ofport_p,
+                      struct hmap *tunnels_p)
 {
     /* Skip the port binding if the port is on a datapath that is neither
      * local nor with any logical patch port connected, because local ports
@@ -217,13 +215,13 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
         if (!binding->tag) {
             return;
         }
-        ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+        ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                       binding->parent_port));
         if (ofport) {
             tag = *binding->tag;
         }
     } else {
-        ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+        ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                       binding->logical_port));
         if ((!strcmp(binding->type, "localnet")
             || !strcmp(binding->type, "l2gateway"))
@@ -242,13 +240,13 @@ consider_port_binding(enum mf_field_id mff_ovn_geneve,
             return;
         }
         if (localnet_port) {
-            ofport = u16_to_ofp(simap_get(&localvif_to_ofport,
+            ofport = u16_to_ofp(simap_get(localvif_to_ofport_p,
                                           localnet_port->logical_port));
             if (!ofport) {
                 return;
             }
         } else {
-            tun = chassis_tunnel_find(binding->chassis->name);
+            tun = chassis_tunnel_find(tunnels_p, binding->chassis->name);
             if (!tun) {
                 return;
             }
@@ -511,7 +509,9 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
                   const struct sbrec_multicast_group *mc,
                   struct ofpbuf *ofpacts_p,
                   struct ofpbuf *remote_ofpacts_p,
-                  struct hmap *flow_table)
+                  struct hmap *flow_table,
+                  struct simap *localvif_to_ofport_p,
+                  struct hmap *tunnels_p)
 {
     struct sset remote_chassis = SSET_INITIALIZER(&remote_chassis);
     struct match match;
@@ -554,7 +554,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
             put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32,
                      remote_ofpacts_p);
             put_resubmit(OFTABLE_CHECK_LOOPBACK, remote_ofpacts_p);
-        } else if (simap_contains(&localvif_to_ofport,
+        } else if (simap_contains(localvif_to_ofport_p,
                            (port->parent_port && *port->parent_port)
                            ? port->parent_port : port->logical_port)) {
             put_load(port->tunnel_key, MFF_LOG_OUTPORT, 0, 32, ofpacts_p);
@@ -600,7 +600,7 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
         const struct chassis_tunnel *prev = NULL;
         SSET_FOR_EACH (chassis, &remote_chassis) {
             const struct chassis_tunnel *tun
-                = chassis_tunnel_find(chassis);
+                = chassis_tunnel_find(tunnels_p, chassis);
             if (!tun) {
                 continue;
             }
@@ -624,13 +624,16 @@ consider_mc_group(enum mf_field_id mff_ovn_geneve,
     sset_destroy(&remote_chassis);
 }
 
-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,
-             struct hmap *local_datapaths, struct hmap *patched_datapaths)
+/* Scans the bridge 'br_int' and compares it to the supplied expected state.
+ * Returns true and updates the expected state if changes are detected.
+ * Returns false if no changes are found. */
+bool
+detect_and_save_physical_changes(struct simap *localvif_to_ofport_p,
+                                 struct hmap *tunnels_p,
+                                 enum mf_field_id mff_ovn_geneve,
+                                 const struct ovsrec_bridge *br_int,
+                                 const char *this_chassis_id)
 {
-
     /* This bool tracks physical mapping changes. */
     bool physical_map_changed = false;
 
@@ -638,6 +641,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
         SIMAP_INITIALIZER(&new_localvif_to_ofport);
     struct simap new_tunnel_to_ofport =
         SIMAP_INITIALIZER(&new_tunnel_to_ofport);
+
     for (int i = 0; i < br_int->n_ports; i++) {
         const struct ovsrec_port *port_rec = br_int->ports[i];
         if (!strcmp(port_rec->name, br_int->name)) {
@@ -706,7 +710,8 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                 }
 
                 simap_put(&new_tunnel_to_ofport, chassis_id, ofport);
-                struct chassis_tunnel *tun = chassis_tunnel_find(chassis_id);
+                struct chassis_tunnel *tun = chassis_tunnel_find(tunnels_p,
+                                                                 chassis_id);
                 if (tun) {
                     /* If the tunnel's ofport has changed, update. */
                     if (tun->ofport != u16_to_ofp(ofport) ||
@@ -717,7 +722,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
                     }
                 } else {
                     tun = xmalloc(sizeof *tun);
-                    hmap_insert(&tunnels, &tun->hmap_node,
+                    hmap_insert(tunnels_p, &tun->hmap_node,
                                 hash_string(chassis_id, 0));
                     tun->chassis_id = chassis_id;
                     tun->ofport = u16_to_ofp(ofport);
@@ -737,9 +742,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     /* Remove tunnels that are no longer here. */
     struct chassis_tunnel *tun, *tun_next;
-    HMAP_FOR_EACH_SAFE (tun, tun_next, hmap_node, &tunnels) {
+    HMAP_FOR_EACH_SAFE (tun, tun_next, hmap_node, tunnels_p) {
         if (!simap_find(&new_tunnel_to_ofport, tun->chassis_id)) {
-            hmap_remove(&tunnels, &tun->hmap_node);
+            hmap_remove(tunnels_p, &tun->hmap_node);
             physical_map_changed = true;
             free(tun);
         }
@@ -747,29 +752,43 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
 
     /* Capture changed or removed openflow ports. */
     struct simap_node *vif_name, *vif_name_next;
-    SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localvif_to_ofport) {
+    SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, localvif_to_ofport_p) {
         int newport;
         if ((newport = simap_get(&new_localvif_to_ofport, vif_name->name))) {
-            if (newport != simap_get(&localvif_to_ofport, vif_name->name)) {
-                simap_put(&localvif_to_ofport, vif_name->name, newport);
+            if (newport != simap_get(localvif_to_ofport_p, vif_name->name)) {
+                simap_put(localvif_to_ofport_p, vif_name->name, newport);
                 physical_map_changed = true;
             }
         } else {
-            simap_find_and_delete(&localvif_to_ofport, vif_name->name);
+            simap_find_and_delete(localvif_to_ofport_p, vif_name->name);
             physical_map_changed = true;
         }
     }
     SIMAP_FOR_EACH (vif_name, &new_localvif_to_ofport) {
-        if (!simap_get(&localvif_to_ofport, vif_name->name)) {
-            simap_put(&localvif_to_ofport, vif_name->name,
+        if (!simap_get(localvif_to_ofport_p, vif_name->name)) {
+            simap_put(localvif_to_ofport_p, vif_name->name,
                       simap_get(&new_localvif_to_ofport, vif_name->name));
             physical_map_changed = true;
         }
     }
-    if (physical_map_changed) {
-        /* Reprocess logical flow table immediately. */
-        poll_immediate_wake();
-    }
+
+    simap_destroy(&new_localvif_to_ofport);
+    simap_destroy(&new_tunnel_to_ofport);
+
+    return physical_map_changed;
+}
+
+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,
+             struct hmap *local_datapaths, struct hmap *patched_datapaths)
+{
+    struct simap localvif_to_ofport = SIMAP_INITIALIZER(&localvif_to_ofport);
+    struct hmap tunnels = HMAP_INITIALIZER(&tunnels);
+
+    detect_and_save_physical_changes(&localvif_to_ofport, &tunnels,
+                                     mff_ovn_geneve, br_int, this_chassis_id);
 
     struct ofpbuf ofpacts;
     ofpbuf_init(&ofpacts, 0);
@@ -783,7 +802,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
          * should clear the old flows to avoid collisions. */
         consider_port_binding(mff_ovn_geneve, ct_zones, local_datapaths,
                               patched_datapaths, binding, &ofpacts,
-                              flow_table);
+                              flow_table, &localvif_to_ofport, &tunnels);
     }
 
     /* Handle output to multicast groups, in tables 32 and 33. */
@@ -796,7 +815,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
          * so that we avoid warning messages on collisions. */
         consider_mc_group(mff_ovn_geneve, ct_zones,
                           local_datapaths, mc, &ofpacts, &remote_ofpacts,
-                          flow_table);
+                          flow_table, &localvif_to_ofport, &tunnels);
     }
 
     ofpbuf_uninit(&remote_ofpacts);
@@ -812,6 +831,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
      * ports.  We set MFF_LOG_DATAPATH, MFF_LOG_INPORT, and
      * MFF_LOG_OUTPORT from the tunnel key data, then resubmit to table
      * 33 to handle packets to the local hypervisor. */
+    struct chassis_tunnel *tun;
     HMAP_FOR_EACH (tun, hmap_node, &tunnels) {
         struct match match = MATCH_CATCHALL_INITIALIZER;
         match_set_in_port(&match, tun->ofport);
@@ -909,7 +929,9 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id mff_ovn_geneve,
     ofctrl_add_flow(flow_table, OFTABLE_SAVE_INPORT, 0, &match, &ofpacts);
 
     ofpbuf_uninit(&ofpacts);
-
-    simap_destroy(&new_localvif_to_ofport);
-    simap_destroy(&new_tunnel_to_ofport);
+    simap_destroy(&localvif_to_ofport);
+    HMAP_FOR_EACH_POP (tun, hmap_node, &tunnels) {
+        free(tun);
+    }
+    hmap_destroy(&tunnels);
 }
diff --git a/ovn/controller/physical.h b/ovn/controller/physical.h
index 86ce93c..d11615e 100644
--- a/ovn/controller/physical.h
+++ b/ovn/controller/physical.h
@@ -41,6 +41,11 @@ struct simap;
 #define OVN_GENEVE_LEN 4
 
 void physical_register_ovs_idl(struct ovsdb_idl *);
+bool detect_and_save_physical_changes(struct simap *localvif_to_ofport_p,
+                                      struct hmap *tunnels_p,
+                                      enum mf_field_id mff_ovn_geneve,
+                                      const struct ovsrec_bridge *br_int,
+                                      const char *this_chassis_id);
 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,
-- 
2.7.4




More information about the dev mailing list