On 9/7/22 22: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]>
> ---
> lib/lb.c | 16 ++++++++++------
> lib/lb.h | 15 +++++++++++----
> northd/northd.c | 30 +++++++++++++++++++++++-------
> 3 files changed, 44 insertions(+), 17 deletions(-)
>
> diff --git a/lib/lb.c b/lib/lb.c
> index 6fb06bf87..f5c342c06 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);
> }
> - 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 e7b2fc61e..9f4fbdfa9 100644
> --- a/lib/lb.h
> +++ b/lib/lb.h
> @@ -27,6 +27,7 @@
> struct nbrec_load_balancer;
> struct sbrec_load_balancer;
> struct sbrec_datapath_binding;
> +struct ovn_datapath;
> struct ovn_port;
> struct uuid;
>
> @@ -89,16 +90,22 @@ 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_lb_group {
> struct hmap_node hmap_node;
> struct uuid uuid;
> size_t n_lbs;
> struct ovn_northd_lb **lbs;
> +
> + /* Datapaths to which this LB group is applied. */
> + size_t n_ls;
> + struct ovn_datapath **ls;
> + size_t n_lr;
> + struct ovn_datapath **lr;
> };
>
> struct ovn_lb_group *ovn_lb_group_find(struct hmap *lb_groups,
> diff --git a/northd/northd.c b/northd/northd.c
> index fbf4c8d6c..2cb3316cd 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3866,6 +3866,8 @@ build_lbs(struct northd_input *input_data, struct hmap
> *datapaths,
> lb_group->uuid = nbrec_lb_group->header_.uuid;
> lb_group->n_lbs = nbrec_lb_group->n_load_balancer;
> lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
> + lb_group->ls = xmalloc(hmap_count(datapaths) * sizeof *lb_group->ls);
> + lb_group->lr = xmalloc(hmap_count(datapaths) * sizeof *lb_group->lr);
>
> for (size_t i = 0; i < nbrec_lb_group->n_load_balancer; i++) {
> const struct uuid *lb_uuid =
> @@ -3887,16 +3889,21 @@ 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_lbs; j++) {
> - ovn_northd_lb_add_ls(lb_group->lbs[j], od);
> - }
> + lb_group->ls[lb_group->n_ls++] = od;
Nit: similar to the comments from patch 1/4, I'd add a small (inline?)
api to lib/lb.h to add a datapath to the set of switches/routers a
lb_group is applied to. E.g., ovn_lb_group_add_ls() and
ovn_lb_group_add_lr().
> + }
> + }
> +
> + HMAP_FOR_EACH (lb_group, hmap_node, lb_groups) {
> + for (size_t j = 0; j < lb_group->n_lbs; j++) {
> + ovn_northd_lb_add_ls(lb_group->lbs[j], lb_group->n_ls,
> + lb_group->ls);
> }
> }
>
> @@ -3909,7 +3916,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);
> }
>
> @@ -3917,12 +3924,19 @@ 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->lr[lb_group->n_lr++] = od;
Here too.
> for (size_t j = 0; j < lb_group->n_lbs; 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_lbs; j++) {
> + ovn_northd_lb_add_lr(lb_group->lbs[j], lb_group->n_lr,
> + lb_group->lr);
> + }
> + }
> }
>
> static void
> @@ -4107,7 +4121,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);
> @@ -15414,6 +15428,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->ls);
> + free(lb_group->lr);
If you add a ovn_lb_group_destroy() in patch 1/4 then this part should
be added to that function instead.
> free(lb_group);
> }
> hmap_destroy(&data->lb_groups);
The comments above are minor, and if you address them feel free to
include my ack in the next revision:
Acked-by: Dumitru Ceara <[email protected]>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
