On 9/12/22 22:53, Han Zhou wrote:
>
>
> On Fri, Sep 9, 2022 at 2:33 PM Ilya Maximets <i.maxim...@ovn.org
> <mailto:i.maxim...@ovn.org>> 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 <dce...@redhat.com <mailto:dce...@redhat.com>>
>> Signed-off-by: Ilya Maximets <i.maxim...@ovn.org <mailto:i.maxim...@ovn.org>>
>
> 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.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
