[ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
Jan Scheurich
jan.scheurich at ericsson.com
Mon Mar 12 21:40:12 UTC 2018
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 at vmware.com]
> Sent: Monday, 12 March, 2018 21:10
> To: Jan Scheurich <jan.scheurich at ericsson.com>; Darrell Ball <dlu998 at gmail.com>; Yi-Hung Wei <yihung.wei at gmail.com>
> Cc: ovs dev <dev at openvswitch.org>; Jiri Benc (jbenc at redhat.com) <jbenc at 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 at openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces at openvswitch.org on behalf
> of dball at vmware.com> wrote:
>
>
>
> On 3/12/18, 2:44 AM, "ovs-dev-bounces at openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces at openvswitch.org on
> behalf of jan.scheurich at 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 at openvswitch.org [mailto:ovs-dev-bounces at openvswitch.org] On Behalf Of Darrell Ball
> > Sent: Sunday, 11 March, 2018 17:51
> > To: Yi-Hung Wei <yihung.wei at gmail.com>
> > Cc: ovs dev <dev at 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 at 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 at 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 at vmware.com>
> > > VMWare-BZ: #2070488
> > > Signed-off-by: Yi-Hung Wei <yihung.wei at 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 at 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 at 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 at 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 at 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=
>
More information about the dev
mailing list