On Fri, Oct 9, 2026 at 3:56 PM Jacob Tanenbaum via dev <
[email protected]> wrote:
> Remove the needs_full_sync fallback for IGMP/MLD and IC-learned service
> monitor flows. lflow_ref_unlink_and_prune() saves the uuids of orphaned
> lflows so that orphaned flows can be deleted without a resync being
> required.
>
> The IGMP/MLD and IC-learned service monitor handlers set
> trk_data.needs_full_sync, which forced dp_group to fall back to a full
> lflow_table_sync_to_sb() instead of the incremental
> lflow_ref_sync_lflows() pass.
>
> The full sync, unlike the incremental sync, does not destroy
> lflow_ref_nodes that are no longer linked on lflows still referenced
> by another lflow_ref. A retained, already-unlinked node can then be
> unlinked again by a later lflow_ref_unlink_lflows() call, which
> releases its datapath refcnt a second time. The count can drop below
> the number of lflow_refs that still use the lflow, emptying its
> datapath bitmap so that a later sync deletes an SB row that other
> lflow_refs still require (e.g. a load balancer VIP flow shared by two
> load balancers on the same datapath).
>
> Drop the fallback and keep these flows incremental.
> lflow_ref_unlink_and_prune() already destroys orphaned lflows in
> memory; the incremental sync cannot see them because the lflow and
> its lflow_ref_node are gone. So record the SB uuid of every
> orphaned lflow that already has an SB row in
> trk_data.orphaned_sb_uuids and have dp_group_lflow_handler() delete
> those rows after the incremental sync pass. This solves a performance
> regression in which ordinary logical switch port changes caused the
> whole logical flow table to recalculate.
>
> This is safe even when an orphaned lflow is re-added by a later build
> (e.g. the per-datapath multicast flood flow): the re-added lflow is a
> fresh in-memory object with a new random sb_uuid, so its freshly
> inserted SB row is never one of the recorded uuids.
>
> Add a northd regression test for the case where two load balancers
> on the same switch share a VIP. Removing the shared VIP from one
> load balancer while a datapath group full sync is in progress,
> followed by a further change to that load balancer, previously
> double-released the shared lflow's datapath refcnt. A later full
> sync then pruned the lflow and deleted its SB row even though the
> second load balancer still required the VIP.
>
> The test keeps the two changes that must land in a single engine run
> (remove the VIP from the first LB and change the port address)
> batched into one transaction, so that the multicast node's
> recompute forces the datapath group onto the full sync path while
> the lflow node stays incremental. It asserts that the shared VIP
> flow is still present after the sequence of changes.
>
>
nit: Missing Reported-at.
> Assisted-by: Qwen3.8-27B-FP8, opencode
> Fixes: b9809453f546 ("northd: Split en_lflow into compute and sync nodes.")
> Signed-off-by: Jacob Tanenbaum <[email protected]>
> ---
> northd/en-dp-group.c | 13 +++++++---
> northd/en-lflow.c | 17 +++++++-----
> northd/en-lflow.h | 7 ++++-
> northd/lflow-mgr.c | 41 +++++++++++++++++++++++++++--
> northd/lflow-mgr.h | 6 ++++-
> tests/ovn-northd.at | 61 ++++++++++++++++++++++++++++++++++++++++++++
> 6 files changed, 132 insertions(+), 13 deletions(-)
>
> diff --git a/northd/en-dp-group.c b/northd/en-dp-group.c
> index e33e1f407..3aeed361b 100644
> --- a/northd/en-dp-group.c
> +++ b/northd/en-dp-group.c
> @@ -89,9 +89,7 @@ dp_group_lflow_handler(struct engine_node *node,
> struct ed_type_global_config *global_config =
> engine_get_input_data("global_config", node);
>
> - if (hmapx_is_empty(&lflow_data->trk_data.dirty_lflow_refs) ||
> - lflow_data->trk_data.needs_full_sync) {
> -
> + if (hmapx_is_empty(&lflow_data->trk_data.dirty_lflow_refs)) {
> dp_group_sync_to_sb(node, lflow_data);
> return EN_HANDLED_UPDATED;
> }
> @@ -107,6 +105,15 @@ dp_group_lflow_handler(struct engine_node *node,
> return EN_UNHANDLED;
> }
> }
> +
> + /* Delete the SB rows for lflows orphaned in-memory this run (see
> + * lflow_table_delete_orphaned_sb_flows()). This only happens on the
> + * incremental path: the handlers that record orphaned_sb_uuids also
> + * mark a lflow_ref dirty, so the full-sync branch above never sees
> + * non-empty orphaned_sb_uuids. */
> + lflow_table_delete_orphaned_sb_flows(
> + sb_flow_table,
> + &lflow_data->trk_data.orphaned_sb_uuids);
> return EN_HANDLED_UPDATED;
> }
>
> diff --git a/northd/en-lflow.c b/northd/en-lflow.c
> index f2ae40ec8..a73d55486 100644
> --- a/northd/en-lflow.c
> +++ b/northd/en-lflow.c
> @@ -221,14 +221,16 @@ lflow_multicast_igmp_handler(struct engine_node
> *node, void *data)
> lflow_get_input_data(node, &lflow_input);
>
> lflow_ref_unlink_and_prune(mcast_igmp_data->lflow_ref,
> - lflow_data->lflow_table);
> + lflow_data->lflow_table,
> + &lflow_data->trk_data.orphaned_sb_uuids);
>
> build_igmp_lflows(&mcast_igmp_data->igmp_groups,
> &lflow_input.ls_datapaths->datapaths,
> lflow_data->lflow_table,
> mcast_igmp_data->lflow_ref);
>
> - lflow_data->trk_data.needs_full_sync = true;
> + hmapx_add(&lflow_data->trk_data.dirty_lflow_refs,
> + mcast_igmp_data->lflow_ref);
>
> return EN_HANDLED_UPDATED;
> }
> @@ -297,7 +299,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node
> *node,
> ic_learned_svc_monitors_data->lflow_ref);
>
> lflow_ref_unlink_and_prune(ic_learned_svc_monitors_data->lflow_ref,
> - lflow_data->lflow_table);
> + lflow_data->lflow_table,
> + &lflow_data->trk_data.orphaned_sb_uuids);
>
> build_lswitch_arp_nd_ic_learned_svc_mon(
> &svc_mons_data,
> @@ -305,7 +308,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node
> *node,
> lflow_input.svc_monitor_mac,
> lflow_data->lflow_table);
>
> - lflow_data->trk_data.needs_full_sync = true;
> + hmapx_add(&lflow_data->trk_data.dirty_lflow_refs,
> + ic_learned_svc_monitors_data->lflow_ref);
>
> return EN_HANDLED_UPDATED;
> }
> @@ -317,7 +321,7 @@ void *en_lflow_init(struct engine_node *node
> OVS_UNUSED,
> data->lflow_table = lflow_table_alloc();
> lflow_table_init(data->lflow_table);
> hmapx_init(&data->trk_data.dirty_lflow_refs);
> - data->trk_data.needs_full_sync = false;
> + uuidset_init(&data->trk_data.orphaned_sb_uuids);
>
> return data;
> }
> @@ -327,11 +331,12 @@ void en_lflow_cleanup(void *data_)
> struct lflow_data *data = data_;
> lflow_table_destroy(data->lflow_table);
> hmapx_destroy(&data->trk_data.dirty_lflow_refs);
> + uuidset_destroy(&data->trk_data.orphaned_sb_uuids);
> }
>
> void en_lflow_clear_tracked_data(void *data_)
> {
> struct lflow_data *data = data_;
> hmapx_clear(&data->trk_data.dirty_lflow_refs);
> - data->trk_data.needs_full_sync = false;
> + uuidset_clear(&data->trk_data.orphaned_sb_uuids);
> }
> diff --git a/northd/en-lflow.h b/northd/en-lflow.h
> index fd3f2427f..c83ef0e4d 100644
> --- a/northd/en-lflow.h
> +++ b/northd/en-lflow.h
> @@ -9,12 +9,17 @@
>
> #include "lib/hmapx.h"
> #include "lib/inc-proc-eng.h"
> +#include "lib/uuidset.h"
>
> struct lflow_table;
>
> struct lflow_tracked_data {
> struct hmapx dirty_lflow_refs; /* lflow_refs changed by handlers. */
> - bool needs_full_sync; /* Full lflow_table_sync_to_sb needed.
> */
> +
> + /* SB Logical_Flow row uuids that were orphaned in-memory by
> + * lflow_ref_unlink_and_prune() and must be deleted from SB after the
> + * incremental lflow_ref_sync_lflows() pass. */
> + struct uuidset orphaned_sb_uuids;
> };
>
> struct lflow_data {
> diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
> index deceadb74..195e00e0f 100644
> --- a/northd/lflow-mgr.c
> +++ b/northd/lflow-mgr.c
> @@ -743,10 +743,16 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref,
> * whose referenced_by list is empty (no other lflow_ref references it).
> * Unlike lflow_ref_unlink_lflows (which only clears dp bits and sets
> * linked=false), this function removes the lrns and orphaned lflows
> - * from the in-memory table entirely, without writing to SB. */
> + * from the in-memory table entirely, without writing to SB.
> + *
> + * Because the orphaned lflows are destroyed here (and so are not visible
> to a
> + * later lflow_ref_sync_lflows() pass), the SB uuid of every orphaned
> lflow
> + * that already has an SB row is recorded in 'orphaned_sb_uuids' so the
> caller
> + * can delete those rows from SB after the incremental sync. */
> void
> lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref,
> - struct lflow_table *lflow_table)
> + struct lflow_table *lflow_table,
> + struct uuidset *orphaned_sb_uuids)
> {
> lflow_ref_unlink_lflows(lflow_ref, lflow_table);
>
> @@ -756,6 +762,9 @@ lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref,
> lflow_ref_node_destroy(lrn);
>
> if (ovs_list_is_empty(&lflow->referenced_by)) {
> + if (!uuid_is_zero(&lflow->sb_uuid)) {
> + uuidset_insert(orphaned_sb_uuids, &lflow->sb_uuid);
> + }
> enum ovn_datapath_type dp_type =
> ovn_stage_to_datapath_type(lflow->stage);
> ovs_assert(dp_type < DP_MAX);
> @@ -767,6 +776,34 @@ lflow_ref_unlink_and_prune(struct lflow_ref
> *lflow_ref,
> }
> }
>
> +/* Deletes the SB Logical_Flow rows for lflows that were orphaned in
> memory
> + * by lflow_ref_unlink_and_prune() this run. lflow_ref_sync_lflows()
> cannot
> + * see them because the lflow (and its lflow_ref_node) has already been
> + * destroyed, so their sb_uuids were recorded in 'orphaned_sb_uuids' and
> are
> + * deleted here.
> + *
> + * This is safe even when an orphaned lflow is re-added by a later build
> + * (e.g. the per-datapath multicast flood flow): the re-added lflow is a
> + * fresh in-memory object with a new random sb_uuid (the old one was
> + * destroyed), so its freshly inserted SB row is never in
> + * 'orphaned_sb_uuids'. */
> +void
> +lflow_table_delete_orphaned_sb_flows(
> + const struct sbrec_logical_flow_table *sb_flow_table,
> + struct uuidset *orphaned_sb_uuids)
> +{
> + struct uuidset_node *node;
> +
> + UUIDSET_FOR_EACH (node, orphaned_sb_uuids) {
> + const struct sbrec_logical_flow *sbflow =
> + sbrec_logical_flow_table_get_for_uuid(sb_flow_table,
> + &node->uuid);
> + if (sbflow) {
> + sbrec_logical_flow_delete(sbflow);
> + }
> + }
> +}
> +
> bool
> lflow_ref_sync_lflows(struct lflow_ref *lflow_ref,
> struct lflow_table *lflow_table,
> diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h
> index 678e97214..f4cccb5e1 100644
> --- a/northd/lflow-mgr.h
> +++ b/northd/lflow-mgr.h
> @@ -25,6 +25,7 @@ struct ovsdb_idl_txn;
> struct ovn_datapath;
> struct ovsdb_idl_row;
> struct ovn_lflow;
> +struct uuidset;
>
> /* lflow map which stores the logical flows. */
> struct lflow_table {
> @@ -58,7 +59,10 @@ 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 *, struct lflow_table *);
> -void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *);
> +void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *,
> + struct uuidset *);
> +void lflow_table_delete_orphaned_sb_flows(
> + const struct sbrec_logical_flow_table *, struct uuidset *);
> bool lflow_ref_sync_lflows(struct lflow_ref *,
> struct lflow_table *lflow_table,
> struct ovsdb_idl_txn *ovnsb_txn,
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index 6866068fa..1097fa998 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -24828,4 +24828,65 @@ CHECK_NO_CHANGE_AFTER_RECOMPUTE
>
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
> +])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([check shared LB VIP flow survives datapath group full sync])
> +AT_KEYWORDS([loadbalancer])
> +ovn_start
> +
> +# Two load balancers on the same switch share the VIP 1.1.1.1:80 with the
> +# same backend, so they both reference one logical flow L in ls_in_lb.
> +# L's datapath refcnt for sw1 is therefore 2. The bug this test guards
> +# against: a full sync of the datapath group retains the lflow_ref_node of
> +# an LB that has already dropped the shared VIP, and a later change to
> that
> +# LB unlinks the retained node again, releasing sw1's datapath refcnt a
> +# second time. That empties L's datapath bitmap and a later full sync
> +# prunes L and deletes its SB row even though lb2 still requires it.
> +check ovn-nbctl --wait=hv ls-add sw1
> +check ovn-nbctl --wait=hv lsp-add sw1 p1 -- \
> + lsp-set-addresses p1 "aa:aa:aa:aa:aa:01 192.168.10.2"
> +
> +check ovn-nbctl --wait=hv lb-add lb1 1.1.1.1:80 192.168.10.2:80
> +check ovn-nbctl --wait=hv lb-add lb1 2.2.2.2:80 192.168.10.2:80
> +check ovn-nbctl --wait=hv lb-add lb2 1.1.1.1:80 192.168.10.2:80
> +check ovn-nbctl --wait=hv ls-lb-add sw1 lb1
> +check ovn-nbctl --wait=hv ls-lb-add sw1 lb2
> +
> +# Baseline: the shared VIP flow exists.
> +OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
> 1.1.1.1" | grep -c .`])
> +
> +# RUN 1: remove 1.1.1.1 from lb1 (lb1 keeps 2.2.2.2) and change p1's
> +# address in the SAME engine run. The port update makes the multicast
> node
> +# recompute, which forces the datapath group to take the FULL sync path.
> +# Unlike the incremental sync, the full sync does not destroy the
> lflow_ref
> +# nodes that were already unlinked, so lb1's now-unlinked 1.1.1.1 node is
> +# retained. The two changes must land in a single run, so they are
> batched
> +# into one transaction.
> +check ovn-nbctl --wait=hv lb-del lb1 1.1.1.1:80 -- \
> + lsp-set-addresses p1 "aa:aa:aa:aa:aa:01 192.168.20.2"
> +# The shared VIP flow is still present (lb2 keeps it).
> +OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
> 1.1.1.1" | grep -c .`])
> +
> +# RUN 2: change lb1 (append VIP 3.3.3.3). This re-unlinks lb1's
> lflow_ref.
> +# Without the fix this hits the retained 1.1.1.1 node and double-releases
> +# sw1's datapath count, emptying the shared flow's datapath bitmap.
> +check ovn-nbctl --wait=hv lb-add lb1 3.3.3.3:80 192.168.10.2:80
> +
> +# RUN 3: another port address change -> needs_full_sync -> full sync.
> +# Without the fix this prunes the shared 1.1.1.1 flow (now empty bitmap)
> +# and deletes its SB row.
> +check ovn-nbctl --wait=hv lsp-set-addresses p1 "aa:aa:aa:aa:aa:01
> 192.168.30.2"
> +
> +# REGRESSION ASSERTION: the shared VIP flow must still be present, since
> lb2
> +# still requires 1.1.1.1. Without the fix it was deleted.
> +OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep "ip4.dst ==
> 1.1.1.1" | grep -c .`])
> +
> +# Sanity: p1's port flows now use the final address 192.168.30.2 (confirms
> +# the port changes in runs 1 and 3 were actually applied).
> +OVS_WAIT_UNTIL([test 1 -le `ovn-sbctl dump-flows sw1 | grep
> "192.168.30.2" | grep -c .`])
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
>
> --
> 2.55.0
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
Thank you Jacob,
applied to main with the nit fixed.
Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
