| Message ID | 1520703034-3602-1-git-send-email-yihung.wei@gmail.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [ovs-dev] odp-util: Fix ofproto/trace with odp flow | expand |
Thanks Yi-hung One minor comment inline. On Sat, Mar 10, 2018 at 9:30 AM, Yi-Hung Wei <yihung.wei@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://travis-ci.org/YiHungWei/ovs/builds/351565383 > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece > mber/045817.html > Reported-by: Su Wang <suwang@vmware.com> > VMWare-BZ: #2070488 > Signed-off-by: Yi-Hung Wei <yihung.wei@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@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev >
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(). 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@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball > Sent: Sunday, 11 March, 2018 17:51 > To: Yi-Hung Wei <yihung.wei@gmail.com> > Cc: ovs dev <dev@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@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://travis-ci.org/YiHungWei/ovs/builds/351565383 > > > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece > > mber/045817.html > > Reported-by: Su Wang <suwang@vmware.com> > > VMWare-BZ: #2070488 > > Signed-off-by: Yi-Hung Wei <yihung.wei@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@openvswitch.org > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on behalf of jan.scheurich@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@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung.wei@gmail.com>
> Cc: ovs dev <dev@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@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@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@vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Yi-Hung Wei <yihung.wei@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@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@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@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=
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@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf of dball@vmware.com> wrote:
On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on behalf of jan.scheurich@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@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball
> Sent: Sunday, 11 March, 2018 17:51
> To: Yi-Hung Wei <yihung.wei@gmail.com>
> Cc: ovs dev <dev@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@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@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@vmware.com>
> > VMWare-BZ: #2070488
> > Signed-off-by: Yi-Hung Wei <yihung.wei@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@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@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@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@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=
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@vmware.com] > Sent: Monday, 12 March, 2018 21:10 > To: Jan Scheurich <jan.scheurich@ericsson.com>; Darrell Ball <dlu998@gmail.com>; Yi-Hung Wei <yihung.wei@gmail.com> > Cc: ovs dev <dev@openvswitch.org>; Jiri Benc (jbenc@redhat.com) <jbenc@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@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf > of dball@vmware.com> wrote: > > > > On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on > behalf of jan.scheurich@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@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball > > Sent: Sunday, 11 March, 2018 17:51 > > To: Yi-Hung Wei <yihung.wei@gmail.com> > > Cc: ovs dev <dev@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@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@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@vmware.com> > > > VMWare-BZ: #2070488 > > > Signed-off-by: Yi-Hung Wei <yihung.wei@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@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@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@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@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= >
On 3/12/18, 2:40 PM, "Jan Scheurich" <jan.scheurich@ericsson.com> wrote:
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.
[Darrell] In general, that is true; they don’t need to match, per se.
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.
[Darrell] I guess part of the value of ofproto/trace is that it matches as close as possible to other injection sources.
BR, Jan
> -----Original Message-----
> From: Darrell Ball [mailto:dball@vmware.com]
> Sent: Monday, 12 March, 2018 21:10
> To: Jan Scheurich <jan.scheurich@ericsson.com>; Darrell Ball <dlu998@gmail.com>; Yi-Hung Wei <yihung.wei@gmail.com>
> Cc: ovs dev <dev@openvswitch.org>; Jiri Benc (jbenc@redhat.com) <jbenc@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@openvswitch.org on behalf of Darrell Ball" <ovs-dev-bounces@openvswitch.org on behalf
> of dball@vmware.com> wrote:
>
>
>
> On 3/12/18, 2:44 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jan Scheurich" <ovs-dev-bounces@openvswitch.org on
> behalf of jan.scheurich@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@openvswitch.org [mailto:ovs-dev-bounces@openvswitch.org] On Behalf Of Darrell Ball
> > Sent: Sunday, 11 March, 2018 17:51
> > To: Yi-Hung Wei <yihung.wei@gmail.com>
> > Cc: ovs dev <dev@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@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@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@vmware.com>
> > > VMWare-BZ: #2070488
> > > Signed-off-by: Yi-Hung Wei <yihung.wei@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@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@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@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@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=
>
Darell, please fix your email client configuration to conform to RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your replies are unreadable. In particular, the text you're replying to must be quoted by ">" character, clearly separating your reply from the original email. Also, wrap your lines at ~78 characters. If your email client does not support this, it is severely broken and you'll need to use a different one. On Tue, 13 Mar 2018 02:35:57 +0000, Darrell Ball wrote: > [Darrell] I guess part of the value of ofproto/trace is that it > matches as close as possible to other injection sources. Kernel datapath API has nothing to do with OpenFlow. In particular, using output from ovs-dpctl dump-flows in a place where OpenFlow is required could not be expected to work. Jiri
On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc <jbenc@redhat.com> wrote: > Darell, please fix your email client configuration to conform to > RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your > replies are unreadable. In particular, the text you're replying to must > be quoted by ">" character, clearly separating your reply from the > original email. > > Also, wrap your lines at ~78 characters. > > If your email client does not support this, it is severely broken and > you'll need to use a different one. > [Darrell] hmm, I thought when sending from my company e-mail client with a prefix annotation would help identify the response clearly. For every other receiver, that seems work. Maybe, gmail client sender works better for you. > > On Tue, 13 Mar 2018 02:35:57 +0000, Darrell Ball wrote: > > [Darrell] I guess part of the value of ofproto/trace is that it > > matches as close as possible to other injection sources. > > Kernel datapath API has nothing to do with OpenFlow. In particular, > using output from ovs-dpctl dump-flows in a place where OpenFlow is > required could not be expected to work. > [Darrell] There was no suggestion otherwise in general. We were discussing for one the difference b/w kernel and userspace DP for handling packet type aware support. Specifically, the difference is here is as fundamental as it gets - eth vs non-eth, The discussed support for ofproto/trace is also in that context; see Jan's response. If you are interested you can read the previous annotated responses and if you have an alternative suggestion, then that would be helpful. > > Jiri >
On Tue, Mar 13, 2018 at 09:36:23AM -0700, Darrell Ball wrote: > On Tue, Mar 13, 2018 at 1:24 AM, Jiri Benc <jbenc@redhat.com> wrote: > > > Darell, please fix your email client configuration to conform to > > RFC 3676 (https://tools.ietf.org/html/rfc3676#section-4.5). Your > > replies are unreadable. In particular, the text you're replying to must > > be quoted by ">" character, clearly separating your reply from the > > original email. > > > > Also, wrap your lines at ~78 characters. > > > > If your email client does not support this, it is severely broken and > > you'll need to use a different one. > > > > > [Darrell] hmm, I thought when sending from my company e-mail client > with a prefix annotation would help identify the response > clearly. > For every other receiver, that seems work. > Maybe, gmail client sender works better for you. Thanks for quoting in the customary way. However, when you do that, you can omit the [Darrell] prefix because it is distracting.
On Tue, 13 Mar 2018 09:36:23 -0700, Darrell Ball wrote: > [Darrell] There was no suggestion otherwise in general. We were discussing > for one the difference b/w kernel and userspace DP for handling packet type > aware support. Specifically, the difference is here is as fundamental as it > gets - > eth vs non-eth, The discussed support for ofproto/trace is also in that > context; > see Jan's response. Ah, right, sorry. So, the problem is that the two datapaths express the same thing (packet type) differently, right? I think the solution would then be to insert some kind of translation. ovs-dpctl dump-flows for the kernel probably should output the flow in a format that ofproto/trace expects and that matches the user space datapath. (And reverse that in the other direction.) Jiri
On Sat, Mar 10, 2018 at 09:30:34AM -0800, Yi-Hung Wei 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://travis-ci.org/YiHungWei/ovs/builds/351565383 > > Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com> > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html > Reported-by: Su Wang <suwang@vmware.com> > VMWare-BZ: #2070488 > Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> I posted an alternative proposal: https://patchwork.ozlabs.org/patch/886052/ I haven't checked that this solves the problem, so I would appreciate testing.
On Wed, Mar 14, 2018 at 2:59 PM, Ben Pfaff <blp@ovn.org> wrote: > I posted an alternative proposal: > https://patchwork.ozlabs.org/patch/886052/ > > I haven't checked that this solves the problem, so I would appreciate > testing. Thanks for this fix. With your patch, eth() is printed with eth_type() for the kernel datapath flow, and that should fix the ofproto/trace issue. Thanks, -Yi-Hung
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.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-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
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://travis-ci.org/YiHungWei/ovs/builds/351565383 Reported-by: Amar Padmanabhan <amarpadmanabhan@fb.com> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-December/045817.html Reported-by: Su Wang <suwang@vmware.com> VMWare-BZ: #2070488 Signed-off-by: Yi-Hung Wei <yihung.wei@gmail.com> --- lib/odp-util.c | 8 -------- tests/ofproto-dpif.at | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 8 deletions(-)