On Fri, Jan 9, 2026 at 5:39 PM Mark Michelson <[email protected]> wrote:
> 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. > Ah fair enough I didn't notice that, but yeah then it would be pointless. > > > > >> > >> 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
