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

Message ID 1520703034-3602-1-git-send-email-yihung.wei@gmail.com
State Not Applicable
Headers show
Series
  • [ovs-dev] odp-util: Fix ofproto/trace with odp flow
Related show

Commit Message

Yi-Hung Wei March 10, 2018, 5:30 p.m.
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 <amarpadmanabhan@fb.com>
Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html
Reported-by: Su Wang <suwang@vmware.com>
VMWare-BZ: #2070488
Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>
---
 lib/odp-util.c        |  8 --------
 tests/ofproto-dpif.at | 24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 8 deletions(-)

Comments

Darrell Ball March 11, 2018, 4:50 p.m. | #1
Thanks Yi-hung
One minor comment inline.


On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung.wei@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 <amarpadmanabhan@fb.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece
> mber/045817.html
> Reported-by: Su Wang <suwang@vmware.com>
> VMWare-BZ: #2070488
> Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Jan Scheurich March 12, 2018, 9:44 a.m. | #2
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, <EtherType>) 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-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung.wei@gmail.com>
> Cc: ovs dev <dev@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.wei@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 <amarpadmanabhan@fb.com>
> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece
> > mber/045817.html
> > Reported-by: Su Wang <suwang@vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Darrell Ball March 12, 2018, 3:56 p.m. | #3
On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on behalf of jan.scheurich@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, <EtherType>) 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-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball
    > Sent: Sunday, 11 March, 2018 17:51
    > To: Yi-Hung Wei <yihung.wei@gmail.com>
    > Cc: ovs dev <dev@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.wei@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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0&e=
    > >
    > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com>
    > > Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-2DDece&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw&e=
    > > mber/045817.html
    > > Reported-by: Su Wang <suwang@vmware.com>
    > > VMWare-BZ: #2070488
    > > Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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
    > > dev@openvswitch.org
    > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=
    > >
    > _______________________________________________
    > dev mailing list
    > dev@openvswitch.org
    > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=
Darrell Ball March 12, 2018, 8:09 p.m. | #4
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-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:

    
    
    On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on behalf of jan.scheurich@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, <EtherType>) 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-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball

        > Sent: Sunday, 11 March, 2018 17:51

        > To: Yi-Hung Wei <yihung.wei@gmail.com>

        > Cc: ovs dev <dev@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.wei@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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0&e=

        > >

        > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com>

        > > Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-2DDece&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw&e=

        > > mber/045817.html

        > > Reported-by: Su Wang <suwang@vmware.com>

        > > VMWare-BZ: #2070488

        > > Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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

        > > dev@openvswitch.org

        > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

        > >

        > _______________________________________________

        > dev mailing list

        > dev@openvswitch.org

        > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

        _______________________________________________
        dev mailing list
        dev@openvswitch.org
        https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=
        
    
    _______________________________________________
    dev mailing list
    dev@openvswitch.org
    https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=_n1cmOnJZ4cdgaFjS68PSPqhQCEp1zvReB55_c3AxBU&s=ZJDWaH3ooIsqgW7JPcX2leR3OKOtHHrJtn04Xf9KCV0&e=
Jan Scheurich March 12, 2018, 9:40 p.m. | #5
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,<Ethertype>)] and adjust the unit tests accordingly.

BR, Jan

> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com]

> Sent: Monday, 12 March, 2018 21:10

> To: Jan Scheurich <jan.scheurich@ericsson.com>; Darrell Ball <dlu998@gmail.com>; Yi-Hung Wei <yihung.wei@gmail.com>

> Cc: ovs dev <dev@openvswitch.org>; Jiri Benc (jbenc@redhat.com) <jbenc@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-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf

> of dball@vmware.com> wrote:

> 

> 

> 

>     On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on

> behalf of jan.scheurich@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, <EtherType>) 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-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball

>         > Sent: Sunday, 11 March, 2018 17:51

>         > To: Yi-Hung Wei <yihung.wei@gmail.com>

>         > Cc: ovs dev <dev@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.wei@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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0&e=

>         > >

>         > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com>

>         > > Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-

> 2DDece&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw&e=

>         > > mber/045817.html

>         > > Reported-by: Su Wang <suwang@vmware.com>

>         > > VMWare-BZ: #2070488

>         > > Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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

>         > > dev@openvswitch.org

>         > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

>         > >

>         > _______________________________________________

>         > dev mailing list

>         > dev@openvswitch.org

>         > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

>         _______________________________________________

>         dev mailing list

>         dev@openvswitch.org

>         https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

> 

> 

>     _______________________________________________

>     dev mailing list

>     dev@openvswitch.org

>     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

> 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

> uZnsw&m=_n1cmOnJZ4cdgaFjS68PSPqhQCEp1zvReB55_c3AxBU&s=ZJDWaH3ooIsqgW7JPcX2leR3OKOtHHrJtn04Xf9KCV0&e=

>
Darrell Ball March 13, 2018, 2:35 a.m. | #6
On 3/12/18, 2:40 PM, "Jan Scheurich" <jan.scheurich@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,<Ethertype>)] 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:dball@vmware.com]

    > Sent: Monday, 12 March, 2018 21:10

    > To: Jan Scheurich <jan.scheurich@ericsson.com>; Darrell Ball <dlu998@gmail.com>; Yi-Hung Wei <yihung.wei@gmail.com>

    > Cc: ovs dev <dev@openvswitch.org>; Jiri Benc (jbenc@redhat.com) <jbenc@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-bounces@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf

    > of dball@vmware.com> wrote:

    > 

    > 

    > 

    >     On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on

    > behalf of jan.scheurich@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, <EtherType>) 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-bounces@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball

    >         > Sent: Sunday, 11 March, 2018 17:51

    >         > To: Yi-Hung Wei <yihung.wei@gmail.com>

    >         > Cc: ovs dev <dev@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.wei@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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0&e=

    >         > >

    >         > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com>

    >         > > Reported-at: https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_pipermail_ovs-2Ddiscuss_2017-

    > 2DDece&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw&e=

    >         > > mber/045817.html

    >         > > Reported-by: Su Wang <suwang@vmware.com>

    >         > > VMWare-BZ: #2070488

    >         > > Signed-off-by: Yi-Hung Wei <yihung.wei@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, &expected_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

    >         > > dev@openvswitch.org

    >         > > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

    > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

    >         > >

    >         > _______________________________________________

    >         > dev mailing list

    >         > dev@openvswitch.org

    >         > https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

    > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

    >         _______________________________________________

    >         dev mailing list

    >         dev@openvswitch.org

    >         https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

    > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=hGW0i_7Oivfm676bIZ_rmhO7opRlSP34RNLffPB_Ys0&e=

    > 

    > 

    >     _______________________________________________

    >     dev mailing list

    >     dev@openvswitch.org

    >     https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-

    > 2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-

    > uZnsw&m=_n1cmOnJZ4cdgaFjS68PSPqhQCEp1zvReB55_c3AxBU&s=ZJDWaH3ooIsqgW7JPcX2leR3OKOtHHrJtn04Xf9KCV0&e=

    >
Jiri Benc March 13, 2018, 8:24 a.m. | #7
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 +0000, 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
Darrell Ball March 13, 2018, 4:36 p.m. | #8
On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc <jbenc@redhat.com> 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 +0000, 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
>
Ben Pfaff March 14, 2018, 7:54 p.m. | #9
On Tue, Mar 13, 2018 at 09:36:23AM -0700, Darrell Ball wrote:
> On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc <jbenc@redhat.com> 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.
Jiri Benc March 14, 2018, 8:15 p.m. | #10
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
Ben Pfaff March 14, 2018, 9:59 p.m. | #11
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 <amarpadmanabhan@fb.com>
> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html
> Reported-by: Su Wang <suwang@vmware.com>
> VMWare-BZ: #2070488
> Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com>

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.
Yi-Hung Wei March 14, 2018, 10:51 p.m. | #12
On Wed, Mar 14, 2018 at 2:59 PM, Ben Pfaff <blp@ovn.org> 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

Patch

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, &expected_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