[ovs-dev] [PATCH 4/4] ovn-northd: Reduce amount of flow hashing.

Mark Michelson mmichels at redhat.com
Wed Feb 21 23:19:30 UTC 2018


Hi,

While testing an unrelated change in OVS master (using make sandbox 
SANDBOXFLAGS="--ovn"), I noticed that my laptop was making way more 
noise than normal. Looking at `top` output, an ovsdb-server process was 
running at 100% CPU, ovn-controller was running at 75% CPU, and 
ovn-northd was running at about 38% CPU. If I start the ovs sandbox, the 
problem does not present itself until after I run the "ovn-setup.sh" 
script. As soon as I run the script, the CPU usage goes soaring.

If I revert to the patch just before this one ("implement synthetic 
columns"), the problem does not occur. At least in a sandbox 
environment, this patch appears to be causing some sort of busy loops 
that drive the CPU% of processes up. Looking at the change, it's not 
immediately obvious why this would be happening.

On 02/14/2018 03:54 PM, Ben Pfaff wrote:
> Jakub Sitnicki demonstrated that repeatedly calculating row hashes is
> expensive, so this should improve ovn-northd performance.
> 
> Reported-by: Jakub Sitnicki <jkbs at redhat.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html
> Signed-off-by: Ben Pfaff <blp at ovn.org>
> ---
>   ovn/lib/ovn-sb-idl.ann  | 20 ++++++++++++++++++++
>   ovn/lib/ovn-util.c      | 27 +++++++++++++++++++++++++++
>   ovn/lib/ovn-util.h      |  7 +++++++
>   ovn/northd/ovn-northd.c | 28 +++++++++++++++++-----------
>   4 files changed, 71 insertions(+), 11 deletions(-)
> 
> diff --git a/ovn/lib/ovn-sb-idl.ann b/ovn/lib/ovn-sb-idl.ann
> index 2dfc64e3c4a7..e51238b92e97 100644
> --- a/ovn/lib/ovn-sb-idl.ann
> +++ b/ovn/lib/ovn-sb-idl.ann
> @@ -7,3 +7,23 @@
>   
>   s["idlPrefix"] = "sbrec_"
>   s["idlHeader"] = "\"ovn/lib/ovn-sb-idl.h\""
> +
> +s["hDecls"] = '#include "ovn/lib/ovn-util.h"'
> +
> +# Adds an integer column named 'column' to 'table' in 's'.  The column
> +# values is calculated with 'expression' based on the values of the columns
> +# named in the array 'dependencies'.
> +def synthesize_integer_column(s, table, column, dependencies, expression):
> +    s["tables"][table]["columns"][column] = {
> +        "type": "integer",
> +        "extensions": {
> +            "dependencies": dependencies,
> +            "parse": "row->%s = %s;" % (column, expression),
> +            "synthetic": True
> +        }
> +    }
> +
> +synthesize_integer_column(s, "Logical_Flow", "hash",
> +                          ["logical_datapath", "table_id", "pipeline",
> +                           "priority", "match", "actions"],
> +                          "sbrec_logical_flow_hash(row)")
> diff --git a/ovn/lib/ovn-util.c b/ovn/lib/ovn-util.c
> index a554c23f5ae8..e9464e926d74 100644
> --- a/ovn/lib/ovn-util.c
> +++ b/ovn/lib/ovn-util.c
> @@ -17,6 +17,7 @@
>   #include "dirs.h"
>   #include "openvswitch/vlog.h"
>   #include "ovn/lib/ovn-nb-idl.h"
> +#include "ovn/lib/ovn-sb-idl.h"
>   
>   VLOG_DEFINE_THIS_MODULE(ovn_util);
>   
> @@ -329,3 +330,29 @@ ovn_is_known_nb_lsp_type(const char *type)
>   
>       return false;
>   }
> +
> +uint32_t
> +sbrec_logical_flow_hash(const struct sbrec_logical_flow *lf)
> +{
> +    const struct sbrec_datapath_binding *ld = lf->logical_datapath;
> +    if (!ld) {
> +        return 0;
> +    }
> +
> +    return ovn_logical_flow_hash(&ld->header_.uuid,
> +                                 lf->table_id, lf->pipeline,
> +                                 lf->priority, lf->match, lf->actions);
> +}
> +
> +uint32_t
> +ovn_logical_flow_hash(const struct uuid *logical_datapath,
> +                      uint8_t table_id, const char *pipeline,
> +                      uint16_t priority,
> +                      const char *match, const char *actions)
> +{
> +    size_t hash = uuid_hash(logical_datapath);
> +    hash = hash_2words((table_id << 16) | priority, hash);
> +    hash = hash_string(pipeline, hash);
> +    hash = hash_string(match, hash);
> +    return hash_string(actions, hash);
> +}
> diff --git a/ovn/lib/ovn-util.h b/ovn/lib/ovn-util.h
> index 9b456426dc67..7ff9f9a1c73b 100644
> --- a/ovn/lib/ovn-util.h
> +++ b/ovn/lib/ovn-util.h
> @@ -19,6 +19,7 @@
>   #include "lib/packets.h"
>   
>   struct nbrec_logical_router_port;
> +struct sbrec_logical_flow;
>   struct uuid;
>   
>   struct ipv4_netaddr {
> @@ -69,4 +70,10 @@ const char *default_sb_db(void);
>   
>   bool ovn_is_known_nb_lsp_type(const char *type);
>   
> +uint32_t sbrec_logical_flow_hash(const struct sbrec_logical_flow *);
> +uint32_t ovn_logical_flow_hash(const struct uuid *logical_datapath,
> +                               uint8_t table_id, const char *pipeline,
> +                               uint16_t priority,
> +                               const char *match, const char *actions);
> +
>   #endif
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 4d95a3d9d40e..5d764f6e9404 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -194,6 +194,13 @@ ovn_stage_get_pipeline(enum ovn_stage stage)
>       return (stage >> 8) & 1;
>   }
>   
> +/* Returns the pipeline name to which 'stage' belongs. */
> +static const char *
> +ovn_stage_get_pipeline_name(enum ovn_stage stage)
> +{
> +    return ovn_stage_get_pipeline(stage) == P_IN ? "ingress" : "egress";
> +}
> +
>   /* Returns the table to which 'stage' belongs. */
>   static uint8_t
>   ovn_stage_get_table(enum ovn_stage stage)
> @@ -2271,10 +2278,11 @@ struct ovn_lflow {
>   static size_t
>   ovn_lflow_hash(const struct ovn_lflow *lflow)
>   {
> -    size_t hash = uuid_hash(&lflow->od->key);
> -    hash = hash_2words((lflow->stage << 16) | lflow->priority, hash);
> -    hash = hash_string(lflow->match, hash);
> -    return hash_string(lflow->actions, hash);
> +    return ovn_logical_flow_hash(&lflow->od->key,
> +                                 ovn_stage_get_table(lflow->stage),
> +                                 ovn_stage_get_pipeline_name(lflow->stage),
> +                                 lflow->priority, lflow->match,
> +                                 lflow->actions);
>   }
>   
>   static bool
> @@ -2331,7 +2339,7 @@ ovn_lflow_add_at(struct hmap *lflow_map, struct ovn_datapath *od,
>   static struct ovn_lflow *
>   ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od,
>                  enum ovn_stage stage, uint16_t priority,
> -               const char *match, const char *actions)
> +               const char *match, const char *actions, uint32_t hash)
>   {
>       struct ovn_lflow target;
>       ovn_lflow_init(&target, od, stage, priority,
> @@ -2339,8 +2347,7 @@ ovn_lflow_find(struct hmap *lflows, struct ovn_datapath *od,
>                      NULL, NULL);
>   
>       struct ovn_lflow *lflow;
> -    HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, ovn_lflow_hash(&target),
> -                             lflows) {
> +    HMAP_FOR_EACH_WITH_HASH (lflow, hmap_node, hash, lflows) {
>           if (ovn_lflow_equal(lflow, &target)) {
>               return lflow;
>           }
> @@ -6014,7 +6021,7 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
>               = !strcmp(sbflow->pipeline, "ingress") ? P_IN : P_OUT;
>           struct ovn_lflow *lflow = ovn_lflow_find(
>               &lflows, od, ovn_stage_build(dp_type, pipeline, sbflow->table_id),
> -            sbflow->priority, sbflow->match, sbflow->actions);
> +            sbflow->priority, sbflow->match, sbflow->actions, sbflow->hash);
>           if (lflow) {
>               ovn_lflow_destroy(&lflows, lflow);
>           } else {
> @@ -6023,13 +6030,12 @@ build_lflows(struct northd_context *ctx, struct hmap *datapaths,
>       }
>       struct ovn_lflow *lflow, *next_lflow;
>       HMAP_FOR_EACH_SAFE (lflow, next_lflow, hmap_node, &lflows) {
> -        enum ovn_pipeline pipeline = ovn_stage_get_pipeline(lflow->stage);
> +        const char *pipeline = ovn_stage_get_pipeline_name(lflow->stage);
>           uint8_t table = ovn_stage_get_table(lflow->stage);
>   
>           sbflow = sbrec_logical_flow_insert(ctx->ovnsb_txn);
>           sbrec_logical_flow_set_logical_datapath(sbflow, lflow->od->sb);
> -        sbrec_logical_flow_set_pipeline(
> -            sbflow, pipeline == P_IN ? "ingress" : "egress");
> +        sbrec_logical_flow_set_pipeline(sbflow, pipeline);
>           sbrec_logical_flow_set_table_id(sbflow, table);
>           sbrec_logical_flow_set_priority(sbflow, lflow->priority);
>           sbrec_logical_flow_set_match(sbflow, lflow->match);
> 



More information about the dev mailing list