| Message ID | 20220824172608.3092562-4-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-_ovn-kubernetes | success | github build: passed |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On 8/24/22 19:26, Ilya Maximets wrote: > Options like 'add_route' and 'neighbor_responder' are looked up > in the smap for each load balancer for each datapath and it takes > significant amount of time. Checking them only once to speed up Makes complete sense! I'm sure we have other places in the code base where we should do this too but for now let's focus on LBs in northd. > processing. 'skip_snat' is not that critical, but it's better to > get all of them in a single place. > To be consistent we should do the same thing for the controller_event flag, NB.Load_Balancer.options:event. Also, it looks like we still have a "skip_snat" lookup in build_lrouter_flows_for_lb(). > Also using enum for the heighbor responder mode to avoid costly Typo: neighbor. > string comparison on a hot path. > > Signed-off-by: Ilya Maximets <i.maximets@ovn.org> > --- > lib/lb.c | 7 +++++++ > lib/lb.h | 9 +++++++++ > northd/northd.c | 19 +++++++------------ > 3 files changed, 23 insertions(+), 12 deletions(-) > > diff --git a/lib/lb.c b/lib/lb.c > index fe6070a40..41564fe9b 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -173,6 +173,13 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb) > lb->n_vips = smap_count(&nbrec_lb->vips); > lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips); > lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb); > + lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false); > + lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat", false); > + const char *mode = > + smap_get_def(&nbrec_lb->options, "neighbor_responder", "reachable"); > + lb->neighbor_responder_mode = strcmp(mode, "all") > + ? LB_NEIGH_RESPOND_REACHABLE > + : LB_NEIGH_RESPOND_ALL; > sset_init(&lb->ips_v4); > sset_init(&lb->ips_v6); > struct smap_node *node; > diff --git a/lib/lb.h b/lib/lb.h > index d7bc28e18..add934fef 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -29,6 +29,11 @@ struct sbrec_datapath_binding; > struct ovn_port; > struct uuid; > > +enum lb_neighbor_responder_mode { > + LB_NEIGH_RESPOND_REACHABLE, > + LB_NEIGH_RESPOND_ALL, > +}; > + > struct ovn_northd_lb { > struct hmap_node hmap_node; > > @@ -40,6 +45,10 @@ struct ovn_northd_lb { > struct ovn_northd_lb_vip *vips_nb; > size_t n_vips; > > + bool routable; > + bool skip_snat; > + enum lb_neighbor_responder_mode neighbor_responder_mode; Super nit: If you send a v2, please move this as first flag, before 'routable'. Can we use a shorter name? Does 'neigh_mode' work too for you? > + > struct sset ips_v4; > struct sset ips_v6; > > diff --git a/northd/northd.c b/northd/northd.c > index c202d27d5..a942346bd 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -3824,18 +3824,17 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, > static void > build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) > { > - bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); > const char *ip_address; > > SSET_FOR_EACH (ip_address, &lb->ips_v4) { > sset_add(&od->lb_ips_v4, ip_address); > - if (is_routable) { > + if (lb->routable) { > sset_add(&od->lb_ips_v4_routable, ip_address); > } > } > SSET_FOR_EACH (ip_address, &lb->ips_v6) { > sset_add(&od->lb_ips_v6, ip_address); > - if (is_routable) { > + if (lb->routable) { > sset_add(&od->lb_ips_v6_routable, ip_address); > } > } > @@ -4007,13 +4006,10 @@ static void > build_lrouter_lb_reachable_ips(struct ovn_datapath *od, > const struct ovn_northd_lb *lb) > { > - const char *neighbor_responder_mode = > - smap_get_def(&lb->nlb->options, "neighbor_responder", "reachable"); > - > /* If configured to reply to neighbor requests for all VIPs force them > * all to be considered "reachable". > */ > - if (!strcmp(neighbor_responder_mode, "all")) { > + if (lb->neighbor_responder_mode == LB_NEIGH_RESPOND_ALL) { > for (size_t i = 0; i < lb->n_vips; i++) { > if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) { > sset_add(&od->lb_ips_v4_reachable, lb->vips[i].vip_str); > @@ -9958,8 +9954,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > lb_vip->vip_str); > } > > - bool lb_skip_snat = smap_get_bool(&lb->nlb->options, "skip_snat", false); > - if (lb_skip_snat) { > + if (lb->skip_snat) { > skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s", > ds_cstr(action)); > skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; " > @@ -10043,7 +10038,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > for (size_t i = 0; i < lb->n_nb_lr; i++) { > struct ovn_datapath *od = lb->nb_lr[i]; > if (!od->n_l3dgw_ports) { > - if (lb_skip_snat) { > + if (lb->skip_snat) { > gw_router_skip_snat[n_gw_router_skip_snat++] = od; > } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || > od->lb_force_snat_router_ip) { > @@ -10114,7 +10109,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > od->l3dgw_ports[0]->cr_port->json_key); > } > > - if (lb_skip_snat) { > + if (lb->skip_snat) { > ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio, > new_match_p, skip_snat_new_action, > NULL, meter, &lb->nlb->header_); > @@ -10154,7 +10149,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, > ds_cstr(&undnat_match), > od->l3dgw_ports[0]->json_key, > od->l3dgw_ports[0]->cr_port->json_key); > - if (lb_skip_snat) { > + if (lb->skip_snat) { > ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, > undnat_match_p, skip_snat_est_action, > &lb->nlb->header_);
On 9/2/22 15:26, Dumitru Ceara wrote: > On 8/24/22 19:26, Ilya Maximets wrote: >> Options like 'add_route' and 'neighbor_responder' are looked up >> in the smap for each load balancer for each datapath and it takes >> significant amount of time. Checking them only once to speed up > > Makes complete sense! I'm sure we have other places in the code base > where we should do this too but for now let's focus on LBs in northd. > >> processing. 'skip_snat' is not that critical, but it's better to >> get all of them in a single place. >> > > To be consistent we should do the same thing for the controller_event > flag, NB.Load_Balancer.options:event. Oops. Missed that one. Will do. > > Also, it looks like we still have a "skip_snat" lookup in > build_lrouter_flows_for_lb(). > >> Also using enum for the heighbor responder mode to avoid costly > > Typo: neighbor. > >> string comparison on a hot path. >> >> Signed-off-by: Ilya Maximets <i.maximets@ovn.org> >> --- >> lib/lb.c | 7 +++++++ >> lib/lb.h | 9 +++++++++ >> northd/northd.c | 19 +++++++------------ >> 3 files changed, 23 insertions(+), 12 deletions(-) >> >> diff --git a/lib/lb.c b/lib/lb.c >> index fe6070a40..41564fe9b 100644 >> --- a/lib/lb.c >> +++ b/lib/lb.c >> @@ -173,6 +173,13 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb) >> lb->n_vips = smap_count(&nbrec_lb->vips); >> lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips); >> lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb); >> + lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false); >> + lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat", false); >> + const char *mode = >> + smap_get_def(&nbrec_lb->options, "neighbor_responder", "reachable"); >> + lb->neighbor_responder_mode = strcmp(mode, "all") >> + ? LB_NEIGH_RESPOND_REACHABLE >> + : LB_NEIGH_RESPOND_ALL; >> sset_init(&lb->ips_v4); >> sset_init(&lb->ips_v6); >> struct smap_node *node; >> diff --git a/lib/lb.h b/lib/lb.h >> index d7bc28e18..add934fef 100644 >> --- a/lib/lb.h >> +++ b/lib/lb.h >> @@ -29,6 +29,11 @@ struct sbrec_datapath_binding; >> struct ovn_port; >> struct uuid; >> >> +enum lb_neighbor_responder_mode { >> + LB_NEIGH_RESPOND_REACHABLE, >> + LB_NEIGH_RESPOND_ALL, >> +}; >> + >> struct ovn_northd_lb { >> struct hmap_node hmap_node; >> >> @@ -40,6 +45,10 @@ struct ovn_northd_lb { >> struct ovn_northd_lb_vip *vips_nb; >> size_t n_vips; >> >> + bool routable; >> + bool skip_snat; >> + enum lb_neighbor_responder_mode neighbor_responder_mode; > > Super nit: If you send a v2, please move this as first flag, before > 'routable'. Can we use a shorter name? Does 'neigh_mode' work too for you? Sounds OK. > >> + >> struct sset ips_v4; >> struct sset ips_v6; >> >> diff --git a/northd/northd.c b/northd/northd.c >> index c202d27d5..a942346bd 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -3824,18 +3824,17 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, >> static void >> build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) >> { >> - bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); >> const char *ip_address; >> >> SSET_FOR_EACH (ip_address, &lb->ips_v4) { >> sset_add(&od->lb_ips_v4, ip_address); >> - if (is_routable) { >> + if (lb->routable) { >> sset_add(&od->lb_ips_v4_routable, ip_address); >> } >> } >> SSET_FOR_EACH (ip_address, &lb->ips_v6) { >> sset_add(&od->lb_ips_v6, ip_address); >> - if (is_routable) { >> + if (lb->routable) { >> sset_add(&od->lb_ips_v6_routable, ip_address); >> } >> } >> @@ -4007,13 +4006,10 @@ static void >> build_lrouter_lb_reachable_ips(struct ovn_datapath *od, >> const struct ovn_northd_lb *lb) >> { >> - const char *neighbor_responder_mode = >> - smap_get_def(&lb->nlb->options, "neighbor_responder", "reachable"); >> - >> /* If configured to reply to neighbor requests for all VIPs force them >> * all to be considered "reachable". >> */ >> - if (!strcmp(neighbor_responder_mode, "all")) { >> + if (lb->neighbor_responder_mode == LB_NEIGH_RESPOND_ALL) { >> for (size_t i = 0; i < lb->n_vips; i++) { >> if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) { >> sset_add(&od->lb_ips_v4_reachable, lb->vips[i].vip_str); >> @@ -9958,8 +9954,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, >> lb_vip->vip_str); >> } >> >> - bool lb_skip_snat = smap_get_bool(&lb->nlb->options, "skip_snat", false); >> - if (lb_skip_snat) { >> + if (lb->skip_snat) { >> skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s", >> ds_cstr(action)); >> skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; " >> @@ -10043,7 +10038,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, >> for (size_t i = 0; i < lb->n_nb_lr; i++) { >> struct ovn_datapath *od = lb->nb_lr[i]; >> if (!od->n_l3dgw_ports) { >> - if (lb_skip_snat) { >> + if (lb->skip_snat) { >> gw_router_skip_snat[n_gw_router_skip_snat++] = od; >> } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || >> od->lb_force_snat_router_ip) { >> @@ -10114,7 +10109,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, >> od->l3dgw_ports[0]->cr_port->json_key); >> } >> >> - if (lb_skip_snat) { >> + if (lb->skip_snat) { >> ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio, >> new_match_p, skip_snat_new_action, >> NULL, meter, &lb->nlb->header_); >> @@ -10154,7 +10149,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, >> ds_cstr(&undnat_match), >> od->l3dgw_ports[0]->json_key, >> od->l3dgw_ports[0]->cr_port->json_key); >> - if (lb_skip_snat) { >> + if (lb->skip_snat) { >> ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, >> undnat_match_p, skip_snat_est_action, >> &lb->nlb->header_); >
diff --git a/lib/lb.c b/lib/lb.c index fe6070a40..41564fe9b 100644 --- a/lib/lb.c +++ b/lib/lb.c @@ -173,6 +173,13 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb) lb->n_vips = smap_count(&nbrec_lb->vips); lb->vips = xcalloc(lb->n_vips, sizeof *lb->vips); lb->vips_nb = xcalloc(lb->n_vips, sizeof *lb->vips_nb); + lb->routable = smap_get_bool(&nbrec_lb->options, "add_route", false); + lb->skip_snat = smap_get_bool(&nbrec_lb->options, "skip_snat", false); + const char *mode = + smap_get_def(&nbrec_lb->options, "neighbor_responder", "reachable"); + lb->neighbor_responder_mode = strcmp(mode, "all") + ? LB_NEIGH_RESPOND_REACHABLE + : LB_NEIGH_RESPOND_ALL; sset_init(&lb->ips_v4); sset_init(&lb->ips_v6); struct smap_node *node; diff --git a/lib/lb.h b/lib/lb.h index d7bc28e18..add934fef 100644 --- a/lib/lb.h +++ b/lib/lb.h @@ -29,6 +29,11 @@ struct sbrec_datapath_binding; struct ovn_port; struct uuid; +enum lb_neighbor_responder_mode { + LB_NEIGH_RESPOND_REACHABLE, + LB_NEIGH_RESPOND_ALL, +}; + struct ovn_northd_lb { struct hmap_node hmap_node; @@ -40,6 +45,10 @@ struct ovn_northd_lb { struct ovn_northd_lb_vip *vips_nb; size_t n_vips; + bool routable; + bool skip_snat; + enum lb_neighbor_responder_mode neighbor_responder_mode; + struct sset ips_v4; struct sset ips_v6; diff --git a/northd/northd.c b/northd/northd.c index c202d27d5..a942346bd 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -3824,18 +3824,17 @@ build_lb_vip_actions(struct ovn_lb_vip *lb_vip, static void build_lrouter_lb_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) { - bool is_routable = smap_get_bool(&lb->nlb->options, "add_route", false); const char *ip_address; SSET_FOR_EACH (ip_address, &lb->ips_v4) { sset_add(&od->lb_ips_v4, ip_address); - if (is_routable) { + if (lb->routable) { sset_add(&od->lb_ips_v4_routable, ip_address); } } SSET_FOR_EACH (ip_address, &lb->ips_v6) { sset_add(&od->lb_ips_v6, ip_address); - if (is_routable) { + if (lb->routable) { sset_add(&od->lb_ips_v6_routable, ip_address); } } @@ -4007,13 +4006,10 @@ static void build_lrouter_lb_reachable_ips(struct ovn_datapath *od, const struct ovn_northd_lb *lb) { - const char *neighbor_responder_mode = - smap_get_def(&lb->nlb->options, "neighbor_responder", "reachable"); - /* If configured to reply to neighbor requests for all VIPs force them * all to be considered "reachable". */ - if (!strcmp(neighbor_responder_mode, "all")) { + if (lb->neighbor_responder_mode == LB_NEIGH_RESPOND_ALL) { for (size_t i = 0; i < lb->n_vips; i++) { if (IN6_IS_ADDR_V4MAPPED(&lb->vips[i].vip)) { sset_add(&od->lb_ips_v4_reachable, lb->vips[i].vip_str); @@ -9958,8 +9954,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, lb_vip->vip_str); } - bool lb_skip_snat = smap_get_bool(&lb->nlb->options, "skip_snat", false); - if (lb_skip_snat) { + if (lb->skip_snat) { skip_snat_new_action = xasprintf("flags.skip_snat_for_lb = 1; %s", ds_cstr(action)); skip_snat_est_action = xasprintf("flags.skip_snat_for_lb = 1; " @@ -10043,7 +10038,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, for (size_t i = 0; i < lb->n_nb_lr; i++) { struct ovn_datapath *od = lb->nb_lr[i]; if (!od->n_l3dgw_ports) { - if (lb_skip_snat) { + if (lb->skip_snat) { gw_router_skip_snat[n_gw_router_skip_snat++] = od; } else if (!lport_addresses_is_empty(&od->lb_force_snat_addrs) || od->lb_force_snat_router_ip) { @@ -10114,7 +10109,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, od->l3dgw_ports[0]->cr_port->json_key); } - if (lb_skip_snat) { + if (lb->skip_snat) { ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT, prio, new_match_p, skip_snat_new_action, NULL, meter, &lb->nlb->header_); @@ -10154,7 +10149,7 @@ build_lrouter_nat_flows_for_lb(struct ovn_lb_vip *lb_vip, ds_cstr(&undnat_match), od->l3dgw_ports[0]->json_key, od->l3dgw_ports[0]->cr_port->json_key); - if (lb_skip_snat) { + if (lb->skip_snat) { ovn_lflow_add_with_hint(lflows, od, S_ROUTER_OUT_UNDNAT, 120, undnat_match_p, skip_snat_est_action, &lb->nlb->header_);
Options like 'add_route' and 'neighbor_responder' are looked up in the smap for each load balancer for each datapath and it takes significant amount of time. Checking them only once to speed up processing. 'skip_snat' is not that critical, but it's better to get all of them in a single place. Also using enum for the heighbor responder mode to avoid costly string comparison on a hot path. Signed-off-by: Ilya Maximets <i.maximets@ovn.org> --- lib/lb.c | 7 +++++++ lib/lb.h | 9 +++++++++ northd/northd.c | 19 +++++++------------ 3 files changed, 23 insertions(+), 12 deletions(-)