[ovs-dev] [RFC PATCH v2 7/7] dpif-netdev: Bug fix and performance improvement.

Jarno Rajahalme jrajahalme at nicira.com
Fri Jul 10 17:35:25 UTC 2015


MINIFLOW_FOR_EACH_IN_MAPS and NETDEV_FLOW_KEY_FOR_EACH_IN_MAPqS had a
bug where tunnel metadata values remaining after the processing of the
tnl_map was not advanced correctly.

Instead of fixing this bug this patch changes the caller to process
each map separately by splitting MINIFLOW_FOR_EACH_IN_MAPS to
MINIFLOW_FOR_EACH_IN_TNL_MAP and MINIFLOW_FOR_EACH_IN_PKT_MAP.  This
should make the inner loops faster, which may help reduce the
performance drop reported by Daniele.

This will be squashed with the previous one before commiting.

Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
---
 lib/classifier-private.h |    5 +++-
 lib/dpif-netdev.c        |   31 +++++++++++++++----------
 lib/flow.c               |    2 +-
 lib/flow.h               |   58 +++++++++++++++++++++-------------------------
 4 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/lib/classifier-private.h b/lib/classifier-private.h
index 435acc9..612092d 100644
--- a/lib/classifier-private.h
+++ b/lib/classifier-private.h
@@ -309,7 +309,10 @@ miniflow_hash_in_minimask(const struct miniflow *flow,
     uint32_t hash = basis;
     uint64_t flow_u64;
 
-    MINIFLOW_FOR_EACH_IN_MAPS(flow_u64, flow, mask->masks) {
+    MINIFLOW_FOR_EACH_IN_TNL_MAP(flow_u64, flow, mask->masks) {
+        hash = hash_add64(hash, flow_u64 & *p++);
+    }
+    MINIFLOW_FOR_EACH_IN_PKT_MAP(flow_u64, flow, mask->masks) {
         hash = hash_add64(hash, flow_u64 & *p++);
     }
 
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 8fd27d2..923e6d1 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -1653,12 +1653,13 @@ netdev_flow_key_init_masked(struct netdev_flow_key *dst,
     dst->hash = hash_finish(hash, (dst_u64 - dst->mf.values) * 8);
 }
 
-/* Iterate through all netdev_flow_key u64 values specified by 'MAP' */
-#define NETDEV_FLOW_KEY_FOR_EACH_IN_MAPS(VALUE, KEY, MAPS)    \
-    for (struct mf_for_each_in_maps_aux aux__                 \
-             = { (KEY)->mf.values, (KEY)->mf, (MAPS) };       \
-         mf_get_next_in_map(&aux__, &(VALUE));                \
-        )
+/* Iterate through netdev_flow_key TNL u64 values specified by 'MAPS'. */
+#define NETDEV_FLOW_KEY_FOR_EACH_IN_TNL_MAP(VALUE, KEY, MAPS)       \
+    MINIFLOW_FOR_EACH_IN_TNL_MAP(VALUE, &(KEY)->mf, MAPS)
+
+/* Iterate through netdev_flow_key PKT u64 values specified by 'MAPS'. */
+#define NETDEV_FLOW_KEY_FOR_EACH_IN_PKT_MAP(VALUE, KEY, MAPS)   \
+    MINIFLOW_FOR_EACH_IN_PKT_MAP(VALUE, &(KEY)->mf, MAPS)
 
 /* Returns a hash value for the bits of 'key' where there are 1-bits in
  * 'mask'. */
@@ -1670,7 +1671,10 @@ netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
     uint32_t hash = 0;
     uint64_t key_u64;
 
-    NETDEV_FLOW_KEY_FOR_EACH_IN_MAPS(key_u64, key, mask->mf) {
+    NETDEV_FLOW_KEY_FOR_EACH_IN_TNL_MAP(key_u64, key, mask->mf) {
+        hash = hash_add64(hash, key_u64 & *p++);
+    }
+    NETDEV_FLOW_KEY_FOR_EACH_IN_PKT_MAP(key_u64, key, mask->mf) {
         hash = hash_add64(hash, key_u64 & *p++);
     }
 
@@ -3866,10 +3870,8 @@ dpcls_remove(struct dpcls *cls, struct dpcls_rule *rule)
     }
 }
 
-/* Returns true if 'target' satisifies 'key' in 'mask', that is, if each 1-bit
- * in 'mask' the values in 'key' and 'target' are the same.
- *
- * Note: 'key' and 'mask' have the same mask, and 'key' is already masked. */
+/* 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 inline bool
 dpcls_rule_matches_key(const struct dpcls_rule *rule,
                        const struct netdev_flow_key *target)
@@ -3878,7 +3880,12 @@ dpcls_rule_matches_key(const struct dpcls_rule *rule,
     const uint64_t *maskp = rule->mask->mf.values;
     uint64_t target_u64;
 
-    NETDEV_FLOW_KEY_FOR_EACH_IN_MAPS(target_u64, target, rule->flow.mf) {
+    NETDEV_FLOW_KEY_FOR_EACH_IN_TNL_MAP(target_u64, target, rule->flow.mf) {
+        if (OVS_UNLIKELY((target_u64 & *maskp++) != *keyp++)) {
+            return false;
+        }
+    }
+    NETDEV_FLOW_KEY_FOR_EACH_IN_PKT_MAP(target_u64, target, rule->flow.mf) {
         if (OVS_UNLIKELY((target_u64 & *maskp++) != *keyp++)) {
             return false;
         }
diff --git a/lib/flow.c b/lib/flow.c
index 7e89a08..7ac057d 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1277,7 +1277,7 @@ miniflow_hash_5tuple(const struct miniflow *flow, uint32_t basis)
                                      | MINIFLOW_PKT_MAP(ipv6_dst), {} };
             uint64_t value;
 
-            MINIFLOW_FOR_EACH_IN_MAPS(value, flow, maps) {
+            MINIFLOW_FOR_EACH_IN_PKT_MAP(value, flow, maps) {
                 hash = hash_add64(hash, value);
             }
         } else {
diff --git a/lib/flow.h b/lib/flow.h
index 308b1e5..16082ad 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -496,42 +496,31 @@ flow_values_get_next_in_maps(struct flow_for_each_in_maps_aux *aux,
     (((UINT64_C(1) << FLOW_U64_SIZE(FIELD)) - 1)                        \
      << ((offsetof(struct flow, FIELD) / sizeof(uint64_t)) - FLOW_TNL_U64S))
 
-struct mf_for_each_in_maps_aux {
+struct mf_for_each_in_map_aux {
     const uint64_t *values;
-    struct miniflow fmaps;
-    struct miniflow maps;
+    uint64_t fmap;
+    uint64_t map;
 };
 
 static inline bool
-mf_get_next_in_map(struct mf_for_each_in_maps_aux *aux, uint64_t *value)
+mf_get_next_in_map(struct mf_for_each_in_map_aux *aux,
+                   uint64_t *value)
 {
-    uint64_t *map;
-    uint64_t *fmap;
-
-    if (aux->maps.tnl_map) {
-        map = &aux->maps.tnl_map;
-        fmap = &aux->fmaps.tnl_map;
-    } else {
-        map = &aux->maps.pkt_map;
-        fmap = &aux->fmaps.pkt_map;
-    }
+    if (aux->map) {
+        uint64_t rm1bit = rightmost_1bit(aux->map);
 
-    if (*map) {
-        uint64_t rm1bit = rightmost_1bit(*map);
-        *map -= rm1bit;
+        aux->map -= rm1bit;
 
-        if (*fmap & rm1bit) {
-            /* Advance 'aux->values' to point to the value for 'rm1bit'. */
-            uint64_t trash = *fmap & (rm1bit - 1);
+        if (aux->fmap & rm1bit) {
+            uint64_t trash = aux->fmap & (rm1bit - 1);
 
-            if (trash) {
-                *fmap -= trash;
-                aux->values += count_1bits(trash);
-            }
+            aux->fmap -= trash;
+            /* count_1bits() is fast for systems where speed matters (e.g.,
+             * DPDK), so we don't try avoid using it.
+             * Advance 'aux->values' to point to the value for 'rm1bit'. */
+            aux->values += count_1bits(trash);
 
-            /* Retrieve the value for 'rm1bit' then advance past it. */
-            *fmap -= rm1bit;
-            *value = *aux->values++;
+            *value = *aux->values;
         } else {
             *value = 0;
         }
@@ -540,10 +529,17 @@ mf_get_next_in_map(struct mf_for_each_in_maps_aux *aux, uint64_t *value)
     return false;
 }
 
-/* Iterate through all miniflow u64 values specified by 'MAPS'. */
-#define MINIFLOW_FOR_EACH_IN_MAPS(VALUE, FLOW, MAPS)   \
-    for (struct mf_for_each_in_maps_aux aux__          \
-             = { (FLOW)->values, *(FLOW), (MAPS) };    \
+/* Iterate through miniflow TNL u64 values specified by 'MAPS'. */
+#define MINIFLOW_FOR_EACH_IN_TNL_MAP(VALUE, FLOW, MAPS)       \
+    for (struct mf_for_each_in_map_aux aux__ =                \
+        { (FLOW)->values, (FLOW)->tnl_map, (MAPS).tnl_map };  \
+         mf_get_next_in_map(&aux__, &(VALUE));)
+
+/* Iterate through miniflow PKT u64 values specified by 'MAPS'. */
+#define MINIFLOW_FOR_EACH_IN_PKT_MAP(VALUE, FLOW, MAPS)       \
+    for (struct mf_for_each_in_map_aux aux__ =                \
+        { (FLOW)->values + count_1bits((FLOW)->tnl_map),      \
+                (FLOW)->pkt_map, (MAPS).pkt_map };            \
          mf_get_next_in_map(&aux__, &(VALUE));)
 
 /* This can be used when it is known that 'u64_idx' is set in 'map'. */
-- 
1.7.10.4




More information about the dev mailing list