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://mail.openvswitch.org/mailman/listinfo/ovs-dev
