Jakub Sitnicki demonstrated that repeatedly calculating row hashes is
expensive, so this should improve ovn-northd performance.
Reported-by: Jakub Sitnicki <j...@redhat.com>
Reported-at:
https://mail.openvswitch.org/pipermail/ovs-dev/2018-February/344404.html
Signed-off-by: Ben Pfaff <b...@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);
