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);
-- 
2.16.1

_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to