[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