> 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:et...@nicira.com] > Sent: Tuesday, February 21, 2012 3:59 PM > To: Kerur, Ravi > Cc: dev@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@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-boun...@openvswitch.org [mailto:dev-boun...@openvswitch.org] On >> Behalf Of Ethan Jackson >> Sent: Tuesday, February 21, 2012 3:20 PM >> To: Kerur, Ravi >> Cc: dev@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:et...@nicira.com] >>> Sent: Tuesday, February 21, 2012 2:31 PM >>> To: Kerur, Ravi >>> Cc: dev@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@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
