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, "[email protected] on behalf of Darrell Ball" <[email protected] on behalf of [email protected]> wrote: On 3/12/18, 2:44 AM, "[email protected] on behalf of Jan Scheurich" <[email protected] on behalf of [email protected]> 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: [email protected] [mailto:[email protected]] On Behalf Of Darrell Ball > Sent: Sunday, 11 March, 2018 17:51 > To: Yi-Hung Wei <[email protected]> > Cc: ovs dev <[email protected]> > 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 <[email protected]> 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 <[email protected]> > > 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 <[email protected]> > > VMWare-BZ: #2070488 > > Signed-off-by: Yi-Hung Wei <[email protected]> > > --- > > 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 > > [email protected] > > 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 > [email protected] > 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 [email protected] 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 [email protected] 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= _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
