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, <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-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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=PHty08hjdhUdJIgcacPKZyKjC5rUHFG5gWM-O6EZUU0&e=
    > >
    > > 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&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih-uZnsw&m=EEDToTtT2qc1tDhIAzYOZCZTQDO66nYE94e7Q37BpoM&s=5QgkRm0VBM6ab9rueZoN0ilXbLBsUWblIih0LrEY7Vw&e=
    > > 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, &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
    > > d...@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
    > d...@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
    d...@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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to