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

Jarno Rajahalme jrajahalme at nicira.com
Sat Oct 18 00:02:53 UTC 2014


Pushed, thanks for the review!

  Jarno

On Oct 17, 2014, at 4:05 PM, Daniele Di Proietto <ddiproietto at 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 <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