On Thu, Jun 1, 2023 at 7:44 AM Han Zhou <hz...@ovn.org> wrote:

>
>
> On Wed, May 24, 2023 at 11:27 PM Ales Musil <amu...@redhat.com> wrote:
> >
> > The incremental processing is broken for addresses
> > that have mask which could "erase" portion of the address
> > itself e.g. 10.16.0.47/4, after applying the mask with token
> > parser the address becomes 0.0.0.0/4, which is fine for
> > logical flows. However, for the deletion/addition to database
> > we need the original string representation which cannot be
> > built from the parsed token.
> >
> > Use strcmp over already sorted lists which should give
> > us the needed diff. The time complexity didn't change,
> > but the performance was slightly improved, see the numbers
> > below. The setup was created by the scale script from Han [0].
> >
> > Numbers with expr:
> > Time spent on processing nb_cfg 1:
> >         ovn-northd delay before processing:     21ms
> >         ovn-northd completion:                  544ms
> >         ovn-controller(s) completion:           544ms
> > Time spent on processing nb_cfg 2:
> >         ovn-northd delay before processing:     17ms
> >         ovn-northd completion:                  537ms
> >         ovn-controller(s) completion:           535ms
> > Time spent on processing nb_cfg 3:
> >         ovn-northd delay before processing:     20ms
> >         ovn-northd completion:                  552ms
> >         ovn-controller(s) completion:           550ms
> > Time spent on processing nb_cfg 4:
> >         ovn-northd delay before processing:     16ms
> >         ovn-northd completion:                  529ms
> >         ovn-controller(s) completion:           528ms
> >
> > Numbers with the strcmp:
> > Time spent on processing nb_cfg 1:
> >         ovn-northd delay before processing:     24ms
> >         ovn-northd completion:                  521ms
> >         ovn-controller(s) completion:           521ms
> > Time spent on processing nb_cfg 2:
> >         ovn-northd delay before processing:     17ms
> >         ovn-northd completion:                  500ms
> >         ovn-controller(s) completion:           500ms
> > Time spent on processing nb_cfg 3:
> >         ovn-northd delay before processing:     17ms
> >         ovn-northd completion:                  496ms
> >         ovn-controller(s) completion:           497ms
> > Time spent on processing nb_cfg 4:
> >         ovn-northd delay before processing:     18ms
> >         ovn-northd completion:                  502ms
> >         ovn-controller(s) completion:           500ms
> >
> > [0]
> https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ov
>
> n_controller_scale.sh
> >
> > Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.")
> > Reported-at: https://bugzilla.redhat.com/2196885
> > Reported-at:
> https://mail.openvswitch.org/pipermail/ovs-discuss/2023-May/052426.html
> > Reported-By: 张祖建 <zhangzujia...@gmail.com>
> > Signed-off-by: Ales Musil <amu...@redhat.com>
> > ---
> > v2: Address comments from Ilya:
> >     Use "custom" algorithm to prevent a ton of allocations with svec.
> >     Add original reporter.
> > ---
> >  northd/en-sync-sb.c | 183 +++++++++++++++++++++++++++-----------------
> >  1 file changed, 112 insertions(+), 71 deletions(-)
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index 758f00bfd..87402d6fb 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -22,7 +22,6 @@
> >  #include "openvswitch/util.h"
> >
> >  #include "en-sync-sb.h"
> > -#include "include/ovn/expr.h"
> >  #include "lib/inc-proc-eng.h"
> >  #include "lib/lb.h"
> >  #include "lib/ovn-nb-idl.h"
> > @@ -34,8 +33,15 @@
> >
> >  VLOG_DEFINE_THIS_MODULE(en_sync_to_sb);
> >
> > +/* This is just a type wrapper to enforce that it has to be sorted. */
> > +struct sorted_addresses {
> > +    const char **arr;
> > +    size_t n;
> > +};
> > +
> > +
> >  static void sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char
> *name,
> > -                          const char **addrs, size_t n_addrs,
> > +                          struct sorted_addresses *addresses,
> >                            struct shash *sb_address_sets);
> >  static void sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >                             const struct nbrec_address_set_table *,
> > @@ -44,11 +50,17 @@ static void sync_addr_sets(struct ovsdb_idl_txn
> *ovnsb_txn,
> >                             const struct ovn_datapaths *lr_datapaths);
> >  static const struct sbrec_address_set *sb_address_set_lookup_by_name(
> >      struct ovsdb_idl_index *, const char *name);
> > -static void update_sb_addr_set(const char **nb_addresses, size_t
> n_addresses,
> > +static void update_sb_addr_set(struct sorted_addresses *,
> >                                 const struct sbrec_address_set *);
> >  static void build_port_group_address_set(const struct nbrec_port_group
> *,
> >                                           struct svec *ipv4_addrs,
> >                                           struct svec *ipv6_addrs);
> > +static struct sorted_addresses
> > +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as);
> > +static struct sorted_addresses
> > +sorted_addresses_from_svec(struct svec *addresses);
> > +static struct sorted_addresses
> > +sorted_addresses_from_sset(struct sset* addresses);
> >
> >  void *
> >  en_sync_to_sb_init(struct engine_node *node OVS_UNUSED,
> > @@ -133,8 +145,9 @@ sync_to_sb_addr_set_nb_address_set_handler(struct
> engine_node *node,
> >          if (!sb_addr_set) {
> >              return false;
> >          }
> > -        update_sb_addr_set((const char **) nb_addr_set->addresses,
> > -                           nb_addr_set->n_addresses, sb_addr_set);
> > +        struct sorted_addresses addrs =
> > +                sorted_addresses_from_nbrec(nb_addr_set);
> > +        update_sb_addr_set(&addrs, sb_addr_set);
> >      }
> >
> >      return true;
> > @@ -180,10 +193,14 @@ sync_to_sb_addr_set_nb_port_group_handler(struct
> engine_node *node,
> >          struct svec ipv4_addrs = SVEC_EMPTY_INITIALIZER;
> >          struct svec ipv6_addrs = SVEC_EMPTY_INITIALIZER;
> >          build_port_group_address_set(nb_pg, &ipv4_addrs, &ipv6_addrs);
> > -        update_sb_addr_set((const char **) ipv4_addrs.names,
> ipv4_addrs.n,
> > -                           sb_addr_set_v4);
> > -        update_sb_addr_set((const char **) ipv6_addrs.names,
> ipv6_addrs.n,
> > -                           sb_addr_set_v6);
> > +
> > +        struct sorted_addresses ipv4_addrs_sorted =
> > +                sorted_addresses_from_svec(&ipv4_addrs);
> > +        struct sorted_addresses ipv6_addrs_sorted =
> > +                sorted_addresses_from_svec(&ipv6_addrs);
> > +
> > +        update_sb_addr_set(&ipv4_addrs_sorted, sb_addr_set_v4);
> > +        update_sb_addr_set(&ipv6_addrs_sorted, sb_addr_set_v6);
> >
> >          free(ipv4_addrs_name);
> >          free(ipv6_addrs_name);
> > @@ -197,7 +214,7 @@ sync_to_sb_addr_set_nb_port_group_handler(struct
> engine_node *node,
> >  /* static functions. */
> >  static void
> >  sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn, const char *name,
> > -              const char **addrs, size_t n_addrs,
> > +              struct sorted_addresses *addresses,
> >                struct shash *sb_address_sets)
> >  {
> >      const struct sbrec_address_set *sb_address_set;
> > @@ -206,10 +223,10 @@ sync_addr_set(struct ovsdb_idl_txn *ovnsb_txn,
> const char *name,
> >      if (!sb_address_set) {
> >          sb_address_set = sbrec_address_set_insert(ovnsb_txn);
> >          sbrec_address_set_set_name(sb_address_set, name);
> > -        sbrec_address_set_set_addresses(sb_address_set,
> > -                                        addrs, n_addrs);
> > +        sbrec_address_set_set_addresses(sb_address_set, addresses->arr,
> > +                                        addresses->n);
> >      } else {
> > -        update_sb_addr_set(addrs, n_addrs, sb_address_set);
> > +        update_sb_addr_set(addresses, sb_address_set);
> >      }
> >  }
> >
> > @@ -244,8 +261,11 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >
> >      /* Service monitor MAC. */
> >      const char *svc_monitor_macp = northd_get_svc_monitor_mac();
> > -    sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc_monitor_macp, 1,
> > -                     &sb_address_sets);
> > +    struct sorted_addresses svc = {
> > +            .arr = &svc_monitor_macp,
> > +            .n = 1,
> > +    };
> > +    sync_addr_set(ovnsb_txn, "svc_monitor_mac", &svc, &sb_address_sets);
> >
> >      /* sync port group generated address sets first */
> >      const struct nbrec_port_group *nb_port_group;
> > @@ -256,14 +276,16 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >          build_port_group_address_set(nb_port_group, &ipv4_addrs,
> &ipv6_addrs);
> >          char *ipv4_addrs_name = xasprintf("%s_ip4",
> nb_port_group->name);
> >          char *ipv6_addrs_name = xasprintf("%s_ip6",
> nb_port_group->name);
> > +
> > +        struct sorted_addresses ipv4_addrs_sorted =
> > +                sorted_addresses_from_svec(&ipv4_addrs);
> > +        struct sorted_addresses ipv6_addrs_sorted =
> > +                sorted_addresses_from_svec(&ipv6_addrs);
> > +
> >          sync_addr_set(ovnsb_txn, ipv4_addrs_name,
> > -                      /* "char **" is not compatible with "const char
> **" */
> > -                      (const char **) ipv4_addrs.names,
> > -                      ipv4_addrs.n, &sb_address_sets);
> > +                      &ipv4_addrs_sorted, &sb_address_sets);
> >          sync_addr_set(ovnsb_txn, ipv6_addrs_name,
> > -                      /* "char **" is not compatible with "const char
> **" */
> > -                      (const char **) ipv6_addrs.names,
> > -                      ipv6_addrs.n, &sb_address_sets);
> > +                      &ipv6_addrs_sorted, &sb_address_sets);
> >          free(ipv4_addrs_name);
> >          free(ipv6_addrs_name);
> >          svec_destroy(&ipv4_addrs);
> > @@ -278,27 +300,26 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >          if (sset_count(&od->lb_ips->ips_v4_reachable)) {
> >              char *ipv4_addrs_name =
> lr_lb_address_set_name(od->tunnel_key,
> >                                                             AF_INET);
> > -            const char **ipv4_addrs =
> > -                sset_array(&od->lb_ips->ips_v4_reachable);
> >
> > -            sync_addr_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs,
> > -                          sset_count(&od->lb_ips->ips_v4_reachable),
> > -                          &sb_address_sets);
> > +            struct sorted_addresses ipv4_addrs_sorted =
> > +
>  sorted_addresses_from_sset(&od->lb_ips->ips_v4_reachable);
> > +
> > +            sync_addr_set(ovnsb_txn, ipv4_addrs_name,
> > +                          &ipv4_addrs_sorted, &sb_address_sets);
> > +            free(ipv4_addrs_sorted.arr);
> >              free(ipv4_addrs_name);
> > -            free(ipv4_addrs);
> >          }
> >
> >          if (sset_count(&od->lb_ips->ips_v6_reachable)) {
> >              char *ipv6_addrs_name =
> lr_lb_address_set_name(od->tunnel_key,
> >                                                             AF_INET6);
> > -            const char **ipv6_addrs =
> > -                sset_array(&od->lb_ips->ips_v6_reachable);
> > +            struct sorted_addresses ipv6_addrs_sorted =
> > +
>  sorted_addresses_from_sset(&od->lb_ips->ips_v6_reachable);
> >
> > -            sync_addr_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs,
> > -                          sset_count(&od->lb_ips->ips_v6_reachable),
> > -                          &sb_address_sets);
> > +            sync_addr_set(ovnsb_txn, ipv6_addrs_name,
> > +                          &ipv6_addrs_sorted, &sb_address_sets);
> > +            free(ipv6_addrs_sorted.arr);
> >              free(ipv6_addrs_name);
> > -            free(ipv6_addrs);
> >          }
> >      }
> >
> > @@ -307,10 +328,10 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >      const struct nbrec_address_set *nb_address_set;
> >      NBREC_ADDRESS_SET_TABLE_FOR_EACH (nb_address_set,
> >                                        nb_address_set_table) {
> > +        struct sorted_addresses addrs =
> > +                sorted_addresses_from_nbrec(nb_address_set);
> >          sync_addr_set(ovnsb_txn, nb_address_set->name,
> > -            /* "char **" is not compatible with "const char **" */
> > -            (const char **) nb_address_set->addresses,
> > -            nb_address_set->n_addresses, &sb_address_sets);
> > +                      &addrs, &sb_address_sets);
> >      }
> >
> >      struct shash_node *node;
> > @@ -322,48 +343,39 @@ sync_addr_sets(struct ovsdb_idl_txn *ovnsb_txn,
> >  }
> >
> >  static void
> > -update_sb_addr_set(const char **nb_addresses, size_t n_addresses,
> > +update_sb_addr_set(struct sorted_addresses *nb_addresses,
> >                     const struct sbrec_address_set *sb_as)
> >  {
> > -    struct expr_constant_set *cs_nb_as =
> > -        expr_constant_set_create_integers(
> > -            (const char *const *) nb_addresses, n_addresses);
> > -    struct expr_constant_set *cs_sb_as =
> > -        expr_constant_set_create_integers(
> > -            (const char *const *) sb_as->addresses, sb_as->n_addresses);
> > -
> > -    struct expr_constant_set *addr_added = NULL;
> > -    struct expr_constant_set *addr_deleted = NULL;
> > -    expr_constant_set_integers_diff(cs_sb_as, cs_nb_as, &addr_added,
> > -                                    &addr_deleted);
> > -
> > -    struct ds ds = DS_EMPTY_INITIALIZER;
> > -    if (addr_added && addr_added->n_values) {
> > -        for (size_t i = 0; i < addr_added->n_values; i++) {
> > -            ds_clear(&ds);
> > -            expr_constant_format(&addr_added->values[i],
> EXPR_C_INTEGER, &ds);
> > -            sbrec_address_set_update_addresses_addvalue(sb_as,
> ds_cstr(&ds));
> > +    size_t nb_index, sb_index;
> > +
> > +    const char **nb_arr = nb_addresses->arr;
> > +    char **sb_arr = sb_as->addresses;
> > +    size_t nb_n = nb_addresses->n;
> > +    size_t sb_n = sb_as->n_addresses;
> > +
> > +    for (nb_index = sb_index = 0; nb_index < nb_n && sb_index < sb_n; )
> {
> > +        int cmp = strcmp(nb_arr[nb_index], sb_arr[sb_index]);
> > +        if (cmp < 0) {
> > +            sbrec_address_set_update_addresses_addvalue(sb_as,
> > +
>  nb_arr[nb_index]);
> > +            nb_index++;
> > +        } else if (cmp > 0) {
> > +            sbrec_address_set_update_addresses_delvalue(sb_as,
> > +
>  sb_arr[sb_index]);
> > +            sb_index++;
> > +        } else {
> > +            nb_index++;
> > +            sb_index++;
> >          }
> >      }
> >
> > -    if (addr_deleted && addr_deleted->n_values) {
> > -        for (size_t i = 0; i < addr_deleted->n_values; i++) {
> > -            ds_clear(&ds);
> > -            expr_constant_format(&addr_deleted->values[i],
> > -                                 EXPR_C_INTEGER, &ds);
> > -            sbrec_address_set_update_addresses_delvalue(sb_as,
> ds_cstr(&ds));
> > -        }
> > +    for (; nb_index < nb_n; nb_index++) {
> > +        sbrec_address_set_update_addresses_addvalue(sb_as,
> nb_arr[nb_index]);
> >      }
> >
> > -    ds_destroy(&ds);
> > -    expr_constant_set_destroy(cs_nb_as);
> > -    free(cs_nb_as);
> > -    expr_constant_set_destroy(cs_sb_as);
> > -    free(cs_sb_as);
> > -    expr_constant_set_destroy(addr_added);
> > -    free(addr_added);
> > -    expr_constant_set_destroy(addr_deleted);
> > -    free(addr_deleted);
> > +    for (; sb_index < sb_n; sb_index++) {
> > +        sbrec_address_set_update_addresses_delvalue(sb_as,
> sb_arr[sb_index]);
> > +    }
> >  }
> >
> >  static void
> > @@ -402,3 +414,32 @@ sb_address_set_lookup_by_name(struct
> ovsdb_idl_index *sbrec_addr_set_by_name,
> >
> >      return retval;
> >  }
> > +
> > +static struct sorted_addresses
> > +sorted_addresses_from_nbrec(const struct nbrec_address_set *nb_as)
> > +{
> > +    /* The DB is already sorted. */
> > +    return (struct sorted_addresses) {
> > +        .arr = (const char **) nb_as->addresses,
> > +        .n = nb_as->n_addresses,
> > +    };
> > +}
> > +
> > +static struct sorted_addresses
> > +sorted_addresses_from_svec(struct svec *addresses)
> > +{
> > +    svec_sort(addresses);
> > +    return (struct sorted_addresses) {
> > +        .arr = (const char **) addresses->names,
> > +        .n = addresses->n,
> > +    };
> > +}
> > +
> > +static struct sorted_addresses
> > +sorted_addresses_from_sset(struct sset* addresses)
> > +{
> > +    return (struct sorted_addresses) {
> > +        .arr = sset_sort(addresses),
> > +        .n = sset_count(addresses),
> > +    };
> > +}
> > --
> > 2.40.1
> >
> > _______________________________________________
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
> Thanks Ales for the fix and sorry for not being able to review v1 sooner.
> This fix is straightforward, but I have a different suggestion.
> In my opinion normalizing the addresses before writing to SB has important
> advantages. For example, we can avoid invalid address strings or duplicated
> addresses, which can make ovn-controller implementation more clean. In
> general, northd is a good place to cleanup dirty data before writing to SB
> when the cost is manageable.
> So, would it be better to keep the update_sb_addr_set() implementation but
> modify the sync_addr_set() so that it always write normalized addresses to
> SB, to be consistent with update_sb_addr_set()?
>
> Regards,
> Han
>

Hi Han,

I guess both suggestions should be fine, however one thing I'm a bit afraid
of is that your suggestion will make it slightly slower when we sync
something new to SB. Because we would need to parse it, format it back,
store it in sset and only then add it to DB. This would also make the
parsing in ovn-controller slightly redundant (at least the mask
application). Anyway I don't have a strong opinion so I'll update it
accordingly to whatever was decided.

Thanks,
Ales

-- 

Ales Musil

Senior Software Engineer - OVN Core

Red Hat EMEA <https://www.redhat.com>

amu...@redhat.com    IM: amusil
<https://red.ht/sig>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to