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.
>
>>
>> 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?
>
>> + }
>> }
>>
>> 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
