From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yongseok Koh Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation routine Date: Fri, 26 Oct 2018 03:07:28 +0000 Message-ID: <20181026030719.GB6434@mtidpdk.mti.labs.mlnx> References: <1538461807-37507-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-1-git-send-email-viacheslavo@mellanox.com> <1539612815-47199-3-git-send-email-viacheslavo@mellanox.com> <20181023100424.GB14792@mtidpdk.mti.labs.mlnx> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable Cc: Shahaf Shuler , "dev@dpdk.org" To: Slava Ovsiienko Return-path: Received: from EUR02-HE1-obe.outbound.protection.outlook.com (mail-eopbgr10084.outbound.protection.outlook.com [40.107.1.84]) by dpdk.org (Postfix) with ESMTP id 237B1532C for ; Fri, 26 Oct 2018 05:07:31 +0200 (CEST) In-Reply-To: Content-Language: en-US Content-ID: <45C5508570C8BB4E897107CED3849CFF@eurprd05.prod.outlook.com> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Oct 25, 2018 at 06:53:11AM -0700, Slava Ovsiienko wrote: > > -----Original Message----- > > From: Yongseok Koh > > Sent: Tuesday, October 23, 2018 13:05 > > To: Slava Ovsiienko > > Cc: Shahaf Shuler ; dev@dpdk.org > > Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation > > routine > >=20 > > On Mon, Oct 15, 2018 at 02:13:30PM +0000, Viacheslav Ovsiienko wrote: [...] > > > @@ -1114,7 +1733,6 @@ struct pedit_parser { > > > error); > > > if (ret < 0) > > > return ret; > > > - item_flags |=3D MLX5_FLOW_LAYER_OUTER_L3_IPV4; > > > mask.ipv4 =3D flow_tcf_item_mask > > > (items, &rte_flow_item_ipv4_mask, > > > &flow_tcf_mask_supported.ipv4, > > > @@ -1135,13 +1753,22 @@ struct pedit_parser { > > > next_protocol =3D > > > ((const struct rte_flow_item_ipv4 *) > > > (items->spec))->hdr.next_proto_id; > > > + if (item_flags & > > MLX5_FLOW_LAYER_OUTER_L3_IPV4) { > > > + /* > > > + * Multiple outer items are not allowed as > > > + * tunnel parameters, will raise an error later. > > > + */ > > > + ipv4 =3D NULL; > >=20 > > Can't it be inner then? > AFAIK, no for tc rules, we can not specify multiple levels (inner + oute= r) for them. > There is just no TCA_FLOWER_KEY_xxx attributes for specifying inner item= s=20 > to match by flower. When I briefly read the kernel code, I thought TCA_FLOWER_KEY_* are for inn= er header before decap. I mean TCA_FLOWER_KEY_IPV4_SRC is for inner L3 and TCA_FLOWER_KEY_ENC_IPV4_SRC is for outer tunnel header. Please do some experiments with tc-flower command. > It is quite unclear comment, not the best one, sorry. I did not like it t= oo,=20 > just forgot to rewrite. >=20 > ipv4, ipv6 , udp variables gather the matching items during the item list= scanning, > later variables are used for VXLAN decap action validation only. So, the = "outer" > means that ipv4 variable contains the VXLAN decap outer addresses, and > should be NULL-ed if multiple items are found in the items list.=20 >=20 > But we can generate an error here if we have valid action_flags > (gathered by prepare function) and VXLAN decap is set. Raising > an error looks more relevant and clear. You can't use flags at this point. It is validate() so prepare() might not = be preceded. > > flow create 1 ingress transfer > > pattern eth src is 66:77:88:99:aa:bb > > dst is 00:11:22:33:44:55 / ipv4 src is 2.2.2.2 dst is 1.1.1.1 / > > udp src is 4789 dst is 4242 / vxlan vni is 0x112233 / > > eth / ipv6 / tcp dst is 42 / end > > actions vxlan_decap / port_id id 2 / end > >=20 > > Is this flow supported by linux tcf? I took this example from Adrien's = patch - > > "[8/8] net/mlx5: add VXLAN decap support to switch flow rules". If so, = isn't it > > possible to have inner L3 layer (MLX5_FLOW_LAYER_INNER_*)? If not, you > > should return error in this case. I don't see any code to check redunda= nt > > outer items. > > Did I miss something? >=20 > Interesting, besides rule has correct syntax, I'm not sure whether it can= be applied w/o errors. Please try. You owns this patchset. However, you just can prohibit such flo= ws (tunneled item) and come up with follow-up patches to enable it later if it= is support by tcf as this whole patchset itself is pretty huge enough and we d= on't have much time. > At least our current flow_tcf_translate() implementation does not support= any INNERs. > But it seems the flow_tcf_validate() does, it's subject to recheck - we s= hould not allow > unsupported items to pass the validation. I'll check and provide the sepa= rate bugfix patch > (if any). Neither has tunnel support. It is the first time to add tunnel support to T= CF. If it was needed, you should've added it, not skipping it. You can check how MLX5_FLOW_LAYER_TUNNEL is used in Verbs/DV as a reference= . > > BTW, for the tunneled items, why don't you follow the code of > > Verbs(mlx5_flow_verbs.c) and DV(mlx5_flow_dv.c)? For tcf, it is the fir= st time > For VXLAN it has some specifics (warning about ignored params, etc.) > I've checked which of verbs/dv code could be reused and did not discovere= d > a lot. I'll recheck the latest code commits, possible it became more appr= opriate > for VXLAN.=20 Agreed. I'm not forcing you to do it because we run out of time but mention= ed it because if there's any redundancy in our code, that usually causes bug late= r. Let's not waste too much time for that. Just grab low hanging fruits if any= . > > to add tunneled item, but Verbs/DV already have validation code for tun= nel, > > so you can reuse the existing code. In flow_tcf_validate_vxlan_decap(),= not > > every validation is VXLAN-specific but some of them can be common code. > >=20 > > And if you need to know whether there's the VXLAN decap action prior to > > outer header item validation, you can relocate the code - action valida= tion > > first and item validation next, as there's no dependency yet in the cur= rent >=20 > We can not validate action first - we need items to be preliminary gather= ed, > to check them in action's specific fashion and to check action itself.=20 > I mean, if we see VXLAN decap action, we should check the presence of > L2, L3, L4 and VNI items. I minimized the number of passes along the item > and action lists. BTW, Adrien's approach performed two passes, mine does = only. >=20 > > code. Defining ipv4, ipv6, udp seems to make the code path more complex= . > Yes, but it allows us to avoid the extra item list scanning and minimizes= the changes > of existing code. > In your approach we should: > - scan actions, w/o full checking, just action_flags gathering and checki= ng > - scan items, performing variating check (depending on gathered action fl= ags) > - scan actions again, performing full check with params (at least for now= =20 > check whether all params gathered) Disagree. flow_tcf_validate_vxlan_encap() doesn't even need any info of ite= ms and flow_tcf_validate_vxlan_decap() needs item_flags to check whether VXLAN item is there or not and ipv4/ipv6/udp are all for item checks. Let me give= you very detailed exmaple: { for (actions[]...) { ... case RTE_FLOW_ACTION_TYPE_VXLAN_ENCAP: ... flow_tcf_validate_vxlan_encap(); ... break; case RTE_FLOW_ACTION_TYPE_VXLAN_DECAP: if (action_flags & (MLX5_ACTION_VXLAN_ENCAP | MLX5_ACTION_VXLAN_DECAP)) return rte_flow_error_set (error, ENOTSUP, RTE_FLOW_ERROR_TYPE_ACTION, actions, "can't have multiple vxlan actions"); /* Don't call flow_tcf_validate_vxlan_decap(). */ action_flags |=3D MLX5_ACTION_VXLAN_DECAP; break; } for (items[]...) { ... case RTE_FLOW_ITEM_TYPE_IPV4: /* Existing common validation. */ ... if (action_flags & MLX5_ACTION_VXLAN_DECAP) { /* Do ipv4 validation in * flow_tcf_validate_vxlan_decap()/ } break; } } Curretly you are doing, - validate items - validate actions - validate items again if decap. But this can simply be - validate actions - validate items Thanks, Yongseok > >=20 > > For example, you just can call vxlan decap item validation (by splittin= g > > flow_tcf_validate_vxlan_decap()) at this point like: > >=20 > > if (action_flags & > > MLX5_FLOW_ACTION_VXLAN_DECAP) > > ret =3D > > flow_tcf_validate_vxlan_decap_ipv4(...); > > ... > >=20 > > Same for other items. > >=20