On 9/2/22 16:45, Ilya Maximets wrote:
> On 9/2/22 15:26, Dumitru Ceara wrote:
>> On 8/24/22 19:26, Ilya Maximets wrote:
>>> Filling arrays of switches and routers for load balancers
>>> one-by-one is not very efficient. Copying them in bulk
>>> allows to save a noticeable amount of time in setups with
>>> large load balancer groups.
>>>
>>> Signed-off-by: Ilya Maximets <[email protected]>
>>> ---
>>
>> The patch makes complete sense. I would change a few minor things, I
>> think. Please see below.
>>
>>> lib/lb.c | 16 ++++++++++------
>>> lib/lb.h | 8 ++++----
>>> northd/northd.c | 41 +++++++++++++++++++++++++++++------------
>>> 3 files changed, 43 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/lib/lb.c b/lib/lb.c
>>> index 7b0ed1abe..fe6070a40 100644
>>> --- a/lib/lb.c
>>> +++ b/lib/lb.c
>>> @@ -239,23 +239,27 @@ ovn_northd_lb_find(struct hmap *lbs, const struct
>>> uuid *uuid)
>>> }
>>>
>>> void
>>> -ovn_northd_lb_add_lr(struct ovn_northd_lb *lb, struct ovn_datapath *od)
>>> +ovn_northd_lb_add_lr(struct ovn_northd_lb *lb, size_t n,
>>> + struct ovn_datapath **ods)
>>> {
>>> - if (lb->n_allocated_nb_lr == lb->n_nb_lr) {
>>> + while (lb->n_allocated_nb_lr <= lb->n_nb_lr + n) {
>>> lb->nb_lr = x2nrealloc(lb->nb_lr, &lb->n_allocated_nb_lr,
>>> sizeof *lb->nb_lr);
>>> }
>>
>> Neat!
>>
>>> - lb->nb_lr[lb->n_nb_lr++] = od;
>>> + memcpy(&lb->nb_lr[lb->n_nb_lr], ods, n * sizeof *ods);
>>> + lb->n_nb_lr += n;
>>> }
>>>
>>> void
>>> -ovn_northd_lb_add_ls(struct ovn_northd_lb *lb, struct ovn_datapath *od)
>>> +ovn_northd_lb_add_ls(struct ovn_northd_lb *lb, size_t n,
>>> + struct ovn_datapath **ods)
>>> {
>>> - if (lb->n_allocated_nb_ls == lb->n_nb_ls) {
>>> + while (lb->n_allocated_nb_ls <= lb->n_nb_ls + n) {
>>> lb->nb_ls = x2nrealloc(lb->nb_ls, &lb->n_allocated_nb_ls,
>>> sizeof *lb->nb_ls);
>>> }
>>> - lb->nb_ls[lb->n_nb_ls++] = od;
>>> + memcpy(&lb->nb_ls[lb->n_nb_ls], ods, n * sizeof *ods);
>>> + lb->n_nb_ls += n;
>>> }
>>>
>>> void
>>> diff --git a/lib/lb.h b/lib/lb.h
>>> index 832ed31fb..d7bc28e18 100644
>>> --- a/lib/lb.h
>>> +++ b/lib/lb.h
>>> @@ -88,10 +88,10 @@ struct ovn_northd_lb_backend {
>>> struct ovn_northd_lb *ovn_northd_lb_create(const struct
>>> nbrec_load_balancer *);
>>> struct ovn_northd_lb * ovn_northd_lb_find(struct hmap *, const struct uuid
>>> *);
>>> void ovn_northd_lb_destroy(struct ovn_northd_lb *);
>>> -void
>>> -ovn_northd_lb_add_lr(struct ovn_northd_lb *lb, struct ovn_datapath *od);
>>> -void
>>> -ovn_northd_lb_add_ls(struct ovn_northd_lb *lb, struct ovn_datapath *od);
>>> +void ovn_northd_lb_add_lr(struct ovn_northd_lb *lb, size_t n,
>>> + struct ovn_datapath **ods);
>>> +void ovn_northd_lb_add_ls(struct ovn_northd_lb *lb, size_t n,
>>> + struct ovn_datapath **ods);
>>>
>>> struct ovn_controller_lb {
>>> const struct sbrec_load_balancer *slb; /* May be NULL. */
>>> diff --git a/northd/northd.c b/northd/northd.c
>>> index b66843581..c202d27d5 100644
>>> --- a/northd/northd.c
>>> +++ b/northd/northd.c
>>> @@ -3845,7 +3845,9 @@ struct ovn_lb_group {
>>> struct hmap_node hmap_node;
>>> struct uuid uuid;
>>> size_t n;
>>> - struct ovn_northd_lb *lbs[];
>>> + struct ovn_northd_lb **lbs;
>>> + size_t n_od;
>>> + struct ovn_datapath **ods;
>>> };
>>>
>>> static struct ovn_lb_group *
>>> @@ -3883,12 +3885,11 @@ build_lbs(struct northd_input *input_data, struct
>>> hmap *datapaths,
>>>
>>> NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
>>>
>>> input_data->nbrec_load_balancer_group_table) {
>>> - size_t size = sizeof *lb_group +
>>> - nbrec_lb_group->n_load_balancer * sizeof(struct ovn_northd_lb
>>> *);
>>> -
>>> - lb_group = xzalloc(size);
>>> + lb_group = xzalloc(sizeof *lb_group);
>>> lb_group->uuid = nbrec_lb_group->header_.uuid;
>>> lb_group->n = nbrec_lb_group->n_load_balancer;
>>> + lb_group->lbs = xzalloc(lb_group->n * sizeof *lb_group->lbs);
>>> + lb_group->ods = xzalloc(hmap_count(datapaths) * sizeof
>>> *lb_group->ods);
>>
>> In an unfortunate case when LB groups are used for a subset of datapaths
>> and the number of datapaths that don't use LB groups is significant then
>> this is inefficient. I'd go for x2nrealloc() below, when populating
>> 'lb_group->ods'.
>
> Maybe. OTOH, the number of LB groups is, likely, not that big, and the
> number of datapaths is also not that huge. 2n realloc will actually end
> up allocating more memory in a general case, I think. But I can change
> this part if you think otherwise. I just didn't want to introduce yet
> another variable to track the array size. And it will be 2 extra variables
> if we'll allocate separate arrays for switches and routers.
>
You're right, we probably will hit other issues before the size of the
array will become a problem if there are a lot of datapaths. I think
it's ok to leave it as is.
>>
>>>
>>> for (size_t i = 0; i < nbrec_lb_group->n_load_balancer; i++) {
>>> const struct uuid *lb_uuid =
>>> @@ -3910,19 +3911,25 @@ build_lbs(struct northd_input *input_data, struct
>>> hmap *datapaths,
>>> const struct uuid *lb_uuid =
>>> &od->nbs->load_balancer[i]->header_.uuid;
>>> lb = ovn_northd_lb_find(lbs, lb_uuid);
>>> - ovn_northd_lb_add_ls(lb, od);
>>> + ovn_northd_lb_add_ls(lb, 1, &od);
>>> }
>>>
>>> for (size_t i = 0; i < od->nbs->n_load_balancer_group; i++) {
>>> nbrec_lb_group = od->nbs->load_balancer_group[i];
>>> lb_group = ovn_lb_group_find(lb_groups,
>>> &nbrec_lb_group->header_.uuid);
>>> - for (size_t j = 0; j < lb_group->n; j++) {
>>> - ovn_northd_lb_add_ls(lb_group->lbs[j], od);
>>> - }
>>> + lb_group->ods[lb_group->n_od++] = od;
>>> }
>>> }
>>>
>>> + HMAP_FOR_EACH (lb_group, hmap_node, lb_groups) {
>>> + for (size_t j = 0; j < lb_group->n; j++) {
>>> + ovn_northd_lb_add_ls(lb_group->lbs[j], lb_group->n_od,
>>> + lb_group->ods);
>>> + }
>>> + lb_group->n_od = 0;
>>> + }
>>> +
>>> HMAP_FOR_EACH (od, key_node, datapaths) {
>>> if (!od->nbr) {
>>> continue;
>>> @@ -3932,7 +3939,7 @@ build_lbs(struct northd_input *input_data, struct
>>> hmap *datapaths,
>>> 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, od);
>>> + ovn_northd_lb_add_lr(lb, 1, &od);
>>> build_lrouter_lb_ips(od, lb);
>>> }
>>>
>>> @@ -3940,12 +3947,20 @@ build_lbs(struct northd_input *input_data, struct
>>> hmap *datapaths,
>>> nbrec_lb_group = od->nbr->load_balancer_group[i];
>>> lb_group = ovn_lb_group_find(lb_groups,
>>> &nbrec_lb_group->header_.uuid);
>>> + lb_group->ods[lb_group->n_od++] = od;
>>> for (size_t j = 0; j < lb_group->n; j++) {
>>> - ovn_northd_lb_add_lr(lb_group->lbs[j], od);
>>> build_lrouter_lb_ips(od, lb_group->lbs[j]);
>>> }
>>> }
>>> }
>>> +
>>> + HMAP_FOR_EACH (lb_group, hmap_node, lb_groups) {
>>> + for (size_t j = 0; j < lb_group->n; j++) {
>>> + ovn_northd_lb_add_lr(lb_group->lbs[j], lb_group->n_od,
>>> + lb_group->ods);
>>> + }
>>> + lb_group->n_od = 0;
>>
>> This is a bit confusing. That's because we reuse the 'lb_group->ods'
>> array; we use it first for switches and then for routers. I think I'd
>> just add two array (like we do for 'struct ovn_northd_lb'). It's more
>> verbose but there's no ambiguity then.
>
> LB group will be destroyed and re-created, so we can avoid re-setting
> the value here. I was trying to avoid having too many variables.
> And since both are datapaths, we can re-use the same array and not
> re-allocate it twice. Either way is fine for me, but having one array
> is slightly more efficient. What do you think?
>
I think I'd prefer two variables. I don't think the performance hit is
visibile but it's up to you I guess. I'm ok either way in the end.
If there are no other changes to this patch then:
Acked-by: Dumitru Ceara <[email protected]>
>>
>>> + }
>>> }
>>>
>>> static void
>>> @@ -4130,7 +4145,7 @@ build_lswitch_lbs_from_lrouter(struct hmap
>>> *datapaths, struct hmap *lbs)
>>> }
>>> }
>>> if (!installed) {
>>> - ovn_northd_lb_add_ls(lb, od);
>>> + ovn_northd_lb_add_ls(lb, 1, &od);
>>> }
>>> if (lb->nlb) {
>>> od->has_lb_vip |= lb_has_vip(lb->nlb);
>>> @@ -15403,6 +15418,8 @@ northd_destroy(struct northd_data *data)
>>>
>>> struct ovn_lb_group *lb_group;
>>> HMAP_FOR_EACH_POP (lb_group, hmap_node, &data->lb_groups) {
>>> + free(lb_group->lbs);
>>> + free(lb_group->ods);
>>> free(lb_group);
>>> }
>>> hmap_destroy(&data->lb_groups);
>>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
