From mboxrd@z Thu Jan 1 00:00:00 1970 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) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=PUlSpd2aYsE+4rWp9Y1p2wSoQaBfcc6VhgpAP/rJ6jE=; b=LHxzOk+srbDrCcMjpLZrBRKHGTkRtYDdmMswZK4prc5aWWZQ4S+Tu3Qq+fdB5mN3U0OlAVrpXeHS9w5U4v/WCMJ/8cuOIXxJu7mOjAnvJEphpzDsFJk8MKNhRiaxLAZu9Qbq+LYNXys1aca4qFalJAatI3qWQlVfYSCqlkjwIGQ= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB4009.eurprd05.prod.outlook.com (52.134.72.138) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1273.25; Fri, 26 Oct 2018 03:07:28 +0000 Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::f8a1:fcab:94f0:97cc]) by DB3PR0502MB3980.eurprd05.prod.outlook.com ([fe80::f8a1:fcab:94f0:97cc%4]) with mapi id 15.20.1273.021; Fri, 26 Oct 2018 03:07:28 +0000 From: Yongseok Koh To: Slava Ovsiienko CC: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation routine Thread-Index: AQHUZJFYDUCLqgXH1E2+cFNxcMrsUaUsMREAgAPZ7oCAAN3iAA== 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> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: BYAPR07CA0042.namprd07.prod.outlook.com (2603:10b6:a03:60::19) To DB3PR0502MB3980.eurprd05.prod.outlook.com (2603:10a6:8:10::27) authentication-results: spf=none (sender IP is ) smtp.mailfrom=yskoh@mellanox.com; x-ms-exchange-messagesentrepresentingtype: 1 x-originating-ip: [209.116.155.178] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; DB3PR0502MB4009; 6:UL1tzuX/+eTwCSuaP3vfG7VvZbOlojJdA6Gh5Y07HaSmkhkMvSY17wqgxHPOIVI3yO4muT9bQImmcztyUGo8XckHzuR7kzL2/Y2WfIIyLHs1P/aZRV9UNH6eBr3gOfB28gVJWsHWiQqxS/cFSpPisY9Z4G0QRxj+SBo5Yg3aO19s9ClvVUjObzLPA7iXD/9IBFPX9A+d/5jKrQPMlm574VdUm/KLTuNmQ3V7IDwwX6sp3TPD8DKy9uMs6xCkVXdJ9aam2JnPoWVT7Nf11DYZHGbQTmyS5HFFjtryHPYMar8AO4Vo4OyZYD26kGx2KWzvflIjDWgRINhAWZZvmCK2C9wyIp6n4zUhnfbuRn0JnEBUsbbsfR8dsG01kXVO40LbaPMNLaXxrkBK4FFvhXW1f0Djg30jFyDEZ2NCrP3ycuvhMJINZWc/g5dCFYp8g2WQIqErps8cCJuCBfDpnFhokQ==; 5:6a0BCEWzcW7pA8qYOhP4/V3gyWpwZsjPoqVR1MDwMoe+eSJ0ufaLHZxh8NivOXDwZcFcacYfHML4VQE7zFGPA4B6Q4dwPLhPebVtAqMN8K1GGYuAvIU77oKmGIUkAkmjFqCsiphRgu4+85Gnri9d3L2mV0y/ok+GNHWbDimP1MA=; 7:sCHTGCB+jYSENv0WvtqA7mQgvk01CD0fiuNzFUzn3Qo55G8mITGsxNuHN6bGPo7NomQYMBWhRgZ8XafrmlCnCQGmUKLC4/6q6NY/j/F/NOWTdc8WmOsrW6BYGF3SGCvPiuuaeivnOm0Ku6ewt+PZ6A== x-ms-office365-filtering-correlation-id: 40a4be75-7520-4769-2423-08d63af0295b x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:DB3PR0502MB4009; x-ms-traffictypediagnostic: DB3PR0502MB4009: x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-ms-exchange-senderadcheck: 1 x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(8211001083)(6040522)(2401047)(5005006)(8121501046)(3002001)(10201501046)(3231355)(944501410)(52105095)(93006095)(93001095)(6055026)(148016)(149066)(150057)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123560045)(20161123558120)(20161123564045)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB4009; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB4009; x-forefront-prvs: 083751FCA6 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(39860400002)(396003)(346002)(376002)(136003)(199004)(189003)(13464003)(99286004)(316002)(6862004)(54906003)(81166006)(4326008)(26005)(186003)(6486002)(33896004)(6512007)(9686003)(6506007)(386003)(486006)(476003)(11346002)(446003)(229853002)(305945005)(7736002)(66066001)(106356001)(6636002)(6246003)(8676002)(81156014)(86362001)(14444005)(256004)(8936002)(25786009)(105586002)(53936002)(68736007)(5660300001)(478600001)(97736004)(6116002)(71190400001)(2900100001)(71200400001)(5250100002)(2906002)(76176011)(93886005)(52116002)(102836004)(53546011)(6436002)(1076002)(3846002)(14454004)(33656002)(21314003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB4009; H:DB3PR0502MB3980.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: eVZCrZybJEnYiHPogpdwwjnY9Azqc1svA08qQ8fjGeUraBuwe6nBrsjNQpHOekej3LPxWwGW1RpJULi+xQJBzL6FxrbKH0k6qaQh9butKLBRHxDObwYx9z/KFE/ndp0FBdvUXT9fr3mcKnZFM+XxOR30xv4AyNsV7Sjfnu4rO4FcaFG7qOXii/GzdzKB1seVTbWFZuNUnWZtIIgKCFob0sAeLmFaxEzbXdWaio+FNPwvJTtWcgvKrhAz0LC1scFl8LuRFo7a3qQFjuIBDqKFTVtGo/IM7G+llLVEMlzvGsKM3kWwYwX8VrjXOTpUGDbkdU4J0iIfFeVlJndR8tIY6WpiUPxRN8cIbyUXGi3JEfY= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-ID: <45C5508570C8BB4E897107CED3849CFF@eurprd05.prod.outlook.com> Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 40a4be75-7520-4769-2423-08d63af0295b X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Oct 2018 03:07:28.4868 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB4009 Subject: Re: [dpdk-dev] [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation routine X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 26 Oct 2018 03:07:31 -0000 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