On Dec 6, 2013, at 12:53 PM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Dec 06, 2013 at 11:48:58AM -0800, Jarno Rajahalme wrote:
>>
>> 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."
>
> That's great, thank you!. I know what to look for now so I'll finish
> the review.
Ben,
Below is the test case that fails on current master. It fails like this:
./ofproto-dpif.at:2971: ovs-appctl dpif/dump-flows br0 | sed '
s/used:[0-9]*\.[0-9]*/used:0.0/
' | sort
--- - 2013-12-06 13:00:19.361025029 -0800
+++ /home/jrajahalme/openvswitch/tests/testsuite.dir/at-groups/727/stdout
2013-12-06 13:00:19.355844086 -0800
@@ -1,3 +1,2 @@
-skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:3, bytes:180, used:0.0s, actions:2
-skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:3, bytes:180, used:0.0s, actions:drop
+skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:7, bytes:420, used:0.0s, actions:2
i.e. packets of both different flows get processed by the same userspace
datapath flow.
Jarno
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 4d8d460..f382571 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -2953,6 +2953,28 @@
skb_priority(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif megaflow - disabled])
+OVS_VSWITCHD_START
+ADD_OF_PORTS([br0], [1], [2])
+AT_DATA([flows.txt], [dnl
+table=0 in_port=1,ip,nw_dst=10.0.0.1 actions=output(2)
+table=0 in_port=1,ip,nw_dst=10.0.0.3 actions=drop
+])
+AT_CHECK([ovs-appctl dpif/disable-megaflows], [0], [megaflows disabled
+], [])
+AT_CHECK([ovs-appctl vlog/set dpif_netdev:dbg], [0], [], [])
+AT_CHECK([ovs-ofctl add-flows br0 flows.txt])
+for i in 1 2 3 4; do
+ AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+ AT_CHECK([ovs-appctl netdev-dummy/receive p1
'in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
+done
+AT_CHECK([ovs-appctl dpif/dump-flows br0 | STRIP_USED], [0], [dnl
+skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:3, bytes:180, used:0.0s, actions:2
+skb_priority(0),skb_mark(0),in_port(1),eth(src=50:54:00:00:00:0b,dst=50:54:00:00:00:0c),eth_type(0x0800),ipv4(src=10.0.0.4,dst=10.0.0.3,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0),
packets:3, bytes:180, used:0.0s, actions:drop
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - datapath port number change])
OVS_VSWITCHD_START([set Bridge br0 fail-mode=standalone])
ADD_OF_PORTS([br0], 1)
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
