On 9/9/22 17:08, Dumitru Ceara wrote: > 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.
No problem. Thanks for the review! I'll split the code out into functions. As we discussed off-list, the create() function in the patch #2 will require an extra argument 'max_datapaths', since we're not re-allocating arrays dynamically. Best regards, Ilya Maximets. > > 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
