[ovs-dev] [PATCH ovn] patch: Use indexes when iterating on localnet/l2gateway port bindings.
Dumitru Ceara
dceara at redhat.com
Mon Mar 29 07:01:04 UTC 2021
On 3/28/21 8:46 AM, Han Zhou wrote:
> On Fri, Mar 26, 2021 at 5:37 AM Dumitru Ceara <dceara at redhat.com> 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 <dceara at redhat.com>
>> ---
>> 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!
More information about the dev
mailing list