On Wed, Aug 28, 2019 at 2:36 PM Numan Siddique <nusid...@redhat.com> wrote:
>
>
>
> On Fri, Aug 23, 2019 at 4:54 PM Dumitru Ceara <dce...@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 <dce...@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
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
