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:[email protected]] > Sent: Monday, 12 March, 2018 21:10 > To: Jan Scheurich <[email protected]>; Darrell Ball > <[email protected]>; Yi-Hung Wei <[email protected]> > Cc: ovs dev <[email protected]>; Jiri Benc ([email protected]) > <[email protected]> > 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, "[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
