Thanks for the patch Lucas! Sorry it took so long to review this. This looks good to me.
I am wondering if replacing the refcount with an actual counter would help us in the future? Currently, if no refcnt entry is created, it means there is at most one live reference. If a refcount entry exists, it holds the total live references, which are removed only at zero. Returning true from dp_refcount_release() when !dp_refcount does not allow us to distinguish from two different situations 1. it is the flows only reference (correct) 2. the entry should exist but is missing (bug) This is just a thought and should not block this patch. Acked-by: Jacob Tanenbaum <[email protected]> On Thu, Aug 20, 2026 at 3:24 PM Lucas Vargas Dias <[email protected]> wrote: > 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. Such an > lflow is synced with a different dp group (or with 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 group in lflow_ref_unlink_lflows(), where the bitmap actually > shrinks. Like the release done by do_ovn_lflow_add(), this happens before > any lflow of the round is synced, which is what allows > ovn_dp_group_create() to modify the SB row of a group that just became > free instead of inserting a new row and leaving the old one to be deleted. > Releasing at sync time would fix the leak but not that churn: a group is > usually referenced by several lflows, so it is still in 'dp_groups' when > the first of them is synced. > > Fixes: 0e2bcf70ac4f ("northd: Change ovn_dp_groups to decrement refcount > in do_ovn_lflow_add.") > Assisted-by: Claude Opus 5, Claude Code > Signed-off-by: Lucas Vargas Dias <[email protected]> > --- > northd/en-lflow.c | 6 ++- > northd/lflow-mgr.c | 39 ++++++++++++++++--- > northd/lflow-mgr.h | 2 +- > northd/northd.c | 16 ++++---- > tests/ovn-northd.at | 94 +++++++++++++++++++++++++++++++++++++++++++++ > 5 files changed, 140 insertions(+), 17 deletions(-) > > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 8cb987777..e8e6cdaa2 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -294,7 +294,8 @@ lflow_group_ecmp_route_change_handler(struct > engine_node *node, > HMAPX_FOR_EACH (hmapx_node, > > &group_ecmp_route_data->trk_data.deleted_datapath_routes) { > route_node = hmapx_node->data; > - lflow_ref_unlink_lflows(route_node->lflow_ref); > + lflow_ref_unlink_lflows(route_node->lflow_ref, > + lflow_data->lflow_table); > > bool handled = lflow_ref_sync_lflows( > route_node->lflow_ref, lflow_data->lflow_table, > @@ -312,7 +313,8 @@ lflow_group_ecmp_route_change_handler(struct > engine_node *node, > &group_ecmp_route_data->trk_data.crupdated_datapath_routes; > HMAPX_FOR_EACH (hmapx_node, crupdated_datapath_routes) { > route_node = hmapx_node->data; > - lflow_ref_unlink_lflows(route_node->lflow_ref); > + lflow_ref_unlink_lflows(route_node->lflow_ref, > + lflow_data->lflow_table); > build_route_data_flows_for_lrouter( > route_node->od, lflow_data->lflow_table, > route_node, lflow_input.bfd_ports); > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index ce9c4f854..f51df9ce0 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -657,26 +657,53 @@ lflow_ref_destroy(struct lflow_ref *lflow_ref) > * from the lrn->lflow's dpg bitmap > */ > void > -lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref) > +lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref, > + struct lflow_table *lflow_table) > { > struct lflow_ref_node *lrn; > > HMAP_FOR_EACH (lrn, ref_node, &lflow_ref->lflow_ref_nodes) { > + struct ovn_lflow *lflow = lrn->lflow; > + bool dpg_bitmap_changed = false; > + > if (lrn->dpgrp_lflow) { > size_t index; > BITMAP_FOR_EACH_1 (index, lrn->dpgrp_bitmap_len, > lrn->dpgrp_bitmap) { > - if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, > index)) { > - dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, index); > + if (dp_refcnt_release(&lflow->dp_refcnts_map, index)) { > + dynamic_bitmap_set0(&lflow->dpg_bitmap, index); > + dpg_bitmap_changed = true; > } > } > } else { > - if (dp_refcnt_release(&lrn->lflow->dp_refcnts_map, > + if (dp_refcnt_release(&lflow->dp_refcnts_map, > lrn->dp_index)) { > - dynamic_bitmap_set0(&lrn->lflow->dpg_bitmap, > lrn->dp_index); > + dynamic_bitmap_set0(&lflow->dpg_bitmap, lrn->dp_index); > + dpg_bitmap_changed = true; > } > } > > + /* The lflow doesn't apply to the same set of datapaths anymore, > so > + * the dp group it is referencing is not the one it will be synced > + * with. Drop the reference now, before any lflow of this round > is > + * synced: as long as the group is still referenced it stays in > + * 'dp_groups' and ovn_dp_group_create() refuses to modify its SB > + * row, inserting a new Logical_DP_Group row and deleting the one > + * that just became free instead of reusing it. > + * > + * This is the counterpart of the release done by > + * do_ovn_lflow_add(), which covers the lflows that are generated > + * again. An lflow that is only unlinked from one of its > lflow_refs > + * and survives because other lflow_refs still reference it never > + * goes through do_ovn_lflow_add(). */ > + if (dpg_bitmap_changed && lflow->dpg) { > + enum ovn_datapath_type dp_type = > + ovn_stage_to_datapath_type(lflow->stage); > + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], > + lflow->dpg); > + lflow->dpg = NULL; > + } > + > lrn->linked = false; > } > } > @@ -690,7 +717,7 @@ lflow_ref_resync_flows(struct lflow_ref *lflow_ref, > const struct sbrec_logical_flow_table > *sbflow_table, > const struct sbrec_logical_dp_group_table > *dpgrp_table) > { > - lflow_ref_unlink_lflows(lflow_ref); > + lflow_ref_unlink_lflows(lflow_ref, lflow_table); > return lflow_ref_sync_lflows__(lflow_ref, lflow_table, ovnsb_txn, > dps, > ovn_internal_version_changed, > sbflow_table, > diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h > index 84d0b3e67..6253be685 100644 > --- a/northd/lflow-mgr.h > +++ b/northd/lflow-mgr.h > @@ -57,7 +57,7 @@ struct lflow_ref; > struct lflow_ref *lflow_ref_create(void); > void lflow_ref_destroy(struct lflow_ref *); > void lflow_ref_clear(struct lflow_ref *lflow_ref); > -void lflow_ref_unlink_lflows(struct lflow_ref *); > +void lflow_ref_unlink_lflows(struct lflow_ref *, struct lflow_table *); > bool lflow_ref_resync_flows(struct lflow_ref *, > struct lflow_table *lflow_table, > struct ovsdb_idl_txn *ovnsb_txn, > diff --git a/northd/northd.c b/northd/northd.c > index 9f33b50cf..7753de970 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -20997,7 +20997,7 @@ lflow_handle_northd_lr_changes(struct > ovsdb_idl_txn *ovnsb_txn, > HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { > struct ovn_datapath *od = hmapx_node->data; > > - lflow_ref_unlink_lflows(od->datapath_lflows); > + lflow_ref_unlink_lflows(od->datapath_lflows, lflows); > build_lswitch_and_lrouter_iterate_by_lr(od, &lsi); > } > > @@ -21052,7 +21052,7 @@ lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > /* Make sure 'op' is an lsp and not lrp. */ > ovs_assert(op->nbsp); > /* Clear old lflows. */ > - lflow_ref_unlink_lflows(op->lflow_ref); > + lflow_ref_unlink_lflows(op->lflow_ref, lflows); > > /* Generate new lflows. */ > struct ds match = DS_EMPTY_INITIALIZER; > @@ -21072,7 +21072,7 @@ lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > if (handled) { > /* Now regenerate the stateful lflows for 'op' */ > /* Clear old lflows. */ > - lflow_ref_unlink_lflows(op->stateful_lflow_ref); > + lflow_ref_unlink_lflows(op->stateful_lflow_ref, lflows); > build_lbnat_lflows_iterate_by_lsp(op, > > lflow_input->lr_stateful_table, > &match, &actions, lflows); > @@ -21164,7 +21164,7 @@ lflow_handle_northd_lb_changes(struct > ovsdb_idl_txn *ovnsb_txn, > lb_dps = hmapx_node->data; > > /* unlink old lflows. */ > - lflow_ref_unlink_lflows(lb_dps->lflow_ref); > + lflow_ref_unlink_lflows(lb_dps->lflow_ref, lflows); > > /* Generate new lflows. */ > struct ds match = DS_EMPTY_INITIALIZER; > @@ -21220,7 +21220,7 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { > lr_stateful_rec = hmapx_node->data; > /* Unlink old lflows. */ > - lflow_ref_unlink_lflows(lr_stateful_rec->lflow_ref); > + lflow_ref_unlink_lflows(lr_stateful_rec->lflow_ref, lflows); > > /* Generate new lflows. */ > build_lr_stateful_flows(lr_stateful_rec, > lflow_input->lr_datapaths, > @@ -21245,7 +21245,7 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > lr_stateful_rec->lr_index); > struct ovn_port *op; > HMAP_FOR_EACH (op, dp_node, &od->ports) { > - lflow_ref_unlink_lflows(op->stateful_lflow_ref); > + lflow_ref_unlink_lflows(op->stateful_lflow_ref, lflows); > > build_lbnat_lflows_iterate_by_lrp(op, > > lflow_input->lr_stateful_table, > @@ -21265,7 +21265,7 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > } > > if (op->peer && op->peer->nbsp) { > - lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref); > + lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref, > lflows); > > build_lbnat_lflows_iterate_by_lsp( > op->peer, lflow_input->lr_stateful_table, &match, > &actions, > @@ -21307,7 +21307,7 @@ lflow_handle_ls_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, > &ls_stateful_rec->nbs_uuid)); > > - lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); > + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref, lflows); > > /* Generate new lflows. */ > build_ls_stateful_flows(ls_stateful_rec, od, > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index 8c8d7852e..aa281c1a2 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -23647,3 +23647,97 @@ 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 > +]) > + > +OVN_FOR_EACH_NORTHD_NO_HV([ > +AT_SETUP([Datapath group SB row reuse when an lflow loses a datapath]) > +ovn_start > + > +# ls1, ls2 and ls3 have tier 0 ACLs, ls4 has a tier 1 ACL. The flows that > +# only depend on "the switch has ACLs" are shared by the four of them, and > +# the egress "acl action" flows of tier 0 are shared by ls1, ls2 and ls3. > +# ls4 is the only user of the tier 1 flows, so those are synced with a > +# single datapath. Hence two datapath groups: {ls1, ls2, ls3} 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 acl-add ls3 to-lport 1000 tcp allow > +check ovn-nbctl --wait=sb --tier=1 acl-add ls4 to-lport 1000 udp allow > + > +acl3=$(fetch_column nb:ACL _uuid match=tcp) > +check_row_count Logical_DP_Group 2 > +ovn-sbctl --bare --columns _uuid list Logical_DP_Group | sort > dpg_uuids > + > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > + > +# Move ls3's ACL to tier 1. The tier 0 "acl action" flows are left with > +# {ls1, ls2}: they are not generated again (ls1 and ls2 didn't change), > they > +# are only unlinked from ls3's lflow_ref. The {ls1, ls2, ls3} group > becomes > +# unused, so its SB row is free and northd is expected to modify it in > place > +# instead of inserting a new row and deleting this one. > +check ovn-nbctl --wait=sb set ACL $acl3 tier=1 > +check_engine_stats lflow norecompute compute > +check_row_count Logical_DP_Group 3 > + > +ovn-sbctl --bare --columns _uuid list Logical_DP_Group | sort > dpg_uuids2 > +AT_CHECK([comm -23 dpg_uuids dpg_uuids2], [0], []) > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +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'.* > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
