"dev" <dev-boun...@openvswitch.org> wrote on 07/26/2016 02:37:30 PM:
> From: "Liran Schour" <lir...@il.ibm.com> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 07/26/2016 02:38 PM > Subject: Re: [ovs-dev] [ovs-dev,ovn-controller] Physical: persist tunnels > Sent by: "dev" <dev-boun...@openvswitch.org> > > "dev" <dev-boun...@openvswitch.org> wrote on 26/07/2016 12:02:37 AM: > > > From: Flavio Fernandes <fla...@flaviof.com> > > To: Ryan Moats <rmo...@us.ibm.com> > > Cc: dev@openvswitch.org > > Date: 26/07/2016 12:06 AM > > Subject: Re: [ovs-dev] [ovs-dev,ovn-controller] Physical: persist > tunnels > > Sent by: "dev" <dev-boun...@openvswitch.org> > > > > > > > On Jul 25, 2016, at 12:28 PM, Ryan Moats <rmo...@us.ibm.com> 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. > > > > > > > I tested this fix by using a Vagrant file that stamps out vms as > > compute nodes. > > To deploy ovn, call the script ?/vagrant/scripts/setup-ovn- > > cluster.sh? and that > > would render ovn-controller in compute nodes to peg the cpu at 100% > before the > > changes. > > > > More info on _easily_ deploying this cluster is available here: > > https://github.com/flavio-fernandes/just-ovn-nodes/blob/master/README.md > > > > > > > [1] http://openvswitch.org/pipermail/dev/2016-July/076058.html > > > > > > Signed-off-by: Ryan Moats <rmo...@us.ibm.com> > > > > Acked-by: Flavio Fernandes <fla...@flaviof.com> > > Tested-by: Flavio Fernandes <fla...@flaviof.com> > > > > Tested this fix with a cluster of 50 hosts. > > Acked-by: Liran Schour <lir...@il.ibm.com> > Tested-by: Liran Schour <lir...@il.ibm.com> This patch fixes the 100% CPU unitization of ovn-controller. Without this patch the ovn-controller reaches 100% CPU in fresh deployment using ovn-scale-test [1]. After applying this patch, the CPU utilization looks good. [1] https://github.com/openvswitch/ovn-scale-test Acked-by: Hui Kang <ka...@us.ibm.com> Tested-by: Hui Kang <ka...@us.ibm.com> > > > > > > --- > > > ovn/controller/physical.c | 59 ++++++++++++++++++++++++++++ > > +------------------ > > > 1 file changed, 37 insertions(+), 22 deletions(-) > > > > > > diff --git a/ovn/controller/physical.c b/ovn/controller/physical.c > > > index a104e33..e788fe5 100644 > > > --- a/ovn/controller/physical.c > > > +++ b/ovn/controller/physical.c > > > @@ -605,8 +605,13 @@ physical_run(struct controller_ctx *ctx, enum > > mf_field_id mff_ovn_geneve, > > > uuid_generate(hc_uuid); > > > } > > > > > > + /* This bool tracks physical mapping changes. */ > > > + bool physical_map_changed = false; > > > + > > > struct simap new_localvif_to_ofport = > > > 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)) { > > > @@ -668,18 +673,23 @@ physical_run(struct controller_ctx *ctx, > > enum mf_field_id mff_ovn_geneve, > > > continue; > > > } > > > > > > - struct chassis_tunnel *tun = xmalloc(sizeof *tun); > > > - hmap_insert(&tunnels, &tun->hmap_node, > > > - hash_string(chassis_id, 0)); > > > - tun->chassis_id = chassis_id; > > > - tun->ofport = u16_to_ofp(ofport); > > > - tun->type = tunnel_type; > > > - full_binding_processing = true; > > > - binding_reset_processing(); > > > - > > > - /* Reprocess logical flow table immediately. */ > > > - lflow_reset_processing(); > > > - poll_immediate_wake(); > > > + simap_put(&new_tunnel_to_ofport, chassis_id, ofport); > > > + struct chassis_tunnel *tun; > > > + if ((tun = chassis_tunnel_find(chassis_id))) { > > > + /* If the tunnel's ofport has changed, update. */ > > > + if (tun->ofport != u16_to_ofp(ofport)) { > > > + tun->ofport = u16_to_ofp(ofport); > > > + physical_map_changed = true; > > > + } > > > + } else { > > > + tun = xmalloc(sizeof *tun); > > > + hmap_insert(&tunnels, &tun->hmap_node, > > > + hash_string(chassis_id, 0)); > > > + tun->chassis_id = chassis_id; > > > + tun->ofport = u16_to_ofp(ofport); > > > + tun->type = tunnel_type; > > > + physical_map_changed = true; > > > + } > > > break; > > > } else { > > > const char *iface_id = > smap_get(&iface_rec->external_ids, > > > @@ -691,29 +701,38 @@ 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) { > > > + if (!simap_find(&new_tunnel_to_ofport, tun->chassis_id)) { > > > + hmap_remove(&tunnels, &tun->hmap_node); > > > + physical_map_changed = true; > > > + free(tun); > > > + } > > > + } > > > + > > > /* Capture changed or removed openflow ports. */ > > > - bool localvif_map_changed = false; > > > struct simap_node *vif_name, *vif_name_next; > > > SIMAP_FOR_EACH_SAFE (vif_name, vif_name_next, &localvif_to_ofport) > { > > > 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); > > > - localvif_map_changed = true; > > > + physical_map_changed = true; > > > } > > > } else { > > > simap_find_and_delete(&localvif_to_ofport, > vif_name->name); > > > - localvif_map_changed = true; > > > + 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, > > > simap_get(&new_localvif_to_ofport, > vif_name->name)); > > > - localvif_map_changed = true; > > > + physical_map_changed = true; > > > } > > > } > > > - if (localvif_map_changed) { > > > + if (physical_map_changed) { > > > full_binding_processing = true; > > > > > > /* Reprocess logical flow table immediately. */ > > > @@ -769,7 +788,6 @@ 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); > > > @@ -858,10 +876,7 @@ physical_run(struct controller_ctx *ctx, enum > > mf_field_id mff_ovn_geneve, > > > ofctrl_add_flow(OFTABLE_DROP_LOOPBACK, 0, &match, &ofpacts, > hc_uuid); > > > > > > ofpbuf_uninit(&ofpacts); > > > - HMAP_FOR_EACH_POP (tun, hmap_node, &tunnels) { > > > - free(tun); > > > - } > > > - hmap_clear(&tunnels); > > > > > > simap_destroy(&new_localvif_to_ofport); > > > + simap_destroy(&new_tunnel_to_ofport); > > > } > > > > _______________________________________________ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
