On Dec 6, 2013, at 9:40 AM, Ben Pfaff <b...@nicira.com> wrote: > On Thu, Dec 05, 2013 at 04:27:24PM -0800, Jarno Rajahalme wrote: >> Ofproto always provides a valid pointer to a mask, but the mask length >> is zero when megaflows are disabled. Use match_wc_init() to get an >> appropriate exact match mask when there is no mask. > > I'm having a little trouble understanding the commit message. > Everything in it makes sense, but it doesn't explain the problem to me. > It's like there should be another clause in the first sentence: "...are > disabled, and this causes a problem because…”
Would this be better: "Normally OVS userspace supplies a mask along with a flow key for each new data path flow that should be created. OVS also provides an option to disable the kernel wildcarding, in which case the flows are created without a mask. When kernel wildcarding is disabled, the datapath should use exact match, i.e. not wildcard any bits in the flow key. Currently, what happens with the userspace datapath instead is that a datapath flow with mostly empty mask is created (i.e., most fields are wildcarded), as the current code does not examine the given mask key length to find out that the mask key is actually empty. This results in the same datapath flow matching on packets of multiple different flows, wrong actions being processed, and stats being incorrect. This patch refactors userspace datapath code to use match_wc_init() to get an appropriate exact match mask when a flow put without a mask is executed." There are a couple of factors that hide this problem currently: - Userspace megaflows have been introduced very recently. - Kernel datapath works as expected when given an empty mask. - Userspace datapath is mostly used by the test suite only. - There are no current test cases that disable megaflows. - This problem was preceded by a related problem in empty mask key parsing fixed in commit df4eeb20 (out of range bit was considered as expected but not provided.) - ovs-appctl dpif/dump-flows dumps the facets, not the actual datapath flows, so it is impossible to see that the masks on the actual datapath flows are incorrect. - Current userspace datapath implementation does not test for duplicate/conflicting classifier rules (as they should not happen). I ran into this while testing a test case with megaflows disabled. I created a test case that shows the problem (fails before applying this patch, passes after it.). I’ll add the test case to the next version of this patch. Jarno _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
