On 5/23/23 10:09, Ales Musil 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 svec instead for the comparison which allows us to keep the > original strings. > > The change to svec shouldn't have any significant performance > impact, see the numbers below show. 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 svec: > Time spent on processing nb_cfg 1: > ovn-northd delay before processing: 19ms > ovn-northd completion: 548ms > ovn-controller(s) completion: 548ms > Time spent on processing nb_cfg 2: > ovn-northd delay before processing: 19ms > ovn-northd completion: 537ms > ovn-controller(s) completion: 537ms > Time spent on processing nb_cfg 3: > ovn-northd delay before processing: 15ms > ovn-northd completion: 522ms > ovn-controller(s) completion: 521ms > Time spent on processing nb_cfg 4: > ovn-northd delay before processing: 17ms > ovn-northd completion: 522ms > ovn-controller(s) completion: 520ms > > [0] > https://github.com/hzhou8/ovn-test-script/blob/e1149318201309db6d96ae8d5a33fcfbbe1872a3/test_ovn_controller_scale.sh > > Fixes: 0d5e0a6ced49 ("northd: Add I-P for syncing SB address sets.") > Reported-at: https://bugzilla.redhat.com/2196885
The issue was originally reported on the mailing list. Please, provide the link as well. And it'd also be nice to credit the original reporter. > Signed-off-by: Ales Musil <[email protected]> > --- > northd/en-sync-sb.c | 66 +++++++++++++++++++++------------------------ > 1 file changed, 30 insertions(+), 36 deletions(-) > > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c > index 758f00bfd..4bd77b168 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" > @@ -325,45 +324,40 @@ static void > update_sb_addr_set(const char **nb_addresses, size_t n_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 i; > + char *ip; > + > + struct svec svec_nb_as = SVEC_EMPTY_INITIALIZER; > + struct svec svec_sb_as = SVEC_EMPTY_INITIALIZER; > + > + for (i = 0; i < n_addresses; i++) { > + svec_add(&svec_nb_as, nb_addresses[i]); > } > > - 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 (i = 0; i < sb_as->n_addresses; i++) { > + svec_add(&svec_sb_as, sb_as->addresses[i]); > + } > + > + struct svec addr_added; > + struct svec addr_deleted; > + > + svec_sort(&svec_nb_as); > + svec_sort(&svec_sb_as); sb_as->addresses is a database column, it's already sorted. > + svec_diff(&svec_nb_as, &svec_sb_as, &addr_added, NULL, &addr_deleted); > + > + SVEC_FOR_EACH (i, ip, &addr_added) { > + sbrec_address_set_update_addresses_addvalue(sb_as, ip); > + } > + > + SVEC_FOR_EACH (i, ip, &addr_deleted) { > + sbrec_address_set_update_addresses_delvalue(sb_as, ip); > } I'm not sure if the svec_diff() function saves any lines of code here. But it definitely makes us waste a lot of time. The workflow here is: 1. Copy all the addresses from the nb set into svec. 2. Copy all the addresses from the sb set into svec. 3. Sort. 3.1 Sort nb svec. 3.2 Sort sb svec. 4. svec_diff() 4.1. Check that nb set is sorted by comparing each string to the next. 4.2 Check that sb set is sorted by comparing each string to the next. 4.3 Prepare the diff by comparing strings between nb and sb sets and copy ones that differ. 5. Update the sb database entry. 6. Destroy svecs and free the copies. Sounds like way too much data copies and string comparisons to me. As I pointed out above, the Sb database column is already sorted. We can make it a requirement that nb_addresses is sorted before this function is called. This way we will also avoid sorting nb addresses if they are also just taken directly from the database. And we will not need to create an extra copy of all of them. Then we could open-code the diff function that will just update the Sb column whenever the difference is found (addvalue/delvalue API doesn't update the record, so we can still safely iterate over it). This way the workflow will degrade down to steps 3.1 (if needed) and 4.3 + 5 combined, but without a copy. What do you think? Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
