From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matan Azrad Subject: Re: [PATCH] net/mlx5: fix GRE flow rule Date: Thu, 24 May 2018 06:34:01 +0000 Message-ID: References: <20180523015157.35716-1-yskoh@mellanox.com> <20180523100116.GA11530@minint-98vp2qg> <20180523183359.GA13339@yongseok-MBP.local> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Cc: Shahaf Shuler , Adrien Mazarguil , =?iso-8859-1?Q?N=E9lio_Laranjeiro?= , "dev@dpdk.org" , "stable@dpdk.org" , "Xueming(Steven) Li" To: Yongseok Koh Return-path: In-Reply-To: <20180523183359.GA13339@yongseok-MBP.local> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi Yongseok From: Yongseok Koh > On Wed, May 23, 2018 at 04:45:33AM -0700, Matan Azrad wrote: > > > > Hi Yongseok > > + Steven > > > > From: Yongseok Koh > > > On Tue, May 22, 2018 at 10:36:43PM -0700, Matan Azrad wrote: > > > > Hi Yongseok > > > > > > > > From: Yongseok Koh > > > > > +#ifdef HAVE_IBV_DEVICE_MPLS_SUPPORT > > > > > > > > The GRE item was here even before the MPLSoGRE support > > > > > > Yes, this bug has existed before adding MPLSoGRE support. > > > > > > > so I think that this is not the correct fix and even that it can > > > > hurt the support of GRE for the current customers use it. > > > > > > How can it hurt? Please clarify. > > > > Someone who uses the next flow and have not the new verbs version of MP= LS: > > flow create 0 ingress pattern gre / ipv4 src is X / end actions ... > > ipv4 src or any other inner specifications. > > > > This flow will probably get any supported tunnel packets with inner ipv= 4 src =3D > X. >=20 > Do you think we should compromise?=20 For sure, no, I'm just want the correct fix, like you :)=20 >This is logically wrong for sure. Let me > give you a specific example. >=20 > If I create the following two flows, >=20 > flow create 0 ingress pattern vxlan vni is 10 / end actions queue index= 3 / > mark id 10 / end > flow create 0 ingress pattern vxlan vni is 20 / end actions queue index= 3 / > mark id 20 / end >=20 > The following two packets will match correctly and have flow ID (10 and 2= 0) > according to VNI. >=20 > Ether()/IP()/UDP()/VXLAN(vni=3D10)/Ether()/IPv6() > Ether()/IP()/UDP()/VXLAN(vni=3D20)/Ether()/IPv6() >=20 > However, if three flows are created as follows, >=20 > flow create 0 ingress pattern gre / ipv6 / end actions queue index 3 / = mark id > 2 / end > flow create 0 ingress pattern vxlan vni is 10 / end actions queue index= 3 / > mark id 10 / end > flow create 0 ingress pattern vxlan vni is 20 / end actions queue index= 3 / > mark id 20 / end >=20 > The packets will hit the first flow regardless of VNI and have wrong flow= ID. > That's why I have to drop this specific GRE case. Whoever is using this k= ind of > GRE flow, that's buggy anyway. They have to know it and change it. I have just checked, the next flow under MPLS support doesn't get neither G= RE packet nor non GRE packet: flow create 0 ingress pattern gre / end actions queue index 3 / mark id 2 / end The issue is that in the first implementation of GRE tunnel we didn't force= d L3 next protocol to be GRE in this case because L3 is not in the pattern. > > It may be enough for the current user (which probably use only 1 tunnel= type > at a certain time). >=20 > Router/switch-like applications can have multiple tunnels for sure. I'm n= ot sure > who 'the current user' is but I don't think we can make such an assumptio= n. > I don't want to allow users create faulty flows. I never take such like assumption, just saying...(you can forget it) The point is that we may have a customer which the current behavior of=20 flow create 0 ingress pattern gre / ipv6 .../ end actions queue index 3 / m= ark id 2 / end is good for him and now after this fix it will fail in port creation(low pr= obability). >=20 > > > > Looks like you must specify at least 1 spec in the GRE to apply it > > > > correctly as you did for VXLAN, Can you try empty vxlan and fully > > > > gre (with protocol field)? > > > > > > That's exactly the reason why I'm taking this out. If you look at > > > the code, it doesn't even set any field for GRE if > > > HAVE_IBV_DEVICE_MPLS_SUPPORT isn't supported. Thus, it is considered > > > as a wildcard (all-matching) rule. But if it has > HAVE_IBV_DEVICE_MPLS_SUPPORT, such pattern can be allowed. > > > > Yes, so your GRE flow will not work even if you have MPLS support. >=20 > I'm not sure what you meant but with IBV MPLS support, I think > IBV_FLOW_SPEC_GRE will make things right. Without the support, > IBV_FLOW_SPEC_VXLAN_TUNNEL is even set for GRE flows. >=20 > > I think the issue is generally in all the items: > > You should not configure them if they miss both at least one > > self-specification or item which points to them by "next protocol" fiel= d. > > > > In case of VXLAN tunnels we just don't allow them without > > self-specification, In case of gre we force the next protocol of the pr= evious > item but only when it exists. > > In case of eth(inner),vlan,ipv4,ipv6,udp,tcp we don't force anything. > > > > I think we need a global fix for all, this is probably the root cause. >=20 > Well, the root-cause is that old device/lib doesn't differentiate GRE fro= m VXLAN > when creating flows. OK, let's continue with GRE only and will discuss about other protocol late= r in the next release. I think that the root cause is at least lack of GRE detection by next proto= col field in the outer L3 in case of first GRE item(verbs limitation). We can remove this option at all or to fix it by forcing L3 next protocol = =3D GRE in this case as done when we have L3 item before the GRE. =20