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 <[email protected]> >> --- >> 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_); > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
