Re: [ovs-dev] [PATCH v2 ovn 1/3] northd: Reallocate LB nb_lr_map/nb_ls_map if needed.

[Mark Michelson via dev]

On 7/19/25 3:54 AM, Lorenzo Bianconi via dev wrote:
From: Numan Siddique <num...@ovn.org>
This is required to add I-P for logical switch and logical router
inserts.

Signed-off-by: Numan Siddique <num...@ovn.org>
---
  lib/ovn-util.h  | 22 +++++++++++++++++++
  northd/lb.c     | 44 +++++++++++++++++++++++++++++++++++--
  northd/lb.h     | 24 ++++++++++++++++----
  northd/northd.c | 58 ++++++++++++++++++++++++++++++++-----------------
  4 files changed, 122 insertions(+), 26 deletions(-)

diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 695058f90..13bafeefe 100644
--- a/lib/ovn-util.h
+++ b/lib/ovn-util.h
@@ -17,6 +17,7 @@
  #define OVN_UTIL_H 1
  
  #include "ovsdb-idl.h"
+#include "lib/bitmap.h"
  #include "lib/packets.h"
  #include "lib/sset.h"
  #include "lib/svec.h"
@@ -477,6 +478,27 @@ void sorted_array_apply_diff(const struct sorted_array *a1,
                                                      bool add),
                               const void *arg);
  
+static inline unsigned long *
+ovn_bitmap_realloc(unsigned long *bitmap, size_t n_bits_old,
+                   size_t n_bits_new)
+{
+    ovs_assert(n_bits_new >= n_bits_old);
+
+    if (bitmap_n_bytes(n_bits_old) == bitmap_n_bytes(n_bits_new)) {
+        return bitmap;
+    }
+
+    bitmap = xrealloc(bitmap, bitmap_n_bytes(n_bits_new));
+    /* Set the unitialized bits to 0 as xrealloc doesn't initialize the
+     * added memory. */
+    for (size_t i = BITMAP_N_LONGS(n_bits_old);
+         i < BITMAP_N_LONGS(n_bits_new); i++) {
+        bitmap[i] = 0;
+    }

Could this for loop be replaced with a single memset() call? If so, is 
it worth it to do that, or can we rely on the compiler to optimize this 
into a single call?

+
+    return bitmap;
+}
+
  /* Utilities around properly handling exit command. */
  struct ovn_exit_args {
      struct unixctl_conn **conns;
diff --git a/northd/lb.c b/northd/lb.c
index b11896cf1..d61818721 100644
--- a/northd/lb.c
+++ b/northd/lb.c
@@ -40,6 +40,11 @@ static struct nbrec_load_balancer_health_check *
  ovn_lb_get_health_check(const struct nbrec_load_balancer *nbrec_lb,
                          const char *vip_port_str, bool template);
  
+static void ovn_lb_datapaths_realloc_ls_map(struct ovn_lb_datapaths *,
+                                            size_t n_ls_datapaths);
+static void ovn_lb_datapaths_realloc_lr_map(struct ovn_lb_datapaths *,
+                                            size_t n_lr_datapaths);
+
  struct ovn_lb_ip_set *
  ovn_lb_ip_set_create(void)
  {
@@ -580,6 +585,8 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb, 
size_t n_ls_datapaths,
      lb_dps->lb = lb;
      lb_dps->nb_ls_map = bitmap_allocate(n_ls_datapaths);
      lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
+    lb_dps->ls_map_size = n_ls_datapaths;
+    lb_dps->lr_map_size = n_lr_datapaths;
      lb_dps->lflow_ref = lflow_ref_create();
      hmapx_init(&lb_dps->ls_lb_with_stateless_mode);
      return lb_dps;
@@ -597,9 +604,12 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
  
  void
  ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *lb_dps, size_t n,
-                        struct ovn_datapath **ods)
+                        struct ovn_datapath **ods,
+                        size_t n_lr_datapaths)
  {
+    ovn_lb_datapaths_realloc_lr_map(lb_dps, n_lr_datapaths);
      for (size_t i = 0; i < n; i++) {
+        ovs_assert(ods[i]->index < lb_dps->lr_map_size);
          if (!bitmap_is_set(lb_dps->nb_lr_map, ods[i]->index)) {
              bitmap_set1(lb_dps->nb_lr_map, ods[i]->index);
              lb_dps->n_nb_lr++;
@@ -609,9 +619,12 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *lb_dps, 
size_t n,
  
  void
  ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *lb_dps, size_t n,
-                        struct ovn_datapath **ods)
+                        struct ovn_datapath **ods,
+                        size_t n_ls_datapaths)
  {
+    ovn_lb_datapaths_realloc_ls_map(lb_dps, n_ls_datapaths);
      for (size_t i = 0; i < n; i++) {
+        ovs_assert(ods[i]->index < lb_dps->ls_map_size);
          if (!bitmap_is_set(lb_dps->nb_ls_map, ods[i]->index)) {
              bitmap_set1(lb_dps->nb_ls_map, ods[i]->index);
              lb_dps->n_nb_ls++;
@@ -643,6 +656,8 @@ ovn_lb_group_datapaths_create(const struct ovn_lb_group 
*lb_group,
      lb_group_dps->lb_group = lb_group;
      lb_group_dps->ls = xmalloc(max_ls_datapaths * sizeof *lb_group_dps->ls);
      lb_group_dps->lr = xmalloc(max_lr_datapaths * sizeof *lb_group_dps->lr);
+    lb_group_dps->max_lr = max_ls_datapaths;
+    lb_group_dps->max_lr = max_lr_datapaths;
  
      return lb_group_dps;
  }
@@ -669,3 +684,28 @@ ovn_lb_group_datapaths_find(const struct hmap 
*lb_group_dps_map,
      }
      return NULL;
  }
+
+/* Static functions. */
+static void
+ovn_lb_datapaths_realloc_ls_map(struct ovn_lb_datapaths *lb_dps,
+                                size_t n_ls_datapaths)
+{
+    if (n_ls_datapaths > lb_dps->ls_map_size) {
+        lb_dps->nb_ls_map = ovn_bitmap_realloc(lb_dps->nb_ls_map,
+                                               lb_dps->ls_map_size,
+                                               n_ls_datapaths);
+        lb_dps->ls_map_size = n_ls_datapaths;
+    }
+}
+
+static void
+ovn_lb_datapaths_realloc_lr_map(struct ovn_lb_datapaths *lb_dps,
+                                size_t n_lr_datapaths)
+{
+    if (n_lr_datapaths > lb_dps->lr_map_size) {
+        lb_dps->nb_lr_map = ovn_bitmap_realloc(lb_dps->nb_lr_map,
+                                               lb_dps->lr_map_size,
+                                               n_lr_datapaths);
+        lb_dps->lr_map_size = n_lr_datapaths;
+    }
+}

This has a bit of "code smell" to it. The two functions are identical, 
except for the names of struct fields that are accessed. The differently 
named struct fields are of identical types, so it's not that the 
functions are needed to cover different types being worked with.

One way to consolidate things would be to do something like this:

struct dynamic_bitmap {
    unsigned long *map;
    size_t n_elems;
    size_t capacity;
};

Then:

struct ovn_lb_datapaths {
    /* Other fields */
    struct dynamic_bitmap nb_ls_map;
    struct dynamic_bitmap nb_lr_map;
    /* More other fields */
};

Then, you can define a generic

static void
dynamic_bitmap_realloc(struct dynamic_bitmap *db, size_t new_n_elems)
{
    if (new_n_elems > db->capacity) {
        db->map = ovn_bitmap_realloc(db->map, db->n_elems, new_n_elems);
        db->capacity = bitmap_n_bytes(new_n_elems);
    }
}

Then you can use the same dynamic_bitmap_realloc() function for both 
maps in ovn_lb_datapaths. This also has the possibility to be useful 
other places where bitmaps are used for incremental processing. It may 
be worthwhile to define the dynamic bitmap in its own library.

diff --git a/northd/lb.h b/northd/lb.h
index eb1942bd4..7a810061b 100644
--- a/northd/lb.h
+++ b/northd/lb.h
@@ -137,9 +137,11 @@ struct ovn_lb_datapaths {
  
      const struct ovn_northd_lb *lb;
      size_t n_nb_ls;
+    size_t ls_map_size;
      unsigned long *nb_ls_map;
  
      size_t n_nb_lr;
+    size_t lr_map_size;
      unsigned long *nb_lr_map;
  
      struct hmapx ls_lb_with_stateless_mode;
@@ -180,9 +182,11 @@ struct ovn_lb_datapaths *ovn_lb_datapaths_find(const 
struct hmap *,
  void ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *);
  
  void ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths *, size_t n,
-                             struct ovn_datapath **);
+                             struct ovn_datapath **,
+                             size_t n_lr_datapaths);
  void ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths *, size_t n,
-                             struct ovn_datapath **);
+                             struct ovn_datapath **,
+                             size_t n_ls_datapaths);
  
  struct ovn_lb_group_datapaths {
      struct hmap_node hmap_node;
@@ -191,8 +195,10 @@ struct ovn_lb_group_datapaths {
  
      /* Datapaths to which 'lb_group' is applied. */
      size_t n_ls;
+    size_t max_ls;
      struct ovn_datapath **ls;
      size_t n_lr;
+    size_t max_lr;
      struct ovn_datapath **lr;
  };
  
@@ -206,16 +212,26 @@ struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_find(
  
  static inline void
  ovn_lb_group_datapaths_add_ls(struct ovn_lb_group_datapaths *lbg_dps, size_t 
n,
-                               struct ovn_datapath **ods)
+                              struct ovn_datapath **ods, size_t n_ls_datapaths)
  {
+    if (n_ls_datapaths > lbg_dps->max_ls) {
+        lbg_dps->ls = xrealloc(lbg_dps->ls,
+                               n_ls_datapaths * sizeof *lbg_dps->ls);
+        lbg_dps->max_ls = n_ls_datapaths;
+    }

Based on my reading of the code, lbg_dps->ls and lbg_dps->lr could be 
implemented as vectors instead of open-coded dynamic arrays. That would 
simplify some of the logic here and in other places.

      memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods);
      lbg_dps->n_ls += n;
  }
  
  static inline void
  ovn_lb_group_datapaths_add_lr(struct ovn_lb_group_datapaths *lbg_dps,
-                               struct ovn_datapath *lr)
+                              struct ovn_datapath *lr, size_t n_lr_datapaths)
  {
+    if (n_lr_datapaths > lbg_dps->max_lr) {
+        lbg_dps->lr = xrealloc(lbg_dps->lr,
+                               n_lr_datapaths * sizeof *lbg_dps->lr);
+        lbg_dps->max_lr = n_lr_datapaths;
+    }
      lbg_dps->lr[lbg_dps->n_lr++] = lr;
  }
  
diff --git a/northd/northd.c b/northd/northd.c
index 9df8f10e0..0e990c19a 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -3669,7 +3669,7 @@ build_lb_datapaths(const struct hmap *lbs, const struct 
hmap *lb_groups,
                  &od->nbs->load_balancer[i]->header_.uuid;
              lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
              ovs_assert(lb_dps);
-            ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+            ovn_lb_datapaths_add_ls(lb_dps, 1, &od, ods_size(ls_datapaths));
              if (od->lb_with_stateless_mode) {
                  hmapx_add(&lb_dps->ls_lb_with_stateless_mode, od);
              }
@@ -3682,7 +3682,8 @@ build_lb_datapaths(const struct hmap *lbs, const struct 
hmap *lb_groups,
                  ovn_lb_group_datapaths_find(lb_group_datapaths_map,
                                              lb_group_uuid);
              ovs_assert(lb_group_dps);
-            ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &od);
+            ovn_lb_group_datapaths_add_ls(lb_group_dps, 1, &od,
+                                          ods_size(ls_datapaths));
          }
      }
  
@@ -3697,7 +3698,8 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups,
                  ovn_lb_group_datapaths_find(lb_group_datapaths_map,
                                              lb_group_uuid);
              ovs_assert(lb_group_dps);
-            ovn_lb_group_datapaths_add_lr(lb_group_dps, od);
+            ovn_lb_group_datapaths_add_lr(lb_group_dps, od,
+                                          ods_size(lr_datapaths));
          }
  
          for (size_t i = 0; i < od->nbr->n_load_balancer; i++) {
@@ -3705,7 +3707,7 @@ build_lb_datapaths(const struct hmap *lbs, const struct 
hmap *lb_groups,
                  &od->nbr->load_balancer[i]->header_.uuid;
              lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
              ovs_assert(lb_dps);
-            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
+            ovn_lb_datapaths_add_lr(lb_dps, 1, &od, ods_size(lr_datapaths));
          }
      }
  
@@ -3716,9 +3718,11 @@ build_lb_datapaths(const struct hmap *lbs, const struct hmap *lb_groups,
              lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
              ovs_assert(lb_dps);
              ovn_lb_datapaths_add_ls(lb_dps, lb_group_dps->n_ls,
-                                    lb_group_dps->ls);
+                                    lb_group_dps->ls,
+                                    ods_size(ls_datapaths));
              ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr,
-                                    lb_group_dps->lr);
+                                    lb_group_dps->lr,
+                                    ods_size(lr_datapaths));
          }
      }
  }
@@ -3764,6 +3768,7 @@ build_lb_svcs(
  
  static void
  build_lswitch_lbs_from_lrouter(struct ovn_datapaths *lr_datapaths,
+                               struct ovn_datapaths *ls_datapaths,
                                 struct hmap *lb_dps_map,
                                 struct hmap *lb_group_dps_map)
  {
@@ -3778,7 +3783,8 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths 
*lr_datapaths,
          BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths), lb_dps->nb_lr_map) {
              struct ovn_datapath *od = lr_datapaths->array[index];
              ovn_lb_datapaths_add_ls(lb_dps, vector_len(&od->ls_peers),
-                                    vector_get_array(&od->ls_peers));
+                                    vector_get_array(&od->ls_peers),
+                                    ods_size(ls_datapaths));
          }
      }
  
@@ -3788,14 +3794,15 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths *lr_datapaths,
              struct ovn_datapath *od = lb_group_dps->lr[i];
              ovn_lb_group_datapaths_add_ls(lb_group_dps,
                                            vector_len(&od->ls_peers),
-                                          vector_get_array(&od->ls_peers));
+                                          vector_get_array(&od->ls_peers), 0);
              for (size_t j = 0; j < lb_group_dps->lb_group->n_lbs; j++) {
                  const struct uuid *lb_uuid =
                      &lb_group_dps->lb_group->lbs[j]->nlb->header_.uuid;
                  lb_dps = ovn_lb_datapaths_find(lb_dps_map, lb_uuid);
                  ovs_assert(lb_dps);
                  ovn_lb_datapaths_add_ls(lb_dps, vector_len(&od->ls_peers),
-                                        vector_get_array(&od->ls_peers));
+                                        vector_get_array(&od->ls_peers),
+                                        ods_size(ls_datapaths));
              }
          }
      }
@@ -3824,7 +3831,9 @@ build_lb_port_related_data(
      const struct sbrec_service_monitor_table *sbrec_service_monitor_table,
      const char *svc_monitor_mac,
      const struct eth_addr *svc_monitor_mac_ea,
-    struct ovn_datapaths *lr_datapaths, struct hmap *ls_ports,
+    struct ovn_datapaths *lr_datapaths,
+    struct ovn_datapaths *ls_datapaths,
+    struct hmap *ls_ports,
      struct hmap *lb_dps_map, struct hmap *lb_group_dps_map,
      struct sset *svc_monitor_lsps,
      struct hmap *svc_monitor_map)
@@ -3832,7 +3841,8 @@ build_lb_port_related_data(
      build_lb_svcs(ovnsb_txn, sbrec_service_monitor_table, svc_monitor_mac,
                    svc_monitor_mac_ea, ls_ports, lb_dps_map,
                    svc_monitor_lsps, svc_monitor_map);
-    build_lswitch_lbs_from_lrouter(lr_datapaths, lb_dps_map, lb_group_dps_map);
+    build_lswitch_lbs_from_lrouter(lr_datapaths, ls_datapaths, lb_dps_map,
+                                   lb_group_dps_map);
  }
  
  /* Returns true if the peer port IPs of op should be added in the nat_addresses
@@ -5321,7 +5331,7 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
          UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbs) {
              lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, &uuidnode->uuid);
              ovs_assert(lb_dps);
-            ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+            ovn_lb_datapaths_add_ls(lb_dps, 1, &od, ods_size(ls_datapaths));
  
              if (od->lb_with_stateless_mode) {
                  hmapx_add(&lb_dps->ls_lb_with_stateless_mode, od);
@@ -5335,7 +5345,8 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
              lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
                                                      &uuidnode->uuid);
              ovs_assert(lbgrp_dps);
-            ovn_lb_group_datapaths_add_ls(lbgrp_dps, 1, &od);
+            ovn_lb_group_datapaths_add_ls(lbgrp_dps, 1, &od,
+                                          ods_size(ls_datapaths));
  
              /* Associate all the lbs of the lbgrp to the datapath 'od' */
              for (size_t j = 0; j < lbgrp_dps->lb_group->n_lbs; j++) {
@@ -5343,7 +5354,8 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
                      = &lbgrp_dps->lb_group->lbs[j]->nlb->header_.uuid;
                  lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
                  ovs_assert(lb_dps);
-                ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+                ovn_lb_datapaths_add_ls(lb_dps, 1, &od,
+                                        ods_size(ls_datapaths));
  
                  /* Add the lb to the northd tracked data. */
                  hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
@@ -5362,7 +5374,7 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
          UUIDSET_FOR_EACH (uuidnode, &codlb->assoc_lbs) {
              lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, &uuidnode->uuid);
              ovs_assert(lb_dps);
-            ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
+            ovn_lb_datapaths_add_lr(lb_dps, 1, &od, ods_size(lr_datapaths));
  
              /* Add the lb to the northd tracked data. */
              hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
@@ -5372,7 +5384,8 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
              lbgrp_dps = ovn_lb_group_datapaths_find(lbgrp_datapaths_map,
                                                      &uuidnode->uuid);
              ovs_assert(lbgrp_dps);
-            ovn_lb_group_datapaths_add_lr(lbgrp_dps, od);
+            ovn_lb_group_datapaths_add_lr(lbgrp_dps, od,
+                                          ods_size(lr_datapaths));
  
              /* Associate all the lbs of the lbgrp to the datapath 'od' */
              for (size_t j = 0; j < lbgrp_dps->lb_group->n_lbs; j++) {
@@ -5380,7 +5393,8 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
                      = &lbgrp_dps->lb_group->lbs[j]->nlb->header_.uuid;
                  lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
                  ovs_assert(lb_dps);
-                ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
+                ovn_lb_datapaths_add_lr(lb_dps, 1, &od,
+                                        ods_size(lr_datapaths));
  
                  /* Add the lb to the northd tracked data. */
                  hmapx_add(&nd_changes->trk_lbs.crupdated, lb_dps);
@@ -5421,12 +5435,14 @@ northd_handle_lb_data_changes(struct tracked_lb_data 
*trk_lb_data,
              ovs_assert(lb_dps);
              for (size_t i = 0; i < lbgrp_dps->n_lr; i++) {
                  od = lbgrp_dps->lr[i];
-                ovn_lb_datapaths_add_lr(lb_dps, 1, &od);
+                ovn_lb_datapaths_add_lr(lb_dps, 1, &od,
+                                        ods_size(lr_datapaths));
              }
  
              for (size_t i = 0; i < lbgrp_dps->n_ls; i++) {
                 od = lbgrp_dps->ls[i];
-                ovn_lb_datapaths_add_ls(lb_dps, 1, &od);
+                ovn_lb_datapaths_add_ls(lb_dps, 1, &od,
+                                        ods_size(ls_datapaths));
  
                  /* Add the ls datapath to the northd tracked data. */
                  hmapx_add(&nd_changes->ls_with_changed_lbs, od);
@@ -19209,7 +19225,9 @@ ovnnb_db_run(struct northd_input *input_data,
                                 input_data->sbrec_service_monitor_table,
                                 input_data->svc_monitor_mac,
                                 &input_data->svc_monitor_mac_ea,
-                               &data->lr_datapaths, &data->ls_ports,
+                               &data->lr_datapaths,
+                               &data->ls_datapaths,
+                               &data->ls_ports,
                                 &data->lb_datapaths_map,
                                 &data->lb_group_datapaths_map,
                                 &data->svc_monitor_lsps,

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev