On 8/24/22 19: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. > > Signed-off-by: Ilya Maximets <[email protected]> > ---
Hi Ilya, This is great, we should've spotted this inefficiency earlier, thanks for the fix! I left three comments below. If you address them feel free to add my ack to the next revision: Acked-by: Dumitru Ceara <[email protected]> Regards, Dumitru > northd/en-northd.c | 2 + > northd/northd.c | 103 +++++++++++++++++++++++++++++++++------------ > northd/northd.h | 3 ++ > 3 files changed, 81 insertions(+), 27 deletions(-) > > diff --git a/northd/en-northd.c b/northd/en-northd.c > index 4907a1ff2..7fe83db64 100644 > --- a/northd/en-northd.c > +++ b/northd/en-northd.c > @@ -68,6 +68,8 @@ void en_northd_run(struct engine_node *node, void *data) > EN_OVSDB_GET(engine_get_input("NB_logical_router", node)); > input_data.nbrec_load_balancer_table = > EN_OVSDB_GET(engine_get_input("NB_load_balancer", node)); > + input_data.nbrec_load_balancer_group_table = > + EN_OVSDB_GET(engine_get_input("NB_load_balancer_group", node)); > input_data.nbrec_port_group_table = > EN_OVSDB_GET(engine_get_input("NB_port_group", node)); > input_data.nbrec_address_set_table = > diff --git a/northd/northd.c b/northd/northd.c > index 33943bfaf..b66843581 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3841,13 +3841,37 @@ build_lrouter_lb_ips(struct ovn_datapath *od, const > struct ovn_northd_lb *lb) > } > } > > +struct ovn_lb_group { > + struct hmap_node hmap_node; > + struct uuid uuid; > + size_t n; Nit: It's more verbose, but in a lot of other places we use 'n_<array_name>' so I'd change this to 'n_lbs'. > + struct ovn_northd_lb *lbs[]; I see why it's useful for this to be an array but in patch 2/4 you turn it into a 'struct ovn_northd_lb **lbs'. To avoid multiple changes of the same thing throughout the patchset I'd make it 'struct ovn_northd_lb **lbs' from the beginning. > +}; > + > +static struct ovn_lb_group * > +ovn_lb_group_find(struct hmap *lb_groups, const struct uuid *uuid) > +{ > + struct ovn_lb_group *lb_group; > + size_t hash = uuid_hash(uuid); > + > + HMAP_FOR_EACH_WITH_HASH (lb_group, hmap_node, hash, lb_groups) { > + if (uuid_equals(&lb_group->uuid, uuid)) { > + return lb_group; > + } > + } > + return NULL; > +} This should go to lib/lb.[hc]. I know it's northd specific but that's where we have the 'struct ovn_northd_lb' definition too. > + > 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 +3881,25 @@ 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) { > + size_t size = sizeof *lb_group + > + nbrec_lb_group->n_load_balancer * sizeof(struct ovn_northd_lb *); > + > + lb_group = xzalloc(size); > + lb_group->uuid = nbrec_lb_group->header_.uuid; > + lb_group->n = nbrec_lb_group->n_load_balancer; > + > + 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); > + } > + > + 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 +3914,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; j++) { > + ovn_northd_lb_add_ls(lb_group->lbs[j], od); > } > } > } > @@ -3896,14 +3937,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; j++) { > + ovn_northd_lb_add_lr(lb_group->lbs[j], od); > + build_lrouter_lb_ips(od, lb_group->lbs[j]); > } > } > } > @@ -4021,7 +4060,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 +4078,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; j++) { > + build_lrouter_lb_reachable_ips(od, lb_group->lbs[j]); > } > } > } > @@ -4105,11 +4146,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); > } > @@ -15340,6 +15382,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) { > @@ -15358,6 +15401,12 @@ 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); > + } > + hmap_destroy(&data->lb_groups); > + > struct ovn_port_group *pg; > HMAP_FOR_EACH_SAFE (pg, key_node, &data->port_groups) { > ovn_port_group_destroy(&data->port_groups, pg); > @@ -15467,12 +15516,12 @@ ovnnb_db_run(struct northd_input *input_data, > > build_chassis_features(input_data, &data->features); > build_datapaths(input_data, ovnsb_txn, &data->datapaths, &data->lr_list); > - build_lbs(input_data, &data->datapaths, &data->lbs); > + build_lbs(input_data, &data->datapaths, &data->lbs, &data->lb_groups); > build_ports(input_data, ovnsb_txn, sbrec_chassis_by_name, > sbrec_chassis_by_hostname, > &data->datapaths, &data->ports); > build_lb_port_related_data(&data->datapaths, &data->ports, &data->lbs, > - input_data, ovnsb_txn); > + &data->lb_groups, input_data, ovnsb_txn); > build_ipam(&data->datapaths, &data->ports); > build_port_group_lswitches(input_data, &data->port_groups, &data->ports); > build_lrouter_groups(&data->ports, &data->lr_list); > diff --git a/northd/northd.h b/northd/northd.h > index d9856af97..8d299864f 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -28,6 +28,8 @@ struct northd_input { > const struct nbrec_logical_switch_table *nbrec_logical_switch; > const struct nbrec_logical_router_table *nbrec_logical_router; > const struct nbrec_load_balancer_table *nbrec_load_balancer_table; > + const struct nbrec_load_balancer_group_table > + *nbrec_load_balancer_group_table; > const struct nbrec_port_group_table *nbrec_port_group_table; > const struct nbrec_address_set_table *nbrec_address_set_table; > const struct nbrec_meter_table *nbrec_meter_table; > @@ -74,6 +76,7 @@ struct northd_data { > struct hmap port_groups; > struct shash meter_groups; > struct hmap lbs; > + struct hmap lb_groups; > struct hmap bfd_connections; > struct ovs_list lr_list; > bool ovn_internal_version_changed; _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
