| Message ID | 20220824172608.3092562-2-i.maximets@ovn.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | northd: Optimize preparation of load balancers. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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 <i.maximets@ovn.org> > --- 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 <dceara@redhat.com> 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;
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; + struct ovn_northd_lb *lbs[]; +}; + +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; +} + 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;
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 <i.maximets@ovn.org> --- northd/en-northd.c | 2 + northd/northd.c | 103 +++++++++++++++++++++++++++++++++------------ northd/northd.h | 3 ++ 3 files changed, 81 insertions(+), 27 deletions(-)