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
