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

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@redhat.com>
---
 controller/ovn-controller.c |   53 +++++++++++++++++++++++++++++++++++++------
 1 file changed, 45 insertions(+), 8 deletions(-)

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

On Wed, Aug 28, 2019 at 2:36 PM Numan Siddique <nusiddiq@redhat.com> wrote:
>
>
>
> On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara <dceara@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@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".
>

Thanks for reviewing this!

If we're adding "a = 2" that means that we'll have the (a, 2) mapping
in ct_add_ids. It was added because strcmp(node->value, zone_str)
returned non-zero above.
This loop builds a copy of external_ids skipping the pairs that need
to be updated or added (ct_add_ids) and those that need to be deleted
(ct_del_ids).
The next loop will do the add/update through smap_replace.

Thanks,
Dumitru

> 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@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev