On Wed, May 24, 2023 at 7:11 AM Ilya Maximets <[email protected]> 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.
>
> > 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.
This suggestion seems reasonable to me. The caller of the function
update_sb_addr_set()
can sort if the "nb_addresses" is not taken from the NB database column.
> 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?
>
The issue is seen because of the below code (present in
lex_parse_mask() in lib/lex.c)
----------
/* Apply invariant that a 1-bit in the value corresponds to a 1-bit in the
* mask. */
for (int i = 0; i < ARRAY_SIZE(token->mask.be32); i++) {
token->value.be32[i] &= token->mask.be32[i];
}
-----------
Maybe we can address this issue by adding a variant of this function
which doesn't overwrite the token->value ?
I'm fine with the svec approach too if there are no performance
degradations (looks like that's not the case with the provided
results of the scale script runs)
Numan
> Best regards, Ilya Maximets.
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
