On Aug 27, Ales Musil wrote:
> 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.
Hi Ales,
sure, I will fix it in v5.
>
> 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.
ack, I will fix it in v5.
>
>
> > /* 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.
ack, I will fix it in v5.
Regards,
Lorenzo
>
>
> > +
> > 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
