Hi Lucas, Jacob,
On 9/16/26 10:17 PM, Jacob Tanenbaum via dev wrote:
> Thanks for the patch Lucas! Sorry it took so long to review this. This
> looks good to me.
>
Lucas, thanks for the fix and Jacob, thanks for the review!
> I am wondering if replacing the refcount with an actual counter would help
> us in the future?
>
I'm sorry, I must be missing something, I don't get it. A reference
counter is a counter, what's an "actual counter"?
> 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.
>
Do you mean this?
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index ce9c4f8543..ac4ef72d0e 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -1462,10 +1462,12 @@ static bool
dp_refcnt_release(struct hmap *dp_refcnts_map, size_t dp_index)
{
struct dp_refcnt *dp_refcnt = dp_refcnt_find(dp_refcnts_map, dp_index);
+ ovs_assert(dp_refcnt);
if (!dp_refcnt) {
return true;
}
+ ovs_assert(dp_refcnt->refcnt > 0);
if (!--dp_refcnt->refcnt) {
hmap_remove(dp_refcnts_map, &dp_refcnt->key_node);
free(dp_refcnt);
> Acked-by: Jacob Tanenbaum <[email protected]>
>
I'll be applying this patch to main and 26.09 soon.
Thanks again!
Regards,
Dumitru
> 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
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
