[ovs-dev] [PATCH ovn v2 3/3] northd: Precompute load balancer IP sets.
Numan Siddique
numans at ovn.org
Tue Jun 15 17:42:58 UTC 2021
On Thu, Jun 10, 2021 at 8:48 AM Dumitru Ceara <dceara at 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 at 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 at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
More information about the dev
mailing list