On Mon, Feb 16, 2026 at 5:52 PM Mark Michelson <[email protected]> wrote:
> Hi Jacob, > > I've had a look at the change, and I think mechanically, it gets the > job done. The big thing this patch changes is that we dereference the > dp group before attempting to allocate a new one. I think there are a > few aspects of the patch that could be improved though. See below for > my inline comments. > > On Mon, Feb 9, 2026 at 11:23 AM Jacob Tanenbaum <[email protected]> > wrote: > > > > If ovn_dp_groups ref_count is decremented when fetching old lflows > > during a recalculation we can successfully determine if we can reuse the > > flow or not. > > > > Reported-by: Mark Michelson <[email protected]> > > Reported-at: https://issues.redhat.com/browse/FDP-2747 > > Signed-off-by: Jacob Tanenbaum <[email protected]> > > > > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > > index f21024903..19c939cc1 100644 > > --- a/northd/lflow-mgr.c > > +++ b/northd/lflow-mgr.c > > @@ -1053,6 +1053,14 @@ do_ovn_lflow_add(struct lflow_table *lflow_table, > size_t dp_bitmap_len, > > if (old_lflow) { > > dynamic_bitmap_realloc(&old_lflow->dpg_bitmap, dp_bitmap_len); > > if (old_lflow->sync_state != LFLOW_STALE) { > > + > > + struct hmap *dp_groups; > > + enum ovn_datapath_type dp_type = > > + ovn_stage_to_datapath_type(old_lflow->stage); > > + dp_groups = &lflow_table->dp_groups[dp_type]; > > + ovn_dp_group_release(dp_groups, old_lflow->dpg); > > + old_lflow->dpg = NULL; > > The above only needs to be done if old_lflow->dpg is non-NULL. It may > be that old_lflow only applied to a single datapath, in which case > old_lflow->dp would be non-NULL and old_lflow->dpg would be NULL. > good spot, I will address this in version 2 > > > + > > return old_lflow; > > } > > sbuuid = old_lflow->sb_uuid; > > @@ -1242,7 +1250,6 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > > > > if (pre_sync_dpg != lflow->dpg) { > > ovn_dp_group_use(lflow->dpg); > > - ovn_dp_group_release(dp_groups, pre_sync_dpg); > > } > > It's hard to comment on this particular change in-line, but I'll do my > best :) > > With the way the code used to work, the lflow->dpg may be one value at > the beginning of the sync and then may change to a different value > after syncing. This if statement ensured that if the lflow->dpg > changed as a result of syncing, then we'd release the old dpg and use > the new dpg. > > With the change you made in do_ovn_lflow_add(), now lflow->dpg will > always be NULL when entering sync_lflow_to_sb(). Therefore, > pre_sync_dpg will always be NULL. Since it's always going to be NULL, > there's not much point in even having pre_sync_dpg exist any longer. > Instead, it may be a good idea at the top of the function to add: > > ovn_assert(lflow->dpg == NULL); > > We can then also move the ovn_dp_group_use(lflow->dpg) call to just > after lflow->dpg is assigned. It's not in this diff, but above this > section, there is a call to sbrec_logical_flow_set_logical_dp_group(). > You can add the ovn_dp_group_use(lflow->dpg) call just above that > line. That way it's more clearly tied to its assignment. > > > > > lflow->sync_state = LFLOW_SYNCED; > This looked right but on further inspection pre_sync_dpg can be set if we process the same flow more then once in the same transaction like in the event of the command `ovn-nbctl --wait=sb lr-add lr0 -- lr-add lr1` which will generate a number of the same flows. If we do not account for that the number of references for the flow will end up being wrong. > > -- > > 2.52.0 > > > > Thanks for the review Mark! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
