| Message ID | 1469464132-7697-1-git-send-email-rmoats@us.ibm.com |
|---|---|
| State | Accepted |
| Headers | show |
> On Jul 25, 2016, at 12:28 PM, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> Acked-by: Flavio Fernandes <flavio@flaviof.com> Tested-by: Flavio Fernandes <flavio@flaviof.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" <dev-bounces@openvswitch.org> wrote on 26/07/2016 12:02:37 AM: > From: Flavio Fernandes <flavio@flaviof.com> > To: Ryan Moats <rmoats@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-bounces@openvswitch.org> > > > > On Jul 25, 2016, at 12:28 PM, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > Acked-by: Flavio Fernandes <flavio@flaviof.com> > Tested-by: Flavio Fernandes <flavio@flaviof.com> > Tested this fix with a cluster of 50 hosts. Acked-by: Liran Schour <lirans@il.ibm.com> Tested-by: Liran Schour <lirans@il.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" <dev-bounces@openvswitch.org> wrote on 07/26/2016 02:37:30 PM: > From: "Liran Schour" <LIRANS@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-bounces@openvswitch.org> > > "dev" <dev-bounces@openvswitch.org> wrote on 26/07/2016 12:02:37 AM: > > > From: Flavio Fernandes <flavio@flaviof.com> > > To: Ryan Moats <rmoats@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-bounces@openvswitch.org> > > > > > > > On Jul 25, 2016, at 12:28 PM, Ryan Moats <rmoats@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 <rmoats@us.ibm.com> > > > > Acked-by: Flavio Fernandes <flavio@flaviof.com> > > Tested-by: Flavio Fernandes <flavio@flaviof.com> > > > > Tested this fix with a cluster of 50 hosts. > > Acked-by: Liran Schour <lirans@il.ibm.com> > Tested-by: Liran Schour <lirans@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 <kangh@us.ibm.com> Tested-by: Hui Kang <kangh@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
Ben Pfaff <blp@ovn.org> wrote on 07/27/2016 04:14:07 PM: > From: Ben Pfaff <blp@ovn.org> > To: Ryan Moats/Omaha/IBM@IBMUS > Cc: dev@openvswitch.org > Date: 07/27/2016 04:14 PM > Subject: Re: [ovs-dev] [PATCH ovn-controller] Physical: persist tunnels > > 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 <rmoats@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-- I think that's fine and if it isn't we'll figure it out... Ryan
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); }
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 <rmoats@us.ibm.com> --- ovn/controller/physical.c | 59 +++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 22 deletions(-)