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 <jrajaha...@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
