Pushed, thanks for the reminder,
Jarno
On Jun 4, 2014, at 8:37 AM, Jarno Rajahalme <jrajaha...@nicira.com> wrote:
> Acked-by: Jarno Rajahalme <jrajah...@nicira.com>
>
> Will push in a moment.
>
> Jarno
>
>> On Jun 4, 2014, at 1:56 AM, Simon Horman <ho...@verge.net.au> wrote:
>>
>> In the case that an flow for an IP packet has an mpls_push action applied
>> the L3 and L4 portions of the flow will be cleared in flow_push_mpls().
>>
>> Without this change commit_set_port_action() will set the tp_src and tp_dst
>> mask for the flow to all-ones because the base and flow port values no
>> longer match. Even though there will be no corresponding set action for the
>> ports; because the flow is no longer IP.
>>
>> In this case where nw_proto is not part of the match this manifests
>> in a problem because the kernel datapath rejects flows whose masks
>> have non-zero values for tp_src or dp_dst if the nw_proto mask is
>> not all-ones.
>>
>> This patch resolves this problem by having commit_set_port_action() return
>> without doing anything if flow->nw_proto is zero. The same logic is present
>> in commit_set_nw_action().
>>
>> Also enhance one of the MPLS tests to exercise this logic. The enhanced
>> tests inputs a UDP packet with non-zero ports rather than an IP packet with
>> zeroed ports: zeroed ports cause commit_set_port_action() always return
>> without doing anything..
>>
>> Commit 691d39b ("upcall: Remove redundant xlate_actions_for_side_effects().")
>> causes xlate_in_init() to be called for every packet that has an upcall.
>> This has the effect of indirectly calling commit_set_port_action() when
>> translating a controller action which may not have previously been the case
>> depending on the flow.
>>
>> The result is that the behaviour described in the changelog above can be
>> exercised via a minor enhancement to one of the existing MPLS tests. This
>> illustrates that the problem exists for the user-space datapath whereas I
>> had previously incorrectly assumed it only manifested when using the kernel
>> datapath because I had only observed it there.
>>
>> Signed-off-by: Simon Horman <ho...@verge.net.au>
>> ---
>> lib/odp-util.c | 5 +++++
>> tests/ofproto-dpif.at | 2 +-
>> 2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/lib/odp-util.c b/lib/odp-util.c
>> index 280e8a6..3c69ada 100644
>> --- a/lib/odp-util.c
>> +++ b/lib/odp-util.c
>> @@ -3790,6 +3790,11 @@ static void
>> commit_set_port_action(const struct flow *flow, struct flow *base,
>> struct ofpbuf *odp_actions, struct flow_wildcards *wc)
>> {
>> + /* Check if 'flow' really has an L3 header. */
>> + if (!flow->nw_proto) {
>> + return;
>> + }
>> +
>> if (!is_ip_any(base) || (!base->tp_src && !base->tp_dst)) {
>> return;
>> }
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 26532c1..296560a 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -1325,7 +1325,7 @@ dnl Modified MPLS controller action.
>> AT_CHECK([ovs-ofctl monitor br0 65534 -P nxm --detach --pidfile 2>
>> ofctl_monitor.log])
>>
>> for i in 1 2 3; do
>> - ovs-appctl netdev-dummy/receive p1
>> 'in_port(1),eth(src=40:44:44:44:44:42,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=16,tos=0,ttl=64,frag=no)'
>> + ovs-appctl netdev-dummy/receive p1
>> 'in_port(1),eth(src=40:44:44:44:44:42,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=64,frag=no),udp(src=7777,dst=80)'
>> done
>> OVS_WAIT_UNTIL([test `wc -l < ofctl_monitor.log` -ge 6])
>> OVS_APP_EXIT_AND_WAIT(ovs-ofctl)
>> --
>> 1.8.5.2
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> http://openvswitch.org/mailman/listinfo/dev
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
