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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
