[ovs-dev] [PATCH] odp-util: Fix ofproto/trace with odp flow
Darrell Ball
dball at vmware.com
Mon Mar 12 20:09:57 UTC 2018
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