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 --- 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++) { + 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++) { + 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
