On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara <[email protected]> wrote:
> commit_ct_zones() is called at every ovn-controller iteration but updates > to > ct-zones don't happen at every iteration. Avoid cloning the > br-int->external_ids map unless an update is needed. > > Signed-off-by: Dumitru Ceara <[email protected]> > --- > controller/ovn-controller.c | 53 > +++++++++++++++++++++++++++++++++++++------ > 1 file changed, 45 insertions(+), 8 deletions(-) > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 86f29ac..1825c1f 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -531,10 +531,9 @@ static void > commit_ct_zones(const struct ovsrec_bridge *br_int, > struct shash *pending_ct_zones) > { > - struct smap new_ids; > - smap_clone(&new_ids, &br_int->external_ids); > + struct smap ct_add_ids = SMAP_INITIALIZER(&ct_add_ids); > + struct sset ct_del_ids = SSET_INITIALIZER(&ct_del_ids); > > - bool updated = false; > struct shash_node *iter; > SHASH_FOR_EACH(iter, pending_ct_zones) { > struct ct_zone_pending_entry *ctzpe = iter->data; > @@ -550,22 +549,60 @@ commit_ct_zones(const struct ovsrec_bridge *br_int, > char *user_str = xasprintf("ct-zone-%s", iter->name); > if (ctzpe->add) { > char *zone_str = xasprintf("%"PRId32, ctzpe->zone); > - smap_replace(&new_ids, user_str, zone_str); > + struct smap_node *node = > + smap_get_node(&br_int->external_ids, user_str); > + if (!node || strcmp(node->value, zone_str)) { > + smap_add_nocopy(&ct_add_ids, user_str, zone_str); > + user_str = NULL; > + zone_str = NULL; > + } > free(zone_str); > } else { > - smap_remove(&new_ids, user_str); > + if (smap_get(&br_int->external_ids, user_str)) { > + sset_add(&ct_del_ids, user_str); > + } > } > free(user_str); > > ctzpe->state = CT_ZONE_DB_SENT; > - updated = true; > } > > - if (updated) { > + /* Update the bridge external IDs only if really needed (i.e., we must > + * add a value or delete one). Rebuilding the external IDs map at > + * every run is a costly operation when having lots of ct_zones. > + */ > + if (!smap_is_empty(&ct_add_ids) || !sset_is_empty(&ct_del_ids)) { > + struct smap new_ids = SMAP_INITIALIZER(&new_ids); > + > + struct smap_node *id; > + SMAP_FOR_EACH (id, &br_int->external_ids) { > + if (sset_find_and_delete(&ct_del_ids, id->key)) { > + continue; > + } > + > + if (smap_get(&ct_add_ids, id->key)) { > What if the value of this key is changed ? Suppose we have "a = 1" in external_ids and the ct_add_ids has "a = 2". Looks like in this case, the external_ids column in the db will not be updated with "a = 2". I am not sure though if this can actually happen. Thanks Numan + continue; > + } > + > + smap_add(&new_ids, id->key, id->value); > + } > + > + struct smap_node *next_id; > + SMAP_FOR_EACH_SAFE (id, next_id, &ct_add_ids) { > + smap_replace(&new_ids, id->key, id->value); > + smap_remove_node(&ct_add_ids, id); > + } > + > ovsrec_bridge_verify_external_ids(br_int); > ovsrec_bridge_set_external_ids(br_int, &new_ids); > + smap_destroy(&new_ids); > } > - smap_destroy(&new_ids); > + > + ovs_assert(smap_is_empty(&ct_add_ids)); > + ovs_assert(sset_is_empty(&ct_del_ids)); > + > + smap_destroy(&ct_add_ids); > + sset_destroy(&ct_del_ids); > } > > static void > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
