Pushed, thanks for the review!
Jarno
On Oct 17, 2014, at 4:05 PM, Daniele Di Proietto <ddiproie...@vmware.com> wrote:
> 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
