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 <ddiproie...@vmware.com>
On 10/17/14, 3:05 PM, "Jarno Rajahalme" <jrajaha...@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 <al...@nicira.com>
>Signed-off-by: Jarno Rajahalme <jrajaha...@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@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
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
