Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-14 Thread Yi-Hung Wei 
On Wed, Mar 14, 2018 at 2:59 PM, Ben Pfaff  wrote:
> I posted an alternative proposal:
> https://patchwork.ozlabs.org/patch/886052/
>
> I haven't checked that this solves the problem, so I would appreciate
> testing.

Thanks for this fix.  With your patch, eth() is printed with
eth_type() for the kernel datapath flow, and that should fix the
ofproto/trace issue.

Thanks,

-Yi-Hung
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-14 Thread Ben Pfaff 
On Sat, Mar 10, 2018 at 09:30:34AM -0800, Yi-Hung Wei wrote:
> Currently, using ofproto/trace to trace a datapath flow with eth_type()
> but without eth() may hit unexpected behavior because OVS sets
> the packet_type to be (1, eth_type) when decoding the odp flow key.
> This patch updates the logic of odp flow key decoding so that the
> packet_type defaults to (0,0) unless user explicitly specifies the
> packet_type.  An unit test is added to prevent future regression.
> 
> Travis build: https://travis-ci.org/YiHungWei/ovs/builds/351565383
> 
> Reported-by: Amar Padmanabhan 
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html
> Reported-by: Su Wang 
> VMWare-BZ: #2070488
> Signed-off-by: Yi-Hung Wei 

I posted an alternative proposal:
https://patchwork.ozlabs.org/patch/886052/

I haven't checked that this solves the problem, so I would appreciate
testing.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-14 Thread Jiri Benc 
On Tue, 13 Mar 2018 09:36:23 -0700, Darrell Ball wrote:
> [Darrell] There was no suggestion otherwise in general. We were discussing
> for one the difference b/w kernel and userspace DP for handling packet type
> aware support. Specifically, the difference is here is as fundamental as it
> gets -
> eth vs non-eth, The discussed support for ofproto/trace is also in that
> context;
> see Jan's response.

Ah, right, sorry.

So, the problem is that the two datapaths express the same thing
(packet type) differently, right? I think the solution would then be to
insert some kind of translation. ovs-dpctl dump-flows for the kernel
probably should output the flow in a format that ofproto/trace expects
and that matches the user space datapath. (And reverse that in the
other direction.)

 Jiri
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-14 Thread Ben Pfaff 
On Tue, Mar 13, 2018 at 09:36:23AM -0700, Darrell Ball wrote:
> On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc  wrote:
> 
> > Darell, please fix your email client configuration to conform to
> > RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your
> > replies are unreadable. In particular, the text you're replying to must
> > be quoted by ">" character, clearly separating your reply from the
> > original email.
> >
> > Also, wrap your lines at ~78 characters.
> >
> > If your email client does not support this, it is severely broken and
> > you'll need to use a different one.
> >
> 
> 
> [Darrell] hmm, I thought when sending from my company e-mail client
>   with a prefix annotation would help identify the response
> clearly.
>   For every other receiver, that seems work.
>   Maybe, gmail client sender works better for you.

Thanks for quoting in the customary way.  However, when you do that, you
can omit the [Darrell] prefix because it is distracting.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-13 Thread Darrell Ball 
On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc  wrote:

> Darell, please fix your email client configuration to conform to
> RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your
> replies are unreadable. In particular, the text you're replying to must
> be quoted by ">" character, clearly separating your reply from the
> original email.
>
> Also, wrap your lines at ~78 characters.
>
> If your email client does not support this, it is severely broken and
> you'll need to use a different one.
>


[Darrell] hmm, I thought when sending from my company e-mail client
  with a prefix annotation would help identify the response
clearly.
  For every other receiver, that seems work.
  Maybe, gmail client sender works better for you.


>
> On Tue, 13 Mar 2018 02:35:57 +, Darrell Ball wrote:
> > [Darrell] I guess part of the value of ofproto/trace is that it
> > matches as close as possible to other injection sources.
>
> Kernel datapath API has nothing to do with OpenFlow. In particular,
> using output from ovs-dpctl dump-flows in a place where OpenFlow is
> required could not be expected to work.
>

[Darrell] There was no suggestion otherwise in general. We were discussing
for one the difference b/w kernel and userspace DP for handling packet type
aware support. Specifically, the difference is here is as fundamental as it
gets -
eth vs non-eth, The discussed support for ofproto/trace is also in that
context;
see Jan's response. If you are interested you can read the previous
annotated
responses and if you have an alternative suggestion, then that would be
helpful.



>
>  Jiri
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-13 Thread Jiri Benc 
Darell, please fix your email client configuration to conform to
RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your
replies are unreadable. In particular, the text you're replying to must
be quoted by ">" character, clearly separating your reply from the
original email.

Also, wrap your lines at ~78 characters.

If your email client does not support this, it is severely broken and
you'll need to use a different one.

On Tue, 13 Mar 2018 02:35:57 +, Darrell Ball wrote:
> [Darrell] I guess part of the value of ofproto/trace is that it
> matches as close as possible to other injection sources.

Kernel datapath API has nothing to do with OpenFlow. In particular,
using output from ovs-dpctl dump-flows in a place where OpenFlow is
required could not be expected to work.

 Jiri
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-12 Thread Darrell Ball 


On 3/12/18, 2:40 PM, "Jan Scheurich" <jan.scheur...@ericsson.com> wrote:

There is a big difference between an OpenFlow match (as received from SDN 
controllers or ovs-ofctl) and a datapath flow representing a received packet:

The OpenFlow specification indeed states that an OpenFlow match without 
packet_type match implies a match on Ethernet packet_type (0,0). This is very 
important for backward compatibility.

But for a datapath flow describing a received packet the situation is 
entirely different. 

[Darrell] In general, that is true; they don’t need to match, per se.

Inside the OVS userspace the packet_type field is mandatory for datapath flows. 
The only exception is the kernel datapath, which, as stated before, does not 
(yet) support an explicit packet_type field and the packet_type is implied by 
the presence or absence of the eth() field with the Ethernet MAC addresses. As 
soon as the a flow is received from the kernel datapath over the netlink API, 
the corresponding packet_type() is added to the flow before further processing 
in the user-space.
   
I seems we missed the ovs-appctl ofproto/trace source for datapath flows to 
be injected into OVS slow path processing. It would have been better to require 
the presence of an explicit packet_type() in ofproto/trace, 
or at least to document the kernel datapath convention [eth() --> 
packet_type(0,0), no eth() --> packet_type(1,)] and adjust the unit 
tests accordingly.

[Darrell] I guess part of the value of ofproto/trace is that it matches as 
close as possible to other injection sources.


BR, Jan

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Monday, 12 March, 2018 21:10
> To: Jan Scheurich <jan.scheur...@ericsson.com>; Darrell Ball 
<dlu...@gmail.com>; Yi-Hung Wei <yihung@gmail.com>
> Cc: ovs dev <d...@openvswitch.org>; Jiri Benc (jb...@redhat.com) 
<jb...@redhat.com>
    > Subject: Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
> 
> ovs-fields has this description regarding packet type:
> 
> “Matching on packet_type is a pre-requisite for matching on any data 
field, but for backward compatibility,
> when a match on a data field is present without a packet_type match, Open 
vSwitch acts as though a match
> on (0,0) (Ethernet) had been supplied.”
> 
> AFAIS, the patch supplied by Yi-hung matches this description.
> 
> If we end up keeping this part of the change in behavior for packet type 
aware support, the following test would delineate the
> present behavior even better.
> 
> Thanks Darrell
> 
> AT_SETUP([ofproto-dpif - debug ofproto/trace])
> OVS_VSWITCHD_START
> add_of_ports br0 1 2 3 4
> AT_DATA([flows.txt], [dnl
> table=0 priority=100 in_port=1,udp actions=output:2
> table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
> table=0 priority=99 in_port=1 actions=output:3
> ])
> AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])
> 
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 
'in_port(1),eth(),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0],
> [stdout])
> AT_CHECK([tail -1 stdout], [0],
>   [Datapath actions: 2
> ])
> 
> # Since the addition of packet type aware support, ethernet packets are 
no longer
> # implied, but must be explicitly specified with "eth()"
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 
'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
>   [Datapath actions: drop
> ])
> 
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 
'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(
> src=8,dst=9)'], [0], [stdout])
> AT_CHECK([cat stdout | grep output], [0],
>   [output:4
> ])
> 
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> 
> 
> 
> 
> On 3/12/18, 8:57 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
Darrell Ball" <ovs-dev-boun...@openvswitch.org on behalf
> of db...@vmware.com> wrote:
> 
> 
> 
> On 3/12/18, 2:44 AM, "ovs-dev-boun...@openvswitch.org on behalf of 
Jan Scheurich" <ovs-dev-boun...@openvswitch.org on
> behalf of jan.scheur...@ericsson.com> wrote:
> 
> The current behavior is *not* a regression. It was in 
intentionally implemented in OVS 2.8 as part of

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-12 Thread Jan Scheurich 
There is a big difference between an OpenFlow match (as received from SDN 
controllers or ovs-ofctl) and a datapath flow representing a received packet:

The OpenFlow specification indeed states that an OpenFlow match without 
packet_type match implies a match on Ethernet packet_type (0,0). This is very 
important for backward compatibility.

But for a datapath flow describing a received packet the situation is entirely 
different. Inside the OVS userspace the packet_type field is mandatory for 
datapath flows. The only exception is the kernel datapath, which, as stated 
before, does not (yet) support an explicit packet_type field and the 
packet_type is implied by the presence or absence of the eth() field with the 
Ethernet MAC addresses. As soon as the a flow is received from the kernel 
datapath over the netlink API, the corresponding packet_type() is added to the 
flow before further processing in the user-space.

I seems we missed the ovs-appctl ofproto/trace source for datapath flows to be 
injected into OVS slow path processing. It would have been better to require 
the presence of an explicit packet_type() in ofproto/trace, or at least to 
document the kernel datapath convention [eth() --> packet_type(0,0), no eth() 
--> packet_type(1,)] and adjust the unit tests accordingly.

BR, Jan

> -Original Message-
> From: Darrell Ball [mailto:db...@vmware.com]
> Sent: Monday, 12 March, 2018 21:10
> To: Jan Scheurich <jan.scheur...@ericsson.com>; Darrell Ball 
> <dlu...@gmail.com>; Yi-Hung Wei <yihung@gmail.com>
> Cc: ovs dev <d...@openvswitch.org>; Jiri Benc (jb...@redhat.com) 
> <jb...@redhat.com>
> Subject: Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
> 
> ovs-fields has this description regarding packet type:
> 
> “Matching on packet_type is a pre-requisite for matching on any data field, 
> but for backward compatibility,
> when a match on a data field is present without a packet_type match, Open 
> vSwitch acts as though a match
> on (0,0) (Ethernet) had been supplied.”
> 
> AFAIS, the patch supplied by Yi-hung matches this description.
> 
> If we end up keeping this part of the change in behavior for packet type 
> aware support, the following test would delineate the
> present behavior even better.
> 
> Thanks Darrell
> 
> AT_SETUP([ofproto-dpif - debug ofproto/trace])
> OVS_VSWITCHD_START
> add_of_ports br0 1 2 3 4
> AT_DATA([flows.txt], [dnl
> table=0 priority=100 in_port=1,udp actions=output:2
> table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
> table=0 priority=99 in_port=1 actions=output:3
> ])
> AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])
> 
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth(),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
>  [0],
> [stdout])
> AT_CHECK([tail -1 stdout], [0],
>   [Datapath actions: 2
> ])
> 
> # Since the addition of packet type aware support, ethernet packets are no 
> longer
> # implied, but must be explicitly specified with "eth()"
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
>  [0], [stdout])
> AT_CHECK([tail -1 stdout], [0],
>   [Datapath actions: drop
> ])
> 
> AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(
> src=8,dst=9)'], [0], [stdout])
> AT_CHECK([cat stdout | grep output], [0],
>   [output:4
> ])
> 
> OVS_VSWITCHD_STOP
> AT_CLEANUP
> 
> 
> 
> 
> On 3/12/18, 8:57 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell 
> Ball" <ovs-dev-boun...@openvswitch.org on behalf
> of db...@vmware.com> wrote:
> 
> 
> 
> On 3/12/18, 2:44 AM, "ovs-dev-boun...@openvswitch.org on behalf of Jan 
> Scheurich" <ovs-dev-boun...@openvswitch.org on
> behalf of jan.scheur...@ericsson.com> wrote:
> 
> The current behavior is *not* a regression. It was in intentionally 
> implemented in OVS 2.8 as part of the effort for L3 tunneling and
> packet-type aware pipeline.
> 
> The kernel datapath does not support an explicit packet_type field 
> but encodes packet types implicitly:
> 
> * Packet type "Ethernet" (0,0) is encoded with combination of eth() 
> and eth_type()
> * Packet types (1, ) are encoded through eth_type() 
> without eth().
> 
> Thanks Jan
> 
> If eth() is required in case 1, then the following minimal new test would 
> document the requirement.
> 
> AT_S

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-12 Thread Darrell Ball 
ovs-fields has this description regarding packet type:

“Matching on packet_type is a pre-requisite for matching on any data field, but 
for backward compatibility,
when a match on a data field is present without a packet_type match, Open 
vSwitch acts as though a match
on (0,0) (Ethernet) had been supplied.”

AFAIS, the patch supplied by Yi-hung matches this description.

If we end up keeping this part of the change in behavior for packet type aware 
support, the following test would delineate the present behavior even better.

Thanks Darrell

AT_SETUP([ofproto-dpif - debug ofproto/trace])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 4
AT_DATA([flows.txt], [dnl
table=0 priority=100 in_port=1,udp actions=output:2
table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
table=0 priority=99 in_port=1 actions=output:3
])
AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([tail -1 stdout], [0],
  [Datapath actions: 2
])

# Since the addition of packet type aware support, ethernet packets are no 
longer
# implied, but must be explicitly specified with "eth()"
AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([tail -1 stdout], [0],
  [Datapath actions: drop
])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([cat stdout | grep output], [0],
  [output:4
])

OVS_VSWITCHD_STOP
AT_CLEANUP




On 3/12/18, 8:57 AM, "ovs-dev-boun...@openvswitch.org on behalf of Darrell 
Ball" <ovs-dev-boun...@openvswitch.org on behalf of db...@vmware.com> wrote:



On 3/12/18, 2:44 AM, "ovs-dev-boun...@openvswitch.org on behalf of Jan 
Scheurich" <ovs-dev-boun...@openvswitch.org on behalf of 
jan.scheur...@ericsson.com> wrote:

The current behavior is *not* a regression. It was in intentionally 
implemented in OVS 2.8 as part of the effort for L3 tunneling and packet-type 
aware pipeline. 

The kernel datapath does not support an explicit packet_type field but 
encodes packet types implicitly:

* Packet type "Ethernet" (0,0) is encoded with combination of eth() and 
eth_type()
* Packet types (1, ) are encoded through eth_type() without 
eth().

Thanks Jan

If eth() is required in case 1, then the following minimal new test would 
document the requirement. 

AT_SETUP([ofproto-dpif - debug ofproto/trace])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 4
AT_DATA([flows.txt], [dnl
table=0 priority=100 in_port=1,udp actions=output:2
table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
table=0 priority=99 in_port=1 actions=output:3
])
AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([tail -1 stdout], [0],
  [Datapath actions: 2
])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([cat stdout | grep output], [0],
  [output:4
])

OVS_VSWITCHD_STOP
AT_CLEANUP



Flows received from the kernel datapath rely on the construction of the 
packet_type() according to this logic. If you change that, you will break the 
L3 tunneling and PTAP for the kernel datapath.

BR, Jan

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org 
[mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung@gmail.com>
        > Cc: ovs dev <d...@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp 
flow
> 
> Thanks Yi-hung
> One minor comment inline.
> 
> 
> On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung@gmail.com> 
wrote:
> 
> > Currently, using ofproto/trace to trace a datapath flow with 
eth_type()
> > but without eth() may hit unexpected behavior because OVS sets
> > the packet_type to be (1, eth_type) when decoding the odp flow key.
> > This pa

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-12 Thread Darrell Ball 


On 3/12/18, 2:44 AM, "ovs-dev-boun...@openvswitch.org on behalf of Jan 
Scheurich" <ovs-dev-boun...@openvswitch.org on behalf of 
jan.scheur...@ericsson.com> wrote:

The current behavior is *not* a regression. It was in intentionally 
implemented in OVS 2.8 as part of the effort for L3 tunneling and packet-type 
aware pipeline. 

The kernel datapath does not support an explicit packet_type field but 
encodes packet types implicitly:

* Packet type "Ethernet" (0,0) is encoded with combination of eth() and 
eth_type()
* Packet types (1, ) are encoded through eth_type() without 
eth().

Thanks Jan

If eth() is required in case 1, then the following minimal new test would 
document the requirement. 

AT_SETUP([ofproto-dpif - debug ofproto/trace])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 4
AT_DATA([flows.txt], [dnl
table=0 priority=100 in_port=1,udp actions=output:2
table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
table=0 priority=99 in_port=1 actions=output:3
])
AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth(),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([tail -1 stdout], [0],
  [Datapath actions: 2
])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
AT_CHECK([cat stdout | grep output], [0],
  [output:4
])

OVS_VSWITCHD_STOP
AT_CLEANUP



Flows received from the kernel datapath rely on the construction of the 
packet_type() according to this logic. If you change that, you will break the 
L3 tunneling and PTAP for the kernel datapath.

BR, Jan

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org 
[mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung@gmail.com>
> Cc: ovs dev <d...@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
> 
> Thanks Yi-hung
> One minor comment inline.
> 
> 
> On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung@gmail.com> wrote:
> 
> > Currently, using ofproto/trace to trace a datapath flow with eth_type()
> > but without eth() may hit unexpected behavior because OVS sets
> > the packet_type to be (1, eth_type) when decoding the odp flow key.
> > This patch updates the logic of odp flow key decoding so that the
> > packet_type defaults to (0,0) unless user explicitly specifies the
> > packet_type.  An unit test is added to prevent future regression.
> >
> > Travis build: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__travis-2Dci.org_YiHungWei_ovs_builds_351565383=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0=
> >
> > Reported-by: Amar Padmanabhan <amarpadmanab...@fb.com>
> > Reported-at: 
https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-2DDece=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw=
> > mber/045817.html
> > Reported-by: Su Wang <suw...@vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Yi-Hung Wei <yihung@gmail.com>
> > ---
> >  lib/odp-util.c|  8 
> >  tests/ofproto-dpif.at | 24 
> >  2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 5da83b4c64c4..682f1d3bd4b5 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -6440,14 +6440,6 @@ odp_flow_key_to_flow__(const struct nlattr *key,
> > size_t key_len,
> >  }
> >  expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
> >  }
> > -else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
> > -ovs_be16 ethertype = nl_attr_get_be16(attrs[OVS_KEY
> > _ATTR_ETHERTYPE]);
> > -if (!is_mask) {
> > -flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
> > -   ntohs(ethertype));
> > -}
> > -expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
> > -}
> >
>

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-12 Thread Jan Scheurich 
The current behavior is *not* a regression. It was in intentionally implemented 
in OVS 2.8 as part of the effort for L3 tunneling and packet-type aware 
pipeline. 

The kernel datapath does not support an explicit packet_type field but encodes 
packet types implicitly:

* Packet type "Ethernet" (0,0) is encoded with combination of eth() and 
eth_type()
* Packet types (1, ) are encoded through eth_type() without eth().

Flows received from the kernel datapath rely on the construction of the 
packet_type() according to this logic. If you change that, you will break the 
L3 tunneling and PTAP for the kernel datapath.

BR, Jan

> -Original Message-
> From: ovs-dev-boun...@openvswitch.org 
> [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung@gmail.com>
> Cc: ovs dev <d...@openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
> 
> Thanks Yi-hung
> One minor comment inline.
> 
> 
> On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung@gmail.com> wrote:
> 
> > Currently, using ofproto/trace to trace a datapath flow with eth_type()
> > but without eth() may hit unexpected behavior because OVS sets
> > the packet_type to be (1, eth_type) when decoding the odp flow key.
> > This patch updates the logic of odp flow key decoding so that the
> > packet_type defaults to (0,0) unless user explicitly specifies the
> > packet_type.  An unit test is added to prevent future regression.
> >
> > Travis build: https://travis-ci.org/YiHungWei/ovs/builds/351565383
> >
> > Reported-by: Amar Padmanabhan <amarpadmanab...@fb.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece
> > mber/045817.html
> > Reported-by: Su Wang <suw...@vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Yi-Hung Wei <yihung@gmail.com>
> > ---
> >  lib/odp-util.c|  8 
> >  tests/ofproto-dpif.at | 24 
> >  2 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/lib/odp-util.c b/lib/odp-util.c
> > index 5da83b4c64c4..682f1d3bd4b5 100644
> > --- a/lib/odp-util.c
> > +++ b/lib/odp-util.c
> > @@ -6440,14 +6440,6 @@ odp_flow_key_to_flow__(const struct nlattr *key,
> > size_t key_len,
> >  }
> >  expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
> >  }
> > -else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
> > -ovs_be16 ethertype = nl_attr_get_be16(attrs[OVS_KEY
> > _ATTR_ETHERTYPE]);
> > -if (!is_mask) {
> > -flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
> > -   ntohs(ethertype));
> > -}
> > -expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
> > -}
> >
> >  /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */
> >  if (!parse_ethertype(attrs, present_attrs, _attrs, flow,
> > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> > index d2058eddd3eb..13e57b90edd1 100644
> > --- a/tests/ofproto-dpif.at
> > +++ b/tests/ofproto-dpif.at
> > @@ -10162,3 +10162,27 @@ AT_CHECK([grep flow_del ovs-vswitchd.log], [1])
> >
> >  OVS_VSWITCHD_STOP
> >  AT_CLEANUP
> > +
> > +AT_SETUP([ofproto-dpif - debug ofproto/trace])
> > +OVS_VSWITCHD_START
> > +add_of_ports br0 1 2 3 4
> > +AT_DATA([flows.txt], [dnl
> > +table=0 priority=100 in_port=1,udp actions=output:2
> > +table=0 priority=99 in_port=1 actions=output:3
> > +])
> > +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> > 'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.16
> > 8.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout])
> > +AT_CHECK([tail -1 stdout], [0],
> > +  [Datapath actions: 2
> > +])
> > +
> > +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 "table=0 priority=100
> > in_port=1,packet_type=(1,0x800) actions=output:4"])
> > +
> > +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> > 'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4
> > (src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
> > [0], [stdout])
> > +AT_CHECK([cat stdout | grep output], [0],
> > +  [output:4
> > +])
> > +
> > +OVS_VSWITCHD_STOP
> > +AT_CLEANUP
> >
> 
> 
> Would the test be more convincing and stronger, i

Re: [ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-11 Thread Darrell Ball 
Thanks Yi-hung
One minor comment inline.


On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei  wrote:

> Currently, using ofproto/trace to trace a datapath flow with eth_type()
> but without eth() may hit unexpected behavior because OVS sets
> the packet_type to be (1, eth_type) when decoding the odp flow key.
> This patch updates the logic of odp flow key decoding so that the
> packet_type defaults to (0,0) unless user explicitly specifies the
> packet_type.  An unit test is added to prevent future regression.
>
> Travis build: https://travis-ci.org/YiHungWei/ovs/builds/351565383
>
> Reported-by: Amar Padmanabhan 
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece
> mber/045817.html
> Reported-by: Su Wang 
> VMWare-BZ: #2070488
> Signed-off-by: Yi-Hung Wei 
> ---
>  lib/odp-util.c|  8 
>  tests/ofproto-dpif.at | 24 
>  2 files changed, 24 insertions(+), 8 deletions(-)
>
> diff --git a/lib/odp-util.c b/lib/odp-util.c
> index 5da83b4c64c4..682f1d3bd4b5 100644
> --- a/lib/odp-util.c
> +++ b/lib/odp-util.c
> @@ -6440,14 +6440,6 @@ odp_flow_key_to_flow__(const struct nlattr *key,
> size_t key_len,
>  }
>  expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
>  }
> -else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
> -ovs_be16 ethertype = nl_attr_get_be16(attrs[OVS_KEY
> _ATTR_ETHERTYPE]);
> -if (!is_mask) {
> -flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
> -   ntohs(ethertype));
> -}
> -expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
> -}
>
>  /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */
>  if (!parse_ethertype(attrs, present_attrs, _attrs, flow,
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index d2058eddd3eb..13e57b90edd1 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -10162,3 +10162,27 @@ AT_CHECK([grep flow_del ovs-vswitchd.log], [1])
>
>  OVS_VSWITCHD_STOP
>  AT_CLEANUP
> +
> +AT_SETUP([ofproto-dpif - debug ofproto/trace])
> +OVS_VSWITCHD_START
> +add_of_ports br0 1 2 3 4
> +AT_DATA([flows.txt], [dnl
> +table=0 priority=100 in_port=1,udp actions=output:2
> +table=0 priority=99 in_port=1 actions=output:3
> +])
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.16
> 8.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout])
> +AT_CHECK([tail -1 stdout], [0],
> +  [Datapath actions: 2
> +])
> +
> +AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 "table=0 priority=100
> in_port=1,packet_type=(1,0x800) actions=output:4"])
> +
> +AT_CHECK([ovs-appctl ofproto/trace ovs-dummy
> 'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4
> (src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
> [0], [stdout])
> +AT_CHECK([cat stdout | grep output], [0],
> +  [output:4
> +])
> +
> +OVS_VSWITCHD_STOP
> +AT_CLEANUP
>


Would the test be more convincing and stronger, if all rules are present
before the first packet test ?
It is not required to check this specific issue, though.
Below is a short way to express this; otherwise LGTM.

AT_SETUP([ofproto-dpif - debug ofproto/trace])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 4
AT_DATA([flows.txt], [dnl
table=0 priority=100 in_port=1,udp actions=output:2
table=0 priority=100 in_port=1,packet_type=(1,0x800),actions=output:4
table=0 priority=99 in_port=1 actions=output:3
])
AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),eth_type(0x0800),i
pv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
[0], [stdout])
AT_CHECK([tail -1 stdout], [0],
  [Datapath actions: 2
])

AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 'in_port(1),packet_type(ns=1,i
d=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0
.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'], [0], [stdout])
AT_CHECK([cat stdout | grep output], [0],
  [output:4
])




> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow

2018-03-10 Thread Yi-Hung Wei 
Currently, using ofproto/trace to trace a datapath flow with eth_type()
but without eth() may hit unexpected behavior because OVS sets
the packet_type to be (1, eth_type) when decoding the odp flow key.
This patch updates the logic of odp flow key decoding so that the
packet_type defaults to (0,0) unless user explicitly specifies the
packet_type.  An unit test is added to prevent future regression.

Travis build: https://travis-ci.org/YiHungWei/ovs/builds/351565383

Reported-by: Amar Padmanabhan 
Reported-at: 
https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html
Reported-by: Su Wang 
VMWare-BZ: #2070488
Signed-off-by: Yi-Hung Wei 
---
 lib/odp-util.c|  8 
 tests/ofproto-dpif.at | 24 
 2 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 5da83b4c64c4..682f1d3bd4b5 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -6440,14 +6440,6 @@ odp_flow_key_to_flow__(const struct nlattr *key, size_t 
key_len,
 }
 expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERNET;
 }
-else if (present_attrs & (UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE)) {
-ovs_be16 ethertype = nl_attr_get_be16(attrs[OVS_KEY_ATTR_ETHERTYPE]);
-if (!is_mask) {
-flow->packet_type = PACKET_TYPE_BE(OFPHTN_ETHERTYPE,
-   ntohs(ethertype));
-}
-expected_attrs |= UINT64_C(1) << OVS_KEY_ATTR_ETHERTYPE;
-}
 
 /* Get Ethertype or 802.1Q TPID or FLOW_DL_TYPE_NONE. */
 if (!parse_ethertype(attrs, present_attrs, _attrs, flow,
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index d2058eddd3eb..13e57b90edd1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -10162,3 +10162,27 @@ AT_CHECK([grep flow_del ovs-vswitchd.log], [1])
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - debug ofproto/trace])
+OVS_VSWITCHD_START
+add_of_ports br0 1 2 3 4
+AT_DATA([flows.txt], [dnl
+table=0 priority=100 in_port=1,udp actions=output:2
+table=0 priority=99 in_port=1 actions=output:3
+])
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-flows br0 flows.txt])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: 2
+])
+
+AT_CHECK([ovs-ofctl -O OpenFlow15 add-flow br0 "table=0 priority=100 
in_port=1,packet_type=(1,0x800) actions=output:4"])
+
+AT_CHECK([ovs-appctl ofproto/trace ovs-dummy 
'in_port(1),packet_type(ns=1,id=0x800),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=17,tos=0,ttl=128,frag=no),udp(src=8,dst=9)'],
 [0], [stdout])
+AT_CHECK([cat stdout | grep output], [0],
+  [output:4
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
2.7.4

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev