On Tue, Sep 13, 2022 at 3:18 AM Ilya Maximets <[email protected]> wrote: > > On 9/12/22 22:53, Han Zhou wrote: > > > > > > On Fri, Sep 9, 2022 at 2:33 PM Ilya Maximets <[email protected] <mailto:[email protected]>> wrote: > >> > >> If the same load balancer group is attached to multiple routers, > >> IP sets will be constructed for each of them by sequentially calling > >> sset_add() for each IP for each load balancer for each router. > >> Instead of doing that, we can create IP sets for load balancer > >> groups and clone them. That will avoid extra ssed_find__() call > >> making the construction of IP sets 2x faster. > >> > >> Only first load balancer group is cloned, the rest as well as > >> standalone load balancers are still added one by one, because > >> there is no more efficient way to merge sets. > >> > >> The order of processing changed to make sure that we're actually > >> optimizing copy of a large group. > >> > >> The code can be optimized further by introduction of a reference > >> counter and not copying at all if the router has no standalone load > >> balancers and only one group. Also, construction of "reachable" > >> sets can be optimized for the "neighbor_responder = all" case. But, > >> for now, only moved to a new structure for better readability. > >> > >> Acked-by: Dumitru Ceara <[email protected] <mailto:[email protected]>> > >> Signed-off-by: Ilya Maximets <[email protected] <mailto: [email protected]>> > > > > Thanks Ilya and Dumitru. I merged the 3 previous commits. This last patch looks good to me, too, but please see a minor comment below. > > > >> --- > >> @@ -3900,23 +3895,34 @@ build_lbs(struct northd_input *input_data, struct hmap *datapaths, > >> continue; > >> } > >> > >> - for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { > >> - const struct uuid *lb_uuid = > >> - &od->nbr->load_balancer[i]->header_.uuid; > >> - lb = ovn_northd_lb_find(lbs, lb_uuid); > >> - ovn_northd_lb_add_lr(lb, 1, &od); > >> - build_lrouter_lb_ips(od, lb); > >> - } > >> - > >> + /* Checking load balancer groups first to more efficiently copy > >> + * IP sets for large groups. */ > >> for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { > >> nbrec_lb_group = od->nbr->load_balancer_group[i]; > >> lb_group = ovn_lb_group_find(lb_groups, > >> &nbrec_lb_group->header_.uuid); > >> ovn_lb_group_add_lr(lb_group, od); > >> - for (size_t j = 0; j < lb_group->n_lbs; j++) { > >> - build_lrouter_lb_ips(od, lb_group->lbs[j]); > >> + > >> + if (!od->lb_ips) { > >> + od->lb_ips = ovn_lb_ip_set_clone(lb_group->lb_ips); > > > > Shall we do this for the lb_group that has the largest number of VIPs instead of the first lb_group? Otherwise, if the first lb_group happens to be a small one, is the optimization of this patch not effective? Since we don't expect a big number lb_groups, I think a simple iteration should be sufficient for this check. > > I think, in most scenarios that we hit today, there is only one > big cluster-wide group. And other load balancers are applied > without grouping. So, it's not a big concern for a time being. > > But I actually thought about trying to find the largest group to > optimize better, just didn't want to make the code more complex. > In any case, it doesn't hurt to implement this and make the code > a bit more future-proof. > > One note here is that we don't really need the largest group, > but a group with a largest number of unique IPs. At the same > time, I'd like to avoid extra hash map lookups. So, the size > of a group itself sounds like a fair approximations. > > If that makes sense, the following diff should do the trick: > > diff --git a/northd/northd.c b/northd/northd.c > index f2c0f5aed..a3f0112c8 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3895,10 +3895,21 @@ build_lbs(struct northd_input *input_data, struct hmap *datapaths, > continue; > } > > - /* Checking load balancer groups first to more efficiently copy > - * IP sets for large groups. */ > + /* Checking load balancer groups first, starting from the largest one, > + * to more efficiently copy IP sets. */ > + size_t largest_group = 0; > + > + for (size_t i = 1; i < od->nbr->n_load_balancer_group; i++) { > + if (od->nbr->load_balancer_group[i]->n_load_balancer > > + od->nbr->load_balancer_group[largest_group]->n_load_balancer) { > + largest_group = i; > + } > + } > + > for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) { > - nbrec_lb_group = od->nbr->load_balancer_group[i]; > + size_t idx = (i + largest_group) % od->nbr->n_load_balancer_group; > + > + nbrec_lb_group = od->nbr->load_balancer_group[idx]; > lb_group = ovn_lb_group_find(lb_groups, > &nbrec_lb_group->header_.uuid); > ovn_lb_group_add_lr(lb_group, od); > --- > > Han, Dumitru, what do you think? > > I can post v4 with that change, or you may fold it in while > applying the change. Either way is fine by me. > > Best regards, Ilya Maximets.
Thanks Ilya, I think this is good enough. I applied this diff and pushed to the main branch. Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
