On Tue, Sep 1, 2026 at 11:04 PM Jacob Tanenbaum via dev < [email protected]> wrote:
> Split ovn-northd's en_lflow engine node into a compute-only > en_lflow node and a new en_dp_group_resolved node that > handles datapath-group resolution and all SB database > writes. This separation makes en_lflow independent of the > SB database, preparing for future incremental processing > improvements. > > en_lflow's handlers now track dirty lflow_refs in a hmapx > instead of calling lflow_ref_sync_lflows directly. > en_dp_group_resolved drains the dirty set on its lflow > handler, falling back to a full lflow_table_sync_to_sb when > en_lflow did a full recompute or when the IGMP/MLD handler > set the needs_full_sync flag. > > A new lflow_ref_unlink_and_prune() function replaces > lflow_ref_resync_flows() for the IGMP and IC-learned > service monitor handlers. These handlers use a single > shared lflow_ref whose flows are also built by the > per-datapath pipeline (with lflow_ref = NULL). The > per-datapath build's dp bits are not tracked by any > lflow_ref, so lflow_ref_unlink_lflows alone cannot clear > them via dp_refcnt. lflow_ref_unlink_and_prune destroys > all lrns and orphaned lflows in-memory without SB writes, > equivalent to lflow_ref_resync_flows' cleanup. > > The lflow_table_add_lflow__ upgrade-to-bitmap logic (for > single lflow_refs that contribute multiple datapaths to the > same lflow) is extended with per-datapath dp_refcnt > accounting during mid-cycle bitmap upgrades, preventing > premature dp bit release when multiple lflow_refs share a > flow across overlapping datapaths. > > Reported-at: https://redhat.atlassian.net/browse/FDP-2747 > Assisted-by: Claude Opus 4.8, Claude Code > Signed-off-by: Jacob Tanenbaum <[email protected]> > --- > Hi Jacob, thank you for the patch. I have a few comments down below. Also did you run ovn-heater tests to see if there are any potential issues at scale? > northd/automake.mk | 2 + > northd/en-dp-group-resolved.c | 129 ++++++++++++++++ > northd/en-dp-group-resolved.h | 30 ++++ > northd/en-lflow.c | 140 +++++------------ > northd/en-lflow.h | 3 + > northd/en-northd-output.c | 4 +- > northd/en-northd-output.h | 3 +- > northd/inc-proc-northd.c | 29 +++- > northd/lflow-mgr.c | 168 +++++++++++++++++---- > northd/lflow-mgr.h | 8 +- > northd/northd.c | 251 +++++++------------------------ > northd/northd.h | 39 +++-- > tests/ovn-inc-proc-graph-dump.at | 12 +- > tests/ovn-northd.at | 81 +++++++--- > 14 files changed, 506 insertions(+), 393 deletions(-) > create mode 100644 northd/en-dp-group-resolved.c > create mode 100644 northd/en-dp-group-resolved.h > > diff --git a/northd/automake.mk b/northd/automake.mk > index 45ca0337f..d1439294d 100644 > --- a/northd/automake.mk > +++ b/northd/automake.mk > @@ -16,6 +16,8 @@ northd_ovn_northd_SOURCES = \ > northd/en-datapath-logical-router.h \ > northd/en-datapath-sync.c \ > northd/en-datapath-sync.h \ > + northd/en-dp-group-resolved.c \ > + northd/en-dp-group-resolved.h \ > northd/en-ecmp-nexthop.c \ > northd/en-ecmp-nexthop.h \ > northd/en-global-config.c \ > diff --git a/northd/en-dp-group-resolved.c b/northd/en-dp-group-resolved.c > new file mode 100644 > index 000000000..d5f9e7738 > --- /dev/null > +++ b/northd/en-dp-group-resolved.c > nit: I would probably leave the resolved out of the name. Just en-dp-group is better IMO. > @@ -0,0 +1,129 @@ > +/* > + * Copyright (c) 2026, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#include <config.h> > + > +#include "en-dp-group-resolved.h" > +#include "en-datapath-sync.h" > +#include "en-global-config.h" > +#include "en-lflow.h" > +#include "lflow-mgr.h" > + > +#include "lib/inc-proc-eng.h" > +#include "northd.h" > +#include "lib/stopwatch-names.h" > +#include "stopwatch.h" > +#include "timeval.h" > +#include "openvswitch/vlog.h" > nit: Not ordered according to coding style. > + > +VLOG_DEFINE_THIS_MODULE(en_dp_group_resolved); > + > +void * > +en_dp_group_resolved_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + return NULL; > +} > + > +static void > +dp_group_resolved_sync_to_sb(struct engine_node *node, > + struct lflow_data *lflow_data) > +{ > + const struct engine_context *eng_ctx = engine_get_context(); > + struct all_synced_datapaths *all_dps = > + engine_get_input_data("datapath_sync", node); > + > + const struct sbrec_logical_flow_table *sb_flow_table = > + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); > + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = > + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); > + > + struct ed_type_global_config *global_config = > + engine_get_input_data("global_config", node); > + > + stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); > + lflow_table_sync_to_sb(lflow_data->lflow_table, > + eng_ctx->ovnsb_idl_txn, > + all_dps->synced_dps, > + global_config->ovn_internal_version_changed, > + sb_flow_table, sb_dpgrp_table); > + stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); > +} > + > +enum engine_node_state > +en_dp_group_resolved_run(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); > + > + /* The full sync below covers every lflow in the table, so any > + * per-ref dirty tracking left over from en_lflow's handlers is > + * redundant. Clear it so the next incremental cycle starts > + * clean. */ > + hmapx_clear(&lflow_data->dirty_lflow_refs); > + lflow_data->needs_full_sync = false; > + > + dp_group_resolved_sync_to_sb(node, lflow_data); > + return EN_UPDATED; > +} > + > +enum engine_input_handler_result > +dp_group_resolved_lflow_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + struct lflow_data *lflow_data = engine_get_input_data("lflow", node); > + > + if (hmapx_is_empty(&lflow_data->dirty_lflow_refs) > + || lflow_data->needs_full_sync) { > + hmapx_clear(&lflow_data->dirty_lflow_refs); > + lflow_data->needs_full_sync = false; > + dp_group_resolved_sync_to_sb(node, lflow_data); > + return EN_HANDLED_UPDATED; > + } > + > + const struct engine_context *eng_ctx = engine_get_context(); > + struct all_synced_datapaths *all_dps = > + engine_get_input_data("datapath_sync", node); > + > + const struct sbrec_logical_flow_table *sb_flow_table = > + EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); > + const struct sbrec_logical_dp_group_table *sb_dpgrp_table = > + EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); > + > + struct ed_type_global_config *global_config = > + engine_get_input_data("global_config", node); > nit: We usually get all the input data before doing any kind of operation. > + > + struct hmapx_node *hmapx_node; > + HMAPX_FOR_EACH (hmapx_node, &lflow_data->dirty_lflow_refs) { > + struct lflow_ref *ref = hmapx_node->data; > + if (!lflow_ref_sync_lflows( > + ref, lflow_data->lflow_table, > + eng_ctx->ovnsb_idl_txn, > + all_dps->synced_dps, > + global_config->ovn_internal_version_changed, > + sb_flow_table, sb_dpgrp_table)) { > + return EN_UNHANDLED; > + } > + } > + hmapx_clear(&lflow_data->dirty_lflow_refs); > + > + return EN_HANDLED_UPDATED; > +} > + > +void > +en_dp_group_resolved_cleanup(void *data OVS_UNUSED) > +{ > +} > diff --git a/northd/en-dp-group-resolved.h b/northd/en-dp-group-resolved.h > new file mode 100644 > index 000000000..9f25072c1 > --- /dev/null > +++ b/northd/en-dp-group-resolved.h > @@ -0,0 +1,30 @@ > +/* > + * Copyright (c) 2026, Red Hat, Inc. > + * > + * Licensed under the Apache License, Version 2.0 (the "License"); > + * you may not use this file except in compliance with the License. > + * You may obtain a copy of the License at: > + * > + * http://www.apache.org/licenses/LICENSE-2.0 > + * > + * Unless required by applicable law or agreed to in writing, software > + * distributed under the License is distributed on an "AS IS" BASIS, > + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or > implied. > + * See the License for the specific language governing permissions and > + * limitations under the License. > + */ > + > +#ifndef EN_DP_GROUP_RESOLVED_H > +#define EN_DP_GROUP_RESOLVED_H 1 > + > +#include "lib/inc-proc-eng.h" > + > +void *en_dp_group_resolved_init(struct engine_node *node, > + struct engine_arg *arg); > +enum engine_node_state en_dp_group_resolved_run(struct engine_node *node, > + void *data); > +void en_dp_group_resolved_cleanup(void *data); > +enum engine_input_handler_result > +dp_group_resolved_lflow_handler(struct engine_node *node, void *data); > + > +#endif /* EN_DP_GROUP_RESOLVED_H */ > diff --git a/northd/en-lflow.c b/northd/en-lflow.c > index 8cb987777..38b1639d6 100644 > --- a/northd/en-lflow.c > +++ b/northd/en-lflow.c > @@ -68,10 +68,6 @@ lflow_get_input_data(struct engine_node *node, > struct all_synced_datapaths *all_dps = > engine_get_input_data("datapath_sync", node); > > - lflow_input->sbrec_logical_flow_table = > - EN_OVSDB_GET(engine_get_input("SB_logical_flow", node)); > - lflow_input->sbrec_logical_dp_group_table = > - EN_OVSDB_GET(engine_get_input("SB_logical_dp_group", node)); > lflow_input->sbrec_acl_id_table = > EN_OVSDB_GET(engine_get_input("SB_acl_id", node)); > > @@ -119,8 +115,6 @@ lflow_get_input_data(struct engine_node *node, > enum engine_node_state > en_lflow_run(struct engine_node *node, void *data) > { > - const struct engine_context *eng_ctx = engine_get_context(); > - > struct lflow_input lflow_input; > lflow_get_input_data(node, &lflow_input); > > @@ -129,9 +123,10 @@ en_lflow_run(struct engine_node *node, void *data) > search_mode == LFLOW_TABLE_SEARCH_FIELDS); > lflow_reset_northd_refs(&lflow_input); > lflow_ref_clear(lflow_input.igmp_lflow_ref); > + hmapx_clear(&lflow_data->dirty_lflow_refs); > + lflow_data->needs_full_sync = false; > > - build_lflows(eng_ctx->ovnsb_idl_txn, &lflow_input, > - lflow_data->lflow_table); > + build_lflows(&lflow_input, lflow_data->lflow_table); > > return EN_UPDATED; > } > @@ -149,31 +144,25 @@ lflow_northd_handler(struct engine_node *node, > return EN_UNHANDLED; > } > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > > struct lflow_input lflow_input; > lflow_get_input_data(node, &lflow_input); > > - if (!lflow_handle_northd_lr_changes(eng_ctx->ovnsb_idl_txn, > - > &northd_data->trk_data.trk_routers, > - &lflow_input, > - lflow_data->lflow_table)) { > - return EN_UNHANDLED; > - } > + lflow_handle_northd_lr_changes(&northd_data->trk_data.trk_routers, > + &lflow_input, > + lflow_data->lflow_table, > + &lflow_data->dirty_lflow_refs); > > - if (!lflow_handle_northd_port_changes(eng_ctx->ovnsb_idl_txn, > - &northd_data->trk_data.trk_lsps, > - &lflow_input, > - lflow_data->lflow_table)) { > - return EN_UNHANDLED; > - } > + lflow_handle_northd_port_changes(&northd_data->trk_data.trk_lsps, > + &lflow_input, > + lflow_data->lflow_table, > + &lflow_data->dirty_lflow_refs); > > - if (!lflow_handle_northd_lb_changes( > - eng_ctx->ovnsb_idl_txn, &northd_data->trk_data.trk_lbs, > - &lflow_input, lflow_data->lflow_table)) { > - return EN_UNHANDLED; > - } > + lflow_handle_northd_lb_changes(&northd_data->trk_data.trk_lbs, > + &lflow_input, > + lflow_data->lflow_table, > + &lflow_data->dirty_lflow_refs); > > return EN_HANDLED_UPDATED; > } > @@ -189,17 +178,14 @@ lflow_lr_stateful_handler(struct engine_node *node, > void *data) > return EN_UNHANDLED; > } > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > > lflow_get_input_data(node, &lflow_input); > - if (!lflow_handle_lr_stateful_changes(eng_ctx->ovnsb_idl_txn, > - &lr_sful_data->trk_data, > - &lflow_input, > - lflow_data->lflow_table)) { > - return EN_UNHANDLED; > - } > + lflow_handle_lr_stateful_changes(&lr_sful_data->trk_data, > + &lflow_input, > + lflow_data->lflow_table, > + &lflow_data->dirty_lflow_refs); > > return EN_HANDLED_UPDATED; > } > @@ -214,17 +200,14 @@ lflow_ls_stateful_handler(struct engine_node *node, > void *data) > return EN_UNHANDLED; > } > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > > lflow_get_input_data(node, &lflow_input); > - if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, > - &ls_sful_data->trk_data, > - &lflow_input, > - lflow_data->lflow_table)) { > - return EN_UNHANDLED; > - } > + lflow_handle_ls_stateful_changes(&ls_sful_data->trk_data, > + &lflow_input, > + lflow_data->lflow_table, > + &lflow_data->dirty_lflow_refs); > > return EN_HANDLED_UPDATED; > } > @@ -235,35 +218,20 @@ lflow_multicast_igmp_handler(struct engine_node > *node, void *data) > struct multicast_igmp_data *mcast_igmp_data = > engine_get_input_data("multicast_igmp", node); > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > lflow_get_input_data(node, &lflow_input); > > - if (!lflow_ref_resync_flows(mcast_igmp_data->lflow_ref, > - lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, > - lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - > lflow_input.sbrec_logical_dp_group_table)) { > - return EN_UNHANDLED; > - } > + lflow_ref_unlink_and_prune(mcast_igmp_data->lflow_ref, > + lflow_data->lflow_table); > > build_igmp_lflows(&mcast_igmp_data->igmp_groups, > &lflow_input.ls_datapaths->datapaths, > lflow_data->lflow_table, > mcast_igmp_data->lflow_ref); > > - if (!lflow_ref_sync_lflows(mcast_igmp_data->lflow_ref, > - lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, > - lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - lflow_input.sbrec_logical_dp_group_table)) > { > - return EN_UNHANDLED; > - } > + lflow_data->needs_full_sync = true; > + hmapx_add(&lflow_data->dirty_lflow_refs, mcast_igmp_data->lflow_ref); > > return EN_HANDLED_UPDATED; > } > @@ -280,7 +248,6 @@ lflow_group_ecmp_route_change_handler(struct > engine_node *node, > return EN_UNHANDLED; > } > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > > struct lflow_input lflow_input; > @@ -289,25 +256,13 @@ lflow_group_ecmp_route_change_handler(struct > engine_node *node, > struct group_ecmp_datapath *route_node; > struct hmapx_node *hmapx_node; > > - /* We need to handle deletions before additions as they could > potentially > - * overlap. */ > 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); > - > - bool handled = lflow_ref_sync_lflows( > - route_node->lflow_ref, lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - lflow_input.sbrec_logical_dp_group_table); > - if (!handled) { > - return EN_UNHANDLED; > - } > + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); > } > > - /* Now we handle created or updated route nodes. */ > struct hmapx *crupdated_datapath_routes = > &group_ecmp_route_data->trk_data.crupdated_datapath_routes; > HMAPX_FOR_EACH (hmapx_node, crupdated_datapath_routes) { > @@ -316,16 +271,7 @@ lflow_group_ecmp_route_change_handler(struct > engine_node *node, > build_route_data_flows_for_lrouter( > route_node->od, lflow_data->lflow_table, > route_node, lflow_input.bfd_ports); > - > - bool handled = lflow_ref_sync_lflows( > - route_node->lflow_ref, lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - lflow_input.sbrec_logical_dp_group_table); > - if (!handled) { > - return EN_UNHANDLED; > - } > + hmapx_add(&lflow_data->dirty_lflow_refs, route_node->lflow_ref); > } > > return EN_HANDLED_UPDATED; > @@ -338,7 +284,6 @@ lflow_ic_learned_svc_mons_handler(struct engine_node > *node, > struct ic_learned_svc_monitors_data *ic_learned_svc_monitors_data = > engine_get_input_data("ic_learned_svcs", node); > > - const struct engine_context *eng_ctx = engine_get_context(); > struct lflow_data *lflow_data = data; > struct lflow_input lflow_input; > lflow_get_input_data(node, &lflow_input); > @@ -349,16 +294,8 @@ lflow_ic_learned_svc_mons_handler(struct engine_node > *node, > &ic_learned_svc_monitors_data->ic_learned_svc_monitors_map, > ic_learned_svc_monitors_data->lflow_ref); > > - if (!lflow_ref_resync_flows( > - ic_learned_svc_monitors_data->lflow_ref, > - lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, > - lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - lflow_input.sbrec_logical_dp_group_table)) { > - return EN_UNHANDLED; > - } > + lflow_ref_unlink_and_prune(ic_learned_svc_monitors_data->lflow_ref, > + lflow_data->lflow_table); > > build_lswitch_arp_nd_ic_learned_svc_mon( > &svc_mons_data, > @@ -366,16 +303,9 @@ lflow_ic_learned_svc_mons_handler(struct engine_node > *node, > lflow_input.svc_monitor_mac, > lflow_data->lflow_table); > > - if (!lflow_ref_sync_lflows( > - ic_learned_svc_monitors_data->lflow_ref, > - lflow_data->lflow_table, > - eng_ctx->ovnsb_idl_txn, > - lflow_input.dps, > - lflow_input.ovn_internal_version_changed, > - lflow_input.sbrec_logical_flow_table, > - lflow_input.sbrec_logical_dp_group_table)) { > - return EN_UNHANDLED; > - } > + lflow_data->needs_full_sync = true; > + hmapx_add(&lflow_data->dirty_lflow_refs, > + ic_learned_svc_monitors_data->lflow_ref); > > return EN_HANDLED_UPDATED; > } > @@ -386,6 +316,7 @@ void *en_lflow_init(struct engine_node *node > OVS_UNUSED, > struct lflow_data *data = xmalloc(sizeof *data); > data->lflow_table = lflow_table_alloc(); > lflow_table_init(data->lflow_table); > + hmapx_init(&data->dirty_lflow_refs); > return data; > } > > @@ -393,4 +324,5 @@ void en_lflow_cleanup(void *data_) > { > struct lflow_data *data = data_; > lflow_table_destroy(data->lflow_table); > + hmapx_destroy(&data->dirty_lflow_refs); > } > diff --git a/northd/en-lflow.h b/northd/en-lflow.h > index 99bcfda15..5dab6cc19 100644 > --- a/northd/en-lflow.h > +++ b/northd/en-lflow.h > @@ -7,12 +7,15 @@ > #include <stdlib.h> > #include <stdio.h> > > +#include "lib/hmapx.h" > #include "lib/inc-proc-eng.h" > > struct lflow_table; > > struct lflow_data { > struct lflow_table *lflow_table; > + struct hmapx dirty_lflow_refs; /* lflow_refs changed by handlers. */ > + bool needs_full_sync; /* Full lflow_table_sync_to_sb needed. > */ > This has very strange relation, it is cleared by the sync node? Shouldn't this be part of the tracked_data and cleared before every engine run? That's why we have those. Same goes for the hmapx, it should be modified only by lflow node and consumed by the sync. }; > > enum engine_node_state en_lflow_run(struct engine_node *node, void *data); > diff --git a/northd/en-northd-output.c b/northd/en-northd-output.c > index b492a771c..ed8dec0ac 100644 > --- a/northd/en-northd-output.c > +++ b/northd/en-northd-output.c > @@ -51,8 +51,8 @@ northd_output_sync_to_sb_handler(struct engine_node > *node OVS_UNUSED, > } > > enum engine_input_handler_result > -northd_output_lflow_handler(struct engine_node *node OVS_UNUSED, > - void *data OVS_UNUSED) > +northd_output_dp_group_resolved_handler(struct engine_node *node > OVS_UNUSED, > + void *data OVS_UNUSED) > { > return EN_HANDLED_UPDATED; > } > diff --git a/northd/en-northd-output.h b/northd/en-northd-output.h > index b7053e60c..bb5e35900 100644 > --- a/northd/en-northd-output.h > +++ b/northd/en-northd-output.h > @@ -13,7 +13,8 @@ enum engine_input_handler_result > northd_output_sync_to_sb_handler(struct engine_node *node, > void *data OVS_UNUSED); > enum engine_input_handler_result > -northd_output_lflow_handler(struct engine_node *node, void *data > OVS_UNUSED); > +northd_output_dp_group_resolved_handler(struct engine_node *node, > + void *data OVS_UNUSED); > enum engine_input_handler_result > northd_output_mac_binding_aging_handler(struct engine_node *node, > void *data OVS_UNUSED); > diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c > index d2f28666c..4c0c8acaa 100644 > --- a/northd/inc-proc-northd.c > +++ b/northd/inc-proc-northd.c > @@ -37,6 +37,7 @@ > #include "en-multicast.h" > #include "en-northd.h" > #include "en-lflow.h" > +#include "en-dp-group-resolved.h" > #include "en-northd-output.h" > #include "en-meters.h" > #include "en-sampling-app.h" > @@ -160,7 +161,8 @@ enum sb_engine_node { > static ENGINE_NODE(northd, CLEAR_TRACKED_DATA, SB_WRITE); > static ENGINE_NODE(sync_from_sb, SB_WRITE); > static ENGINE_NODE(sampling_app); > -static ENGINE_NODE(lflow, SB_WRITE); > +static ENGINE_NODE(lflow); > +static ENGINE_NODE(dp_group_resolved, SB_WRITE); > static ENGINE_NODE(mac_binding_aging, SB_WRITE); > static ENGINE_NODE(mac_binding_aging_waker); > static ENGINE_NODE(northd_output); > @@ -399,9 +401,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_multicast_igmp, &en_sb_igmp_group, NULL); > > engine_add_input(&en_lflow, &en_sync_meters, NULL); > - engine_add_input(&en_lflow, &en_sb_logical_flow, NULL); > engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); > - engine_add_input(&en_lflow, &en_sb_logical_dp_group, NULL); > engine_add_input(&en_lflow, &en_bfd_sync, NULL); > engine_add_input(&en_lflow, &en_route_policies, NULL); > /* Route changes are propagated to en_lflow through the > en_group_ecmp_route > @@ -436,6 +436,25 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_lflow, &en_ic_learned_svc_monitors, > lflow_ic_learned_svc_mons_handler); > > + engine_add_input(&en_dp_group_resolved, &en_lflow, > + dp_group_resolved_lflow_handler); > + engine_add_input(&en_dp_group_resolved, &en_sb_logical_flow, NULL); > + engine_add_input(&en_dp_group_resolved, &en_sb_logical_dp_group, > NULL); > + /* dp_group_resolved reads synced datapath arrays (dps[DP_MAX]) when > + * calling lflow_table_sync_to_sb / lflow_ref_sync_lflows. A noop > + * handler is sufficient because en_lflow already recomputes when > + * datapath_sync changes; dp_group_resolved just needs access to > + * the data. */ > + engine_add_input(&en_dp_group_resolved, &en_datapath_sync, > + engine_noop_handler); > + /* dp_group_resolved reads ovn_internal_version_changed from > + * global_config when syncing flows to SB. Changes to > + * global_config propagate through en_lflow (which has its own > + * global_config handler), so a noop handler here avoids a > + * redundant recompute. */ > + engine_add_input(&en_dp_group_resolved, &en_global_config, > + engine_noop_handler); > + > engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL); > engine_add_input(&en_sync_to_sb_addr_set, &en_lr_stateful, NULL); > engine_add_input(&en_sync_to_sb_addr_set, &en_sb_address_set, NULL); > @@ -490,8 +509,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, > engine_add_input(&en_northd_output, &en_sync_from_sb, NULL); > engine_add_input(&en_northd_output, &en_sync_to_sb, > northd_output_sync_to_sb_handler); > - engine_add_input(&en_northd_output, &en_lflow, > - northd_output_lflow_handler); > + engine_add_input(&en_northd_output, &en_dp_group_resolved, > + northd_output_dp_group_resolved_handler); > engine_add_input(&en_northd_output, &en_mac_binding_aging, > northd_output_mac_binding_aging_handler); > engine_add_input(&en_northd_output, &en_fdb_aging, > diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c > index ce9c4f854..7a61212bc 100644 > --- a/northd/lflow-mgr.c > +++ b/northd/lflow-mgr.c > @@ -261,6 +261,28 @@ lflow_table_set_size(struct lflow_table *lflow_table, > size_t size) > lflow_table->entries.n = size; > } > > +/* An lflow that applies to no datapaths (its dp group bitmap is empty) > must > + * not exist in the SB DB. Such lflows can be left in the table by > en_lflow's > + * incremental handlers, which unlink lflows (clearing their dp bits) and > defer > + * the teardown to the dp-group-resolved sync stage. Release the lflow's > dp > + * group and destroy it; its SB row (if any) is removed by the caller. > Returns > + * true if the lflow was destroyed. */ > +static bool > +lflow_prune_if_no_datapaths(struct lflow_table *lflow_table, > + struct ovn_lflow *lflow) > +{ > + if (dynamic_bitmap_count1(&lflow->dpg_bitmap)) { > + return false; > + } > + > + enum ovn_datapath_type dp_type = > ovn_stage_to_datapath_type(lflow->stage); > + ovs_assert(dp_type < DP_MAX); > + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], lflow->dpg); > + lflow->dpg = NULL; > + ovn_lflow_destroy(lflow_table, lflow); > + return true; > +} > + > void > lflow_table_sync_to_sb(struct lflow_table *lflow_table, > struct ovsdb_idl_txn *ovnsb_txn, > @@ -287,6 +309,12 @@ lflow_table_sync_to_sb(struct lflow_table > *lflow_table, > ovn_lflow_destroy(lflow_table, lflow); > continue; > } > + /* An lflow with no datapaths must be removed from the SB DB. It > is > + * skipped here (not added to 'sb_uuid_set'), so its SB row, if > any, is > + * deleted by the reconciliation loop below. */ > + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { > + continue; > + } > sbflow = NULL; > if (!uuid_is_zero(&lflow->sb_uuid)) { > sbflow = sbrec_logical_flow_table_get_for_uuid(sb_flow_table, > @@ -372,7 +400,7 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table, > lflows, &stage, > sbflow->priority, sbflow->match, sbflow->actions, > sbflow->controller_meter, acl_ct_translation, sbflow->hash); > - if (lflow) { > + if (lflow && !lflow_prune_if_no_datapaths(lflow_table, lflow)) { > const struct ovn_synced_datapaths *datapaths; > struct hmap *dp_groups; > dp_groups = &lflow_table->dp_groups[dp_type]; > @@ -393,6 +421,9 @@ lflow_table_sync_to_sb(struct lflow_table *lflow_table, > if (search_mode != LFLOW_TABLE_SEARCH_FIELDS) { > break; > } > + if (lflow_prune_if_no_datapaths(lflow_table, lflow)) { > + continue; > + } > const struct ovn_synced_datapaths *datapaths; > struct hmap *dp_groups; > enum ovn_datapath_type dp_type = > @@ -681,20 +712,32 @@ lflow_ref_unlink_lflows(struct lflow_ref *lflow_ref) > } > } > > -bool > -lflow_ref_resync_flows(struct lflow_ref *lflow_ref, > - struct lflow_table *lflow_table, > - struct ovsdb_idl_txn *ovnsb_txn, > - const struct ovn_synced_datapaths dps[DP_MAX], > - bool ovn_internal_version_changed, > - const struct sbrec_logical_flow_table > *sbflow_table, > - const struct sbrec_logical_dp_group_table > *dpgrp_table) > +/* Unlinks and destroys all lrns in 'lflow_ref', then destroys any lflow > + * 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. */ > +void > +lflow_ref_unlink_and_prune(struct lflow_ref *lflow_ref, > + struct lflow_table *lflow_table) > { > lflow_ref_unlink_lflows(lflow_ref); > - return lflow_ref_sync_lflows__(lflow_ref, lflow_table, ovnsb_txn, > - dps, > - ovn_internal_version_changed, > sbflow_table, > - dpgrp_table); > + > + struct lflow_ref_node *lrn; > + HMAP_FOR_EACH_SAFE (lrn, ref_node, &lflow_ref->lflow_ref_nodes) { > + struct ovn_lflow *lflow = lrn->lflow; > + lflow_ref_node_destroy(lrn); > + > + if (ovs_list_is_empty(&lflow->referenced_by)) { > + enum ovn_datapath_type dp_type = > + ovn_stage_to_datapath_type(lflow->stage); > + ovs_assert(dp_type < DP_MAX); > + ovn_dp_group_release(&lflow_table->dp_groups[dp_type], > + lflow->dpg); > + lflow->dpg = NULL; > + ovn_lflow_destroy(lflow_table, lflow); > + } > + } > } > > bool > @@ -781,10 +824,78 @@ lflow_table_add_lflow__(struct lflow_table > *lflow_table, > } > ovs_list_insert(&lflow->referenced_by, &lrn->ref_list_node); > hmap_insert(&lflow_ref->lflow_ref_nodes, &lrn->ref_node, > hash); > + } else if (sdp) { > + /* Single-datapath (re-)add of an existing node. */ > + if (!lrn->dpgrp_lflow) { > + if (!lrn->linked) { > + /* First add of this (re-)link cycle establishes the > base > + * datapath this reference tracks for the lflow. */ > + lrn->dp_index = sdp->index; > + } else if (lrn->dp_index != sdp->index) { > + /* The same 'lflow_ref' references this lflow L(M, A) > for a > + * second datapath in this cycle (e.g. the single > lflow_ref > + * shared by all IGMP flows). A single-datapath > + * lflow_ref_node tracks only one datapath index, so > + * lflow_ref_unlink_lflows() would leave the other > + * datapaths set in the lflow's dp group bitmap. > Upgrade > + * the node to track a datapath bitmap instead, so > that > + * unlinking clears every datapath this reference > + * contributed. */ > + size_t len = sparse_array_len(&sdp->dps->dps_array); > + lrn->dpgrp_bitmap = bitmap_allocate(len); > + lrn->dpgrp_bitmap_len = len; > + bitmap_set1(lrn->dpgrp_bitmap, lrn->dp_index); > + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); > + lrn->dpgrp_lflow = true; > + > + /* This reference already accounted for > 'lrn->dp_index' > + * (the first datapath it added this cycle) in the > block > + * below. This add contributes a second datapath, > which > + * the block below will not see because it only runs > on > + * the first link of the cycle. Account for it here > so > + * that a datapath shared with another reference is > not > + * released prematurely. */ > + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, > + sdp->index)) { > + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); > + } > + } > + } else { > + /* A node previously upgraded to track a datapath bitmap > is > + * re-linked one datapath at a time. */ > + if (!lrn->linked) { > + /* First add of this (re-)link cycle: rebuild the > tracked > + * bitmap from scratch so it reflects the current > datapath > + * set and the current datapath array length (which > may > + * have grown or shrunk since the node was last > linked). */ > + size_t len = sparse_array_len(&sdp->dps->dps_array); > + bitmap_free(lrn->dpgrp_bitmap); > + lrn->dpgrp_bitmap = bitmap_allocate(len); > + lrn->dpgrp_bitmap_len = len; > + } else if (!bitmap_is_set(lrn->dpgrp_bitmap, sdp->index)) > { > + /* A second or later datapath this reference > contributes in > + * the same cycle. The first datapath was accounted > for by > + * the block below (which only runs on the first link > of > + * the cycle); account for this one too so that a > datapath > + * shared with another reference is not released > + * prematurely. */ > + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, > + sdp->index)) { > + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); > + } > + } > + bitmap_set1(lrn->dpgrp_bitmap, sdp->index); > + } > } > > if (!lrn->linked) { > - if (lrn->dpgrp_lflow) { > + /* Allocate a reference counter only if the datapath(s) added > by > + * this reference are already used by the lflow. */ > + if (sdp) { > + if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, > sdp->index)) { > + dp_refcnt_use(&lflow->dp_refcnts_map, sdp->index); > + } > + } else { > ovs_assert(lrn->dpgrp_bitmap_len == dp_bitmap_len); > size_t index; > BITMAP_FOR_EACH_1 (index, dp_bitmap_len, dp_bitmap) { > @@ -793,11 +904,6 @@ lflow_table_add_lflow__(struct lflow_table > *lflow_table, > dp_refcnt_use(&lflow->dp_refcnts_map, index); > } > } > - } else { > - /* Allocate a reference counter only if already used. */ > - if (dynamic_bitmap_is_set(&lflow->dpg_bitmap, > lrn->dp_index)) { > - dp_refcnt_use(&lflow->dp_refcnts_map, lrn->dp_index); > - } > } > } > lrn->linked = true; > @@ -1231,17 +1337,12 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > &lflow->dpg->dpg_uuid); > > if (!lflow->dpg->dp_group) { > - /* Ideally this should not happen. But it can still > happen > - * due to 2 reasons: > - * 1. There is a bug in the dp_group management. We > should > - * perhaps assert here. > - * 2. A User or CMS may delete the logical_dp_groups in > SB DB > - * or clear the SB:Logical_flow.logical_dp_groups > column > - * (intentionally or accidentally) > - * > - * Because of (2) it is better to return false instead of > - * assert,so that we recover from th inconsistent SB DB. > - */ > + /* The SB Logical_DP_Group row referenced by this > in-memory dp > + * group no longer exists. This can happen if a user or > CMS > + * deletes the SB:Logical_DP_Group rows or clears the > + * SB:Logical_Flow.logical_dp_group column (intentionally > or > + * accidentally). Release the stale dp group and fall > through > + * to create a fresh one. */ > static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(1, 1); > VLOG_WARN_RL(&rl, "SB Logical flow ["UUID_FMT"]'s " > "logical_dp_group column is not set " > @@ -1249,10 +1350,11 @@ sync_lflow_to_sb(struct ovn_lflow *lflow, > "referencing the dp group ["UUID_FMT"]", > UUID_ARGS(&sbflow->header_.uuid), > UUID_ARGS(&lflow->dpg->dpg_uuid)); > - lflow->sync_state = LFLOW_STALE; > - return false; > + ovn_dp_group_release(dp_groups, lflow->dpg); > + lflow->dpg = NULL; > Caught by AI: ovn_dp_group_get() can return an in-memory group shared by multiple lflows. In that case ovn_dp_group_release() only drops this lflow's reference, leaving the stale group in dp_groups, and ovn_dp_group_create() inserts another group with the same bitmap. Subsequent lflows can move to the new group, but sync_lflow_to_sb() only increments the new group at the end; it does not release their pre_sync_dpg references. The stale group and its reference counts can therefore remain indefinitely. > } > - } else { > + } > + if (!lflow->dpg) { > lflow->dpg = ovn_dp_group_create( > ovnsb_txn, dp_groups, sbrec_dp_group, > &lflow->dpg_bitmap, > diff --git a/northd/lflow-mgr.h b/northd/lflow-mgr.h > index 84d0b3e67..e924ffc22 100644 > --- a/northd/lflow-mgr.h > +++ b/northd/lflow-mgr.h > @@ -58,13 +58,7 @@ 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 *); > -bool lflow_ref_resync_flows(struct lflow_ref *, > - struct lflow_table *lflow_table, > - struct ovsdb_idl_txn *ovnsb_txn, > - const struct ovn_synced_datapaths dps[DP_MAX], > - bool ovn_internal_version_changed, > - const struct sbrec_logical_flow_table *, > - const struct sbrec_logical_dp_group_table *); > +void lflow_ref_unlink_and_prune(struct lflow_ref *, struct lflow_table *); > bool lflow_ref_sync_lflows(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 47f76b108..fbc0068b7 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -20965,10 +20965,9 @@ void run_update_worker_pool(int n_threads) > } > } > > -/* Updates the Logical_Flow and Multicast_Group tables in the OVN_SB > database, > - * constructing their contents based on the OVN_NB database. */ > -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > - struct lflow_input *input_data, > +/* Builds the in-memory logical flow table from the OVN_NB database. > + * The flows are synced to the SB database by en_dp_group_resolved. */ > +void build_lflows(struct lflow_input *input_data, > struct lflow_table *lflows) > { > struct svc_monitors_map_data svc_mons_data = > @@ -21012,14 +21011,6 @@ void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > /* Parallel build may result in a suboptimal hash. Resize the > * lflow map to a correct size before doing lookups */ > lflow_table_expand(lflows); > - > - stopwatch_start(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); > - lflow_table_sync_to_sb(lflows, ovnsb_txn, input_data->dps, > - input_data->ovn_internal_version_changed, > - input_data->sbrec_logical_flow_table, > - input_data->sbrec_logical_dp_group_table); > - > - stopwatch_stop(LFLOWS_TO_SB_STOPWATCH_NAME, time_msec()); > } > > void > @@ -21064,24 +21055,17 @@ lflow_reset_northd_refs(struct lflow_input > *lflow_input) > } > } > > -bool > -lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct tracked_dps *tracked_lrs, > - struct lflow_input *lflow_input, > - struct lflow_table *lflows) > +void > +lflow_handle_northd_lr_changes(struct tracked_dps *tracked_lrs, > + struct lflow_input *lflow_input, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs) > { > - bool handled = true; > struct hmapx_node *hmapx_node; > HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->deleted) { > struct ovn_datapath *od = hmapx_node->data; > - handled = lflow_ref_resync_flows( > - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - return handled; > - } > + lflow_ref_unlink_lflows(od->datapath_lflows); > + hmapx_add(dirty_lflow_refs, od->datapath_lflows); > } > > struct lswitch_flow_build_info lsi = { > @@ -21099,34 +21083,18 @@ lflow_handle_northd_lr_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > lflow_ref_unlink_lflows(od->datapath_lflows); > build_lswitch_and_lrouter_iterate_by_lr(od, &lsi); > - } > - > - /* We need to make sure that all datapath groups are allocated before > - * trying to sync logical flows. Otherwise, we would need to recompute > - * those datapath groups within those flows over and over again. */ > - HMAPX_FOR_EACH (hmapx_node, &tracked_lrs->crupdated) { > - struct ovn_datapath *od = hmapx_node->data; > - > - handled = lflow_ref_sync_lflows( > - od->datapath_lflows, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - break; > - } > + hmapx_add(dirty_lflow_refs, od->datapath_lflows); > } > > ds_destroy(&lsi.actions); > ds_destroy(&lsi.match); > - return handled; > } > > -bool > -lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct tracked_ovn_ports *trk_lsps, > +void > +lflow_handle_northd_port_changes(struct tracked_ovn_ports *trk_lsps, > struct lflow_input *lflow_input, > - struct lflow_table *lflows) > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs) > { > struct hmapx_node *hmapx_node; > struct ovn_port *op; > @@ -21135,14 +21103,8 @@ lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > op = hmapx_node->data; > /* Make sure 'op' is an lsp and not lrp. */ > ovs_assert(op->nbsp); > - bool handled = lflow_ref_resync_flows( > - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - return false; > - } > + lflow_ref_unlink_lflows(op->lflow_ref); > + hmapx_add(dirty_lflow_refs, op->lflow_ref); > /* No need to update SB multicast groups, thanks to weak > * references. */ > } > @@ -21163,33 +21125,17 @@ lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > &match, &actions, > > lflow_input->svc_monitor_mac, > lflows); > - /* Sync the new flows to SB. */ > - bool handled = lflow_ref_sync_lflows( > - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (handled) { > - /* Now regenerate the stateful lflows for 'op' */ > - /* Clear old lflows. */ > - lflow_ref_unlink_lflows(op->stateful_lflow_ref); > - build_lbnat_lflows_iterate_by_lsp(op, > - > lflow_input->lr_stateful_table, > - &match, &actions, lflows); > - handled = lflow_ref_sync_lflows( > - op->stateful_lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - } > + hmapx_add(dirty_lflow_refs, op->lflow_ref); > + > + /* Now regenerate the stateful lflows for 'op' */ > + lflow_ref_unlink_lflows(op->stateful_lflow_ref); > + build_lbnat_lflows_iterate_by_lsp(op, > + lflow_input->lr_stateful_table, > + &match, &actions, lflows); > + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); > > ds_destroy(&match); > ds_destroy(&actions); > - > - if (!handled) { > - return false; > - } > } > > HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { > @@ -21205,42 +21151,24 @@ lflow_handle_northd_port_changes(struct > ovsdb_idl_txn *ovnsb_txn, > &match, &actions, > > lflow_input->svc_monitor_mac, > lflows); > + hmapx_add(dirty_lflow_refs, op->lflow_ref); > > - /* Sync the newly added flows to SB. */ > - bool handled = lflow_ref_sync_lflows( > - op->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (handled) { > - /* Now generate the stateful lflows for 'op' */ > - build_lbnat_lflows_iterate_by_lsp(op, > - > lflow_input->lr_stateful_table, > - &match, &actions, lflows); > - handled = lflow_ref_sync_lflows( > - op->stateful_lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - } > + /* Now generate the stateful lflows for 'op' */ > + build_lbnat_lflows_iterate_by_lsp(op, > + lflow_input->lr_stateful_table, > + &match, &actions, lflows); > + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); > > ds_destroy(&match); > ds_destroy(&actions); > - > - if (!handled) { > - return false; > - } > } > - > - return true; > } > > -bool > -lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct tracked_lbs *trk_lbs, > +void > +lflow_handle_northd_lb_changes(struct tracked_lbs *trk_lbs, > struct lflow_input *lflow_input, > - struct lflow_table *lflows) > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs) > { > struct ovn_lb_datapaths *lb_dps; > struct hmapx_node *hmapx_node; > @@ -21252,12 +21180,8 @@ lflow_handle_northd_lb_changes(struct > ovsdb_idl_txn *ovnsb_txn, > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->deleted) { > lb_dps = hmapx_node->data; > - > - lflow_ref_resync_flows( > - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > + lflow_ref_unlink_lflows(lb_dps->lflow_ref); > + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); > } > > HMAPX_FOR_EACH (hmapx_node, &trk_lbs->crupdated) { > @@ -21291,31 +21215,20 @@ lflow_handle_northd_lb_changes(struct > ovsdb_idl_txn *ovnsb_txn, > ds_destroy(&match); > ds_destroy(&actions); > > - /* Sync the new flows to SB. */ > - bool handled = lflow_ref_sync_lflows( > - lb_dps->lflow_ref, lflows, ovnsb_txn, lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - return false; > - } > + hmapx_add(dirty_lflow_refs, lb_dps->lflow_ref); > } > - > - return true; > } > > -bool > -lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct lr_stateful_tracked_data *trk_data, > - struct lflow_input *lflow_input, > - struct lflow_table *lflows) > +void > +lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data > *trk_data, > + struct lflow_input *lflow_input, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs) > { > struct lr_stateful_record *lr_stateful_rec; > struct ds actions = DS_EMPTY_INITIALIZER; > struct ds match = DS_EMPTY_INITIALIZER; > struct hmapx_node *hmapx_node; > - bool handled = true; > > HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { > lr_stateful_rec = hmapx_node->data; > @@ -21328,17 +21241,7 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > &match, &actions, > lflow_input->meter_groups, > lflow_input->features); > - > - /* Sync the new flows to SB. */ > - handled = lflow_ref_sync_lflows( > - lr_stateful_rec->lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - goto exit; > - } > + hmapx_add(dirty_lflow_refs, lr_stateful_rec->lflow_ref); > > const struct ovn_datapath *od = > ovn_datapaths_find_by_index(lflow_input->lr_datapaths, > @@ -21353,16 +21256,7 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > lflow_input->bfd_ports, > &match, &actions, > lflows); > - > - handled = lflow_ref_sync_lflows( > - op->stateful_lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - goto exit; > - } > + hmapx_add(dirty_lflow_refs, op->stateful_lflow_ref); > > if (op->peer && op->peer->nbsp) { > lflow_ref_unlink_lflows(op->peer->stateful_lflow_ref); > @@ -21370,32 +21264,20 @@ lflow_handle_lr_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > build_lbnat_lflows_iterate_by_lsp( > op->peer, lflow_input->lr_stateful_table, &match, > &actions, > lflows); > - > - handled = lflow_ref_sync_lflows( > - op->peer->stateful_lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - goto exit; > - } > + hmapx_add(dirty_lflow_refs, op->peer->stateful_lflow_ref); > } > } > } > > -exit: > ds_destroy(&match); > ds_destroy(&actions); > - > - return handled; > } > > -bool > -lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct ls_stateful_tracked_data *trk_data, > - struct lflow_input *lflow_input, > - struct lflow_table *lflows) > +void > +lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data > *trk_data, > + struct lflow_input *lflow_input, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs) > { > struct hmapx_node *hmapx_node; > > @@ -21420,39 +21302,14 @@ lflow_handle_ls_stateful_changes(struct > ovsdb_idl_txn *ovnsb_txn, > build_network_function(od, lflows, > lflow_input->ls_port_groups, > ls_stateful_rec->lflow_ref); > - } > - > - /* We need to make sure that all datapath groups are allocated before > - * trying to sync logical flows. Otherwise, we would need to recompute > - * those datapath groups within those flows over and over again. */ > - HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { > - struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; > - /* Sync the new flows to SB. */ > - bool handled = lflow_ref_sync_lflows( > - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table); > - if (!handled) { > - return false; > - } > + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); > } > > HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { > struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; > - > - if (!lflow_ref_resync_flows( > - ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, > - lflow_input->dps, > - lflow_input->ovn_internal_version_changed, > - lflow_input->sbrec_logical_flow_table, > - lflow_input->sbrec_logical_dp_group_table)) { > - return false; > - } > + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); > + hmapx_add(dirty_lflow_refs, ls_stateful_rec->lflow_ref); > } > - > - return true; > } > > static bool > diff --git a/northd/northd.h b/northd/northd.h > index 2e3a9e00d..df2189c97 100644 > --- a/northd/northd.h > +++ b/northd/northd.h > @@ -260,9 +260,7 @@ struct lflow_ref; > struct lr_nat_table; > > struct lflow_input { > - /* Southbound table references */ > - const struct sbrec_logical_flow_table *sbrec_logical_flow_table; > - const struct sbrec_logical_dp_group_table > *sbrec_logical_dp_group_table; > + /* Southbound table references (used during lflow computation). */ > const struct sbrec_acl_id_table *sbrec_acl_id_table; > > /* Indexes */ > @@ -979,8 +977,7 @@ struct lr_stateful_tracked_data; > struct ls_stateful_tracked_data; > struct group_ecmp_datapath; > > -void build_lflows(struct ovsdb_idl_txn *ovnsb_txn, > - struct lflow_input *input_data, > +void build_lflows(struct lflow_input *input_data, > struct lflow_table *); > void lflow_reset_northd_refs(struct lflow_input *); > void build_route_data_flows_for_lrouter( > @@ -988,26 +985,26 @@ void build_route_data_flows_for_lrouter( > const struct group_ecmp_datapath *route_node, > const struct sset *bfd_ports); > > -bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsh_txn, > - struct tracked_dps *, > - struct lflow_input *, > - struct lflow_table *lflows); > -bool lflow_handle_northd_port_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct tracked_ovn_ports *, > +void lflow_handle_northd_lr_changes(struct tracked_dps *, > + struct lflow_input *, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs); > +void lflow_handle_northd_port_changes(struct tracked_ovn_ports *, > struct lflow_input *, > - struct lflow_table *lflows); > -bool lflow_handle_northd_lb_changes(struct ovsdb_idl_txn *ovnsb_txn, > - struct tracked_lbs *, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs); > +void lflow_handle_northd_lb_changes(struct tracked_lbs *, > struct lflow_input *, > - struct lflow_table *lflows); > -bool lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *, > - struct lr_stateful_tracked_data *, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs); > +void lflow_handle_lr_stateful_changes(struct lr_stateful_tracked_data *, > struct lflow_input *, > - struct lflow_table *lflows); > -bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, > - struct ls_stateful_tracked_data *, > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs); > +void lflow_handle_ls_stateful_changes(struct ls_stateful_tracked_data *, > struct lflow_input *, > - struct lflow_table *lflows); > + struct lflow_table *lflows, > + struct hmapx *dirty_lflow_refs); > bool northd_handle_sb_port_binding_changes( > const struct sbrec_port_binding_table *, struct hmap *ls_ports, > struct hmap *lr_ports); > diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ > ovn-inc-proc-graph-dump.at > index 44bf5689e..af951da8d 100644 > --- a/tests/ovn-inc-proc-graph-dump.at > +++ b/tests/ovn-inc-proc-graph-dump.at > @@ -143,7 +143,6 @@ digraph "Incremental-Processing-Engine" { > sync_meters -> sync_to_sb [[label=""]]; > sync_to_sb_lb -> sync_to_sb [[label=""]]; > sync_to_sb_pb -> sync_to_sb [[label=""]]; > - SB_logical_flow [[style=filled, shape=box, fillcolor=white, > label="SB_logical_flow"]]; > SB_multicast_group [[style=filled, shape=box, fillcolor=white, > label="SB_multicast_group"]]; > NB_bfd [[style=filled, shape=box, fillcolor=white, > label="NB_bfd"]]; > SB_bfd [[style=filled, shape=box, fillcolor=white, > label="SB_bfd"]]; > @@ -186,9 +185,7 @@ digraph "Incremental-Processing-Engine" { > SB_igmp_group -> multicast_igmp [[label=""]]; > lflow [[style=filled, shape=box, fillcolor=white, label="lflow"]]; > sync_meters -> lflow [[label=""]]; > - SB_logical_flow -> lflow [[label=""]]; > SB_multicast_group -> lflow [[label=""]]; > - SB_logical_dp_group -> lflow [[label=""]]; > bfd_sync -> lflow [[label=""]]; > route_policies -> lflow [[label=""]]; > routes -> lflow [[label="engine_noop_handler"]]; > @@ -203,6 +200,13 @@ digraph "Incremental-Processing-Engine" { > multicast_igmp -> lflow [[label="lflow_multicast_igmp_handler"]]; > SB_acl_id -> lflow [[label=""]]; > ic_learned_svc_monitors -> lflow > [[label="lflow_ic_learned_svc_mons_handler"]]; > + SB_logical_flow [[style=filled, shape=box, fillcolor=white, > label="SB_logical_flow"]]; > + dp_group_resolved [[style=filled, shape=box, fillcolor=white, > label="dp_group_resolved"]]; > + lflow -> dp_group_resolved > [[label="dp_group_resolved_lflow_handler"]]; > + SB_logical_flow -> dp_group_resolved [[label=""]]; > + SB_logical_dp_group -> dp_group_resolved [[label=""]]; > + datapath_sync -> dp_group_resolved [[label="engine_noop_handler"]]; > + global_config -> dp_group_resolved [[label="engine_noop_handler"]]; > mac_binding_aging_waker [[style=filled, shape=box, > fillcolor=white, label="mac_binding_aging_waker"]]; > mac_binding_aging [[style=filled, shape=box, fillcolor=white, > label="mac_binding_aging"]]; > SB_mac_binding -> mac_binding_aging [[label=""]]; > @@ -237,7 +241,7 @@ digraph "Incremental-Processing-Engine" { > acl_id -> northd_output [[label="northd_output_acl_id_handler"]]; > sync_from_sb -> northd_output [[label=""]]; > sync_to_sb -> northd_output > [[label="northd_output_sync_to_sb_handler"]]; > - lflow -> northd_output [[label="northd_output_lflow_handler"]]; > + dp_group_resolved -> northd_output > [[label="northd_output_dp_group_resolved_handler"]]; > mac_binding_aging -> northd_output > [[label="northd_output_mac_binding_aging_handler"]]; > fdb_aging -> northd_output > [[label="northd_output_fdb_aging_handler"]]; > ecmp_nexthop -> northd_output > [[label="northd_output_ecmp_nexthop_handler"]]; > diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at > index d19978a62..2a070901b 100644 > --- a/tests/ovn-northd.at > +++ b/tests/ovn-northd.at > @@ -13266,17 +13266,7 @@ check ovn-nbctl --wait=sb set load_balancer lb1 > options:bar=foo > check_engine_stats lflow norecompute compute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > -# Clear the SB:Logical_Flow.logical_dp_groups column of all the > -# logical flows and then modify the NB:Load_balancer. ovn-northd > -# should resync the logical flows. > -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) > -do > - check ovn-sbctl clear logical_flow $l logical_dp_group > -done > - > -check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > Why was this removed? We should still check if the resync happens in those cases. check ovn-nbctl --wait=sb set load_balancer lb1 options:foo=bar > -check_engine_stats lflow recompute nocompute > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && > ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') > @@ -13445,16 +13435,7 @@ AT_CHECK([echo $dpgrp_dps | grep $sw3_uuid], [0], > [ignore]) > AT_CHECK([echo $dpgrp_dps | grep $sw4_uuid], [0], [ignore]) > AT_CHECK([echo $dpgrp_dps | grep $sw5_uuid], [0], [ignore]) > > -# Clear the SB:Logical_Flow.logical_dp_groups column of all the > -# logical flows and then modify the NB:Load_balancer. ovn-northd > -# should resync the logical flows. > -for l in $(ovn-sbctl --bare --columns _uuid list logical_flow) > -do > - check ovn-sbctl clear logical_flow $l logical_dp_group > -done > - > check ovn-nbctl --wait=sb set load_balancer lb2 vips='{"10.0.0.10:80"=" > 10.0.0.3:80,10.0.0.4:80"}' > -check_engine_stats lflow recompute compute Same here. > CHECK_NO_CHANGE_AFTER_RECOMPUTE > > lb_lflow_uuid=$(fetch_column Logical_flow _uuid match='"ct.new && > ip4.dst == 10.0.0.10 && reg1[[16..23]] == 6 && reg1[[0..15]] == 80"') > @@ -23883,3 +23864,65 @@ AT_CHECK([as northd ovn-appctl -t ovn-northd > inc-engine/enable-stopwatch nonexis > OVN_CLEANUP_NORTHD > AT_CLEANUP > ]) > + > +AT_SETUP([Datapath group SB writes only when bitmap changes]) > +ovn_start > + > +# Create three switches, each with an ACL so they share the > +# "has ACLs" lflows in the same datapath group {sw0, sw1, sw2}. > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl ls-add sw1 > +check ovn-nbctl ls-add sw2 > +check ovn-nbctl lsp-add sw0 sw0-p0 > +check ovn-nbctl lsp-add sw1 sw1-p0 > +check ovn-nbctl lsp-add sw2 sw2-p0 > +check ovn-nbctl acl-add sw0 to-lport 1000 ip4 allow > +check ovn-nbctl acl-add sw1 to-lport 1000 ip6 allow > +check ovn-nbctl --wait=sb acl-add sw2 to-lport 1000 tcp allow > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +# Record dp_group state (UUIDs + content). > +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ > + | sort > dpg_before > + > +# ---------------------------------------------------------- > +# Case 1: Change that triggers incremental lflow compute but > +# does NOT alter the dp-group bitmap. > +# > +# Setting a port address rebuilds the port's lflows but the > +# set of datapaths that share the "has ACLs" lflows is still > +# {sw0, sw1, sw2}. The dp_group rows should not change. > +# ---------------------------------------------------------- > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +check ovn-nbctl --wait=sb lsp-set-addresses sw0-p0 "00:00:00:00:00:01" > +check_engine_stats lflow norecompute compute > + > +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ > + | sort > dpg_after > +AT_CHECK([diff dpg_before dpg_after]) > + > +# ---------------------------------------------------------- > +# Case 2: Change that removes a datapath from a shared flow, > +# altering the dp-group bitmap. > +# > +# Move sw0's ACL to tier 1. The egress "acl action" flows > +# now differ for sw0, so the {sw0, sw1, sw2} group loses sw0 > +# and a {sw1, sw2} group appears (or the existing one is > +# updated). The dp_group rows must change. > +# ---------------------------------------------------------- > +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats > +acl0=$(fetch_column nb:ACL _uuid match=ip4) > +check ovn-nbctl --wait=sb set ACL $acl0 tier=1 > +check_engine_stats lflow norecompute compute > + > +ovn-sbctl --bare --columns=_uuid,datapaths list Logical_DP_Group \ > + | sort > dpg_changed > +AT_CHECK([diff dpg_before dpg_changed], [1], [ignore]) > + > +check ovn-nbctl --wait=sb ls-del sw0 > + > +CHECK_NO_CHANGE_AFTER_RECOMPUTE > + > +OVN_CLEANUP_NORTHD > +AT_CLEANUP > -- > 2.55.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
