On Wed, Aug 20, 2025 at 6:56 PM Lorenzo Bianconi via dev <
ovs-dev@openvswitch.org> 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>
> ---
>
Hi Lorenzo,
thank you for the patch. The commit message is not telling the whole
story though, this is also replacing arrays with vectors. That isn't
a bad thing, but it should be stated in the commit message.
I have also a couple of comments down below.
lib/ovn-util.h | 20 ++++++++++++++
> northd/en-lr-stateful.c | 4 +--
> northd/lb.c | 31 ++++++++++++++++-----
> northd/lb.h | 21 +++++++--------
> northd/northd.c | 60 ++++++++++++++++++++++++-----------------
> 5 files changed, 92 insertions(+), 44 deletions(-)
>
> diff --git a/lib/ovn-util.h b/lib/ovn-util.h
> index 63beae3e5..c0c3d959a 100644
> --- a/lib/ovn-util.h
> +++ b/lib/ovn-util.h
> @@ -18,6 +18,7 @@
>
> #include "openvswitch/meta-flow.h"
> #include "ovsdb-idl.h"
> +#include "lib/bitmap.h"
> #include "lib/packets.h"
> #include "lib/sset.h"
> #include "lib/svec.h"
> @@ -482,6 +483,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;
> +}
> +
>
Arguably this whole realloc should be part of the 1/5
in the API.
> /* Utilities around properly handling exit command. */
> struct ovn_exit_args {
> struct unixctl_conn **conns;
> diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> index 58e90f2f2..6a546f0d2 100644
> --- a/northd/en-lr-stateful.c
> +++ b/northd/en-lr-stateful.c
> @@ -297,8 +297,8 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> void *data_)
> const struct ovn_lb_datapaths *lb_dps = ovn_lb_datapaths_find(
> input_data.lb_datapaths_map, lb_uuid);
> ovs_assert(lb_dps);
> - for (size_t i = 0; i < lbgrp_dps->n_lr; i++) {
> - const struct ovn_datapath *od = lbgrp_dps->lr[i];
> + struct ovn_datapath *od;
> + VECTOR_FOR_EACH (&lbgrp_dps->lr, od) {
> struct lr_stateful_record *lr_stateful_rec =
> lr_stateful_table_find_(&data->table, od->nbr);
> ovs_assert(lr_stateful_rec);
> diff --git a/northd/lb.c b/northd/lb.c
> index 9ce9f7bc9..11bd17515 100644
> --- a/northd/lb.c
> +++ b/northd/lb.c
> @@ -632,7 +632,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;
> @@ -648,11 +650,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 = new_n_elems;
> + }
> +}
>
Again this should be part of the 1/5.
> +
> 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++;
> @@ -662,9 +676,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++;
> @@ -694,8 +711,10 @@ ovn_lb_group_datapaths_create(const struct
> ovn_lb_group *lb_group,
> struct ovn_lb_group_datapaths *lb_group_dps =
> xzalloc(sizeof *lb_group_dps);
> 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->ls = VECTOR_CAPACITY_INITIALIZER(struct ovn_datapath *,
> + max_ls_datapaths);
> + lb_group_dps->lr = VECTOR_CAPACITY_INITIALIZER(struct ovn_datapath *,
> + max_lr_datapaths);
>
> return lb_group_dps;
> }
> @@ -703,8 +722,8 @@ ovn_lb_group_datapaths_create(const struct
> ovn_lb_group *lb_group,
> void
> ovn_lb_group_datapaths_destroy(struct ovn_lb_group_datapaths
> *lb_group_dps)
> {
> - free(lb_group_dps->ls);
> - free(lb_group_dps->lr);
> + vector_destroy(&lb_group_dps->ls);
> + vector_destroy(&lb_group_dps->lr);
> free(lb_group_dps);
> }
>
> diff --git a/northd/lb.h b/northd/lb.h
> index 1c03c3763..81cb59fec 100644
> --- a/northd/lb.h
> +++ b/northd/lb.h
> @@ -191,9 +191,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;
> @@ -201,10 +203,8 @@ struct ovn_lb_group_datapaths {
> const struct ovn_lb_group *lb_group;
>
> /* Datapaths to which 'lb_group' is applied. */
> - size_t n_ls;
> - struct ovn_datapath **ls;
> - size_t n_lr;
> - struct ovn_datapath **lr;
> + struct vector ls;
> + struct vector lr;
> };
>
> struct ovn_lb_group_datapaths *ovn_lb_group_datapaths_create(
> @@ -217,17 +217,16 @@ 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)
> {
> - memcpy(&lbg_dps->ls[lbg_dps->n_ls], ods, n * sizeof *ods);
> - lbg_dps->n_ls += n;
> + vector_push_array(&lbg_dps->ls, ods, 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)
> {
> - lbg_dps->lr[lbg_dps->n_lr++] = lr;
> + vector_push(&lbg_dps->lr, &lr);
> }
>
> #endif /* OVN_NORTHD_LB_H */
> diff --git a/northd/northd.c b/northd/northd.c
> index 2e1cce6d0..0de440559 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3291,7 +3291,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);
> }
> @@ -3327,7 +3327,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));
> }
> }
>
> @@ -3337,10 +3337,12 @@ build_lb_datapaths(const struct hmap *lbs, const
> struct hmap *lb_groups,
> &lb_group_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, lb_group_dps->n_ls,
> - lb_group_dps->ls);
> - ovn_lb_datapaths_add_lr(lb_dps, lb_group_dps->n_lr,
> - lb_group_dps->lr);
> + ovn_lb_datapaths_add_ls(lb_dps, vector_len(&lb_group_dps->ls),
> + vector_get_array(&lb_group_dps->ls),
> + ods_size(ls_datapaths));
> + ovn_lb_datapaths_add_lr(lb_dps, vector_len(&lb_group_dps->lr),
> + vector_get_array(&lb_group_dps->lr),
> + ods_size(lr_datapaths));
> }
> }
> }
> @@ -3400,6 +3402,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)
> {
> @@ -3415,14 +3418,15 @@ 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));
> }
> }
>
> struct ovn_lb_group_datapaths *lb_group_dps;
> HMAP_FOR_EACH (lb_group_dps, hmap_node, lb_group_dps_map) {
> - for (size_t i = 0; i < lb_group_dps->n_lr; i++) {
> - struct ovn_datapath *od = lb_group_dps->lr[i];
> + struct ovn_datapath *od;
> + VECTOR_FOR_EACH (&lb_group_dps->lr, od) {
> ovn_lb_group_datapaths_add_ls(lb_group_dps,
> vector_len(&od->ls_peers),
>
> vector_get_array(&od->ls_peers));
> @@ -3432,7 +3436,8 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths
> *lr_datapaths,
> 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));
> }
> }
> }
> @@ -3463,7 +3468,9 @@ build_lb_port_related_data(
> struct ovsdb_idl_index *sbrec_service_monitor_by_learned_type,
> 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 *local_svc_monitors_map,
> @@ -3473,7 +3480,8 @@ build_lb_port_related_data(
> svc_monitor_mac, svc_monitor_mac_ea, ls_ports,
> lb_dps_map, svc_monitor_lsps,
> local_svc_monitors_map, ic_learned_svc_monitors_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
> @@ -5069,7 +5077,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);
> @@ -5091,7 +5099,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);
> @@ -5110,7 +5119,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);
> @@ -5128,7 +5137,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);
> @@ -5167,14 +5177,14 @@ northd_handle_lb_data_changes(struct
> tracked_lb_data *trk_lb_data,
> lb_uuid = &lb->nlb->header_.uuid;
> lb_dps = ovn_lb_datapaths_find(lb_datapaths_map, lb_uuid);
> 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);
> + VECTOR_FOR_EACH (&lbgrp_dps->lr, 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);
> + VECTOR_FOR_EACH (&lbgrp_dps->ls, 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);
> @@ -19178,9 +19188,9 @@ ovnnb_db_run(struct northd_input *input_data,
> build_lb_port_related_data(ovnsb_txn,
> input_data->sbrec_service_monitor_by_learned_type,
> input_data->svc_monitor_mac, &input_data->svc_monitor_mac_ea,
> - &data->lr_datapaths, &data->ls_ports, &data->lb_datapaths_map,
> - &data->lb_group_datapaths_map, &data->svc_monitor_lsps,
> - &data->local_svc_monitors_map,
> + &data->lr_datapaths, &data->ls_datapaths, &data->ls_ports,
> + &data->lb_datapaths_map, &data->lb_group_datapaths_map,
> + &data->svc_monitor_lsps, &data->local_svc_monitors_map,
> input_data->ic_learned_svc_monitors_map);
> build_lb_count_dps(&data->lb_datapaths_map,
> ods_size(&data->ls_datapaths),
> --
> 2.50.1
>
> _______________________________________________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thanks,
Ales
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
