On Aug 27, Ales Musil wrote:
> On Wed, Aug 20, 2025 at 6:56 PM Lorenzo Bianconi via dev <
> ovs-dev@openvswitch.org> wrote:
>
> > Reduce duplicated code between nb_ls_map and nb_lr_map.
> >
> > Acked-by: Mark Michelson <mmich...@redhat.com>
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianc...@redhat.com>
> > ---
> >
>
> Hi Lorenzo,
> before proceeding with the series I think this newly introduced
> helper should have at least basic API around the most common
> operations. The code of the series is accessing members of the
> struct directly which is very error prone. At least the most basic
> operations like adding allocation/reallocation/set1/set0 should be
> wrapped in some way because it operates on multiple struct members.
>
> For a good measure there could also be DYNAMIC_BITMAP_FOR_EACH_1
> wrapper macro.
ack, I will fix in v5.
Regards,
Lorenzo
>
>
> > northd/en-lr-stateful.c | 2 +-
> > northd/en-sync-sb.c | 24 ++++++++++---------
> > northd/lb.c | 20 ++++++++--------
> > northd/lb.h | 13 +++++++----
> > northd/northd.c | 52 +++++++++++++++++++++++------------------
> > 5 files changed, 62 insertions(+), 49 deletions(-)
> >
> > diff --git a/northd/en-lr-stateful.c b/northd/en-lr-stateful.c
> > index 56e93f3c4..58e90f2f2 100644
> > --- a/northd/en-lr-stateful.c
> > +++ b/northd/en-lr-stateful.c
> > @@ -252,7 +252,7 @@ lr_stateful_lb_data_handler(struct engine_node *node,
> > void *data_)
> >
> > size_t index;
> > BITMAP_FOR_EACH_1 (index, ods_size(input_data.lr_datapaths),
> > - lb_dps->nb_lr_map) {
> > + lb_dps->nb_lr_map.map) {
> > const struct ovn_datapath *od =
> > ovn_datapaths_find_by_index(input_data.lr_datapaths,
> > index);
> >
> > diff --git a/northd/en-sync-sb.c b/northd/en-sync-sb.c
> > index a111f14fd..343c76b54 100644
> > --- a/northd/en-sync-sb.c
> > +++ b/northd/en-sync-sb.c
> > @@ -676,7 +676,7 @@ sb_lb_table_build_and_sync(
> > struct sb_lb_record *sb_lb;
> >
> > HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
> > - if (!lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
> > + if (!lb_dps->nb_ls_map.n_elems && !lb_dps->nb_lr_map.n_elems) {
> > continue;
> > }
> >
> > @@ -754,10 +754,10 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb,
> > sbrec_lr_dp_group = sbrec_lb->lr_datapath_group;
> > }
> >
> > - if (lb_dps->n_nb_ls) {
> > + if (lb_dps->nb_ls_map.n_elems) {
> > sb_lb->ls_dpg = ovn_dp_group_get(&sb_lbs->ls_dp_groups,
> > - lb_dps->n_nb_ls,
> > - lb_dps->nb_ls_map,
> > + lb_dps->nb_ls_map.n_elems,
> > + lb_dps->nb_ls_map.map,
> > ods_size(ls_datapaths));
> > if (sb_lb->ls_dpg) {
> > /* Update the dpg's sb dp_group. */
> > @@ -787,7 +787,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb,
> > } else {
> > sb_lb->ls_dpg = ovn_dp_group_create(
> > ovnsb_txn, &sb_lbs->ls_dp_groups, sbrec_ls_dp_group,
> > - lb_dps->n_nb_ls, lb_dps->nb_ls_map,
> > + lb_dps->nb_ls_map.n_elems, lb_dps->nb_ls_map.map,
> > ods_size(ls_datapaths), true,
> > ls_datapaths, lr_datapaths);
> > }
> > @@ -809,10 +809,10 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb,
> > }
> >
> >
> > - if (lb_dps->n_nb_lr) {
> > + if (lb_dps->nb_lr_map.n_elems) {
> > sb_lb->lr_dpg = ovn_dp_group_get(&sb_lbs->lr_dp_groups,
> > - lb_dps->n_nb_lr,
> > - lb_dps->nb_lr_map,
> > + lb_dps->nb_lr_map.n_elems,
> > + lb_dps->nb_lr_map.map,
> > ods_size(lr_datapaths));
> > if (sb_lb->lr_dpg) {
> > /* Update the dpg's sb dp_group. */
> > @@ -842,7 +842,7 @@ sync_sb_lb_record(struct sb_lb_record *sb_lb,
> > } else {
> > sb_lb->lr_dpg = ovn_dp_group_create(
> > ovnsb_txn, &sb_lbs->lr_dp_groups, sbrec_lr_dp_group,
> > - lb_dps->n_nb_lr, lb_dps->nb_lr_map,
> > + lb_dps->nb_lr_map.n_elems, lb_dps->nb_lr_map.map,
> > ods_size(lr_datapaths), false,
> > ls_datapaths, lr_datapaths);
> > }
> > @@ -919,7 +919,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs,
> >
> > sb_lb = sb_lb_table_find(&sb_lbs->entries, nb_uuid);
> >
> > - if (!sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
> > + if (!sb_lb && !lb_dps->nb_ls_map.n_elems &&
> > + !lb_dps->nb_lr_map.n_elems) {
> > continue;
> > }
> >
> > @@ -933,7 +934,8 @@ sync_changed_lbs(struct sb_lb_table *sb_lbs,
> > sbrec_load_balancer_table_get_for_uuid(sb_lb_table,
> > nb_uuid);
> > }
> >
> > - if (sb_lb && !lb_dps->n_nb_ls && !lb_dps->n_nb_lr) {
> > + if (sb_lb && !lb_dps->nb_ls_map.n_elems &&
> > + !lb_dps->nb_lr_map.n_elems) {
> > const struct sbrec_load_balancer *sbrec_lb =
> > sbrec_load_balancer_table_get_for_uuid(sb_lb_table,
> > nb_uuid);
> > if (sbrec_lb) {
> > diff --git a/northd/lb.c b/northd/lb.c
> > index 30726cd27..9ce9f7bc9 100644
> > --- a/northd/lb.c
> > +++ b/northd/lb.c
> > @@ -631,8 +631,8 @@ 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 = bitmap_allocate(n_ls_datapaths);
> > - lb_dps->nb_lr_map = bitmap_allocate(n_lr_datapaths);
> > + lb_dps->nb_ls_map.map = bitmap_allocate(n_ls_datapaths);
> > + lb_dps->nb_lr_map.map = bitmap_allocate(n_lr_datapaths);
> > lb_dps->lflow_ref = lflow_ref_create();
> > hmapx_init(&lb_dps->ls_lb_with_stateless_mode);
> > return lb_dps;
> > @@ -641,8 +641,8 @@ ovn_lb_datapaths_create(const struct ovn_northd_lb
> > *lb, size_t n_ls_datapaths,
> > void
> > ovn_lb_datapaths_destroy(struct ovn_lb_datapaths *lb_dps)
> > {
> > - bitmap_free(lb_dps->nb_lr_map);
> > - bitmap_free(lb_dps->nb_ls_map);
> > + bitmap_free(lb_dps->nb_lr_map.map);
> > + bitmap_free(lb_dps->nb_ls_map.map);
> > lflow_ref_destroy(lb_dps->lflow_ref);
> > hmapx_destroy(&lb_dps->ls_lb_with_stateless_mode);
> > free(lb_dps);
> > @@ -653,9 +653,9 @@ ovn_lb_datapaths_add_lr(struct ovn_lb_datapaths
> > *lb_dps, size_t n,
> > struct ovn_datapath **ods)
> > {
> > for (size_t i = 0; i < n; i++) {
> > - 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++;
> > + 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++;
> > }
> > }
> > }
> > @@ -665,9 +665,9 @@ ovn_lb_datapaths_add_ls(struct ovn_lb_datapaths
> > *lb_dps, size_t n,
> > struct ovn_datapath **ods)
> > {
> > for (size_t i = 0; i < n; i++) {
> > - 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++;
> > + 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++;
> > }
> > }
> > }
> > diff --git a/northd/lb.h b/northd/lb.h
> > index a0e560204..1c03c3763 100644
> > --- a/northd/lb.h
> > +++ b/northd/lb.h
> > @@ -138,15 +138,20 @@ void ovn_lb_group_reinit(
> > const struct hmap *lbs);
> >
> > struct lflow_ref;
> > +
> > +struct dynamic_bitmap {
> > + unsigned long *map;
> > + size_t n_elems;
> > + size_t capacity;
> > +};
> > +
> > struct ovn_lb_datapaths {
> > struct hmap_node hmap_node;
> >
> > const struct ovn_northd_lb *lb;
> > - size_t n_nb_ls;
> > - unsigned long *nb_ls_map;
> >
> > - size_t n_nb_lr;
> > - unsigned long *nb_lr_map;
> > + struct dynamic_bitmap nb_ls_map;
> > + struct dynamic_bitmap nb_lr_map;
> >
> > struct hmapx ls_lb_with_stateless_mode;
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 015f30a35..2e1cce6d0 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3411,7 +3411,8 @@ build_lswitch_lbs_from_lrouter(struct ovn_datapaths
> > *lr_datapaths,
> > size_t index;
> >
> > HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
> > - BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> > lb_dps->nb_lr_map) {
> > + BITMAP_FOR_EACH_1 (index, ods_size(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));
> > @@ -3445,8 +3446,10 @@ build_lb_count_dps(struct hmap *lb_dps_map,
> > struct ovn_lb_datapaths *lb_dps;
> >
> > HMAP_FOR_EACH (lb_dps, hmap_node, lb_dps_map) {
> > - lb_dps->n_nb_lr = bitmap_count1(lb_dps->nb_lr_map,
> > n_lr_datapaths);
> > - lb_dps->n_nb_ls = bitmap_count1(lb_dps->nb_ls_map,
> > n_ls_datapaths);
> > + lb_dps->nb_lr_map.n_elems = bitmap_count1(lb_dps->nb_lr_map.map,
> > + n_lr_datapaths);
> > + lb_dps->nb_ls_map.n_elems = bitmap_count1(lb_dps->nb_ls_map.map,
> > + n_ls_datapaths);
> > }
> > }
> >
> > @@ -5007,7 +5010,7 @@ northd_handle_lb_data_changes(struct tracked_lb_data
> > *trk_lb_data,
> >
> > size_t index;
> > BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > - lb_dps->nb_ls_map) {
> > + lb_dps->nb_ls_map.map) {
> > od = ls_datapaths->array[index];
> >
> > /* Add the ls datapath to the northd tracked data. */
> > @@ -5141,7 +5144,7 @@ northd_handle_lb_data_changes(struct tracked_lb_data
> > *trk_lb_data,
> > ovs_assert(lb_dps);
> > size_t index;
> > BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > - lb_dps->nb_ls_map) {
> > + lb_dps->nb_ls_map.map) {
> > od = ls_datapaths->array[index];
> >
> > /* Add the ls datapath to the northd tracked data. */
> > @@ -7589,7 +7592,7 @@ build_lb_rules_pre_stateful(struct lflow_table
> > *lflows,
> > const struct ovn_datapaths *ls_datapaths,
> > struct ds *match, struct ds *action)
> > {
> > - if (!lb_dps->n_nb_ls) {
> > + if (!lb_dps->nb_ls_map.n_elems) {
> > return;
> > }
> >
> > @@ -7630,7 +7633,7 @@ build_lb_rules_pre_stateful(struct lflow_table
> > *lflows,
> > }
> >
> > ovn_lflow_add_with_dp_group(
> > - lflows, lb_dps->nb_ls_map, ods_size(ls_datapaths),
> > + lflows, lb_dps->nb_ls_map.map, ods_size(ls_datapaths),
> > S_SWITCH_IN_PRE_STATEFUL, 120, ds_cstr(match),
> > ds_cstr(action),
> > &lb->nlb->header_, lb_dps->lflow_ref);
> >
> > @@ -7887,7 +7890,7 @@ build_lb_affinity_ls_flows(struct lflow_table
> > *lflows,
> > const struct ovn_datapaths *ls_datapaths,
> > struct lflow_ref *lflow_ref)
> > {
> > - if (!lb_dps->lb->affinity_timeout || !lb_dps->n_nb_ls) {
> > + if (!lb_dps->lb->affinity_timeout || !lb_dps->nb_ls_map.n_elems) {
> > return;
> > }
> >
> > @@ -7909,7 +7912,7 @@ build_lb_affinity_ls_flows(struct lflow_table
> > *lflows,
> > static char *aff_check = REGBIT_KNOWN_LB_SESSION" = chk_lb_aff();
> > next;";
> >
> > ovn_lflow_add_with_dp_group(
> > - lflows, lb_dps->nb_ls_map, ods_size(ls_datapaths),
> > + lflows, lb_dps->nb_ls_map.map, ods_size(ls_datapaths),
> > S_SWITCH_IN_LB_AFF_CHECK, 100, ds_cstr(&new_lb_match), aff_check,
> > &lb_dps->lb->nlb->header_, lflow_ref);
> > ds_destroy(&new_lb_match);
> > @@ -7991,14 +7994,14 @@ build_lb_affinity_ls_flows(struct lflow_table
> > *lflows,
> >
> > /* Forward to OFTABLE_CHK_LB_AFFINITY table to store flow tuple.
> > */
> > ovn_lflow_add_with_dp_group(
> > - lflows, lb_dps->nb_ls_map, ods_size(ls_datapaths),
> > + lflows, lb_dps->nb_ls_map.map, ods_size(ls_datapaths),
> > S_SWITCH_IN_LB_AFF_LEARN, 100, ds_cstr(&aff_match_learn),
> > ds_cstr(&aff_action_learn), &lb->nlb->header_,
> > lflow_ref);
> >
> > /* Use already selected backend within affinity timeslot. */
> > ovn_lflow_add_with_dp_group(
> > - lflows, lb_dps->nb_ls_map, ods_size(ls_datapaths),
> > + lflows, lb_dps->nb_ls_map.map, ods_size(ls_datapaths),
> > S_SWITCH_IN_LB, 150, ds_cstr(&aff_match),
> > ds_cstr(&aff_action),
> > &lb->nlb->header_, lflow_ref);
> >
> > @@ -8082,10 +8085,10 @@ build_lb_rules(struct lflow_table *lflows, struct
> > ovn_lb_datapaths *lb_dps,
> > if (reject) {
> > size_t index;
> >
> > - dp_non_meter = bitmap_clone(lb_dps->nb_ls_map,
> > + dp_non_meter = bitmap_clone(lb_dps->nb_ls_map.map,
> > ods_size(ls_datapaths));
> > BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > - lb_dps->nb_ls_map) {
> > + lb_dps->nb_ls_map.map) {
> > struct ovn_datapath *od = ls_datapaths->array[index];
> >
> > meter = copp_meter_get(COPP_REJECT, od->nbs->copp,
> > @@ -8104,7 +8107,7 @@ build_lb_rules(struct lflow_table *lflows, struct
> > ovn_lb_datapaths *lb_dps,
> > }
> > if (!reject || build_non_meter) {
> > ovn_lflow_add_with_dp_group(
> > - lflows, dp_non_meter ? dp_non_meter : lb_dps->nb_ls_map,
> > + lflows, dp_non_meter ? dp_non_meter :
> > lb_dps->nb_ls_map.map,
> > ods_size(ls_datapaths), S_SWITCH_IN_LB, priority,
> > ds_cstr(match), ds_cstr(action), &lb->nlb->header_,
> > lb_dps->lflow_ref);
> > @@ -12227,7 +12230,7 @@ build_lrouter_nat_flows_for_lb(
> > size_t index;
> > bool use_stateless_nat = smap_get_bool(&lb->nlb->options,
> > "use_stateless_nat", false);
> > - BITMAP_FOR_EACH_1 (index, bitmap_len, lb_dps->nb_lr_map) {
> > + BITMAP_FOR_EACH_1 (index, bitmap_len, lb_dps->nb_lr_map.map) {
> > struct ovn_datapath *od = lr_datapaths->array[index];
> > enum lrouter_nat_lb_flow_type type;
> >
> > @@ -12312,7 +12315,7 @@ build_lswitch_flows_for_lb(struct ovn_lb_datapaths
> > *lb_dps,
> > const struct svc_monitors_map_data
> > *svc_mons_data,
> > struct ds *match, struct ds *action)
> > {
> > - if (!lb_dps->n_nb_ls) {
> > + if (!lb_dps->nb_ls_map.n_elems) {
> > return;
> > }
> >
> > @@ -12326,7 +12329,8 @@ build_lswitch_flows_for_lb(struct ovn_lb_datapaths
> > *lb_dps,
> > }
> >
> > size_t index;
> > - BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > lb_dps->nb_ls_map) {
> > + BITMAP_FOR_EACH_1 (index, ods_size(ls_datapaths),
> > + lb_dps->nb_ls_map.map) {
> > struct ovn_datapath *od = ls_datapaths->array[index];
> >
> > ovn_lflow_add_with_hint__(lflows, od,
> > @@ -12370,7 +12374,7 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_lb_datapaths *lb_dps,
> > const struct ovn_datapaths
> > *lr_datapaths,
> > struct ds *match)
> > {
> > - if (!lb_dps->n_nb_lr) {
> > + if (!lb_dps->nb_lr_map.n_elems) {
> > return;
> > }
> >
> > @@ -12384,7 +12388,7 @@ build_lrouter_defrag_flows_for_lb(struct
> > ovn_lb_datapaths *lb_dps,
> > lb_vip->vip_str);
> >
> > ovn_lflow_add_with_dp_group(
> > - lflows, lb_dps->nb_lr_map, ods_size(lr_datapaths),
> > + lflows, lb_dps->nb_lr_map.map, ods_size(lr_datapaths),
> > S_ROUTER_IN_DEFRAG, prio, ds_cstr(match), "ct_dnat;",
> > &lb_dps->lb->nlb->header_, lb_dps->lflow_ref);
> > }
> > @@ -12404,7 +12408,7 @@ build_lrouter_allow_vip_traffic_template(struct
> > lflow_table *lflows,
> > struct ds match = DS_EMPTY_INITIALIZER;
> >
> > size_t index;
> > - BITMAP_FOR_EACH_1 (index, ods_size(lr_dps), lb_dps->nb_lr_map) {
> > + BITMAP_FOR_EACH_1 (index, ods_size(lr_dps), lb_dps->nb_lr_map.map) {
> > struct ovn_datapath *od = lr_dps->array[index];
> > /* Do not drop ip traffic with destination the template VIP. */
> > ds_clear(&match);
> > @@ -12430,7 +12434,7 @@ build_lrouter_flows_for_lb(struct ovn_lb_datapaths
> > *lb_dps,
> > {
> > size_t index;
> >
> > - if (!lb_dps->n_nb_lr) {
> > + if (!lb_dps->nb_lr_map.n_elems) {
> > return;
> > }
> >
> > @@ -12450,7 +12454,8 @@ build_lrouter_flows_for_lb(struct ovn_lb_datapaths
> > *lb_dps,
> > continue;
> > }
> >
> > - BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> > lb_dps->nb_lr_map) {
> > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> > + lb_dps->nb_lr_map.map) {
> > struct ovn_datapath *od = lr_datapaths->array[index];
> >
> > ovn_lflow_add_with_hint__(lflows, od, S_ROUTER_IN_DNAT,
> > @@ -12464,7 +12469,8 @@ build_lrouter_flows_for_lb(struct ovn_lb_datapaths
> > *lb_dps,
> > }
> >
> > if (lb->skip_snat) {
> > - BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> > lb_dps->nb_lr_map) {
> > + BITMAP_FOR_EACH_1 (index, ods_size(lr_datapaths),
> > + lb_dps->nb_lr_map.map) {
> > struct ovn_datapath *od = lr_datapaths->array[index];
> >
> > ovn_lflow_add(lflows, od, S_ROUTER_OUT_SNAT, 120,
> > --
> > 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
