[ovs-dev] [PATCH v2 ovn 2/3] ovn-controller: Optimize update of ct-zones external-ids.

Numan Siddique nusiddiq at redhat.com
Wed Aug 28 12:36:13 UTC 2019


On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara <dceara at redhat.com> 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 <dceara at redhat.com>
> ---
>  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
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>


More information about the dev mailing list