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

Dumitru Ceara dceara at redhat.com
Thu Sep 12 14:12:38 UTC 2019


On Wed, Aug 28, 2019 at 2:36 PM Numan Siddique <nusiddiq at redhat.com> wrote:
>
>
>
> 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".
>

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


More information about the dev mailing list