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: Mon, 29 Oct 2018 18:26:58 +0000 Message-ID: <20181029182648.GA9272@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> <20181026030719.GB6434@mtidpdk.mti.labs.mlnx> <20181026215646.GC13615@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-AM5-obe.outbound.protection.outlook.com (mail-eopbgr00045.outbound.protection.outlook.com [40.107.0.45]) by dpdk.org (Postfix) with ESMTP id BFDED1E2F for ; Mon, 29 Oct 2018 19:26:59 +0100 (CET) In-Reply-To: Content-Language: en-US Content-ID: 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 Mon, Oct 29, 2018 at 02:33:03AM -0700, Slava Ovsiienko wrote: > > -----Original Message----- > > From: Yongseok Koh > > Sent: Saturday, October 27, 2018 0:57 > > 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 Fri, Oct 26, 2018 at 01:39:38AM -0700, Slava Ovsiienko wrote: > > > > -----Original Message----- > > > > From: Yongseok Koh > > > > Sent: Friday, October 26, 2018 6:07 > > > > To: Slava Ovsiienko > > > > Cc: Shahaf Shuler ; dev@dpdk.org > > > > Subject: Re: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validatio= n > > > > routine > > > > > > > > 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 > > > > > > > > > > > > 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; > > > > > > > > > > > > Can't it be inner then? > > > > > AFAIK, no for tc rules, we can not specify multiple levels (inne= r > > > > > + outer) for > > > > them. > > > > > There is just no TCA_FLOWER_KEY_xxx attributes for specifying > > > > > inner > > > > items > > > > > to match by flower. > > > > > > > > When I briefly read the kernel code, I thought TCA_FLOWER_KEY_* are > > > > for inner 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. > > > > > > Hm. Interesting. I will check. > > > > > > > > It is quite unclear comment, not the best one, sorry. I did not > > > > > like it too, just forgot to rewrite. > > > > > > > > > > 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 li= st. > > > > > > > > > > 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 > > > > > > > > > > > > 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 > > > > > > redundant outer items. > > > > > > Did I miss something? > > > > > > > > > > Interesting, besides rule has correct syntax, I'm not sure whethe= r > > > > > it can be > > > > applied w/o errors. > > > > > > > > Please try. You owns this patchset. However, you just can prohibit > > > > such flows (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 don'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 rechec= k > > > > > - we > > > > should not allow > > > > > unsupported items to pass the validation. I'll check and provide > > > > > the > > > > separate bugfix patch > > > > > (if any). > > > > > > > > Neither has tunnel support. It is the first time to add tunnel supp= ort to > > TCF. > > > > 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. > > > > > > Yes. I understood your point. Will check and add tunnel support for T= CF > > rules. > > > Anyway, inner MAC addresses are supported for VXLAN decap, I think we > > > should specify these ones in the rule as inners (after VNI item), > > > definitely some tunnel support in validate/parse/translate should be = added. > > > > > > > > > > > > > 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 first > > > > 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 > > > > discovered > > > > > a lot. I'll recheck the latest code commits, possible it became > > > > > more > > > > appropriate > > > > > for VXLAN. > > > > > > > > Agreed. I'm not forcing you to do it because we run out of time but > > > > mentioned it because if there's any redundancy in our code, that > > > > usually causes bug later. > > > > Let's not waste too much time for that. Just grab low hanging fruit= s if > > any. > > > > > > > > > > to add tunneled item, but Verbs/DV already have validation code > > > > > > for > > > > tunnel, > > > > > > 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. > > > > > > > > > > > > And if you need to know whether there's the VXLAN decap action > > > > > > prior to outer header item validation, you can relocate the cod= e > > > > > > - action > > > > validation > > > > > > first and item validation next, as there's no dependency yet in > > > > > > the current > > > > > > > > > > We can not validate action first - we need items to be preliminar= y > > > > gathered, > > > > > to check them in action's specific fashion and to check action it= self. > > > > > I mean, if we see VXLAN decap action, we should check the presenc= e > > > > > 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. > > > > > > > > > > > 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 an= d > > > > > checking > > > > > - scan items, performing variating check (depending on gathered > > > > > action > > > > flags) > > > > > - scan actions again, performing full check with params (at least > > > > > for now check whether all params gathered) > > > > > > > > Disagree. flow_tcf_validate_vxlan_encap() doesn't even need any inf= o > > > > of items 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 > > > How we could validate VXLAN decap at this stage? > > > As we do not have item_flags set yet? > > > Do I miss something? > >=20 > > Look at my pseudo code above. > > Nothing much to be done in validating decap action. And item validation= for > > decap can be done together in item validation code. > >=20 > VXLAB decap action should check: > - whether outer destination UDP port is present (otherwise we cannot assi= gn VTEP VXLAN) > - whether outer destination IP is present (otherwise we cannot assign IP = to ifouter/build route) > - whether VNI is present (to identify VXLAN traffic) >=20 > How do you propose check these issues in your approach? Did you look at my pseudo code? We are not validating vxlan decap action it= self but items when vxlan decap is present. { 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) { /* * check whether outer destination IP is present */ } break; ... case RTE_FLOW_ITEM_TYPE_UDP: /* Existing common validation. */ ... if (action_flags & MLX5_ACTION_VXLAN_DECAP) { /* * check whether outer destination UDP port is * present */ } break; ... case RTE_FLOW_ITEM_TYPE_VXLAN: /* Do the same for vni. */ } ... if (action_flags & MLX5_ACTION_VXLAN_DECAP) { if (!(items_flags & MLX5_FLOW_LAYER_OUTER_L3_IPV4 || ... IPV6)) return rte_flow_error_set (error, EINVAL, RTE_FLOW_ERROR_TYPE_ITEM, items, "vxlan decap requires item IP"); if (!(items_flags & MLX5_FLOW_LAYER_OUTER_L4_UDP)) return rte_flow_error_set (error, EINVAL, RTE_FLOW_ERROR_TYPE_ITEM, items, "vxlan decap requires item UDP"); if (!(items_flags & MLX5_FLOW_LAYER_VXLAN)) /* Do the same . */ } } Still problem? Thanks, Yongseok