[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