Hi, Em qua., 12 de ago. de 2026 às 11:34, Jacob Tanenbaum <[email protected]> escreveu:
> 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 > > I adjust in the next patch. > 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 >> > I agree > >> >>> 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 >> >> > I agree > 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. >> > You're right. I'll adjust. Regards, Lucas > + >>> + /* 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 >>> >>> -- _‘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
