Hi Mark, This patch was superseded by [0].
[0] https://patchwork.ozlabs.org/project/ovn/patch/[email protected]/ Regards, Lucas Em qui., 20 de ago. de 2026 às 10:16, Lucas Vargas Dias <[email protected]> escreveu: > Hi Mark, > Thanks. > > This version just re-add ovn_dp_group_release in sync_lflow_to_sb. I'll > work in a new version to separate > the logical datapath group syncing from lflow syncing. > > Regards, > Lucas > > Em qua., 19 de ago. de 2026 às 13:29, Mark Michelson <[email protected]> > escreveu: > >> On Tue, Aug 18, 2026 at 2:59 PM Lucas Vargas Dias >> <[email protected]> wrote: >> > >> > Hi, >> > >> > I would like to know your option about this patch, since you >> created/acked patch [0]. >> > Or if is there another way to resolve the Warn log message? >> > >> > [0] >> https://github.com/ovn-org/ovn/commit/0e2bcf70ac4f769704c62de715b4905720d8ada3 >> > >> > Regards, >> > Lucas >> >> Hi Lucas. >> >> First, I haven't had an opportunity to look at your patch. But I >> figured I could give a bit of background on the commit you linked. >> >> The commit you linked was to solve >> https://redhat.atlassian.net/browse/FDP-2747 . The addition of logical >> router incremental processing introduced an issue in the southbound >> database. Originally, we would avoid deleting and re-inserting logical >> datapath groups by reusing existing ones when possible. But the >> logical router incremental processing exposed a flaw in the logical >> flow syncing code. With that addition, we were now deleting and >> re-inserting logical datapath groups in the SB DB, causing a lot of >> extra unnecessary work. The issue suggested that the best way to >> approach this problem is to separate the logical datapath group >> syncing into a separate engine node that runs after logical flows have >> been synchronized. This way, the datapath group syncing node would >> have the full picture of what logical flows exist and what datapaths >> they apply to. >> >> This is not trivial work. This item was assigned to Jacob, and he >> figured out that by changing the way we were handling the dp group >> refcount, we could avoid deleting a SB logical datapath group too >> early. Since this appeared to solve the issue of the unnecessary >> datapath group deletion/re-insertion, this seemed like a good stop-gap >> until we could do the proper thing of actually separating the dp group >> syncing from logical flow syncing. While this solved the issue of the >> unnecessary churn, it appears to have caused a different problem, >> which results in unnecessary recomputes in en-lflow. >> >> I'll need to take a closer look at your patch and determine if this >> manages to fix the recompute issue you mentioned while also allowing >> for reuse of SB logical datapath groups. >> >> In the long term, we need to do what the linked issue says: separate >> the logical datapath group syncing from lflow syncing. It has been >> difficult to find the time to implement it though. >> >> > >> > Em ter., 18 de ago. de 2026 às 15:52, Lucas Vargas Dias >> <[email protected]> escreveu: >> >> >> >> Commit 0e2bcf70ac4f moved the release of the dp group referenced by an >> >> lflow from sync_lflow_to_sb() to do_ovn_lflow_add(), so that the group >> >> is already free when a new one is looked up and the SB row can be >> >> reused. However, do_ovn_lflow_add() is only called when the lflow is >> >> generated again. An lflow that is merely unlinked from one of its >> >> lflow_refs, and that survives because other lflow_refs still reference >> >> it, never goes through do_ovn_lflow_add(): only its dp group bitmap >> >> shrinks. When such an lflow is synced, it points to a different dp >> >> group (or to a single datapath), and the reference to the previous >> >> group is never dropped. >> >> >> >> The leaked group stays in the 'dp_groups' map with a non zero refcount >> >> while no lflow uses it anymore, so its SB Logical_DP_Group row is >> >> garbage collected. Any lflow that later needs that same set of >> >> datapaths finds the leaked group, fails to look its row up and northd >> >> logs: >> >> >> >> SB Logical flow [...]'s logical_dp_group column is not set (which is >> >> unexpected). It should have been referencing the dp group [...] >> >> >> >> and falls back to a full recompute. >> >> >> >> Release the previous dp group after the new one has been taken. The >> >> release done by do_ovn_lflow_add() clears 'lflow->dpg', so lflows that >> >> were regenerated are not released twice. >> >> >> >> Fixes: 0e2bcf70ac4f ("northd: Change ovn_dp_groups to decrement >> refcount in do_ovn_lflow_add.") >> >> Signed-off-by: Lucas Vargas Dias <[email protected]> >> >> --- >> >> northd/lflow-mgr.c | 7 +++++++ >> >> tests/ovn-northd.at | 51 >> +++++++++++++++++++++++++++++++++++++++++++++ >> >> 2 files changed, 58 insertions(+) >> >> >> >> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c >> >> index ce9c4f854..2367156ad 100644 >> >> --- a/northd/lflow-mgr.c >> >> +++ b/northd/lflow-mgr.c >> >> @@ -1264,6 +1264,13 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, >> >> >> >> if (pre_sync_dpg != lflow->dpg) { >> >> ovn_dp_group_use(lflow->dpg); >> >> + /* The lflow's dp group bitmap may have changed without the >> lflow >> >> + * being re-added (e.g. when it was only unlinked from one of >> its >> >> + * lflow_refs), in which case do_ovn_lflow_add() didn't get >> the >> >> + * chance to drop the reference to the previous dp group. >> Drop it >> >> + * here, otherwise the dp group is leaked in 'dp_groups' with >> a >> >> + * dangling reference to an SB row that gets garbage >> collected. */ >> >> + ovn_dp_group_release(dp_groups, pre_sync_dpg); >> >> } >> >> >> >> lflow->sync_state = LFLOW_SYNCED; >> >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> >> index 8c8d7852e..19887e6d7 100644 >> >> --- a/tests/ovn-northd.at >> >> +++ b/tests/ovn-northd.at >> >> @@ -23647,3 +23647,54 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd >> inc-engine/enable-stopwatch nonexis >> >> OVN_CLEANUP_NORTHD >> >> AT_CLEANUP >> >> ]) >> >> + >> >> +OVN_FOR_EACH_NORTHD_NO_HV([ >> >> +AT_SETUP([Datapath group reuse after an lflow loses a datapath]) >> >> +ovn_start >> >> + >> >> +# All four switches have ACLs, so the logical flows that only depend >> on >> >> +# "the switch has ACLs" are shared by the four of them. ls1 and ls2 >> use >> >> +# tier 0 while ls3 and ls4 use tier 1, which changes the actions of >> the >> >> +# egress "acl action" flows. Hence three datapath groups are >> expected: >> >> +# {ls1, ls2}, {ls3, ls4} and {ls1, ls2, ls3, ls4}. >> >> +check ovn-nbctl ls-add ls1 >> >> +check ovn-nbctl ls-add ls2 >> >> +check ovn-nbctl ls-add ls3 >> >> +check ovn-nbctl ls-add ls4 >> >> +check ovn-nbctl acl-add ls1 to-lport 1000 ip4 allow >> >> +check ovn-nbctl acl-add ls2 to-lport 1000 ip6 allow >> >> +check ovn-nbctl --tier=1 acl-add ls3 to-lport 1000 tcp allow >> >> +check ovn-nbctl --wait=sb --tier=1 acl-add ls4 to-lport 1000 udp allow >> >> + >> >> +acl1=$(fetch_column nb:ACL _uuid match=ip4) >> >> +check_row_count Logical_DP_Group 3 >> >> + >> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> + >> >> +# Move ls1's ACL to tier 1. ls1 stops generating the egress "acl >> action" >> >> +# flows it shared with ls2, so those flows are left with a single >> datapath >> >> +# and the {ls1, ls2} datapath group becomes unused and is deleted. >> northd >> >> +# has to drop the reference it holds to that group, otherwise the >> group is >> >> +# leaked in the in-memory dp group table, still pointing to the SB >> row that >> >> +# has just been deleted. >> >> +check ovn-nbctl --wait=sb set ACL $acl1 tier=1 >> >> +check_engine_stats lflow norecompute compute >> >> +check_row_count Logical_DP_Group 2 >> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> + >> >> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >> >> + >> >> +# Move ls1's ACL back to tier 0. The {ls1, ls2} datapath group is >> needed >> >> +# again: a leaked group would be picked up here and northd would >> complain >> >> +# about its dangling SB reference. >> >> +check ovn-nbctl --wait=sb set ACL $acl1 tier=0 >> >> +check_engine_stats lflow norecompute compute >> >> +check_row_count Logical_DP_Group 3 >> >> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >> >> + >> >> +AT_CHECK([grep -q "logical_dp_group column is not set" \ >> >> + northd/ovn-northd.log], [1]) >> >> + >> >> +OVN_CLEANUP_NORTHD >> >> +AT_CLEANUP >> >> +]) >> >> -- >> >> 2.43.0 >> >> >> > >> > >> > ‘Esta mensagem é direcionada apenas para os endereços constantes no >> cabeçalho inicial. Se você não está listado nos endereços constantes no >> cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa >> mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão >> imediatamente anuladas e proibidas’. >> > >> > ‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para >> assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não >> poderá aceitar a responsabilidade por quaisquer perdas ou danos causados >> por esse e-mail ou por seus anexos’. >> >> -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
