On Wed, May 24, 2023 at 1:11 PM Ilya Maximets <i.maxim...@ovn.org> wrote:
> 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. > Added in v2. > > > Signed-off-by: Ales Musil <amu...@redhat.com> > > --- > > 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. > > Sounds like a good idea. I went the svec way because it performed the same as the previous implementation as I wasn't sure if the DB value is ordered, now it's clear. Also it seems that this suggestion is even faster than the previous one. Updated in v2. 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
