[ovs-dev] [PATCH ovn] patch: Use indexes when iterating on localnet/l2gateway port bindings.

Han Zhou hzhou at ovn.org
Sun Mar 28 06:46:37 UTC 2021


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


More information about the dev mailing list