Message ID | 20210610124807.30043.68684.stgit@dceara.remote.csb |
---|---|
State | Accepted |
Headers | show |
Series | Optimize load balancer IP config parsing. | expand |
On Thu, Jun 10, 2021 at 8:48 AM Dumitru Ceara <dceara@redhat.com> wrote: > > There's no need to parse the IP sets every time we iterate through them. > It's enough to parse them once for every main loop iteration. > > Reported-at: https://bugzilla.redhat.com/1962338 > Signed-off-by: Dumitru Ceara <dceara@redhat.com> Thanks for the series. I pushed all the 3 patches of the series to the main branch with below small changes in patch 3. ---- diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 17ed550755..aae7314c89 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -6554,8 +6554,7 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, struct ds ips_v6_match = DS_EMPTY_INITIALIZER; const char *ip_addr; - const char *ip_addr_next; - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v4) { + SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v4) { ovs_be32 ipv4_addr; /* Check if the ovn port has a network configured on which we could @@ -6566,7 +6565,7 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, ds_put_format(&ips_v4_match, "%s, ", ip_addr); } } - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v6) { + SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v6) { struct in6_addr ipv6_addr; /* Check if the ovn port has a network configured on which we could ------- Thanks Numan > --- > lib/lb.c | 9 +++ > lib/lb.h | 4 + > northd/ovn-northd.c | 175 ++++++++++++++++++++++++--------------------------- > 3 files changed, 95 insertions(+), 93 deletions(-) > > diff --git a/lib/lb.c b/lib/lb.c > index 18dc226e9..4cb46b346 100644 > --- a/lib/lb.c > +++ b/lib/lb.c > @@ -170,6 +170,8 @@ 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); > + sset_init(&lb->ips_v4); > + sset_init(&lb->ips_v6); > struct smap_node *node; > size_t n_vips = 0; > > @@ -184,6 +186,11 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb) > } > ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb, > node->key, node->value); > + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { > + sset_add(&lb->ips_v4, lb_vip->vip_str); > + } else { > + sset_add(&lb->ips_v6, lb_vip->vip_str); > + } > n_vips++; > } > > @@ -247,6 +254,8 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb) > } > free(lb->vips); > free(lb->vips_nb); > + sset_destroy(&lb->ips_v4); > + sset_destroy(&lb->ips_v6); > free(lb->selection_fields); > free(lb->dps); > free(lb); > diff --git a/lib/lb.h b/lib/lb.h > index 0486b3d89..58e6bb031 100644 > --- a/lib/lb.h > +++ b/lib/lb.h > @@ -20,6 +20,7 @@ > #include <sys/types.h> > #include <netinet/in.h> > #include "openvswitch/hmap.h" > +#include "sset.h" > #include "ovn-util.h" > > struct nbrec_load_balancer; > @@ -38,6 +39,9 @@ struct ovn_northd_lb { > struct ovn_northd_lb_vip *vips_nb; > size_t n_vips; > > + struct sset ips_v4; > + struct sset ips_v6; > + > size_t n_dps; > size_t n_allocated_dps; > const struct sbrec_datapath_binding **dps; > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 224ea9543..f59dba469 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -661,6 +661,8 @@ struct ovn_datapath { > struct lport_addresses dnat_force_snat_addrs; > struct lport_addresses lb_force_snat_addrs; > bool lb_force_snat_router_ip; > + struct sset lb_ips_v4; > + struct sset lb_ips_v6; > > struct ovn_port **localnet_ports; > size_t n_localnet_ports; > @@ -827,6 +829,24 @@ destroy_nat_entries(struct ovn_datapath *od) > } > } > > +static void > +init_lb_ips(struct ovn_datapath *od) > +{ > + sset_init(&od->lb_ips_v4); > + sset_init(&od->lb_ips_v6); > +} > + > +static void > +destroy_lb_ips(struct ovn_datapath *od) > +{ > + if (!od->nbs && !od->nbr) { > + return; > + } > + > + sset_destroy(&od->lb_ips_v4); > + sset_destroy(&od->lb_ips_v6); > +} > + > /* A group of logical router datapaths which are connected - either > * directly or indirectly. > * Each logical router can belong to only one group. */ > @@ -871,6 +891,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) > destroy_ipam_info(&od->ipam_info); > free(od->router_ports); > destroy_nat_entries(od); > + destroy_lb_ips(od); > free(od->nat_entries); > free(od->localnet_ports); > ovn_ls_port_group_destroy(&od->nb_pgs); > @@ -1193,6 +1214,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, > > init_ipam_info_for_datapath(od); > init_mcast_info_for_datapath(od); > + init_lb_ips(od); > } > > const struct nbrec_logical_router *nbr; > @@ -1224,6 +1246,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, > } > init_mcast_info_for_datapath(od); > init_nat_entries(od); > + init_lb_ips(od); > ovs_list_push_back(lr_list, &od->lr_list); > } > } > @@ -2437,46 +2460,6 @@ join_logical_ports(struct northd_context *ctx, > } > } > > -static void > -get_router_load_balancer_ips(const struct ovn_datapath *od, > - struct sset *all_ips_v4, struct sset *all_ips_v6) > -{ > - if (!od->nbr) { > - return; > - } > - > - for (int i = 0; i < od->nbr->n_load_balancer; i++) { > - struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; > - struct smap *vips = &lb->vips; > - struct smap_node *node; > - > - SMAP_FOR_EACH (node, vips) { > - /* node->key contains IP:port or just IP. */ > - char *ip_address; > - uint16_t port; > - int addr_family; > - > - if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port, > - &addr_family)) { > - continue; > - } > - > - struct sset *all_ips; > - if (addr_family == AF_INET) { > - all_ips = all_ips_v4; > - } else { > - all_ips = all_ips_v6; > - } > - > - if (!sset_contains(all_ips, ip_address)) { > - sset_add(all_ips, ip_address); > - } > - > - free(ip_address); > - } > - } > -} > - > /* Returns an array of strings, each consisting of a MAC address followed > * by one or more IP addresses, and if the port is a distributed gateway > * port, followed by 'is_chassis_resident("LPORT_NAME")', where the > @@ -2561,22 +2544,15 @@ get_nat_addresses(const struct ovn_port *op, size_t *n) > } > } > > - /* Two sets to hold all load-balancer vips. */ > - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); > - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); > - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > - > const char *ip_address; > - SSET_FOR_EACH (ip_address, &all_ips_v4) { > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) { > ds_put_format(&c_addresses, " %s", ip_address); > central_ip_address = true; > } > - SSET_FOR_EACH (ip_address, &all_ips_v6) { > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { > ds_put_format(&c_addresses, " %s", ip_address); > central_ip_address = true; > } > - sset_destroy(&all_ips_v4); > - sset_destroy(&all_ips_v6); > > if (central_ip_address) { > /* Gratuitous ARP for centralized NAT rules on distributed gateway > @@ -3546,6 +3522,31 @@ build_ovn_lb_svcs(struct northd_context *ctx, struct hmap *ports, > hmap_destroy(&monitor_map); > } > > +static void > +build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) > +{ > + struct ovn_datapath *od; > + > + HMAP_FOR_EACH (od, key_node, datapaths) { > + if (!od->nbr) { > + continue; > + } > + > + for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { > + struct ovn_northd_lb *lb = > + ovn_northd_lb_find(lbs, > + &od->nbr->load_balancer[i]->header_.uuid); > + const char *ip_address; > + SSET_FOR_EACH (ip_address, &lb->ips_v4) { > + sset_add(&od->lb_ips_v4, ip_address); > + } > + SSET_FOR_EACH (ip_address, &lb->ips_v6) { > + sset_add(&od->lb_ips_v6, ip_address); > + } > + } > + } > +} > + > static bool > ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) > { > @@ -6486,7 +6487,7 @@ build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, > * switching domain as regular broadcast. > */ > static void > -build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, > +build_lswitch_rport_arp_req_flow_for_ip(struct ds *ip_match, > int addr_family, > struct ovn_port *patch_op, > struct ovn_datapath *od, > @@ -6510,13 +6511,7 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, > ds_put_cstr(&match, "nd_ns && nd.target == { "); > } > > - const char *ip_address; > - SSET_FOR_EACH (ip_address, ips) { > - ds_put_format(&match, "%s, ", ip_address); > - } > - > - ds_chomp(&match, ' '); > - ds_chomp(&match, ','); > + ds_put_cstr(&match, ds_cstr_ro(ip_match)); > ds_put_cstr(&match, "}"); > > /* Send a the packet to the router pipeline. If the switch has non-router > @@ -6566,14 +6561,12 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > * router port. > * Priority: 80. > */ > - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); > - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); > - > - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > + struct ds ips_v4_match = DS_EMPTY_INITIALIZER; > + struct ds ips_v6_match = DS_EMPTY_INITIALIZER; > > const char *ip_addr; > const char *ip_addr_next; > - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v4) { > + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v4) { > ovs_be32 ipv4_addr; > > /* Check if the ovn port has a network configured on which we could > @@ -6581,12 +6574,10 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > */ > if (ip_parse(ip_addr, &ipv4_addr) && > lrouter_port_ipv4_reachable(op, ipv4_addr)) { > - continue; > + ds_put_format(&ips_v4_match, "%s, ", ip_addr); > } > - > - sset_delete(&all_ips_v4, SSET_NODE_FROM_NAME(ip_addr)); > } > - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v6) { > + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v6) { > struct in6_addr ipv6_addr; > > /* Check if the ovn port has a network configured on which we could > @@ -6594,10 +6585,8 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > */ > if (ipv6_parse(ip_addr, &ipv6_addr) && > lrouter_port_ipv6_reachable(op, &ipv6_addr)) { > - continue; > + ds_put_format(&ips_v6_match, "%s, ", ip_addr); > } > - > - sset_delete(&all_ips_v6, SSET_NODE_FROM_NAME(ip_addr)); > } > > for (size_t i = 0; i < op->od->nbr->n_nat; i++) { > @@ -6618,39 +6607,44 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > if (nat_entry_is_v6(nat_entry)) { > struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr; > > - if (lrouter_port_ipv6_reachable(op, addr)) { > - sset_add(&all_ips_v6, nat->external_ip); > + if (lrouter_port_ipv6_reachable(op, addr) && > + !sset_contains(&op->od->lb_ips_v6, nat->external_ip)) { > + ds_put_format(&ips_v6_match, "%s, ", nat->external_ip); > } > } else { > ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr; > > - if (lrouter_port_ipv4_reachable(op, addr)) { > - sset_add(&all_ips_v4, nat->external_ip); > + if (lrouter_port_ipv4_reachable(op, addr) && > + !sset_contains(&op->od->lb_ips_v4, nat->external_ip)) { > + ds_put_format(&ips_v4_match, "%s, ", nat->external_ip); > } > } > } > > for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { > - sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); > + ds_put_format(&ips_v4_match, "%s, ", > + op->lrp_networks.ipv4_addrs[i].addr_s); > } > for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { > - sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); > + ds_put_format(&ips_v6_match, "%s, ", > + op->lrp_networks.ipv6_addrs[i].addr_s); > } > > - if (!sset_is_empty(&all_ips_v4)) { > - build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op, > + if (ds_last(&ips_v4_match) != EOF) { > + ds_chomp(&ips_v4_match, ' '); > + ds_chomp(&ips_v4_match, ','); > + build_lswitch_rport_arp_req_flow_for_ip(&ips_v4_match, AF_INET, sw_op, > sw_od, 80, lflows, > stage_hint); > } > - if (!sset_is_empty(&all_ips_v6)) { > - build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op, > + if (ds_last(&ips_v6_match) != EOF) { > + ds_chomp(&ips_v6_match, ' '); > + ds_chomp(&ips_v6_match, ','); > + build_lswitch_rport_arp_req_flow_for_ip(&ips_v6_match, AF_INET6, sw_op, > sw_od, 80, lflows, > stage_hint); > } > > - sset_destroy(&all_ips_v4); > - sset_destroy(&all_ips_v6); > - > /* Self originated ARP requests/ND need to be flooded as usual. > * > * However, if the switch doesn't have any non-router ports we shouldn't > @@ -6661,6 +6655,8 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > if (sw_od->n_router_ports != sw_od->nbs->n_ports) { > build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows); > } > + ds_destroy(&ips_v4_match); > + ds_destroy(&ips_v6_match); > } > > static void > @@ -11099,13 +11095,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > &op->nbrp->header_, lflows); > } > > - /* A set to hold all load-balancer vips that need ARP responses. */ > - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); > - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); > - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); > - > const char *ip_address; > - if (sset_count(&all_ips_v4)) { > + if (sset_count(&op->od->lb_ips_v4)) { > ds_clear(match); > if (op == op->od->l3dgw_port) { > ds_put_format(match, "is_chassis_resident(%s)", > @@ -11117,7 +11108,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > /* For IPv4 we can just create one rule with all required IPs. */ > ds_put_cstr(&load_balancer_ips_v4, "{ "); > ds_put_and_free_cstr(&load_balancer_ips_v4, > - sset_join(&all_ips_v4, ", ", " }")); > + sset_join(&op->od->lb_ips_v4, ", ", " }")); > > build_lrouter_arp_flow(op->od, op, ds_cstr(&load_balancer_ips_v4), > REG_INPORT_ETH_ADDR, > @@ -11125,7 +11116,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > ds_destroy(&load_balancer_ips_v4); > } > > - SSET_FOR_EACH (ip_address, &all_ips_v6) { > + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { > ds_clear(match); > if (op == op->od->l3dgw_port) { > ds_put_format(match, "is_chassis_resident(%s)", > @@ -11137,9 +11128,6 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, > match, false, 90, NULL, lflows); > } > > - sset_destroy(&all_ips_v4); > - sset_destroy(&all_ips_v6); > - > if (!smap_get(&op->od->nbr->options, "chassis") > && !op->od->l3dgw_port) { > /* UDP/TCP/SCTP port unreachable. */ > @@ -13347,6 +13335,7 @@ ovnnb_db_run(struct northd_context *ctx, > > build_datapaths(ctx, datapaths, lr_list); > build_ovn_lbs(ctx, datapaths, &lbs); > + build_lrouter_lbs(datapaths, &lbs); > build_ports(ctx, sbrec_chassis_by_name, datapaths, ports); > build_ovn_lb_svcs(ctx, ports, &lbs); > build_ipam(datapaths, ports); > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
On 6/15/21 7:42 PM, Numan Siddique wrote: > On Thu, Jun 10, 2021 at 8:48 AM Dumitru Ceara <dceara@redhat.com> wrote: >> There's no need to parse the IP sets every time we iterate through them. >> It's enough to parse them once for every main loop iteration. >> >> Reported-at: https://bugzilla.redhat.com/1962338 >> Signed-off-by: Dumitru Ceara <dceara@redhat.com> > Thanks for the series. > > I pushed all the 3 patches of the series to the main branch with below > small changes in patch 3. > > ---- > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c > index 17ed550755..aae7314c89 100644 > --- a/northd/ovn-northd.c > +++ b/northd/ovn-northd.c > @@ -6554,8 +6554,7 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > struct ds ips_v6_match = DS_EMPTY_INITIALIZER; > > const char *ip_addr; > - const char *ip_addr_next; > - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v4) { > + SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v4) { > ovs_be32 ipv4_addr; > > /* Check if the ovn port has a network configured on which we could > @@ -6566,7 +6565,7 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, > ds_put_format(&ips_v4_match, "%s, ", ip_addr); > } > } > - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v6) { > + SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v6) { > struct in6_addr ipv6_addr; > > /* Check if the ovn port has a network configured on which we could > ------- > LGTM, thanks for fixing this up! Regards, Dumitru
diff --git a/lib/lb.c b/lib/lb.c index 18dc226e9..4cb46b346 100644 --- a/lib/lb.c +++ b/lib/lb.c @@ -170,6 +170,8 @@ 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); + sset_init(&lb->ips_v4); + sset_init(&lb->ips_v6); struct smap_node *node; size_t n_vips = 0; @@ -184,6 +186,11 @@ ovn_northd_lb_create(const struct nbrec_load_balancer *nbrec_lb) } ovn_northd_lb_vip_init(lb_vip_nb, lb_vip, nbrec_lb, node->key, node->value); + if (IN6_IS_ADDR_V4MAPPED(&lb_vip->vip)) { + sset_add(&lb->ips_v4, lb_vip->vip_str); + } else { + sset_add(&lb->ips_v6, lb_vip->vip_str); + } n_vips++; } @@ -247,6 +254,8 @@ ovn_northd_lb_destroy(struct ovn_northd_lb *lb) } free(lb->vips); free(lb->vips_nb); + sset_destroy(&lb->ips_v4); + sset_destroy(&lb->ips_v6); free(lb->selection_fields); free(lb->dps); free(lb); diff --git a/lib/lb.h b/lib/lb.h index 0486b3d89..58e6bb031 100644 --- a/lib/lb.h +++ b/lib/lb.h @@ -20,6 +20,7 @@ #include <sys/types.h> #include <netinet/in.h> #include "openvswitch/hmap.h" +#include "sset.h" #include "ovn-util.h" struct nbrec_load_balancer; @@ -38,6 +39,9 @@ struct ovn_northd_lb { struct ovn_northd_lb_vip *vips_nb; size_t n_vips; + struct sset ips_v4; + struct sset ips_v6; + size_t n_dps; size_t n_allocated_dps; const struct sbrec_datapath_binding **dps; diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 224ea9543..f59dba469 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -661,6 +661,8 @@ struct ovn_datapath { struct lport_addresses dnat_force_snat_addrs; struct lport_addresses lb_force_snat_addrs; bool lb_force_snat_router_ip; + struct sset lb_ips_v4; + struct sset lb_ips_v6; struct ovn_port **localnet_ports; size_t n_localnet_ports; @@ -827,6 +829,24 @@ destroy_nat_entries(struct ovn_datapath *od) } } +static void +init_lb_ips(struct ovn_datapath *od) +{ + sset_init(&od->lb_ips_v4); + sset_init(&od->lb_ips_v6); +} + +static void +destroy_lb_ips(struct ovn_datapath *od) +{ + if (!od->nbs && !od->nbr) { + return; + } + + sset_destroy(&od->lb_ips_v4); + sset_destroy(&od->lb_ips_v6); +} + /* A group of logical router datapaths which are connected - either * directly or indirectly. * Each logical router can belong to only one group. */ @@ -871,6 +891,7 @@ ovn_datapath_destroy(struct hmap *datapaths, struct ovn_datapath *od) destroy_ipam_info(&od->ipam_info); free(od->router_ports); destroy_nat_entries(od); + destroy_lb_ips(od); free(od->nat_entries); free(od->localnet_ports); ovn_ls_port_group_destroy(&od->nb_pgs); @@ -1193,6 +1214,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, init_ipam_info_for_datapath(od); init_mcast_info_for_datapath(od); + init_lb_ips(od); } const struct nbrec_logical_router *nbr; @@ -1224,6 +1246,7 @@ join_datapaths(struct northd_context *ctx, struct hmap *datapaths, } init_mcast_info_for_datapath(od); init_nat_entries(od); + init_lb_ips(od); ovs_list_push_back(lr_list, &od->lr_list); } } @@ -2437,46 +2460,6 @@ join_logical_ports(struct northd_context *ctx, } } -static void -get_router_load_balancer_ips(const struct ovn_datapath *od, - struct sset *all_ips_v4, struct sset *all_ips_v6) -{ - if (!od->nbr) { - return; - } - - for (int i = 0; i < od->nbr->n_load_balancer; i++) { - struct nbrec_load_balancer *lb = od->nbr->load_balancer[i]; - struct smap *vips = &lb->vips; - struct smap_node *node; - - SMAP_FOR_EACH (node, vips) { - /* node->key contains IP:port or just IP. */ - char *ip_address; - uint16_t port; - int addr_family; - - if (!ip_address_and_port_from_lb_key(node->key, &ip_address, &port, - &addr_family)) { - continue; - } - - struct sset *all_ips; - if (addr_family == AF_INET) { - all_ips = all_ips_v4; - } else { - all_ips = all_ips_v6; - } - - if (!sset_contains(all_ips, ip_address)) { - sset_add(all_ips, ip_address); - } - - free(ip_address); - } - } -} - /* Returns an array of strings, each consisting of a MAC address followed * by one or more IP addresses, and if the port is a distributed gateway * port, followed by 'is_chassis_resident("LPORT_NAME")', where the @@ -2561,22 +2544,15 @@ get_nat_addresses(const struct ovn_port *op, size_t *n) } } - /* Two sets to hold all load-balancer vips. */ - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); - const char *ip_address; - SSET_FOR_EACH (ip_address, &all_ips_v4) { + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) { ds_put_format(&c_addresses, " %s", ip_address); central_ip_address = true; } - SSET_FOR_EACH (ip_address, &all_ips_v6) { + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { ds_put_format(&c_addresses, " %s", ip_address); central_ip_address = true; } - sset_destroy(&all_ips_v4); - sset_destroy(&all_ips_v6); if (central_ip_address) { /* Gratuitous ARP for centralized NAT rules on distributed gateway @@ -3546,6 +3522,31 @@ build_ovn_lb_svcs(struct northd_context *ctx, struct hmap *ports, hmap_destroy(&monitor_map); } +static void +build_lrouter_lbs(struct hmap *datapaths, struct hmap *lbs) +{ + struct ovn_datapath *od; + + HMAP_FOR_EACH (od, key_node, datapaths) { + if (!od->nbr) { + continue; + } + + for (size_t i = 0; i < od->nbr->n_load_balancer; i++) { + struct ovn_northd_lb *lb = + ovn_northd_lb_find(lbs, + &od->nbr->load_balancer[i]->header_.uuid); + const char *ip_address; + SSET_FOR_EACH (ip_address, &lb->ips_v4) { + sset_add(&od->lb_ips_v4, ip_address); + } + SSET_FOR_EACH (ip_address, &lb->ips_v6) { + sset_add(&od->lb_ips_v6, ip_address); + } + } + } +} + static bool ovn_port_add_tnlid(struct ovn_port *op, uint32_t tunnel_key) { @@ -6486,7 +6487,7 @@ build_lswitch_rport_arp_req_self_orig_flow(struct ovn_port *op, * switching domain as regular broadcast. */ static void -build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, +build_lswitch_rport_arp_req_flow_for_ip(struct ds *ip_match, int addr_family, struct ovn_port *patch_op, struct ovn_datapath *od, @@ -6510,13 +6511,7 @@ build_lswitch_rport_arp_req_flow_for_ip(struct sset *ips, ds_put_cstr(&match, "nd_ns && nd.target == { "); } - const char *ip_address; - SSET_FOR_EACH (ip_address, ips) { - ds_put_format(&match, "%s, ", ip_address); - } - - ds_chomp(&match, ' '); - ds_chomp(&match, ','); + ds_put_cstr(&match, ds_cstr_ro(ip_match)); ds_put_cstr(&match, "}"); /* Send a the packet to the router pipeline. If the switch has non-router @@ -6566,14 +6561,12 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, * router port. * Priority: 80. */ - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); - - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); + struct ds ips_v4_match = DS_EMPTY_INITIALIZER; + struct ds ips_v6_match = DS_EMPTY_INITIALIZER; const char *ip_addr; const char *ip_addr_next; - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v4) { + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v4) { ovs_be32 ipv4_addr; /* Check if the ovn port has a network configured on which we could @@ -6581,12 +6574,10 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, */ if (ip_parse(ip_addr, &ipv4_addr) && lrouter_port_ipv4_reachable(op, ipv4_addr)) { - continue; + ds_put_format(&ips_v4_match, "%s, ", ip_addr); } - - sset_delete(&all_ips_v4, SSET_NODE_FROM_NAME(ip_addr)); } - SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &all_ips_v6) { + SSET_FOR_EACH_SAFE (ip_addr, ip_addr_next, &op->od->lb_ips_v6) { struct in6_addr ipv6_addr; /* Check if the ovn port has a network configured on which we could @@ -6594,10 +6585,8 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, */ if (ipv6_parse(ip_addr, &ipv6_addr) && lrouter_port_ipv6_reachable(op, &ipv6_addr)) { - continue; + ds_put_format(&ips_v6_match, "%s, ", ip_addr); } - - sset_delete(&all_ips_v6, SSET_NODE_FROM_NAME(ip_addr)); } for (size_t i = 0; i < op->od->nbr->n_nat; i++) { @@ -6618,39 +6607,44 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, if (nat_entry_is_v6(nat_entry)) { struct in6_addr *addr = &nat_entry->ext_addrs.ipv6_addrs[0].addr; - if (lrouter_port_ipv6_reachable(op, addr)) { - sset_add(&all_ips_v6, nat->external_ip); + if (lrouter_port_ipv6_reachable(op, addr) && + !sset_contains(&op->od->lb_ips_v6, nat->external_ip)) { + ds_put_format(&ips_v6_match, "%s, ", nat->external_ip); } } else { ovs_be32 addr = nat_entry->ext_addrs.ipv4_addrs[0].addr; - if (lrouter_port_ipv4_reachable(op, addr)) { - sset_add(&all_ips_v4, nat->external_ip); + if (lrouter_port_ipv4_reachable(op, addr) && + !sset_contains(&op->od->lb_ips_v4, nat->external_ip)) { + ds_put_format(&ips_v4_match, "%s, ", nat->external_ip); } } } for (size_t i = 0; i < op->lrp_networks.n_ipv4_addrs; i++) { - sset_add(&all_ips_v4, op->lrp_networks.ipv4_addrs[i].addr_s); + ds_put_format(&ips_v4_match, "%s, ", + op->lrp_networks.ipv4_addrs[i].addr_s); } for (size_t i = 0; i < op->lrp_networks.n_ipv6_addrs; i++) { - sset_add(&all_ips_v6, op->lrp_networks.ipv6_addrs[i].addr_s); + ds_put_format(&ips_v6_match, "%s, ", + op->lrp_networks.ipv6_addrs[i].addr_s); } - if (!sset_is_empty(&all_ips_v4)) { - build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v4, AF_INET, sw_op, + if (ds_last(&ips_v4_match) != EOF) { + ds_chomp(&ips_v4_match, ' '); + ds_chomp(&ips_v4_match, ','); + build_lswitch_rport_arp_req_flow_for_ip(&ips_v4_match, AF_INET, sw_op, sw_od, 80, lflows, stage_hint); } - if (!sset_is_empty(&all_ips_v6)) { - build_lswitch_rport_arp_req_flow_for_ip(&all_ips_v6, AF_INET6, sw_op, + if (ds_last(&ips_v6_match) != EOF) { + ds_chomp(&ips_v6_match, ' '); + ds_chomp(&ips_v6_match, ','); + build_lswitch_rport_arp_req_flow_for_ip(&ips_v6_match, AF_INET6, sw_op, sw_od, 80, lflows, stage_hint); } - sset_destroy(&all_ips_v4); - sset_destroy(&all_ips_v6); - /* Self originated ARP requests/ND need to be flooded as usual. * * However, if the switch doesn't have any non-router ports we shouldn't @@ -6661,6 +6655,8 @@ build_lswitch_rport_arp_req_flows(struct ovn_port *op, if (sw_od->n_router_ports != sw_od->nbs->n_ports) { build_lswitch_rport_arp_req_self_orig_flow(op, 75, sw_od, lflows); } + ds_destroy(&ips_v4_match); + ds_destroy(&ips_v6_match); } static void @@ -11099,13 +11095,8 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, &op->nbrp->header_, lflows); } - /* A set to hold all load-balancer vips that need ARP responses. */ - struct sset all_ips_v4 = SSET_INITIALIZER(&all_ips_v4); - struct sset all_ips_v6 = SSET_INITIALIZER(&all_ips_v6); - get_router_load_balancer_ips(op->od, &all_ips_v4, &all_ips_v6); - const char *ip_address; - if (sset_count(&all_ips_v4)) { + if (sset_count(&op->od->lb_ips_v4)) { ds_clear(match); if (op == op->od->l3dgw_port) { ds_put_format(match, "is_chassis_resident(%s)", @@ -11117,7 +11108,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, /* For IPv4 we can just create one rule with all required IPs. */ ds_put_cstr(&load_balancer_ips_v4, "{ "); ds_put_and_free_cstr(&load_balancer_ips_v4, - sset_join(&all_ips_v4, ", ", " }")); + sset_join(&op->od->lb_ips_v4, ", ", " }")); build_lrouter_arp_flow(op->od, op, ds_cstr(&load_balancer_ips_v4), REG_INPORT_ETH_ADDR, @@ -11125,7 +11116,7 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, ds_destroy(&load_balancer_ips_v4); } - SSET_FOR_EACH (ip_address, &all_ips_v6) { + SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) { ds_clear(match); if (op == op->od->l3dgw_port) { ds_put_format(match, "is_chassis_resident(%s)", @@ -11137,9 +11128,6 @@ build_lrouter_ipv4_ip_input(struct ovn_port *op, match, false, 90, NULL, lflows); } - sset_destroy(&all_ips_v4); - sset_destroy(&all_ips_v6); - if (!smap_get(&op->od->nbr->options, "chassis") && !op->od->l3dgw_port) { /* UDP/TCP/SCTP port unreachable. */ @@ -13347,6 +13335,7 @@ ovnnb_db_run(struct northd_context *ctx, build_datapaths(ctx, datapaths, lr_list); build_ovn_lbs(ctx, datapaths, &lbs); + build_lrouter_lbs(datapaths, &lbs); build_ports(ctx, sbrec_chassis_by_name, datapaths, ports); build_ovn_lb_svcs(ctx, ports, &lbs); build_ipam(datapaths, ports);
There's no need to parse the IP sets every time we iterate through them. It's enough to parse them once for every main loop iteration. Reported-at: https://bugzilla.redhat.com/1962338 Signed-off-by: Dumitru Ceara <dceara@redhat.com> --- lib/lb.c | 9 +++ lib/lb.h | 4 + northd/ovn-northd.c | 175 ++++++++++++++++++++++++--------------------------- 3 files changed, 95 insertions(+), 93 deletions(-)