Okay I see what I didn't understand. The deleted switches ls_stateful_record's are taken specifically from the ls_sful_trk hmapx but for created/updated the ls_stateful_record is found from the lflow_input->ls_stateful_table. So you are fully processesing the ls_stateful entries
On Wed, Aug 12, 2026 at 10:17 AM Jacob Tanenbaum <[email protected]> wrote: > > > On Fri, Jul 24, 2026 at 12:41 PM Lucas Vargas Dias > <[email protected]> wrote: > >> Until now, creating or deleting a logical switch forced a full recompute >> of the en_lflow engine node. Handle it incrementally instead, mirroring >> the existing logical router datapath handling. >> >> Add lflow_handle_northd_ls_changes(), invoked from lflow_northd_handler() >> for the tracked switch changes: deleted switches have their datapath >> flows resynced away via od->datapath_lflows, while created/updated >> switches are (re)built with build_lswitch_and_lrouter_iterate_by_ls() and >> synced. To make this possible the datapath-wide switch flows built there >> are now anchored to od->datapath_lflows instead of being untracked (NULL >> lflow_ref), so they can be generated and torn down incrementally. >> >> Two of those flows have a different owner and are handled accordingly: >> >> - The network function flows are moved into build_ls_stateful_flows() >> so they are owned by the per-switch ls_stateful lflow_ref (which is >> already processed incrementally for switch add/delete). The now >> redundant build_network_function() calls in the ls_stateful and port >> change handlers are removed. >> >> - The multicast flood flow is owned by the multicast_igmp node's >> lflow_ref (built in build_igmp_lflows()). >> multicast_igmp_northd_handler() >> now recomputes that (cheap) node when switches are created/deleted so >> the flood flows are added/removed for them. >> >> Update the "Logical switch incremental processing" test: adding and >> deleting a logical switch no longer recomputes the lflow node. >> >> Assisted-bt: Claude Opus 4.8, Claude Code >> > > nit: Assisted-by > > >> Signed-off-by: Lucas Vargas Dias <[email protected]> >> --- >> northd/en-lflow.c | 23 ++++- >> northd/en-multicast.c | 9 ++ >> northd/northd.c | 191 ++++++++++++++++++++++++++++++++++++------ >> northd/northd.h | 5 ++ >> tests/ovn-northd.at | 4 +- >> 5 files changed, 201 insertions(+), 31 deletions(-) >> >> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >> index 99df5f08f..9a517ae48 100644 >> --- a/northd/en-lflow.c >> +++ b/northd/en-lflow.c >> @@ -145,16 +145,31 @@ lflow_northd_handler(struct engine_node *node, >> return EN_UNHANDLED; >> } >> >> - if (northd_has_lswitches_in_tracked_data(&northd_data->trk_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); >> >> + /* The switch datapath handler also (re)builds the per-switch >> ls_stateful >> + * flows of created/deleted switches, coordinated with their >> + * datapath_lflows so that shared datapath groups are updated in >> place >> + * rather than churned. >> + * It therefore needs the ls_stateful tracked data. */ >> + struct ed_type_ls_stateful *ls_stateful_data = >> + engine_get_input_data("ls_stateful", node); >> + struct ls_stateful_tracked_data *ls_sful_trk = >> + ls_stateful_has_tracked_data(&ls_stateful_data->trk_data) >> + ? &ls_stateful_data->trk_data : NULL; >> + >> + if (!lflow_handle_northd_ls_changes(eng_ctx->ovnsb_idl_txn, >> + >> &northd_data->trk_data.trk_switches, >> + ls_sful_trk, >> + &lflow_input, >> + lflow_data->lflow_table)) { >> + return EN_UNHANDLED; >> + } >> + >> if (!lflow_handle_northd_lr_changes(eng_ctx->ovnsb_idl_txn, >> >> &northd_data->trk_data.trk_routers, >> &lflow_input, >> diff --git a/northd/en-multicast.c b/northd/en-multicast.c >> index bfe3c4d92..c8167b479 100644 >> --- a/northd/en-multicast.c >> +++ b/northd/en-multicast.c >> @@ -153,6 +153,15 @@ multicast_igmp_northd_handler(struct engine_node >> *node, void *data OVS_UNUSED) >> return EN_UNHANDLED; >> } >> >> + /* A created/deleted logical switch owns per-datapath multicast flood >> + * flows (build_mcast_flood_lswitch() via build_igmp_lflows()). The >> lflow >> + * node now processes switch datapaths incrementally, so it no longer >> + * forces a full recompute; recompute this (cheap) node so its >> lflow_ref >> + * picks up (or drops) the switch's flood flows. */ >> + if (northd_has_lswitches_in_tracked_data(&northd_data->trk_data)) { >> + return EN_UNHANDLED; >> + } >> + >> struct tracked_ovn_ports *trk_lsps = &northd_data->trk_data.trk_lsps; >> if (hmapx_count(&trk_lsps->created) || >> hmapx_count(&trk_lsps->updated) || >> diff --git a/northd/northd.c b/northd/northd.c >> index eca5d6f0e..9ecc072f3 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -6962,10 +6962,11 @@ enum mirror_filter { >> >> static void >> build_mirror_default_lflow(struct ovn_datapath *od, >> - struct lflow_table *lflows) >> + struct lflow_table *lflows, >> + struct lflow_ref *lflow_ref) >> { >> - ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", NULL); >> - ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", >> NULL); >> + ovn_lflow_add(lflows, od, S_SWITCH_IN_MIRROR, 0, "1", "next;", >> lflow_ref); >> + ovn_lflow_add(lflows, od, S_SWITCH_OUT_MIRROR, 0, "1", "next;", >> lflow_ref); >> } >> >> static void >> @@ -19912,6 +19913,11 @@ build_lr_stateful_flows(const struct >> lr_stateful_record *lr_stateful_rec, >> lr_stateful_rec->lflow_ref); >> } >> >> +static void build_network_function(const struct ovn_datapath *od, >> + struct lflow_table *lflows, >> + const struct ls_port_group_table >> *ls_pgs, >> + struct lflow_ref *lflow_ref); >> + >> > > I think moving the build_network_function() and its helpers above > build_ls_stateful_flows() would make it cleaner and avoid the forward > decleration > > >> static void >> build_ls_stateful_flows(const struct ls_stateful_record *ls_stateful_rec, >> const struct ovn_datapath *od, >> @@ -19943,6 +19949,12 @@ build_ls_stateful_flows(const struct >> ls_stateful_record *ls_stateful_rec, >> } >> >> build_lb_hairpin(ls_stateful_rec, od, lflows, >> ls_stateful_rec->lflow_ref); >> + >> + /* Network function flows are datapath-wide but owned by the >> per-switch >> + * ls_stateful lflow_ref, so that they are (re)generated and torn >> down >> + * together with the rest of the switch's stateful flows during >> + * incremental processing. */ >> + build_network_function(od, lflows, ls_pgs, >> ls_stateful_rec->lflow_ref); >> } >> >> /* For packets received on tunnel and egressing towards a >> network-function port >> @@ -20764,33 +20776,41 @@ build_lswitch_and_lrouter_iterate_by_ls(struct >> ovn_datapath *od, >> struct lswitch_flow_build_info >> *lsi) >> { >> ovs_assert(od->nbs); >> - build_mirror_default_lflow(od, lsi->lflows); >> + build_mirror_default_lflow(od, lsi->lflows, od->datapath_lflows); >> build_lswitch_lflows_pre_acl_and_acl(od, lsi->lflows, >> - lsi->meter_groups, NULL); >> - build_network_function(od, lsi->lflows, lsi->ls_port_groups, NULL); >> - build_fwd_group_lflows(od, lsi->lflows, NULL); >> - build_lswitch_lflows_admission_control(od, lsi->lflows, NULL); >> - build_lswitch_learn_fdb_od(od, lsi->lflows, NULL); >> + lsi->meter_groups, >> + od->datapath_lflows); >> + build_fwd_group_lflows(od, lsi->lflows, od->datapath_lflows); >> + build_lswitch_lflows_admission_control(od, lsi->lflows, >> + od->datapath_lflows); >> + build_lswitch_learn_fdb_od(od, lsi->lflows, od->datapath_lflows); >> build_lswitch_arp_nd_evpn_responder(od, lsi->lflows, >> lsi->meter_groups, >> - NULL); >> - build_lswitch_arp_nd_responder_default(od, lsi->lflows, NULL); >> + od->datapath_lflows); >> + build_lswitch_arp_nd_responder_default(od, lsi->lflows, >> + od->datapath_lflows); >> build_lswitch_dns_lookup_and_response(od, lsi->lflows, >> lsi->meter_groups, >> - NULL); >> - build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, NULL); >> + od->datapath_lflows); >> + build_lswitch_dhcp_and_dns_defaults(od, lsi->lflows, >> od->datapath_lflows); >> build_lswitch_destination_lookup_bmcast(od, lsi->lflows, >> &lsi->actions, >> - lsi->meter_groups, NULL); >> - build_lswitch_output_port_sec_od(od, lsi->lflows, NULL); >> + lsi->meter_groups, >> + od->datapath_lflows); >> + build_lswitch_output_port_sec_od(od, lsi->lflows, >> od->datapath_lflows); >> /* CT extraction flows are built with stateful flows, but default >> rule is >> * always needed */ >> ovn_lflow_add(lsi->lflows, od, S_SWITCH_IN_CT_EXTRACT, 0, "1", >> "next;", >> - NULL); >> - build_lswitch_lb_affinity_default_flows(od, lsi->lflows, NULL); >> + od->datapath_lflows); >> + build_lswitch_lb_affinity_default_flows(od, lsi->lflows, >> + od->datapath_lflows); >> if (od->has_evpn_vni) { >> - build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, NULL); >> + build_lswitch_lflows_evpn_l2_unknown(od, lsi->lflows, >> + od->datapath_lflows); >> } else { >> - build_lswitch_lflows_l2_unknown(od, lsi->lflows, NULL); >> + build_lswitch_lflows_l2_unknown(od, lsi->lflows, >> od->datapath_lflows); >> } >> - build_mcast_flood_lswitch(od, lsi->lflows, &lsi->actions, NULL); >> + /* build_network_function() flows are owned by the per-switch >> ls_stateful >> + * lflow_ref (built in build_ls_stateful_flows()). The multicast >> flood >> + * flow (build_mcast_flood_lswitch()) is owned by the multicast_igmp >> + * node's lflow_ref (built in build_igmp_lflows()). */ >> } >> >> /* Helper function to combine all lflow generation which is iterated by >> @@ -21480,6 +21500,132 @@ lflow_reset_northd_refs(struct lflow_input >> *lflow_input) >> } >> } >> >> +bool >> +lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, >> + struct tracked_dps *tracked_lses, >> + struct ls_stateful_tracked_data >> *ls_sful_trk, >> + struct lflow_input *lflow_input, >> + struct lflow_table *lflows) >> +{ >> + bool handled = true; >> + struct hmapx_node *hmapx_node; >> + >> + struct lswitch_flow_build_info lsi = { >> + .ls_datapaths = lflow_input->ls_datapaths, >> + .ls_ports = lflow_input->ls_ports, >> + .ls_port_groups = lflow_input->ls_port_groups, >> + .meter_groups = lflow_input->meter_groups, >> + .features = lflow_input->features, >> + .lflows = lflows, >> + .match = DS_EMPTY_INITIALIZER, >> + .actions = DS_EMPTY_INITIALIZER, >> + }; >> + >> + /* A switch datapath's logical flows are split across two >> lflow_refs: the >> + * per-switch 'od->datapath_lflows' (built here, by_ls) and the >> per-switch >> + * ls_stateful lflow_ref (built by build_ls_stateful_flows()). When >> a >> + * switch datapath is added or removed, any datapath group shared by >> these >> + * flows gains or loses that datapath. To let ovn_dp_group_create() >> update >> + * the group's SB row in place (instead of deleting and re-creating >> it, >> + * which would churn the SB) the old group must be fully released >> before it >> + * is re-synced. That only happens if _all_ of the switch's flows >> -- from >> + * both refs -- are unlinked/rebuilt before _any_ of them is >> synced. So we >> + * do all the unlinking and building first, then sync. >> + * >> + * The ls_stateful records are also (re)processed by >> + * lflow_ls_stateful_handler(); doing it here as well is redundant >> but >> + * harmless (the group is already stable, so the second pass reuses >> it). */ >> > > I don't think this last sentance matches the code. Could it be changed to > something like: > "The ls_stateful records are fully processed by > lflow_ls_stateful_handler(); unlinking the flows for ls_stateful records > for deleted switches insures that we can correctly remove flows that are no > longer referenced. The ls_stateful records are fully processed by > lflow_ls_stateful_handler()" > > Something like that. Mention that only the deletion occurs here and why. > >> + >> + /* Phase 1: unlink (and, for created/updated switches, rebuild). */ >> + HMAPX_FOR_EACH (hmapx_node, &tracked_lses->deleted) { >> + struct ovn_datapath *od = hmapx_node->data; >> + lflow_ref_unlink_lflows(od->datapath_lflows); >> + } >> + if (ls_sful_trk) { >> + HMAPX_FOR_EACH (hmapx_node, &ls_sful_trk->deleted) { >> + struct ls_stateful_record *ls_stateful_rec = >> hmapx_node->data; >> + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); >> > + } >> + } >> + HMAPX_FOR_EACH (hmapx_node, &tracked_lses->crupdated) { >> + struct ovn_datapath *od = hmapx_node->data; >> + >> + lflow_ref_unlink_lflows(od->datapath_lflows); >> + build_lswitch_and_lrouter_iterate_by_ls(od, &lsi); >> + >> + const struct ls_stateful_record *ls_stateful_rec = >> + ls_stateful_table_find(lflow_input->ls_stateful_table, >> od->nbs); >> + if (ls_stateful_rec) { >> + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref); >> + build_ls_stateful_flows(ls_stateful_rec, od, >> + lflow_input->ls_port_groups, >> + lflow_input->meter_groups, >> + lflow_input->sampling_apps, >> + lflow_input->features, lflows, >> + lflow_input->sbrec_acl_id_table); >> + } >> + } >> + >> + /* Phase 2: sync. All datapath groups are now allocated, so this >> won't >> + * recompute the same groups over and over again. */ >> + HMAPX_FOR_EACH (hmapx_node, &tracked_lses->deleted) { >> + 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) { >> + goto out; >> + } >> + } >> + if (ls_sful_trk) { >> + HMAPX_FOR_EACH (hmapx_node, &ls_sful_trk->deleted) { >> + struct ls_stateful_record *ls_stateful_rec = >> hmapx_node->data; >> + 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) { >> + goto out; >> + } >> + } >> + } >> + HMAPX_FOR_EACH (hmapx_node, &tracked_lses->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) { >> + goto out; >> + } >> + >> + const struct ls_stateful_record *ls_stateful_rec = >> + ls_stateful_table_find(lflow_input->ls_stateful_table, >> od->nbs); >> + if (ls_stateful_rec) { >> + 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) { >> + goto out; >> + } >> + } >> + } >> + >> +out: >> + ds_destroy(&lsi.actions); >> + ds_destroy(&lsi.match); >> + return handled; >> +} >> + >> bool >> lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsb_txn, >> struct tracked_dps *tracked_lrs, >> @@ -21699,8 +21845,6 @@ lflow_handle_northd_port_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> lflow_input->features, >> lflows, >> lflow_input->sbrec_acl_id_table); >> - build_network_function(od, lflows, lflow_input->ls_port_groups, >> - ls_stateful_rec->lflow_ref); >> handled = lflow_ref_sync_lflows( >> ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, >> lflow_input->dps, >> @@ -22005,9 +22149,6 @@ lflow_handle_ls_stateful_changes(struct >> ovsdb_idl_txn *ovnsb_txn, >> lflow_input->features, >> lflows, >> lflow_input->sbrec_acl_id_table); >> - 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 >> diff --git a/northd/northd.h b/northd/northd.h >> index 61546bdd2..217f3c6fb 100644 >> --- a/northd/northd.h >> +++ b/northd/northd.h >> @@ -997,6 +997,11 @@ void build_route_data_flows_for_lrouter( >> const struct group_ecmp_datapath *route_node, >> const struct sset *bfd_ports); >> >> +bool lflow_handle_northd_ls_changes(struct ovsdb_idl_txn *ovnsb_txn, >> + struct tracked_dps *, >> + struct ls_stateful_tracked_data *, >> + struct lflow_input *, >> + struct lflow_table *lflows); >> bool lflow_handle_northd_lr_changes(struct ovsdb_idl_txn *ovnsh_txn, >> struct tracked_dps *, >> struct lflow_input *, >> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >> index 9a0ed2f01..eaaec17a2 100644 >> --- a/tests/ovn-northd.at >> +++ b/tests/ovn-northd.at >> @@ -16186,7 +16186,7 @@ check as northd ovn-appctl -t ovn-northd >> inc-engine/clear-stats >> check ovn-nbctl --wait=sb ls-add sw0 >> check_engine_stats northd norecompute compute >> check_engine_stats ls_stateful norecompute compute >> -check_engine_stats lflow recompute nocompute >> +check_engine_stats lflow norecompute compute >> >> # For the below engine nodes, en_northd is input. So check >> # their engine status. >> @@ -16224,7 +16224,7 @@ check as northd ovn-appctl -t ovn-northd >> inc-engine/clear-stats >> check ovn-nbctl --wait=sb ls-del sw0 >> check_engine_stats northd norecompute compute >> check_engine_stats ls_stateful norecompute compute >> -check_engine_stats lflow recompute nocompute >> +check_engine_stats lflow norecompute compute >> >> # For the below engine nodes, en_northd is input. So check >> # their engine status. >> -- >> 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
