On 3/28/21 8:46 AM, Han Zhou wrote: > On Fri, Mar 26, 2021 at 5:37 AM Dumitru Ceara <[email protected]> wrote: >> >> In large scale deployments it's expected that there are many port >> bindings. Out of these only a small fraction is usually a >> localnet/l2gateway port. >> >> Instead of iterating on all SB port bindings at every ovn-controller >> iteration in patch_run()/add_bridge_mappings(), use IDL indexes to only >> iterate over relevant port bindings, based on 'type'. >> >> For example, running perf on ovn-controller on such deployments we get: >> >> Without using IDL indexed iteration: >> Children Self Command Shared Object Symbol >> + 5.79% 5.34% ovn-controller ovn-controller [.] patch_run >> >> With IDL indexed iteration: >> Children Self Command Shared Object Symbol >> + 0.55% 0.04% ovn-controller ovn-controller [.] patch_run >> >> Reported-at: https://bugzilla.redhat.com/1938950 >> Signed-off-by: Dumitru Ceara <[email protected]> >> --- >> controller/ovn-controller.c | 5 +- >> controller/patch.c | 114 ++++++++++++++++++++---------------- >> controller/patch.h | 3 +- >> 3 files changed, 70 insertions(+), 52 deletions(-) >> >> diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c >> index 5dd643f52..ea0b677bd 100644 >> --- a/controller/ovn-controller.c >> +++ b/controller/ovn-controller.c >> @@ -2489,6 +2489,9 @@ main(int argc, char *argv[]) >> struct ovsdb_idl_index *sbrec_port_binding_by_datapath >> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >> &sbrec_port_binding_col_datapath); >> + struct ovsdb_idl_index *sbrec_port_binding_by_type >> + = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >> + &sbrec_port_binding_col_type); >> struct ovsdb_idl_index *sbrec_datapath_binding_by_key >> = ovsdb_idl_index_create1(ovnsb_idl_loop.idl, >> > &sbrec_datapath_binding_col_tunnel_key); >> @@ -2912,10 +2915,10 @@ main(int argc, char *argv[]) >> runtime_data = engine_get_data(&en_runtime_data); >> if (runtime_data) { >> patch_run(ovs_idl_txn, >> + sbrec_port_binding_by_type, >> ovsrec_bridge_table_get(ovs_idl_loop.idl), >> > ovsrec_open_vswitch_table_get(ovs_idl_loop.idl), >> ovsrec_port_table_get(ovs_idl_loop.idl), >> - > sbrec_port_binding_table_get(ovnsb_idl_loop.idl), >> br_int, chassis, > &runtime_data->local_datapaths); >> pinctrl_run(ovnsb_idl_txn, >> sbrec_datapath_binding_by_key, >> diff --git a/controller/patch.c b/controller/patch.c >> index a2a7bcd79..e54b56354 100644 >> --- a/controller/patch.c >> +++ b/controller/patch.c >> @@ -187,26 +187,24 @@ add_ovs_bridge_mappings(const struct > ovsrec_open_vswitch_table *ovs_table, >> } >> } >> >> -/* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch > ports for >> - * the local bridge mappings. Removes any patch ports for bridge > mappings that >> - * already existed from 'existing_ports'. */ >> static void >> -add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, >> - const struct ovsrec_bridge_table *bridge_table, >> - const struct ovsrec_open_vswitch_table *ovs_table, >> - const struct sbrec_port_binding_table > *port_binding_table, >> - const struct ovsrec_bridge *br_int, >> - struct shash *existing_ports, >> - const struct sbrec_chassis *chassis, >> - const struct hmap *local_datapaths) >> +add_bridge_mappings_by_type(struct ovsdb_idl_txn *ovs_idl_txn, >> + struct ovsdb_idl_index > *sbrec_port_binding_by_type, >> + const struct ovsrec_bridge *br_int, >> + struct shash *existing_ports, >> + const struct sbrec_chassis *chassis, >> + struct shash *bridge_mappings, >> + const char *pb_type, const char > *patch_port_id, >> + const struct hmap *local_datapaths, >> + bool log_missing_bridge) >> { >> - /* Get ovn-bridge-mappings. */ >> - struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); >> - >> - add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); >> + struct sbrec_port_binding *target = >> + sbrec_port_binding_index_init_row(sbrec_port_binding_by_type); >> + sbrec_port_binding_index_set_type(target, pb_type); >> >> const struct sbrec_port_binding *binding; >> - SBREC_PORT_BINDING_TABLE_FOR_EACH (binding, port_binding_table) { >> + SBREC_PORT_BINDING_FOR_EACH_EQUAL (binding, target, >> + sbrec_port_binding_by_type) { >> /* When ovn-monitor-all is true, there can be port-bindings >> * on datapaths that are not related to this chassis. Ignore > them. */ >> if (!get_local_datapath(local_datapaths, >> @@ -214,21 +212,9 @@ add_bridge_mappings(struct ovsdb_idl_txn > *ovs_idl_txn, >> continue; >> } >> >> - bool is_localnet = false; >> - const char *patch_port_id; >> - if (!strcmp(binding->type, "localnet")) { >> - is_localnet = true; >> - patch_port_id = "ovn-localnet-port"; >> - } else if (!strcmp(binding->type, "l2gateway")) { >> - if (!binding->chassis >> - || strcmp(chassis->name, binding->chassis->name)) { >> - /* This L2 gateway port is not bound to this chassis, >> - * so we should not create any patch ports for it. */ >> - continue; >> - } >> - patch_port_id = "ovn-l2gateway-port"; >> - } else { >> - /* not a localnet or L2 gateway port. */ >> + /* If needed, check if the port is bound to this chassis. */ >> + const struct sbrec_chassis *bchassis = binding->chassis; >> + if (chassis && (!bchassis || strcmp(chassis->name, > bchassis->name))) { >> continue; >> } >> >> @@ -240,25 +226,18 @@ add_bridge_mappings(struct ovsdb_idl_txn > *ovs_idl_txn, >> continue; >> } >> char *msg_key = xasprintf("%s;%s", binding->logical_port, > network); >> - struct ovsrec_bridge *br_ln = shash_find_data(&bridge_mappings, > network); >> + struct ovsrec_bridge *br_ln = shash_find_data(bridge_mappings, > network); >> if (!br_ln) { >> - if (!is_localnet) { >> + if (log_missing_bridge) { >> static struct vlog_rate_limit rl = > VLOG_RATE_LIMIT_INIT(5, 1); >> VLOG_ERR_RL(&rl, "bridge not found for %s port '%s' " >> - "with network name '%s'", >> - binding->type, binding->logical_port, network); >> - } else { >> - /* Since having localnet ports that are not mapped on > some >> - * chassis is a supported configuration used to implement >> - * multisegment switches with fabric L3 routing between >> - * segments, log the following message once per run but > don't >> - * unnecessarily pollute the log file. */ >> - if (!sset_contains(&missed_bridges, msg_key)) { >> - VLOG_INFO("bridge not found for localnet port '%s' > with " >> - "network name '%s'; skipping", >> - binding->logical_port, network); >> - sset_add(&missed_bridges, msg_key); >> - } >> + "with network name '%s'", >> + binding->type, binding->logical_port, > network); >> + } else if (!sset_contains(&missed_bridges, msg_key)) { >> + VLOG_INFO("bridge not found for %s port '%s' with " >> + "network name '%s'; skipping", >> + binding->type, binding->logical_port, network); >> + sset_add(&missed_bridges, msg_key); >> } >> free(msg_key); >> continue; >> @@ -275,16 +254,51 @@ add_bridge_mappings(struct ovsdb_idl_txn > *ovs_idl_txn, >> free(name1); >> free(name2); >> } >> + sbrec_port_binding_index_destroy_row(target); >> +} >> + >> +/* Obtains external-ids:ovn-bridge-mappings from OVSDB and adds patch > ports for >> + * the local bridge mappings. Removes any patch ports for bridge > mappings that >> + * already existed from 'existing_ports'. */ >> +static void >> +add_bridge_mappings(struct ovsdb_idl_txn *ovs_idl_txn, >> + struct ovsdb_idl_index *sbrec_port_binding_by_type, >> + const struct ovsrec_bridge_table *bridge_table, >> + const struct ovsrec_open_vswitch_table *ovs_table, >> + const struct ovsrec_bridge *br_int, >> + struct shash *existing_ports, >> + const struct sbrec_chassis *chassis, >> + const struct hmap *local_datapaths) >> +{ >> + /* Get ovn-bridge-mappings. */ >> + struct shash bridge_mappings = SHASH_INITIALIZER(&bridge_mappings); >> + >> + add_ovs_bridge_mappings(ovs_table, bridge_table, &bridge_mappings); >> >> + add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, >> + br_int, existing_ports, chassis, >> + &bridge_mappings, "l2gateway", >> + "ovn-l2gateway-port", local_datapaths, > true); >> + >> + /* Since having localnet ports that are not mapped on some chassis > is a >> + * supported configuration used to implement multisegment switches > with >> + * fabric L3 routing between segments, log the following message > once per >> + * run but don't unnecessarily pollute the log file; pass >> + * 'log_missing_bridge = false'. >> + */ >> + add_bridge_mappings_by_type(ovs_idl_txn, sbrec_port_binding_by_type, >> + br_int, existing_ports, NULL, >> + &bridge_mappings, "localnet", >> + "ovn-localnet-port", local_datapaths, > false); >> shash_destroy(&bridge_mappings); >> } >> >> void >> patch_run(struct ovsdb_idl_txn *ovs_idl_txn, >> + struct ovsdb_idl_index *sbrec_port_binding_by_type, >> const struct ovsrec_bridge_table *bridge_table, >> const struct ovsrec_open_vswitch_table *ovs_table, >> const struct ovsrec_port_table *port_table, >> - const struct sbrec_port_binding_table *port_binding_table, >> const struct ovsrec_bridge *br_int, >> const struct sbrec_chassis *chassis, >> const struct hmap *local_datapaths) >> @@ -313,8 +327,8 @@ patch_run(struct ovsdb_idl_txn *ovs_idl_txn, >> /* Create in the database any patch ports that should exist. Remove > from >> * 'existing_ports' any patch ports that do exist in the database and >> * should be there. */ >> - add_bridge_mappings(ovs_idl_txn, bridge_table, ovs_table, >> - port_binding_table, br_int, &existing_ports, > chassis, >> + add_bridge_mappings(ovs_idl_txn, sbrec_port_binding_by_type, > bridge_table, >> + ovs_table, br_int, &existing_ports, chassis, >> local_datapaths); >> >> /* Now 'existing_ports' only still contains patch ports that exist > in the >> diff --git a/controller/patch.h b/controller/patch.h >> index e470d502c..208b8d3ee 100644 >> --- a/controller/patch.h >> +++ b/controller/patch.h >> @@ -24,6 +24,7 @@ >> >> struct hmap; >> struct ovsdb_idl_txn; >> +struct ovsdb_idl_index; >> struct ovsrec_bridge; >> struct ovsrec_bridge_table; >> struct ovsrec_open_vswitch_table; >> @@ -36,10 +37,10 @@ void add_ovs_bridge_mappings(const struct > ovsrec_open_vswitch_table *ovs_table, >> const struct ovsrec_bridge_table > *bridge_table, >> struct shash *bridge_mappings); >> void patch_run(struct ovsdb_idl_txn *ovs_idl_txn, >> + struct ovsdb_idl_index *sbrec_port_binding_by_type, >> const struct ovsrec_bridge_table *, >> const struct ovsrec_open_vswitch_table *, >> const struct ovsrec_port_table *, >> - const struct sbrec_port_binding_table *, >> const struct ovsrec_bridge *br_int, >> const struct sbrec_chassis *, >> const struct hmap *local_datapaths); >> -- >> 2.27.0 >> > Thanks Dumitru. I applied this to master. > > Han >
Thanks! _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
