On 25 Feb 2019, at 17:18, Harry van Haaren wrote:

This commit splits the generic hash-lookup-verify
function to its own file. In doing so, we must move
some MACRO definitions to dpif-netdev.h

Changes look good, but want to wait for some performance numbers before ack’ing the series.

Did you do any performance tests on the change? It does not seem to impact much, but I’ve been surprised before.

Signed-off-by: Harry van Haaren <harry.van.haa...@intel.com>
---
 lib/automake.mk                  |  1 +
lib/dpif-netdev-lookup-generic.c | 95 ++++++++++++++++++++++++++++++++
 lib/dpif-netdev.c                | 81 +--------------------------
 lib/dpif-netdev.h                | 16 ++++++
 4 files changed, 113 insertions(+), 80 deletions(-)
 create mode 100644 lib/dpif-netdev-lookup-generic.c

diff --git a/lib/automake.mk b/lib/automake.mk
index bae032bd8..3a5baf2b8 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -78,6 +78,7 @@ lib_libopenvswitch_la_SOURCES = \
        lib/dp-packet.h \
        lib/dp-packet.c \
        lib/dpdk.h \
+       lib/dpif-netdev-lookup-generic.c \
        lib/dpif-netdev.c \
        lib/dpif-netdev.h \
        lib/dpif-netdev-perf.c \
diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-generic.c
new file mode 100644
index 000000000..8ae8ff59d
--- /dev/null
+++ b/lib/dpif-netdev-lookup-generic.c
@@ -0,0 +1,95 @@
+/*
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016, 2017 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#include "dpif-netdev.h"
+
+#include "bitmap.h"
+#include "cmap.h"
+
+#include "dp-packet.h"
+#include "dpif.h"
+#include "dpif-netdev-perf.h"
+#include "dpif-provider.h"
+#include "flow.h"
+#include "packets.h"
+#include "pvector.h"
+
+/* Returns a hash value for the bits of 'key' where there are 1-bits in
+ * 'mask'. */
+static inline uint32_t
+netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
+                             const struct netdev_flow_key *mask)
+{
+    const uint64_t *p = miniflow_get_values(&mask->mf);
+    uint32_t hash = 0;
+    uint64_t value;
+
+    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
+        hash = hash_add64(hash, value & *p);
+        p++;
+    }
+
+ return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 8);
+}
+
+uint32_t
+dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
+                              uint32_t keys_map,
+                              const struct netdev_flow_key *keys[],
+                              struct dpcls_rule **rules)
+{
+        int i;
+        /* Compute hashes for the remaining keys.  Each search-key is
+ * masked with the subtable's mask to avoid hashing the wildcarded
+         * bits. */
+        uint32_t hashes[NETDEV_MAX_BURST];
+        ULLONG_FOR_EACH_1(i, keys_map) {
+            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
+ &subtable->mask);
+        }
+
+        /* Lookup. */
+        const struct cmap_node *nodes[NETDEV_MAX_BURST];
+        uint32_t found_map =
+ cmap_find_batch(&subtable->rules, keys_map, hashes, nodes); + /* Check results. When the i-th bit of found_map is set, it means + * that a set of nodes with a matching hash value was found for the + * i-th search-key. Due to possible hash collisions we need to check + * which of the found rules, if any, really matches our masked
+         * search-key. */
+        ULLONG_FOR_EACH_1(i, found_map) {
+            struct dpcls_rule *rule;
+
+            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
+ if (OVS_LIKELY(dpcls_rule_matches_key(rule, keys[i]))) {
+                    rules[i] = rule;
+                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
+                     * within one second optimization interval. */
+                    subtable->hit_cnt++;
+                    //lookups_match += subtable_pos;
+                    goto next;
+                }
+            }
+ /* None of the found rules was a match. Reset the i-th bit to
+             * keep searching this key in the next subtable. */
+            ULLONG_SET0(found_map, i);  /* Did not match. */
+        next:
+            ;                     /* Keep Sparse happy. */
+        }
+
+        return found_map;
+}
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 143f1c288..bc0339a25 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -234,14 +234,6 @@ struct dpcls {
     struct pvector subtables;
 };

-/* A rule to be inserted to the classifier. */
-struct dpcls_rule {
- struct cmap_node cmap_node; /* Within struct dpcls_subtable 'rules'. */
-    struct netdev_flow_key *mask; /* Subtable's mask. */
-    struct netdev_flow_key flow;  /* Matching key. */
- /* 'flow' must be the last field, additional space is allocated here. */
-};
-
 /* Data structure to keep packet order till fastpath processing. */
 struct dp_packet_flow_map {
     struct dp_packet *packet;
@@ -259,8 +251,6 @@ static bool dpcls_lookup(struct dpcls *cls,
                          const struct netdev_flow_key *keys[],
                          struct dpcls_rule **rules, size_t cnt,
                          int *num_lookups_p);
-static bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
-                            const struct netdev_flow_key *target);
 /* Set of supported meter flags */
 #define DP_SUPPORTED_METER_FLAGS_MASK \
     (OFPMF13_STATS | OFPMF13_PKTPS | OFPMF13_KBPS | OFPMF13_BURST)
@@ -2734,27 +2724,6 @@ netdev_flow_key_init_masked(struct netdev_flow_key *dst, (dst_u64 - miniflow_get_values(&dst->mf)) * 8);
 }

-/* Iterate through netdev_flow_key TNL u64 values specified by 'FLOWMAP'. */
-#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
-    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
-
-/* Returns a hash value for the bits of 'key' where there are 1-bits in
- * 'mask'. */
-static inline uint32_t
-netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
-                             const struct netdev_flow_key *mask)
-{
-    const uint64_t *p = miniflow_get_values(&mask->mf);
-    uint32_t hash = 0;
-    uint64_t value;
-
-    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
-        hash = hash_add64(hash, value & *p++);
-    }
-
- return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 8);
-}
-
 static inline bool
 emc_entry_alive(struct emc_entry *ce)
 {
@@ -7767,7 +7736,7 @@ dpcls_remove(struct dpcls *cls, struct dpcls_rule *rule)

/* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit
  * in 'mask' the values in 'key' and 'target' are the same. */
-static bool
+bool
 dpcls_rule_matches_key(const struct dpcls_rule *rule,
                        const struct netdev_flow_key *target)
 {
@@ -7783,54 +7752,6 @@ dpcls_rule_matches_key(const struct dpcls_rule *rule,
     return true;
 }

-uint32_t
-dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
-                              uint32_t keys_map,
-                              const struct netdev_flow_key *keys[],
-                              struct dpcls_rule **rules)
-{
-        int i;
-        /* Compute hashes for the remaining keys.  Each search-key is
- * masked with the subtable's mask to avoid hashing the wildcarded
-         * bits. */
-        uint32_t hashes[NETDEV_MAX_BURST];
-        ULLONG_FOR_EACH_1(i, keys_map) {
-            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
- &subtable->mask);
-        }
-
-        /* Lookup. */
-        const struct cmap_node *nodes[NETDEV_MAX_BURST];
-        uint32_t found_map =
- cmap_find_batch(&subtable->rules, keys_map, hashes, nodes); - /* Check results. When the i-th bit of found_map is set, it means - * that a set of nodes with a matching hash value was found for the - * i-th search-key. Due to possible hash collisions we need to check - * which of the found rules, if any, really matches our masked
-         * search-key. */
-        ULLONG_FOR_EACH_1(i, found_map) {
-            struct dpcls_rule *rule;
-
-            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
- if (OVS_LIKELY(dpcls_rule_matches_key(rule, keys[i]))) {
-                    rules[i] = rule;
-                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
-                     * within one second optimization interval. */
-                    subtable->hit_cnt++;
-                    //lookups_match += subtable_pos;
-                    goto next;
-                }
-            }
- /* None of the found rules was a match. Reset the i-th bit to
-             * keep searching this key in the next subtable. */
-            ULLONG_SET0(found_map, i);  /* Did not match. */
-        next:
-            ;                     /* Keep Sparse happy. */
-        }
-
-        return found_map;
-}
-
/* For each miniflow in 'keys' performs a classifier lookup writing the result * into the corresponding slot in 'rules'. If a particular entry in 'keys' is
  * NULL it is skipped.
diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h
index 720425fb3..7195b9791 100644
--- a/lib/dpif-netdev.h
+++ b/lib/dpif-netdev.h
@@ -51,6 +51,14 @@ struct netdev_flow_key {
     uint64_t buf[FLOW_MAX_PACKET_U64S];
 };

+/* A rule to be inserted to the classifier. */
+struct dpcls_rule {
+ struct cmap_node cmap_node; /* Within struct dpcls_subtable 'rules'. */
+    struct netdev_flow_key *mask; /* Subtable's mask. */
+    struct netdev_flow_key flow;  /* Matching key. */
+ /* 'flow' must be the last field, additional space is allocated here. */
+};
+
/** Lookup function for a subtable in the dpcls. This function is called * by each subtable with an array of packets, and a bitmask of packets to * perform the lookup on. Using a function pointer gives flexibility to
@@ -88,6 +96,14 @@ struct dpcls_subtable {
/* 'mask' must be the last field, additional space is allocated here. */
 };

+/* Iterate through netdev_flow_key TNL u64 values specified by 'FLOWMAP'. */
+#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
+    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
+
+bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
+                            const struct netdev_flow_key *target);
+
+
 #ifdef  __cplusplus
 }
 #endif
--
2.17.1

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

Reply via email to