On Tue, Jul 14, 2026 at 11:13 AM Jun Gu <[email protected]> wrote:
> The non_vif_data engine node tracks two unrelated things: the ofports of > patch (localnet / L2 gateway) ports and the tunnel ports with their > chassis information. The patch ofports are consumed only by the physical > flow output, while the tunnel data is consumed by both the physical and > the logical flow output. > > Because they share a single node and en_lflow_output has no handler for > its non_vif_data input, any patch port change forces a full recompute of > the logical flow output. This is hit on the datapath every time a > bridged / L2 logical port is bound or moved between chassis: the peer > patch port is added and gets its ofport assigned, invalidating > non_vif_data twice and triggering two full logical flow recomputes, even > though the logical flows do not depend on patch ofports at all. In > deployments with a large number of logical flows each such recompute can > take several seconds, dominating the port's dataplane downtime. > > Move the patch ofports into their own engine node (en_patch_port_data) > that feeds only the physical flow output. The logical flow output keeps > depending on en_non_vif_data, which now only changes on tunnel ofport > changes. A patch port bind / migration therefore no longer recomputes > the logical flows; only the (cheap) physical flow output is refreshed. > > Signed-off-by: Jun Gu <[email protected]> > Assisted-by: Claude Opus 4.8, Claude Code > nit: Signed-off-by should be after Assisted-by. > --- > Hi Jun, thank you for the patch I have a couple of small comments down below. > controller/local_data.c | 180 ++++++++++++++++++++----------- > controller/local_data.h | 22 ++-- > controller/ovn-controller.c | 94 +++++++++++++--- > tests/ovn-inc-proc-graph-dump.at | 5 + > 4 files changed, 219 insertions(+), 82 deletions(-) > > diff --git a/controller/local_data.c b/controller/local_data.c > index af6c75b40..d33bef47b 100644 > --- a/controller/local_data.c > +++ b/controller/local_data.c > @@ -452,14 +452,57 @@ tracked_datapaths_destroy(struct hmap > *tracked_datapaths) > hmap_destroy(tracked_datapaths); > } > > -/* Iterates the br_int ports and build the simap of patch to ofports > - * and chassis tunnels. */ > +/* Iterates the br_int ports and builds the simap of patch port to > ofport. */ > void > -local_nonvif_data_run(const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis_rec, > - struct simap *patch_ofports, > - struct hmap *chassis_tunnels, > - struct flow_based_tunnel *flow_tunnels) > +local_patch_ports_run(const struct ovsrec_bridge *br_int, > + struct simap *patch_ofports) > +{ > + for (int i = 0; i < br_int->n_ports; i++) { > Since we are chaning this: size_t. + const struct ovsrec_port *port_rec = br_int->ports[i]; > + if (!strcmp(port_rec->name, br_int->name)) { > + continue; > + } > + > + const char *localnet = smap_get(&port_rec->external_ids, > + "ovn-localnet-port"); > + const char *l2gateway = smap_get(&port_rec->external_ids, > + "ovn-l2gateway-port"); > + if (!localnet && !l2gateway) { > + continue; > + } > + > + for (int j = 0; j < port_rec->n_interfaces; j++) { > Since we are chaning this: size_t > + const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > + > + /* Get OpenFlow port number. */ > + if (!iface_rec->n_ofport) { > + continue; > + } > + int64_t ofport = iface_rec->ofport[0]; > + if (ofport < 1 || ofport > ofp_to_u16(OFPP_MAX)) { > + continue; > + } > + > + if (strcmp(iface_rec->type, "patch")) { > + continue; > + } > + if (localnet) { > + simap_put(patch_ofports, localnet, ofport); > + break; > + } else if (l2gateway) { > + /* L2 gateway patch ports can be handled just like VIFs. > */ > + simap_put(patch_ofports, l2gateway, ofport); > + break; > + } > + } > + } > +} > + > +void > +local_tunnels_run(const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis_rec, > + struct hmap *chassis_tunnels, > + struct flow_based_tunnel *flow_tunnels) > { > for (int i = 0; i < br_int->n_ports; i++) { > const struct ovsrec_port *port_rec = br_int->ports[i]; > @@ -477,10 +520,9 @@ local_nonvif_data_run(const struct ovsrec_bridge > *br_int, > > track_flow_based_tunnel(port_rec, chassis_rec, flow_tunnels); > > - const char *localnet = smap_get(&port_rec->external_ids, > - "ovn-localnet-port"); > - const char *l2gateway = smap_get(&port_rec->external_ids, > - "ovn-l2gateway-port"); > + if (!tunnel_id) { > + continue; > + } > > for (int j = 0; j < port_rec->n_interfaces; j++) { > const struct ovsrec_interface *iface_rec = > port_rec->interfaces[j]; > @@ -494,67 +536,63 @@ local_nonvif_data_run(const struct ovsrec_bridge > *br_int, > continue; > } > > - bool is_patch = !strcmp(iface_rec->type, "patch"); > - if (is_patch && localnet) { > - simap_put(patch_ofports, localnet, ofport); > - break; > - } else if (is_patch && l2gateway) { > - /* L2 gateway patch ports can be handled just like VIFs. > */ > - simap_put(patch_ofports, l2gateway, ofport); > - break; > - } else if (tunnel_id) { > - enum chassis_tunnel_type tunnel_type; > - if (!strcmp(iface_rec->type, "geneve")) { > - tunnel_type = GENEVE; > - } else if (!strcmp(iface_rec->type, "vxlan")) { > - tunnel_type = VXLAN; > - } else { > - continue; > - } > - > - /* We split the tunnel_id to get the chassis-id > - * and hash the tunnel list on the chassis-id. The > - * reason to use the chassis-id alone is because > - * there might be cases (multicast, gateway chassis) > - * where we need to tunnel to the chassis, but won't > - * have the encap-ip specifically. > - */ > - char *hash_id = NULL; > - char *ip = NULL; > - > - if (!encaps_tunnel_id_parse(tunnel_id, &hash_id, &ip, > NULL)) { > - continue; > - } > - struct chassis_tunnel *tun = xmalloc(sizeof *tun); > - hmap_insert(chassis_tunnels, &tun->hmap_node, > - hash_string(hash_id, 0)); > - tun->chassis_id = xstrdup(tunnel_id); > - tun->ofport = u16_to_ofp(ofport); > - tun->type = tunnel_type; > - tun->is_ipv6 = ip ? addr_is_ipv6(ip) : false; > - tun->is_ramp_tunnel = > is_ramp_tunnel(&iface_rec->other_config); > - > - free(hash_id); > - free(ip); > - break; > + enum chassis_tunnel_type tunnel_type; > + if (!strcmp(iface_rec->type, "geneve")) { > + tunnel_type = GENEVE; > + } else if (!strcmp(iface_rec->type, "vxlan")) { > + tunnel_type = VXLAN; > + } else { > + continue; > + } > + > + /* We split the tunnel_id to get the chassis-id > + * and hash the tunnel list on the chassis-id. The > + * reason to use the chassis-id alone is because > + * there might be cases (multicast, gateway chassis) > + * where we need to tunnel to the chassis, but won't > + * have the encap-ip specifically. > + */ > + char *hash_id = NULL; > + char *ip = NULL; > + > + if (!encaps_tunnel_id_parse(tunnel_id, &hash_id, &ip, NULL)) { > + continue; > } > + struct chassis_tunnel *tun = xmalloc(sizeof *tun); > + hmap_insert(chassis_tunnels, &tun->hmap_node, > + hash_string(hash_id, 0)); > + tun->chassis_id = xstrdup(tunnel_id); > + tun->ofport = u16_to_ofp(ofport); > + tun->type = tunnel_type; > + tun->is_ipv6 = ip ? addr_is_ipv6(ip) : false; > + tun->is_ramp_tunnel = > is_ramp_tunnel(&iface_rec->other_config); > + > + free(hash_id); > + free(ip); > + break; > } > } > } > > -bool > -local_nonvif_data_handle_ovs_iface_changes( > - const struct ovsrec_interface_table *iface_table) > +/* Returns false (i.e. a recompute is required) if any tracked OVS > interface of > + * one of the given 'types' had its ofport added, removed or changed. */ > +static bool > +nonvif_iface_ofport_unchanged(const struct ovsrec_interface_table > *iface_table, > + const char *const *types, size_t n_types) > { > const struct ovsrec_interface *iface_rec; > OVSREC_INTERFACE_TABLE_FOR_EACH_TRACKED (iface_rec, iface_table) { > - /* Check only patch ports or tunnels. */ > - if (strcmp(iface_rec->type, "geneve") && > - strcmp(iface_rec->type, "patch") && > - strcmp(iface_rec->type, "vxlan")) { > + bool type_match = false; > + for (size_t i = 0; i < n_types; i++) { > + if (!strcmp(iface_rec->type, types[i])) { > + type_match = true; > + break; > + } > + } > + if (!type_match) { > continue; > } > - /* We are interested only in ofport changes for this handler. */ > + /* We are interested only in ofport changes for these handlers. */ > if (ovsrec_interface_is_new(iface_rec) || > ovsrec_interface_is_deleted(iface_rec) || > ovsrec_interface_is_updated(iface_rec, > @@ -566,6 +604,24 @@ local_nonvif_data_handle_ovs_iface_changes( > return true; > } > > +bool > +local_patch_ports_handle_ovs_iface_changes( > + const struct ovsrec_interface_table *iface_table) > +{ > + static const char *const types[] = { "patch" }; > + return nonvif_iface_ofport_unchanged(iface_table, types, > + ARRAY_SIZE(types)); > +} > + > +bool > +local_tunnels_handle_ovs_iface_changes( > + const struct ovsrec_interface_table *iface_table) > +{ > + static const char *const types[] = { "geneve", "vxlan" }; > + return nonvif_iface_ofport_unchanged(iface_table, types, > + ARRAY_SIZE(types)); > +} > + > bool > get_chassis_tunnel_ofport(const struct hmap *chassis_tunnels, > const char *chassis_name, ofp_port_t *ofport) > diff --git a/controller/local_data.h b/controller/local_data.h > index cbb8899eb..2e0fee0ca 100644 > --- a/controller/local_data.h > +++ b/controller/local_data.h > @@ -159,13 +159,23 @@ struct flow_based_tunnel { > }; > > > -void local_nonvif_data_run(const struct ovsrec_bridge *br_int, > - const struct sbrec_chassis *chassis, > - struct simap *patch_ofports, > - struct hmap *chassis_tunnels, > - struct flow_based_tunnel *flow_tunnels); > +/* Patch (localnet / L2 gateway) OVS ports. These are consumed only by > the > + * physical flow output, so they are tracked in their own engine node to > avoid > + * invalidating the logical flow output on every patch port change. */ > +void local_patch_ports_run(const struct ovsrec_bridge *br_int, > + struct simap *patch_ofports); > > -bool local_nonvif_data_handle_ovs_iface_changes( > +bool local_patch_ports_handle_ovs_iface_changes( > + const struct ovsrec_interface_table *); > + > +/* Tunnel OVS ports and the related chassis information. These are > consumed by > + * both the physical and the logical flow output. */ > +void local_tunnels_run(const struct ovsrec_bridge *br_int, > + const struct sbrec_chassis *chassis, > + struct hmap *chassis_tunnels, > + struct flow_based_tunnel *flow_tunnels); > + > +bool local_tunnels_handle_ovs_iface_changes( > const struct ovsrec_interface_table *); > > struct chassis_tunnel *chassis_tunnel_find(const struct hmap > *chassis_tunnels, > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index 552e87b53..f4b60f0e8 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -3637,12 +3637,72 @@ en_dns_cache_cleanup(void *data OVS_UNUSED) > } > > > -/* Engine node which is used to handle the Non VIF data like > - * - OVS patch ports > - * - Tunnel ports and the related chassis information. > +/* Engine node which handles the OVS patch ports (localnet / L2 gateway). > + * > + * Kept separate from the tunnel data (en_non_vif_data) because patch > ofports > + * are consumed only by the physical flow output. Bundling them with the > + * tunnel data would force a full recompute of the logical flow output > (which > + * has no handler for its non_vif_data input) on every patch port change, > e.g. > + * whenever a bridged/L2 logical port is bound or moved between chassis. > */ > -struct ed_type_non_vif_data { > +struct ed_type_patch_port_data { > struct simap patch_ofports; /* simap of patch ovs ports. */ > +}; > + > +static void * > +en_patch_port_data_init(struct engine_node *node OVS_UNUSED, > + struct engine_arg *arg OVS_UNUSED) > +{ > + struct ed_type_patch_port_data *data = xzalloc(sizeof *data); > + simap_init(&data->patch_ofports); > + return data; > +} > + > +static void > +en_patch_port_data_cleanup(void *data OVS_UNUSED) > +{ > + struct ed_type_patch_port_data *ed_patch_port_data = data; > + simap_destroy(&ed_patch_port_data->patch_ofports); > +} > + > +static enum engine_node_state > +en_patch_port_data_run(struct engine_node *node, void *data) > +{ > + struct ed_type_patch_port_data *ed_patch_port_data = data; > + simap_destroy(&ed_patch_port_data->patch_ofports); > + simap_init(&ed_patch_port_data->patch_ofports); > + > + const struct ovsrec_open_vswitch_table *ovs_table = > + EN_OVSDB_GET(engine_get_input("OVS_open_vswitch", node)); > + const struct ovsrec_bridge_table *bridge_table = > + EN_OVSDB_GET(engine_get_input("OVS_bridge", node)); > + > + const struct ovsrec_bridge *br_int = get_br_int(bridge_table, > ovs_table); > + ovs_assert(br_int); > + > + local_patch_ports_run(br_int, &ed_patch_port_data->patch_ofports); > + > + return EN_UPDATED; > +} > + > +static enum engine_input_handler_result > +patch_port_data_ovs_iface_handler(struct engine_node *node, > + void *data OVS_UNUSED) > +{ > + const struct ovsrec_interface_table *iface_table = > + EN_OVSDB_GET(engine_get_input("OVS_interface", node)); > + > + if (local_patch_ports_handle_ovs_iface_changes(iface_table)) { > + return EN_HANDLED_UNCHANGED; > + } else { > + return EN_UNHANDLED; > + } > +} > + > +/* Engine node which handles the tunnel ports and the related chassis > + * information. Consumed by both the physical and the logical flow > output. > + */ > +struct ed_type_non_vif_data { > struct hmap chassis_tunnels; /* hmap of 'struct chassis_tunnel' from > the > * tunnel OVS ports. */ > struct flow_based_tunnel flow_tunnels[TUNNEL_TYPE_MAX]; > @@ -3656,7 +3716,6 @@ en_non_vif_data_init(struct engine_node *node > OVS_UNUSED, > struct engine_arg *arg OVS_UNUSED) > { > struct ed_type_non_vif_data *data = xzalloc(sizeof *data); > - simap_init(&data->patch_ofports); > hmap_init(&data->chassis_tunnels); > flow_based_tunnels_init(data->flow_tunnels); > data->use_flow_based_tunnels = false; > @@ -3667,7 +3726,6 @@ static void > en_non_vif_data_cleanup(void *data OVS_UNUSED) > { > struct ed_type_non_vif_data *ed_non_vif_data = data; > - simap_destroy(&ed_non_vif_data->patch_ofports); > chassis_tunnels_destroy(&ed_non_vif_data->chassis_tunnels); > flow_based_tunnels_destroy(ed_non_vif_data->flow_tunnels); > } > @@ -3676,11 +3734,9 @@ static enum engine_node_state > en_non_vif_data_run(struct engine_node *node, void *data) > { > struct ed_type_non_vif_data *ed_non_vif_data = data; > - simap_destroy(&ed_non_vif_data->patch_ofports); > chassis_tunnels_destroy(&ed_non_vif_data->chassis_tunnels); > flow_based_tunnels_destroy(ed_non_vif_data->flow_tunnels); > > - simap_init(&ed_non_vif_data->patch_ofports); > hmap_init(&ed_non_vif_data->chassis_tunnels); > flow_based_tunnels_init(ed_non_vif_data->flow_tunnels); > > @@ -3705,10 +3761,9 @@ en_non_vif_data_run(struct engine_node *node, void > *data) > ed_non_vif_data->use_flow_based_tunnels = > is_flow_based_tunnels_enabled(ovs_table, chassis); > > - local_nonvif_data_run(br_int, chassis, > - &ed_non_vif_data->patch_ofports, > - &ed_non_vif_data->chassis_tunnels, > - ed_non_vif_data->flow_tunnels); > + local_tunnels_run(br_int, chassis, > + &ed_non_vif_data->chassis_tunnels, > + ed_non_vif_data->flow_tunnels); > > return EN_UPDATED; > } > @@ -3719,7 +3774,7 @@ non_vif_data_ovs_iface_handler(struct engine_node > *node, void *data OVS_UNUSED) > const struct ovsrec_interface_table *iface_table = > EN_OVSDB_GET(engine_get_input("OVS_interface", node)); > > - if (local_nonvif_data_handle_ovs_iface_changes(iface_table)) { > + if (local_tunnels_handle_ovs_iface_changes(iface_table)) { > return EN_HANDLED_UNCHANGED; > } else { > return EN_UNHANDLED; > @@ -4740,6 +4795,9 @@ static void init_physical_ctx(struct engine_node > *node, > const struct ed_type_mff_ovn_geneve *ed_mff_ovn_geneve = > engine_get_input_data("mff_ovn_geneve", node); > > + struct ed_type_patch_port_data *patch_port_data = > + engine_get_input_data("patch_port_data", node); > + > const struct ovsrec_interface_table *ovs_interface_table = > EN_OVSDB_GET(engine_get_input("if_status_mgr", node)); > > @@ -4791,7 +4849,7 @@ static void init_physical_ctx(struct engine_node > *node, > p_ctx->ct_zones = ct_zones; > p_ctx->mff_ovn_geneve = ed_mff_ovn_geneve->mff_ovn_geneve; > p_ctx->local_bindings = &rt_data->lbinding_data.bindings; > - p_ctx->patch_ofports = &non_vif_data->patch_ofports; > + p_ctx->patch_ofports = &patch_port_data->patch_ofports; > p_ctx->chassis_tunnels = &non_vif_data->chassis_tunnels; > p_ctx->flow_tunnels = non_vif_data->flow_tunnels; > p_ctx->use_flow_based_tunnels = non_vif_data->use_flow_based_tunnels; > @@ -6922,6 +6980,7 @@ static ENGINE_NODE(template_vars, > CLEAR_TRACKED_DATA); > static ENGINE_NODE(ct_zones, CLEAR_TRACKED_DATA, IS_VALID); > static ENGINE_NODE(ovs_interface_shadow, CLEAR_TRACKED_DATA); > static ENGINE_NODE(runtime_data, CLEAR_TRACKED_DATA, SB_WRITE); > +static ENGINE_NODE(patch_port_data); > static ENGINE_NODE(non_vif_data); > static ENGINE_NODE(mff_ovn_geneve); > static ENGINE_NODE(ofctrl_is_connected); > @@ -7015,6 +7074,11 @@ inc_proc_ovn_controller_init( > engine_add_input(&en_port_groups, &en_runtime_data, > port_groups_runtime_data_handler); > > + engine_add_input(&en_patch_port_data, &en_ovs_open_vswitch, NULL); > + engine_add_input(&en_patch_port_data, &en_ovs_bridge, NULL); > + engine_add_input(&en_patch_port_data, &en_ovs_interface, > + patch_port_data_ovs_iface_handler); > + > engine_add_input(&en_non_vif_data, &en_ovs_open_vswitch, NULL); > engine_add_input(&en_non_vif_data, &en_ovs_bridge, NULL); > engine_add_input(&en_non_vif_data, &en_sb_chassis, NULL); > @@ -7030,6 +7094,8 @@ inc_proc_ovn_controller_init( > /* Note: The order of inputs is important, all OVS interface changes > must > * be handled before any ct_zone changes. > */ > + engine_add_input(&en_pflow_output, &en_patch_port_data, > + NULL); > engine_add_input(&en_pflow_output, &en_non_vif_data, > NULL); > engine_add_input(&en_pflow_output, &en_northd_options, NULL); > diff --git a/tests/ovn-inc-proc-graph-dump.at b/tests/ > ovn-inc-proc-graph-dump.at > index 269766ded..b53ae3988 100644 > --- a/tests/ovn-inc-proc-graph-dump.at > +++ b/tests/ovn-inc-proc-graph-dump.at > @@ -373,6 +373,10 @@ digraph "Incremental-Processing-Engine" { > lb_data -> lflow_output [[label="lflow_output_lb_data_handler"]]; > SB_fdb -> lflow_output [[label="lflow_output_sb_fdb_handler"]]; > SB_meter -> lflow_output [[label="lflow_output_sb_meter_handler"]]; > + patch_port_data [[style=filled, shape=box, fillcolor=white, > label="patch_port_data"]]; > + OVS_open_vswitch -> patch_port_data [[label=""]]; > + OVS_bridge -> patch_port_data [[label=""]]; > + OVS_interface -> patch_port_data > [[label="patch_port_data_ovs_iface_handler"]]; > SB_sb_global [[style=filled, shape=box, fillcolor=white, > label="SB_sb_global"]]; > northd_options [[style=filled, shape=box, fillcolor=white, > label="northd_options"]]; > SB_sb_global -> northd_options > [[label="en_northd_options_sb_sb_global_handler"]]; > @@ -419,6 +423,7 @@ digraph "Incremental-Processing-Engine" { > neighbor_exchange -> evpn_arp [[label=""]]; > evpn_vtep_binding -> evpn_arp > [[label="evpn_arp_vtep_binding_handler"]]; > pflow_output [[style=filled, shape=box, fillcolor=white, > label="pflow_output"]]; > + patch_port_data -> pflow_output [[label=""]]; > non_vif_data -> pflow_output [[label=""]]; > northd_options -> pflow_output [[label=""]]; > ct_zones -> pflow_output [[label="pflow_output_ct_zones_handler"]]; > -- > 2.34.1 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > I think we should add a simple test to check if the change doesn't trigger the lflow_output recompute. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
