[ovs-dev] Q on FLOW_SIG_SIZE and hashing

[Ravi.Kerur at telekom.com]

I might have found the problem. Will keep you posted.

----- Original Message -----
From: Ethan Jackson [mailto:ethan at nicira.com]
Sent: Wednesday, February 22, 2012 02:30 AM
To: Kerur, Ravi
Cc: dev at openvswitch.org <dev at openvswitch.org>
Subject: Re: [ovs-dev] Q on FLOW_SIG_SIZE and hashing

> 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?

We don't want to use __attribute__((packed)) because it isn't
portable.  For this reason we've been carefully placing the elements
to avoid adding padding.  That's why we build assert on the size of
struct flow.

Ethan

>
> -----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