On Thu, Jan 8, 2026 at 9:48 PM Mark Michelson via dev < [email protected]> wrote:
> The lflow_table had separate ls and lr datapath groups as individual > pointers. If we expand to allow new types of datapaths, then this is > overly restrictive. Using an array allows us to automatically expand as > new datapath types are added. > > Signed-off-by: Mark Michelson <[email protected]> > --- > Hi Mark, thank you for the v2, I have a couple of small comments down below. > northd/lflow-mgr.c | 39 ++++++++++++++++++++++----------------- > northd/lflow-mgr.h | 3 +-- > northd/northd.c | 3 +-- > 3 files changed, 24 insertions(+), 21 deletions(-) > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index 4e12c646c..36f903ce3 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -207,8 +207,9 @@ lflow_table_init(struct lflow_table *lflow_table) > { > fast_hmap_size_for(&lflow_table->entries, > lflow_table->max_seen_lflow_size); > - ovn_dp_groups_init(&lflow_table->ls_dp_groups); > - ovn_dp_groups_init(&lflow_table->lr_dp_groups); > + for (enum ovn_datapath_type i = DP_MIN; i < DP_MAX; i++) { > + ovn_dp_groups_init(&lflow_table->dp_groups[i]); > + } > } > > void > @@ -223,8 +224,9 @@ lflow_table_clear(struct lflow_table *lflow_table, > bool destroy_all) > } > } > > - ovn_dp_groups_clear(&lflow_table->ls_dp_groups); > - ovn_dp_groups_clear(&lflow_table->lr_dp_groups); > + for (enum ovn_datapath_type i = DP_MIN; i < DP_MAX; i++) { > + ovn_dp_groups_clear(&lflow_table->dp_groups[i]); > + } > } > > void > @@ -232,8 +234,9 @@ lflow_table_destroy(struct lflow_table *lflow_table) > { > lflow_table_clear(lflow_table, true); > hmap_destroy(&lflow_table->entries); > - ovn_dp_groups_destroy(&lflow_table->ls_dp_groups); > - ovn_dp_groups_destroy(&lflow_table->lr_dp_groups); > + for (enum ovn_datapath_type i = DP_MIN; i < DP_MAX; i++) { > + ovn_dp_groups_destroy(&lflow_table->dp_groups[i]); > + } > free(lflow_table); > } > > @@ -288,12 +291,13 @@ lflow_table_sync_to_sb(struct lflow_table > *lflow_table, > } > const struct ovn_datapaths *datapaths; > struct hmap *dp_groups; > - if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { > + enum ovn_datapath_type dp_type = > + ovn_stage_to_datapath_type(lflow->stage); > + dp_groups = &lflow_table->dp_groups[dp_type]; > + if (dp_type == DP_SWITCH) { > We could reduce the if to ternary now that there is only single variable being populated. > datapaths = ls_datapaths; > - dp_groups = &lflow_table->ls_dp_groups; > } else { > datapaths = lr_datapaths; > - dp_groups = &lflow_table->lr_dp_groups; > } > sync_lflow_to_sb(lflow, ovnsb_txn, dp_groups, datapaths, > ovn_internal_version_changed, > @@ -366,12 +370,11 @@ lflow_table_sync_to_sb(struct lflow_table > *lflow_table, > if (lflow) { > const struct ovn_datapaths *datapaths; > struct hmap *dp_groups; > + dp_groups = &lflow_table->dp_groups[dp_type]; > if (dp_type == DP_SWITCH) { > Same here. > datapaths = ls_datapaths; > - dp_groups = &lflow_table->ls_dp_groups; > } else { > datapaths = lr_datapaths; > - dp_groups = &lflow_table->lr_dp_groups; > } > sync_lflow_to_sb(lflow, ovnsb_txn, dp_groups, datapaths, > ovn_internal_version_changed, > @@ -391,12 +394,13 @@ lflow_table_sync_to_sb(struct lflow_table > *lflow_table, > } > const struct ovn_datapaths *datapaths; > struct hmap *dp_groups; > - if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { > + enum ovn_datapath_type dp_type = > + ovn_stage_to_datapath_type(lflow->stage); > + dp_groups = &lflow_table->dp_groups[dp_type]; > + if (dp_type == DP_SWITCH) { > Same here. > datapaths = ls_datapaths; > - dp_groups = &lflow_table->ls_dp_groups; > } else { > datapaths = lr_datapaths; > - dp_groups = &lflow_table->lr_dp_groups; > } > sync_lflow_to_sb(lflow, ovnsb_txn, dp_groups, datapaths, > ovn_internal_version_changed, NULL, dpgrp_table); > @@ -1358,11 +1362,12 @@ lflow_ref_sync_lflows__(struct lflow_ref > *lflow_ref, > > struct hmap *dp_groups = NULL; > const struct ovn_datapaths *datapaths; > - if (ovn_stage_to_datapath_type(lflow->stage) == DP_SWITCH) { > - dp_groups = &lflow_table->ls_dp_groups; > + enum ovn_datapath_type dp_type = > + ovn_stage_to_datapath_type(lflow->stage); > + dp_groups = &lflow_table->dp_groups[dp_type]; > + if (dp_type == DP_SWITCH) { > Same here. > datapaths = ls_datapaths; > } else { > - dp_groups = &lflow_table->lr_dp_groups; > datapaths = lr_datapaths; > } > > diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h > index d4d98390b..cbd9f9e03 100644 > --- a/northd/lflow-mgr.h > +++ b/northd/lflow-mgr.h > @@ -28,8 +28,7 @@ struct ovsdb_idl_row; > /* lflow map which stores the logical flows. */ > struct lflow_table { > struct hmap entries; /* hmap of lflows. */ > - struct hmap ls_dp_groups; /* hmap of logical switch dp groups. */ > - struct hmap lr_dp_groups; /* hmap of logical router dp groups. */ > + struct hmap dp_groups[DP_MAX]; > ssize_t max_seen_lflow_size; > }; > > diff --git a/northd/northd.c b/northd/northd.c > index a9f1686dd..1e21b78a5 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -19850,8 +19850,7 @@ lflow_handle_ls_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > if (!lflow_ref_resync_flows( > ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, > - lflow_input->ls_datapaths, > - lflow_input->lr_datapaths, > + lflow_input->ls_datapaths, lflow_input->lr_datapaths, > nit: Unrelated change. > lflow_input->ovn_internal_version_changed, > lflow_input->sbrec_logical_flow_table, > lflow_input->sbrec_logical_dp_group_table)) { > -- > 2.51.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > With that addressed: Acked-by: Ales Musil <[email protected]> Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
