[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