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: [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://travis-ci.org/YiHungWei/ovs/builds/351565383 > > > > Reported-by: Amar Padmanabhan <[email protected]> > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2017-Dece > > 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://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
