On Mon, Jul 25, 2016 at 04:28:52PM +0000, Ryan Moats wrote:
> While commit ab39371d68842b7e4000cc5d8718e6fc04e92795
> (ovn-controller: Handle physical changes correctly) addressed
> unit test failures, it did so at the cost of performance: [1]
> notes that ovn-controller cpu usage is now pegged at 100%.
> 
> Root cause of this is that while the storage for tunnels is
> persisted, their creation is not (which the above changed
> incorrectly assumed was the case).  This patch persists
> tunneled data across invocations of physical_run.  A side
> effect is that renaming of localfvif_map_changed variable
> to physical_map_changed and extending its scope to include
> tunnel changes.
> 
> [1] http://openvswitch.org/pipermail/dev/2016-July/076058.html
> 
> Signed-off-by: Ryan Moats <rmo...@us.ibm.com>

Thanks for the fix.

I applied this to master, folding in the following incremental which is
mostly style but I think that the extra check on tun->type is a bug fix
albeit for a corner case.  Please take a look.

--8<--------------------------cut here-------------------------->8--

diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c
index e788fe5..e5df4f2 100644
--- a/ovn/controller/physical.c
+++ b/ovn/controller/physical.c
@@ -610,7 +610,7 @@ physical_run(struct controller_ctx *ctx, enum mf_field_id 
mff_ovn_geneve,
 
     struct simap new_localvif_to_ofport =
         SIMAP_INITIALIZER(&new_localvif_to_ofport);
-    struct simap new_tunnel_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];
@@ -674,11 +674,13 @@ 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;
-                if ((tun = chassis_tunnel_find(chassis_id))) {
+                struct chassis_tunnel *tun = chassis_tunnel_find(chassis_id);
+                if (tun) {
                     /* If the tunnel's ofport has changed, update. */
-                    if (tun->ofport != u16_to_ofp(ofport)) {
+                    if (tun->ofport != u16_to_ofp(ofport) ||
+                        tun->type != tunnel_type) {
                         tun->ofport = u16_to_ofp(ofport);
+                        tun->type = tunnel_type;
                         physical_map_changed = true;
                     }
                 } else {
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Reply via email to