Re: [ovs-dev] [PATCH ovn v3 2/4] northd: Reallocate LB nb_lr_map/nb_ls_map if needed.

[Mark Michelson via dev]

Hi Lorenzo,

I have feedback inline.
On 7/24/25 1:20 PM, Lorenzo Bianconi 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  | 20 +++++++++++++++++
  northd/lb.c     | 23 ++++++++++++++++++--
  northd/lb.h     | 22 +++++++++++++++----
  northd/northd.c | 58 ++++++++++++++++++++++++++++++++-----------------
  4 files changed, 97 insertions(+), 26 deletions(-)

diff --git a/lib/ovn-util.h b/lib/ovn-util.h
index 695058f90..a8ade6bb4 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,25 @@ 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. */
+    size_t delta = BITMAP_N_LONGS(n_bits_new) - BITMAP_N_LONGS(n_bits_old);
+    memset(&bitmap[BITMAP_N_LONGS(n_bits_old)], 0, delta * sizeof *bitmap);
+
+    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 d8327c2fc..9615aa244 100644
--- a/northd/lb.c
+++ b/northd/lb.c
@@ -579,7 +579,9 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb *lb, 
size_t n_ls_datapaths,
      struct ovn_lb_datapaths *lb_dps = xzalloc(sizeof *lb_dps);
      lb_dps->lb = lb;
      lb_dps->nb_ls_map.map = bitmap_allocate(n_ls_datapaths);
+    lb_dps->nb_ls_map.capacity = n_ls_datapaths;
      lb_dps->nb_lr_map.map = bitmap_allocate(n_lr_datapaths);
+    lb_dps->nb_lr_map.capacity = n_lr_datapaths;
      lb_dps->lflow_ref = lflow_ref_create();
      hmapx_init(&lb_dps->ls_lb_with_stateless_mode);
      return lb_dps;
@@ -595,11 +597,23 @@ ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
      free(lb_dps);
  }
  
+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->capacity, new_n_elems);
+        db->capacity = bitmap_n_bytes(new_n_elems);

This is my fault because of the feedback I gave on v2.

Based on how db->capacity is used throughout the rest of this file, it 
should be set to new_n_elems here instead of 
bitmap_n_bytes(new_n_elems). In the rest of the file, db->capacity 
indicates the highest number of bits the bitmap can hold, but here, 
db->capacity is being set to the size of the bitmap allocation in bytes. 
This will make db->capacity lower than it should be. This likely will 
result in the bitmap being reallocated when it doesn't need to be, and 
even worse, we'll probably assert in ovn_lb_datapaths_add_lr() or 
ovn_lb_datapaths_add_ls() eventually.

+    }
+}
+
  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)
  {
+    dynamic_bitmap_realloc(&lb_dps->nb_lr_map, n_lr_datapaths);
      for (size_t i = 0; i < n; i++) {
+        ovs_assert(ods[i]->index < lb_dps->nb_lr_map.capacity);
          if (!bitmap_is_set(lb_dps->nb_lr_map.map, ods[i]->index)) {
              bitmap_set1(lb_dps->nb_lr_map.map, ods[i]->index);
              lb_dps->nb_lr_map.n_elems++;
@@ -609,9 +623,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)
  {
+    dynamic_bitmap_realloc(&lb_dps->nb_ls_map, n_ls_datapaths);
      for (size_t i = 0; i < n; i++) {
+        ovs_assert(ods[i]->index < lb_dps->nb_ls_map.capacity);
          if (!bitmap_is_set(lb_dps->nb_ls_map.map, ods[i]->index)) {
              bitmap_set1(lb_dps->nb_ls_map.map, ods[i]->index);
              lb_dps->nb_ls_map.n_elems++;
@@ -643,6 +660,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;
  }
diff --git a/northd/lb.h b/northd/lb.h
index 1d6152a1d..be9f9a837 100644
--- a/northd/lb.h
+++ b/northd/lb.h
@@ -185,9 +185,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;
@@ -196,8 +198,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;
  };
  
@@ -211,16 +215,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;
+    }
>       memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods);> 
  lbg_dps->n_ls += n;

In the previous iteration of this patch, I suggested using a vector 
instead of coding the dynamic array manually.

If this were done, then the entire ovn_lb_group_datapaths_add_ls() 
function could be replaced with a call to vector_push_array().

All vector operations that insert data (vector_push(), 
vector_push_array(), and vector_insert()) will automatically reallocate 
the vector's dynamic array.

  }
  
  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;
  }

And ovn_lb_group_datapaths_add_lr() could be replaced with a call to 
vector_push().

  
diff --git a/northd/northd.c b/northd/northd.c
index 95cd67a5a..e4125606a 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)
  {
@@ -3779,7 +3784,8 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths 
*lr_datapaths,
                             lb_dps->nb_lr_map.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));
          }
      }
  
@@ -3789,14 +3795,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));
              }
          }
      }
@@ -3827,7 +3834,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)
@@ -3835,7 +3844,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
@@ -5324,7 +5334,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);
@@ -5338,7 +5348,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++) {
@@ -5346,7 +5357,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);
@@ -5365,7 +5377,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);
@@ -5375,7 +5387,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++) {
@@ -5383,7 +5396,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);
@@ -5424,12 +5438,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);
@@ -19215,7 +19231,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