On Tue, Jun 6, 2023 at 10:17 PM Han Zhou <[email protected]> wrote: > > > On Thu, Jun 1, 2023 at 2:55 PM Han Zhou <[email protected]> wrote: > > > > > > > > On Wed, May 31, 2023 at 11:16 PM Ales Musil <[email protected]> wrote: > > > > > > > > > > > > On Thu, Jun 1, 2023 at 7:44 AM Han Zhou <[email protected]> wrote: > > >> > > >> > > >> > > >> On Wed, May 24, 2023 at 11:27 PM Ales Musil <[email protected]> > 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: 张祖建 <[email protected]> > > >> > Signed-off-by: Ales Musil <[email protected]> > > >> > --- > > >> > 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 > > >> > [email protected] > > >> > 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 > > > > > > > It is a valid concern for the cost of conversion - now it doesn't matter > that much but when more I-P implemented this may become one of the next > bottlenecks. I have been thinking about more optimizations that we can do > with the address set preprocessed in northd, but those are not required for > the current bug fix. Now that you had this version working already, I think > we can go with your approach now. We can change it in the future whenever > it is needed. So > > > > Acked-by: Han Zhou <[email protected]> > > > > (Numan/Ilya may want to take another look since they had comments for v1) > > Hi Ales, > > Sorry that I forgot to ask if you could add a test case for this bug? > > Thanks, > Han >
Hi Han, no worries, added in v3. Thanks, Ales -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] IM: amusil <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
