Hi Ales, thanks for the reviews. I made all changes suggested in your reviews, except for the ones noted below. I'll post v3 as soon as I can.
On Fri, Jan 9, 2026 at 5:27 AM Ales Musil <[email protected]> wrote: > > > > 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. I chose not to make this or the other ternary changes since these if statements are removed entirely in patch 9. If the if statements were not slated for removal in a later patch, I would make the ternary change as you suggested. > >> >> 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
