[ovs-dev] [PATCH ovn] ovn-controller: Use partial map updates for ct zones.

[Numan Siddique]

On Mon, Dec 14, 2020 at 8:37 PM Ilya Maximets <i.maximets at ovn.org> wrote:
>
> There could be hundreds or thousands of ct zones in external-ids map.
> Iteration over all of them and reconstruction of the whole new map
> is unnecessary and only hurts performance of both ovn-controller and
> ovsdb-server on the node.  Replacing with partial map updates to avoid
> unnecessary work.
>
> Signed-off-by: Ilya Maximets <i.maximets at ovn.org>

Thanks Ilya for this improvement patch. I applied to master and also
to branch-20.12
since this patch has performance improvements.

Thanks
Numan

> ---
>  controller/ovn-controller.c | 47 +++----------------------------------
>  1 file changed, 3 insertions(+), 44 deletions(-)
>
> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c
> index b5f0c1315..7540e2eee 100644
> --- a/controller/ovn-controller.c
> +++ b/controller/ovn-controller.c
> @@ -729,9 +729,6 @@ static void
>  commit_ct_zones(const struct ovsrec_bridge *br_int,
>                  struct shash *pending_ct_zones)
>  {
> -    struct smap ct_add_ids = SMAP_INITIALIZER(&ct_add_ids);
> -    struct sset ct_del_ids = SSET_INITIALIZER(&ct_del_ids);
> -
>      struct shash_node *iter;
>      SHASH_FOR_EACH(iter, pending_ct_zones) {
>          struct ct_zone_pending_entry *ctzpe = iter->data;
> @@ -750,57 +747,19 @@ commit_ct_zones(const struct ovsrec_bridge *br_int,
>              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;
> +                ovsrec_bridge_update_external_ids_setkey(br_int,
> +                                                         user_str, zone_str);
>              }
>              free(zone_str);
>          } else {
>              if (smap_get(&br_int->external_ids, user_str)) {
> -                sset_add(&ct_del_ids, user_str);
> +                ovsrec_bridge_update_external_ids_delkey(br_int, user_str);
>              }
>          }
>          free(user_str);
>
>          ctzpe->state = CT_ZONE_DB_SENT;
>      }
> -
> -    /* 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)) {
> -                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);
> -    }
> -
> -    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
> --
> 2.25.4
>
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>