[ovs-dev] Q on FLOW_SIG_SIZE and hashing

[Ravi.Kerur at telekom.com]

No I haven't run with valgrind yet. I will probably run it later today.

The fact that I shuffled around elements within struct flow and it increased the size of the structure indicates padding(2 bytes) were added, which makes flow hash doesn't work because flow_sig_data now has uninitialized 2 bytes. In this case wouldn't it make sense to pack the structure? Or do you recommend to carefully place elements/members such that no padding bytes are added?

-----Original Message-----
From: Ethan Jackson [mailto:ethan at nicira.com] 
Sent: Tuesday, February 21, 2012 3:59 PM
To: Kerur, Ravi
Cc: dev at openvswitch.org
Subject: Re: [ovs-dev] Q on FLOW_SIG_SIZE and hashing

> Just to give you another data, I shuffled elements/members in struct flow which increased the size of struct flow to 132(please note I have not added any new fields) and tested it on a 32 bit centos 6.2 system and it fails to match any flow that is programmed. Unfortunately the diffs are on a system which is currently at my home. I will send out those diffs tomorrow if you are interested.

Yep, sounds like you have a bug which is causing the hash to use
uninitialized data.  Have you tried running under valgrind?  It's
pretty good at catching that sort of thing.

Ethan



>
> Thanks
> Ravi
>
> -----Original Message-----
> From: Kerur, Ravi
> Sent: Tuesday, February 21, 2012 3:41 PM
> To: Ethan Jackson
> Cc: dev at openvswitch.org
> Subject: RE: [ovs-dev] Q on FLOW_SIG_SIZE and hashing
>
> Thanks Ethan. I did zeroed out stack and ttl bits in my diffs earlier while debugging. It didn't help so I removed them. I will take a closer look one more time.
>
> Thanks
> Ravi
>
> -----Original Message-----
> From: dev-bounces at openvswitch.org [mailto:dev-bounces at openvswitch.org] On Behalf Of Ethan Jackson
> Sent: Tuesday, February 21, 2012 3:20 PM
> To: Kerur, Ravi
> Cc: dev at openvswitch.org
> Subject: Re: [ovs-dev] Q on FLOW_SIG_SIZE and hashing
>
>> Yes I do. Attached complete diffs. The debugging information I mentioned earlier are with these diffs as well. I shifted to just flow struct diffs after I went through FLOW_WC_SEQ changes I had and thought it might not have an impact, as most of the checks are in ".c" and with build_assert_decl and compilation should have failed if I had missed something. Anyways let me know your inputs.
>
> OK great.  So typically bugs like this come around when you have
> uninitialized data in the flow or in the flow wildcards which could
> cause cls_rule_hash() to return unpredictable results.  This
> completely breaks the classifier.  I haven't read the diff you've sent
> carefully, but it sounds to me like that's the issue you're running
> into.  One place to look:  in flow_zero_wildcards() it looks to me
> like you are only initializing the MPLS_LABEL_MASK and MPLS_TC_MASK
> pits of the 'mpls_lse' field.  If you aren't planning to match on the
> TTL or STACk flags, you'll need to zero them out so the uninitialized
> memory doesn't break the hash.  That's just a hunch though, I know
> very little about MPLS and I haven't read the diff carefully.
>
> Ethan
>
>
>>
>> Thanks
>> Ravi
>>
>> -----Original Message-----
>> From: Ethan Jackson [mailto:ethan at nicira.com]
>> Sent: Tuesday, February 21, 2012 2:31 PM
>> To: Kerur, Ravi
>> Cc: dev at openvswitch.org
>> Subject: Re: [ovs-dev] Q on FLOW_SIG_SIZE and hashing
>>
>>> I have attached diffs which includes adding a member to struct flow and adjusting FLOW_SIG_SIZE accordingly. This is experimental so I haven't bothered to change FLOW_WC_SEQ...
>>
>> Oh I'm sorry for the confusion, I thought you had a more involved
>> patch which makes the necessary changes demanded by FLOW_WC_SEQ.  The
>> code really does require those changes to work, simply adding the
>> field to the structure is insufficient.  The behavior you're seeing is
>> what I'd expect to see without the FLOW_WC_SEQ changes.  There may be
>> other changes necessary as well.
>>
>> Ethan
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev