On Thu, Sep 17, 2026 at 3:20 AM Dumitru Ceara <[email protected]> wrote:
> 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"?
>
What I mean is the first reference to the flow does not create the refcount
so it only starts counting when refcount = 2 but once the refcount is
created it isn't cleaned up until it reaches zero. This leads to the
observation below.
>
> > 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?
>
Yes, this patch didn't introduce this refcount and it maintains how it is
being used correctly. I was just wondering if putting in the work to change
the refcount would clear up this potential source of confusion in the
future.
>
> 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
