On 9/7/22 22:26, Ilya Maximets wrote:
> ovn_northd_lb_lookup() takes significant percent of northd runtime
> in scenarios with large number of load balancers. In many cases,
> like in ovn-kubernetes, a lot of load balancers are actually groupped
> and applied to most of the switches and routers. So, instead of
> looking up all the same load balancers from the group for each
> datapath, we can look them up once and store as a group. Later we
> can lookup the entire group at once.
>
> Acked-by: Dumitru Ceara <[email protected]>
> Signed-off-by: Ilya Maximets <[email protected]>
> ---
[..]
> diff --git a/northd/northd.c b/northd/northd.c
> index deda4a9d3..fbf4c8d6c 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3843,11 +3843,14 @@ build_lrouter_lb_ips(struct ovn_datapath *od, const
> struct ovn_northd_lb *lb)
>
> static void
> build_lbs(struct northd_input *input_data, struct hmap *datapaths,
> - struct hmap *lbs)
> + struct hmap *lbs, struct hmap *lb_groups)
> {
> + const struct nbrec_load_balancer_group *nbrec_lb_group;
> + struct ovn_lb_group *lb_group;
> struct ovn_northd_lb *lb;
>
> hmap_init(lbs);
> + hmap_init(lb_groups);
>
> const struct nbrec_load_balancer *nbrec_lb;
> NBREC_LOAD_BALANCER_TABLE_FOR_EACH (nbrec_lb,
> @@ -3857,6 +3860,23 @@ build_lbs(struct northd_input *input_data, struct hmap
> *datapaths,
> uuid_hash(&nbrec_lb->header_.uuid));
> }
>
> + NBREC_LOAD_BALANCER_GROUP_TABLE_FOR_EACH (nbrec_lb_group,
> + input_data->nbrec_load_balancer_group_table) {
> + lb_group = xzalloc(sizeof *lb_group);
> + 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);
> +
> + for (size_t i = 0; i < nbrec_lb_group->n_load_balancer; i++) {
> + const struct uuid *lb_uuid =
> + &nbrec_lb_group->load_balancer[i]->header_.uuid;
> + lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
> + }
Would it be possible to factor this part out into a
ovn_lb_group_create() function in lib/lb.c, similar to what we do with
ovn_northd_lb_create()?
> +
> + hmap_insert(lb_groups, &lb_group->hmap_node,
> + uuid_hash(&lb_group->uuid));
> + }
> +
> struct ovn_datapath *od;
> HMAP_FOR_EACH (od, key_node, datapaths) {
> if (!od->nbs) {
> @@ -3871,13 +3891,11 @@ build_lbs(struct northd_input *input_data, struct
> hmap *datapaths,
> }
>
> for (size_t i = 0; i < od->nbs->n_load_balancer_group; i++) {
> - const struct nbrec_load_balancer_group *lbg =
> - od->nbs->load_balancer_group[i];
> - for (size_t j = 0; j < lbg->n_load_balancer; j++) {
> - const struct uuid *lb_uuid =
> - &lbg->load_balancer[j]->header_.uuid;
> - lb = ovn_northd_lb_find(lbs, lb_uuid);
> - ovn_northd_lb_add_ls(lb, od);
> + 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);
> }
> }
> }
> @@ -3896,14 +3914,12 @@ build_lbs(struct northd_input *input_data, struct
> hmap *datapaths,
> }
>
> for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> - const struct nbrec_load_balancer_group *lbg =
> - od->nbr->load_balancer_group[i];
> - for (size_t j = 0; j < lbg->n_load_balancer; j++) {
> - const struct uuid *lb_uuid =
> - &lbg->load_balancer[j]->header_.uuid;
> - lb = ovn_northd_lb_find(lbs, lb_uuid);
> - ovn_northd_lb_add_lr(lb, od);
> - build_lrouter_lb_ips(od, lb);
> + nbrec_lb_group = od->nbr->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_lr(lb_group->lbs[j], od);
> + build_lrouter_lb_ips(od, lb_group->lbs[j]);
> }
> }
> }
> @@ -4021,7 +4037,8 @@ build_lrouter_lbs_check(const struct hmap *datapaths)
> }
>
> static void
> -build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs)
> +build_lrouter_lbs_reachable_ips(struct hmap *datapaths, struct hmap *lbs,
> + struct hmap *lb_groups)
> {
> struct ovn_datapath *od;
>
> @@ -4038,13 +4055,14 @@ build_lrouter_lbs_reachable_ips(struct hmap
> *datapaths, struct hmap *lbs)
> }
>
> for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
> - const struct nbrec_load_balancer_group *lbg =
> + const struct nbrec_load_balancer_group *nbrec_lb_group =
> od->nbr->load_balancer_group[i];
> - for (size_t j = 0; j < lbg->n_load_balancer; j++) {
> - struct ovn_northd_lb *lb =
> - ovn_northd_lb_find(lbs,
> - &lbg->load_balancer[j]->header_.uuid);
> - build_lrouter_lb_reachable_ips(od, lb);
> + struct ovn_lb_group *lb_group;
> +
> + lb_group = ovn_lb_group_find(lb_groups,
> + &nbrec_lb_group->header_.uuid);
> + for (size_t j = 0; j < lb_group->n_lbs; j++) {
> + build_lrouter_lb_reachable_ips(od, lb_group->lbs[j]);
> }
> }
> }
> @@ -4105,11 +4123,12 @@ build_lswitch_lbs_from_lrouter(struct hmap
> *datapaths, struct hmap *lbs)
> */
> static void
> build_lb_port_related_data(struct hmap *datapaths, struct hmap *ports,
> - struct hmap *lbs, struct northd_input *input_data,
> + struct hmap *lbs, struct hmap *lb_groups,
> + struct northd_input *input_data,
> struct ovsdb_idl_txn *ovnsb_txn)
> {
> build_lrouter_lbs_check(datapaths);
> - build_lrouter_lbs_reachable_ips(datapaths, lbs);
> + build_lrouter_lbs_reachable_ips(datapaths, lbs, lb_groups);
> build_lb_svcs(input_data, ovnsb_txn, ports, lbs);
> build_lswitch_lbs_from_lrouter(datapaths, lbs);
> }
> @@ -15373,6 +15392,7 @@ northd_init(struct northd_data *data)
> hmap_init(&data->port_groups);
> shash_init(&data->meter_groups);
> hmap_init(&data->lbs);
> + hmap_init(&data->lb_groups);
> hmap_init(&data->bfd_connections);
> ovs_list_init(&data->lr_list);
> data->features = (struct chassis_features) {
> @@ -15391,6 +15411,13 @@ northd_destroy(struct northd_data *data)
> }
> hmap_destroy(&data->lbs);
>
> + struct ovn_lb_group *lb_group;
> + HMAP_FOR_EACH_POP (lb_group, hmap_node, &data->lb_groups) {
> + free(lb_group->lbs);
> + free(lb_group);
Here too, we could move this to a ovn_lb_group_destroy() function in
lib/lb.c, for consistency.
Sorry for the late suggestions, I should've thought about this during
the review of the previous revision.
If these are the only relevant changes you make in the next revision
please feel free to keep my ack.
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
