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