diff mbox series

[ovs-dev,v2,4/4] northd: Re-use IP sets created for load balancer groups.

Message ID 20220907202616.624528-5-i.maximets@ovn.org
State Superseded
Headers show
Series northd: Optimize preparation of load balancers. | expand

Checks

Context Check Description
ovsrobot/apply-robot success apply and check: success
ovsrobot/github-robot-_Build_and_Test success github build: passed
ovsrobot/github-robot-_ovn-kubernetes success github build: passed

Commit Message

Ilya Maximets Sept. 7, 2022, 8:26 p.m. UTC 
If the same load balancer group is attached to multiple routers,
IP sets will be constructed for each of them by sequentially calling
sset_add() for each IP for each load balancer for each router.
Instead of doing that, we can create IP sets for load balancer
groups and clone them.  That will avoid extra ssed_find__() call
making the construction of IP sets 2x faster.

Only first load balancer group is cloned, the rest as well as
standalone load balancers are still added one by one, because
there is no more efficient way to merge sets.

The order of processing changed to make sure that we're actually
optimizing copy of a large group.

The code can be optimized further by introduction of a reference
counter and not copying at all if the router has no standalone load
balancers and only one group.  Also, construction of "reachable"
sets can be optimized for the "neighbor_responder = all" case.  But,
for now, only moved to a new structure for better readability.

Acked-by: Dumitru Ceara <dceara@redhat.com>
Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
---
 lib/lb.c        |  45 +++++++++++++++++++
 lib/lb.h        |  16 +++++++
 northd/northd.c | 112 ++++++++++++++++++++++++++----------------------
 northd/northd.h |  13 ++----
 4 files changed, 125 insertions(+), 61 deletions(-)

Comments

Dumitru Ceara Sept. 9, 2022, 3:08 p.m. UTC | #1 
On 9/7/22 22:26, Ilya Maximets wrote:
> If the same load balancer group is attached to multiple routers,
> IP sets will be constructed for each of them by sequentially calling
> sset_add() for each IP for each load balancer for each router.
> Instead of doing that, we can create IP sets for load balancer
> groups and clone them.  That will avoid extra ssed_find__() call
> making the construction of IP sets 2x faster.
> 
> Only first load balancer group is cloned, the rest as well as
> standalone load balancers are still added one by one, because
> there is no more efficient way to merge sets.
> 
> The order of processing changed to make sure that we're actually
> optimizing copy of a large group.
> 
> The code can be optimized further by introduction of a reference
> counter and not copying at all if the router has no standalone load
> balancers and only one group.  Also, construction of "reachable"
> sets can be optimized for the "neighbor_responder = all" case.  But,
> for now, only moved to a new structure for better readability.
> 
> Acked-by: Dumitru Ceara <dceara@redhat.com>
> Signed-off-by: Ilya Maximets <i.maximets@ovn.org>
> ---

Still looks good to me, thanks!
diff mbox series

Patch

diff --git a/lib/lb.c b/lib/lb.c
index 36f914228..f2339cd71 100644
--- a/lib/lb.c
+++ b/lib/lb.c
@@ -26,6 +26,51 @@ 
 
 VLOG_DEFINE_THIS_MODULE(lb);
 
+struct ovn_lb_ip_set *
+ovn_lb_ip_set_create(void)
+{
+    struct ovn_lb_ip_set *lb_ip_set = xzalloc(sizeof *lb_ip_set);
+
+    sset_init(&lb_ip_set->ips_v4);
+    sset_init(&lb_ip_set->ips_v4_routable);
+    sset_init(&lb_ip_set->ips_v4_reachable);
+    sset_init(&lb_ip_set->ips_v6);
+    sset_init(&lb_ip_set->ips_v6_routable);
+    sset_init(&lb_ip_set->ips_v6_reachable);
+
+    return lb_ip_set;
+}
+
+void
+ovn_lb_ip_set_destroy(struct ovn_lb_ip_set *lb_ip_set)
+{
+    if (!lb_ip_set) {
+        return;
+    }
+    sset_destroy(&lb_ip_set->ips_v4);
+    sset_destroy(&lb_ip_set->ips_v4_routable);
+    sset_destroy(&lb_ip_set->ips_v4_reachable);
+    sset_destroy(&lb_ip_set->ips_v6);
+    sset_destroy(&lb_ip_set->ips_v6_routable);
+    sset_destroy(&lb_ip_set->ips_v6_reachable);
+    free(lb_ip_set);
+}
+
+struct ovn_lb_ip_set *
+ovn_lb_ip_set_clone(struct ovn_lb_ip_set *lb_ip_set)
+{
+    struct ovn_lb_ip_set *clone = ovn_lb_ip_set_create();
+
+    sset_clone(&clone->ips_v4, &lb_ip_set->ips_v4);
+    sset_clone(&clone->ips_v4_routable, &lb_ip_set->ips_v4_routable);
+    sset_clone(&clone->ips_v4_reachable, &lb_ip_set->ips_v4_reachable);
+    sset_clone(&clone->ips_v6, &lb_ip_set->ips_v6);
+    sset_clone(&clone->ips_v6_routable, &lb_ip_set->ips_v6_routable);
+    sset_clone(&clone->ips_v6_reachable, &lb_ip_set->ips_v6_reachable);
+
+    return clone;
+}
+
 static
 bool ovn_lb_vip_init(struct ovn_lb_vip *lb_vip, const char *lb_key,
                      const char *lb_value)
diff --git a/lib/lb.h b/lib/lb.h
index f939e49ae..cb2b33de7 100644
--- a/lib/lb.h
+++ b/lib/lb.h
@@ -36,6 +36,21 @@  enum lb_neighbor_responder_mode {
     LB_NEIGH_RESPOND_ALL,
 };
 
+/* The "routable" ssets are subsets of the load balancer IPs for which IP
+ * routes and ARP resolution flows are automatically added. */
+struct ovn_lb_ip_set {
+    struct sset ips_v4;
+    struct sset ips_v4_routable;
+    struct sset ips_v4_reachable;
+    struct sset ips_v6;
+    struct sset ips_v6_routable;
+    struct sset ips_v6_reachable;
+};
+
+struct ovn_lb_ip_set *ovn_lb_ip_set_create(void);
+void ovn_lb_ip_set_destroy(struct ovn_lb_ip_set *);
+struct ovn_lb_ip_set *ovn_lb_ip_set_clone(struct ovn_lb_ip_set *);
+
 struct ovn_northd_lb {
     struct hmap_node hmap_node;
 
@@ -110,6 +125,7 @@  struct ovn_lb_group {
     struct uuid uuid;
     size_t n_lbs;
     struct ovn_northd_lb **lbs;
+    struct ovn_lb_ip_set *lb_ips;
 
     /* Datapaths to which this LB group is applied. */
     size_t n_ls;
diff --git a/northd/northd.c b/northd/northd.c
index 7a47f93d0..55406fe26 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -821,13 +821,6 @@  lr_lb_address_set_ref(const struct ovn_datapath *od, int addr_family)
 static void
 init_lb_for_datapath(struct ovn_datapath *od)
 {
-    sset_init(&od->lb_ips_v4);
-    sset_init(&od->lb_ips_v4_routable);
-    sset_init(&od->lb_ips_v4_reachable);
-    sset_init(&od->lb_ips_v6);
-    sset_init(&od->lb_ips_v6_routable);
-    sset_init(&od->lb_ips_v6_reachable);
-
     if (od->nbs) {
         od->has_lb_vip = ls_has_lb_vip(od);
     } else {
@@ -838,16 +831,12 @@  init_lb_for_datapath(struct ovn_datapath *od)
 static void
 destroy_lb_for_datapath(struct ovn_datapath *od)
 {
+    ovn_lb_ip_set_destroy(od->lb_ips);
+    od->lb_ips = NULL;
+
     if (!od->nbs && !od->nbr) {
         return;
     }
-
-    sset_destroy(&od->lb_ips_v4);
-    sset_destroy(&od->lb_ips_v4_routable);
-    sset_destroy(&od->lb_ips_v4_reachable);
-    sset_destroy(&od->lb_ips_v6);
-    sset_destroy(&od->lb_ips_v6_routable);
-    sset_destroy(&od->lb_ips_v6_reachable);
 }
 
 /* A group of logical router datapaths which are connected - either
@@ -2818,20 +2807,20 @@  get_nat_addresses(const struct ovn_port *op, size_t *n, bool routable_only,
     if (include_lb_ips) {
         const char *ip_address;
         if (routable_only) {
-            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4_routable) {
+            SSET_FOR_EACH (ip_address, &op->od->lb_ips->ips_v4_routable) {
                 ds_put_format(&c_addresses, " %s", ip_address);
                 central_ip_address = true;
             }
-            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6_routable) {
+            SSET_FOR_EACH (ip_address, &op->od->lb_ips->ips_v6_routable) {
                 ds_put_format(&c_addresses, " %s", ip_address);
                 central_ip_address = true;
             }
         } else {
-            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v4) {
+            SSET_FOR_EACH (ip_address, &op->od->lb_ips->ips_v4) {
                 ds_put_format(&c_addresses, " %s", ip_address);
                 central_ip_address = true;
             }
-            SSET_FOR_EACH (ip_address, &op->od->lb_ips_v6) {
+            SSET_FOR_EACH (ip_address, &op->od->lb_ips->ips_v6) {
                 ds_put_format(&c_addresses, " %s", ip_address);
                 central_ip_address = true;
             }
@@ -3822,20 +3811,21 @@  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)
+build_lrouter_lb_ips(struct ovn_lb_ip_set *lb_ips,
+                     const struct ovn_northd_lb *lb)
 {
     const char *ip_address;
 
     SSET_FOR_EACH (ip_address, &lb->ips_v4) {
-        sset_add(&od->lb_ips_v4, ip_address);
+        sset_add(&lb_ips->ips_v4, ip_address);
         if (lb->routable) {
-            sset_add(&od->lb_ips_v4_routable, ip_address);
+            sset_add(&lb_ips->ips_v4_routable, ip_address);
         }
     }
     SSET_FOR_EACH (ip_address, &lb->ips_v6) {
-        sset_add(&od->lb_ips_v6, ip_address);
+        sset_add(&lb_ips->ips_v6, ip_address);
         if (lb->routable) {
-            sset_add(&od->lb_ips_v6_routable, ip_address);
+            sset_add(&lb_ips->ips_v6_routable, ip_address);
         }
     }
 }
@@ -3867,11 +3857,13 @@  build_lbs(struct northd_input *input_data, struct hmap *datapaths,
         lb_group->lbs = xmalloc(lb_group->n_lbs * sizeof *lb_group->lbs);
         lb_group->ls = xmalloc(hmap_count(datapaths) * sizeof *lb_group->ls);
         lb_group->lr = xmalloc(hmap_count(datapaths) * sizeof *lb_group->lr);
+        lb_group->lb_ips = ovn_lb_ip_set_create();
 
         for (size_t i = 0; i < nbrec_lb_group->n_load_balancer; i++) {
             const struct uuid *lb_uuid =
                 &nbrec_lb_group->load_balancer[i]->header_.uuid;
             lb_group->lbs[i] = ovn_northd_lb_find(lbs, lb_uuid);
+            build_lrouter_lb_ips(lb_group->lb_ips, lb_group->lbs[i]);
         }
 
         hmap_insert(lb_groups, &lb_group->hmap_node,
@@ -3911,23 +3903,34 @@  build_lbs(struct northd_input *input_data, struct hmap *datapaths,
             continue;
         }
 
-        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
-            const struct uuid *lb_uuid =
-                &od->nbr->load_balancer[i]->header_.uuid;
-            lb = ovn_northd_lb_find(lbs, lb_uuid);
-            ovn_northd_lb_add_lr(lb, 1, &od);
-            build_lrouter_lb_ips(od, lb);
-        }
-
+        /* Checking load balancer groups first to more efficiently copy
+         * IP sets for large groups. */
         for (size_t i = 0; i < od->nbr->n_load_balancer_group; i++) {
             nbrec_lb_group = od->nbr->load_balancer_group[i];
             lb_group = ovn_lb_group_find(lb_groups,
                                          &nbrec_lb_group->header_.uuid);
             lb_group->lr[lb_group->n_lr++] = od;
-            for (size_t j = 0; j < lb_group->n_lbs; j++) {
-                build_lrouter_lb_ips(od, lb_group->lbs[j]);
+
+            if (!od->lb_ips) {
+                od->lb_ips = ovn_lb_ip_set_clone(lb_group->lb_ips);
+            } else {
+                for (size_t j = 0; j < lb_group->n_lbs; j++) {
+                    build_lrouter_lb_ips(od->lb_ips, lb_group->lbs[j]);
+                }
             }
         }
+
+        if (!od->lb_ips) {
+            od->lb_ips = ovn_lb_ip_set_create();
+        }
+
+        for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
+            const struct uuid *lb_uuid =
+                &od->nbr->load_balancer[i]->header_.uuid;
+            lb = ovn_northd_lb_find(lbs, lb_uuid);
+            ovn_northd_lb_add_lr(lb, 1, &od);
+            build_lrouter_lb_ips(od->lb_ips, lb);
+        }
     }
 
     HMAP_FOR_EACH (lb_group, hmap_node, lb_groups) {
@@ -3988,9 +3991,9 @@  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
     if (lb->neigh_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);
+                sset_add(&od->lb_ips->ips_v4_reachable, lb->vips[i].vip_str);
             } else {
-                sset_add(&od->lb_ips_v6_reachable, lb->vips[i].vip_str);
+                sset_add(&od->lb_ips->ips_v6_reachable, lb->vips[i].vip_str);
             }
         }
         return;
@@ -4006,7 +4009,8 @@  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
 
             LIST_FOR_EACH (op, dp_node, &od->port_list) {
                 if (lrouter_port_ipv4_reachable(op, vip_ip4)) {
-                    sset_add(&od->lb_ips_v4_reachable, lb->vips[i].vip_str);
+                    sset_add(&od->lb_ips->ips_v4_reachable,
+                             lb->vips[i].vip_str);
                     break;
                 }
             }
@@ -4015,7 +4019,8 @@  build_lrouter_lb_reachable_ips(struct ovn_datapath *od,
 
             LIST_FOR_EACH (op, dp_node, &od->port_list) {
                 if (lrouter_port_ipv6_reachable(op, &lb->vips[i].vip)) {
-                    sset_add(&od->lb_ips_v6_reachable, lb->vips[i].vip_str);
+                    sset_add(&od->lb_ips->ips_v6_reachable,
+                             lb->vips[i].vip_str);
                     break;
                 }
             }
@@ -7478,7 +7483,7 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
      */
 
     const char *ip_addr;
-    SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v4) {
+    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v4) {
         ovs_be32 ipv4_addr;
 
         /* Check if the ovn port has a network configured on which we could
@@ -7491,7 +7496,7 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
                 stage_hint);
         }
     }
-    SSET_FOR_EACH (ip_addr, &op->od->lb_ips_v6) {
+    SSET_FOR_EACH (ip_addr, &op->od->lb_ips->ips_v6) {
         struct in6_addr ipv6_addr;
 
         /* Check if the ovn port has a network configured on which we could
@@ -7521,13 +7526,13 @@  build_lswitch_rport_arp_req_flows(struct ovn_port *op,
          * expect ARP requests/NS for the DNAT external_ip.
          */
         if (nat_entry_is_v6(nat_entry)) {
-            if (!sset_contains(&op->od->lb_ips_v6, nat->external_ip)) {
+            if (!sset_contains(&op->od->lb_ips->ips_v6, nat->external_ip)) {
                 build_lswitch_rport_arp_req_flow(
                     nat->external_ip, AF_INET6, sw_op, sw_od, 80, lflows,
                     stage_hint);
             }
         } else {
-            if (!sset_contains(&op->od->lb_ips_v4, nat->external_ip)) {
+            if (!sset_contains(&op->od->lb_ips->ips_v4, nat->external_ip)) {
                 build_lswitch_rport_arp_req_flow(
                     nat->external_ip, AF_INET, sw_op, sw_od, 80, lflows,
                     stage_hint);
@@ -10720,7 +10725,8 @@  build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
             const char *ip = op->lrp_networks.ipv4_addrs[i].addr_s;
 
             bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
-            bool router_ip_in_lb_ips = !!sset_find(&op->od->lb_ips_v4, ip);
+            bool router_ip_in_lb_ips =
+                    !!sset_find(&op->od->lb_ips->ips_v4, ip);
             bool drop_router_ip = (drop_snat_ip == (router_ip_in_snat_ips ||
                                                     router_ip_in_lb_ips));
 
@@ -10748,7 +10754,8 @@  build_lrouter_drop_own_dest(struct ovn_port *op, enum ovn_stage stage,
             const char *ip = op->lrp_networks.ipv6_addrs[i].addr_s;
 
             bool router_ip_in_snat_ips = !!shash_find(&op->od->snat_ips, ip);
-            bool router_ip_in_lb_ips = !!sset_find(&op->od->lb_ips_v6, ip);
+            bool router_ip_in_lb_ips =
+                    !!sset_find(&op->od->lb_ips->ips_v6, ip);
             bool drop_router_ip = (drop_snat_ip == (router_ip_in_snat_ips ||
                                                     router_ip_in_lb_ips));
 
@@ -12815,7 +12822,7 @@  build_lrouter_ipv4_ip_input(struct ovn_port *op,
                                    &op->nbrp->header_, lflows);
         }
 
-        if (sset_count(&op->od->lb_ips_v4_reachable)) {
+        if (sset_count(&op->od->lb_ips->ips_v4_reachable)) {
             ds_clear(match);
             if (is_l3dgw_port(op)) {
                 ds_put_format(match, "is_chassis_resident(%s)",
@@ -12830,7 +12837,7 @@  build_lrouter_ipv4_ip_input(struct ovn_port *op,
             free(lb_ips_v4_as);
         }
 
-        if (sset_count(&op->od->lb_ips_v6_reachable)) {
+        if (sset_count(&op->od->lb_ips->ips_v6_reachable)) {
             ds_clear(match);
 
             if (is_l3dgw_port(op)) {
@@ -14643,23 +14650,25 @@  sync_address_sets(struct northd_input *input_data,
             continue;
         }
 
-        if (sset_count(&od->lb_ips_v4_reachable)) {
+        if (sset_count(&od->lb_ips->ips_v4_reachable)) {
             char *ipv4_addrs_name = lr_lb_address_set_name(od, AF_INET);
-            const char **ipv4_addrs = sset_array(&od->lb_ips_v4_reachable);
+            const char **ipv4_addrs =
+                sset_array(&od->lb_ips->ips_v4_reachable);
 
             sync_address_set(ovnsb_txn, ipv4_addrs_name, ipv4_addrs,
-                             sset_count(&od->lb_ips_v4_reachable),
+                             sset_count(&od->lb_ips->ips_v4_reachable),
                              &sb_address_sets);
             free(ipv4_addrs_name);
             free(ipv4_addrs);
         }
 
-        if (sset_count(&od->lb_ips_v6_reachable)) {
+        if (sset_count(&od->lb_ips->ips_v6_reachable)) {
             char *ipv6_addrs_name = lr_lb_address_set_name(od, AF_INET6);
-            const char **ipv6_addrs = sset_array(&od->lb_ips_v6_reachable);
+            const char **ipv6_addrs =
+                sset_array(&od->lb_ips->ips_v6_reachable);
 
             sync_address_set(ovnsb_txn, ipv6_addrs_name, ipv6_addrs,
-                             sset_count(&od->lb_ips_v6_reachable),
+                             sset_count(&od->lb_ips->ips_v6_reachable),
                              &sb_address_sets);
             free(ipv6_addrs_name);
             free(ipv6_addrs);
@@ -15421,6 +15430,7 @@  northd_destroy(struct northd_data *data)
 
     struct ovn_lb_group *lb_group;
     HMAP_FOR_EACH_POP (lb_group, hmap_node, &data->lb_groups) {
+        ovn_lb_ip_set_destroy(lb_group->lb_ips);
         free(lb_group->lbs);
         free(lb_group->ls);
         free(lb_group->lr);
diff --git a/northd/northd.h b/northd/northd.h
index 8d299864f..0723d900a 100644
--- a/northd/northd.h
+++ b/northd/northd.h
@@ -236,16 +236,9 @@  struct ovn_datapath {
     struct lport_addresses dnat_force_snat_addrs;
     struct lport_addresses lb_force_snat_addrs;
     bool lb_force_snat_router_ip;
-    /* The "routable" ssets are subsets of the load balancer
-     * IPs for which IP routes and ARP resolution flows are automatically
-     * added
-     */
-    struct sset lb_ips_v4;
-    struct sset lb_ips_v4_routable;
-    struct sset lb_ips_v4_reachable;
-    struct sset lb_ips_v6;
-    struct sset lb_ips_v6_routable;
-    struct sset lb_ips_v6_reachable;
+
+    /* Load Balancer vIPs relevant for this datapath. */
+    struct ovn_lb_ip_set *lb_ips;
 
     struct ovn_port **localnet_ports;
     size_t n_localnet_ports;