[ovs-dev] [PATCH] lib/dpif-netdev: Fix EMC lookup.

Daniele Di Proietto ddiproietto at vmware.com
Fri Oct 17 23:05:43 UTC 2014


Thanks for fixing this Jarno!

I've tested the patch and it fixes the problem.
Would you mind adding a comment on struct netdev_flow_key that 'len'
accounts for the length of the miniflow only (including the map)?
Other comments inline, otherwise:

Acked-by: Daniele Di Proietto <ddiproietto at vmware.com>

On 10/17/14, 3:05 PM, "Jarno Rajahalme" <jrajahalme at nicira.com> wrote:

>Patch 0de8783a9 (lib/dpif-netdev: Integrate megaflow classifier.)
>broke exact match cache lookup, but it went undetected since there are
>no separate stats for EMC.
>
>This patch fixes the problem by changing the struct netdev_flow_key
>'len' member to cover only the 'mf' member, not the whole
>netdev_flow_key, and ignoring the 'len' field in
>netdev_flow_key_equal.  Comparison is still accurate, as the miniflow
>'map' field encodes the length in the number of 1-bits, and the map is
>included in the comparison.
>
>Reported-by: Alex Wang <alexw at nicira.com>
>Signed-off-by: Jarno Rajahalme <jrajahalme at nicira.com>
>---
> lib/dpif-netdev.c |   26 ++++++++++++++------------
> 1 file changed, 14 insertions(+), 12 deletions(-)
>
>diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>index f6a0c48..8c8e6c5 100644
>--- a/lib/dpif-netdev.c
>+++ b/lib/dpif-netdev.c
>@@ -433,10 +433,13 @@ emc_cache_init(struct emc_cache *flow_cache)
> {
>     int i;
> 
>+    BUILD_ASSERT(offsetof(struct netdev_flow_key, mf) == 2 *
>sizeof(uint32_t));
>+

I believe now we should assert that offsetof(struct miniflow,
inline_values) == sizeof(uint64_t).

>     for (i = 0; i < ARRAY_SIZE(flow_cache->entries); i++) {
>         flow_cache->entries[i].flow = NULL;
>         flow_cache->entries[i].key.hash = 0;
>-        flow_cache->entries[i].key.len = 0;
>+        flow_cache->entries[i].key.len
>+            = offsetof(struct miniflow, inline_values);
>         miniflow_initialize(&flow_cache->entries[i].key.mf,
>                             flow_cache->entries[i].key.buf);
>     }
>@@ -1269,11 +1272,11 @@ BUILD_ASSERT_DECL(offsetof(struct miniflow,
>inline_values)
>                   == sizeof(uint64_t));
> 
> /* Given the number of bits set in the miniflow map, returns the size of
>the
>- * netdev_flow key */
>+ * 'netdev_flow_key.mf' */
> static inline uint32_t
> netdev_flow_key_size(uint32_t flow_u32s)
> {
>-    return offsetof(struct netdev_flow_key, mf.inline_values) +
>+    return offsetof(struct miniflow, inline_values) +
>         MINIFLOW_VALUES_SIZE(flow_u32s);
> }
> 
>@@ -1281,8 +1284,8 @@ static inline bool
> netdev_flow_key_equal(const struct netdev_flow_key *a,
>                       const struct netdev_flow_key *b)
> {
>-    /* 'b's size and hash may be not set yet. */
>-    return !memcmp(a, b, a->len);
>+    /* 'b->len' may be not set yet. */
>+    return a->hash == b->hash && !memcmp(&a->mf, &b->mf, a->len);
> }
> 
> /* Used to compare 'netdev_flow_key' in the exact match cache to a
>miniflow.
>@@ -1292,15 +1295,15 @@ static inline bool
> netdev_flow_key_equal_mf(const struct netdev_flow_key *key,
>                          const struct miniflow *mf)
> {
>-    return !memcmp(&key->mf, mf,
>-                   key->len - offsetof(struct netdev_flow_key, mf));
>+    return !memcmp(&key->mf, mf, key->len);
> }
> 
> static inline void
> netdev_flow_key_clone(struct netdev_flow_key *dst,
>                       const struct netdev_flow_key *src)
> {
>-    memcpy(dst, src, src->len);
>+    memcpy(dst, src,
>+           offsetof(struct netdev_flow_key, mf) + src->len);
> }
> 
> /* Slow. */
>@@ -1685,7 +1688,7 @@ dp_netdev_flow_add(struct dp_netdev *dp, struct
>match *match,
>     ovs_assert(!(mask.mf.map & (MINIFLOW_MAP(metadata) |
>MINIFLOW_MAP(regs))));
> 
>     /* Do not allocate extra space. */
>-    flow = xmalloc(sizeof *flow - sizeof flow->cr.flow + mask.len);
>+    flow = xmalloc(sizeof *flow - sizeof flow->cr.flow.mf + mask.len);
>     flow->dead = false;
>     *CONST_CAST(struct flow *, &flow->flow) = match->flow;
>     ovs_refcount_init(&flow->ref_cnt);
>@@ -2840,8 +2843,6 @@ fast_path_processing(struct dp_netdev_pmd_thread
>*pmd,
>                 }
>                 ovs_mutex_unlock(&dp->flow_mutex);
> 
>-                /* EMC uses different hash. */
>-                keys[i].hash = dpif_packet_get_dp_hash(packets[i]);

There's another emc_insert call right below this. Would you mind removing
the same line before that one too?

>                 emc_insert(flow_cache, &keys[i], netdev_flow);
>             }
>         }
>@@ -3277,7 +3278,8 @@ dpcls_create_subtable(struct dpcls *cls, const
>struct netdev_flow_key *mask)
>     struct dpcls_subtable *subtable;
> 
>     /* Need to add one. */
>-    subtable = xmalloc(sizeof *subtable - sizeof subtable->mask +
>mask->len);
>+    subtable = xmalloc(sizeof *subtable
>+                       - sizeof subtable->mask.mf + mask->len);
>     cmap_init(&subtable->rules);
>     netdev_flow_key_clone(&subtable->mask, mask);
>     cmap_insert(&cls->subtables_map, &subtable->cmap_node, mask->hash);
>-- 
>1.7.10.4
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>https://urldefense.proofpoint.com/v1/url?u=http://openvswitch.org/mailman/
>listinfo/dev&k=oIvRg1%2BdGAgOoM1BIlLLqw%3D%3D%0A&r=MV9BdLjtFIdhBDBaw5z%2BU
>6SSA2gAfY4L%2F1HCy3VjlKU%3D%0A&m=GuhCT4j3qQvrxJnuaofNybmagNeSSidRWUfudzOF0
>Ag%3D%0A&s=1a34fd3d98b00a832eb6134ed74a9a17aa4c55cc506edb1cc490853ec4000b1
>0




More information about the dev mailing list