From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR01-VE1-obe.outbound.protection.outlook.com (mail-ve1eur01on0046.outbound.protection.outlook.com [104.47.1.46]) by dpdk.org (Postfix) with ESMTP id 8EC1754AE for ; Fri, 26 Oct 2018 23:56:55 +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=iPbgxMMDUICNizo5GbbfCExkgTMjLc2xiEtsQ0nXLpM=; b=XGi+/Ab99qeX/Y0/O9yH/sHGTWSgKuLukYICc4E7eW72YJKVPrAvXkevHZR3CjnC4AoYHGm/s7KIXLwDBzsmU1M8VzpP+GB2YUomTp6gBeoWcCZ8Bg8VdFwoEzKev3tJo5Qd4LlcFghAR7ehsy3toCeFyDSawesJH65PsQxvXe4= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB3948.eurprd05.prod.outlook.com (52.134.72.19) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1273.21; Fri, 26 Oct 2018 21:56:53 +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.025; Fri, 26 Oct 2018 21:56:53 +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+cFNxcMrsUaUsMREAgAPZ7oCAAGiIgIAA0jIAgADetwA= Date: Fri, 26 Oct 2018 21:56:53 +0000 Message-ID: <20181026215646.GC13615@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> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: BYAPR05CA0105.namprd05.prod.outlook.com (2603:10b6:a03:e0::46) 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; DB3PR0502MB3948; 6:6p/jW27z4tiGUt/G0/rxQNBVZvqc0vvA7e5rOF3ze1BS/SyLYw9xNMarfWmrf+tzgc2UngFYun58q1c747KLD1jUE5AbrbftJE5zdtfzYrByQJodjOIg81PGRj8Lssenw96b5P1WmtMJH753ERRBkwOH9hLOja4fgxJsUp5XqwjSxTlDBHidMQClsODhMhg/LBwCogQJcs/LAqhtPDtuT0R7hV67Qh0LUOc09XOP3a/bANtbMV/mIJZXcPlbcQnx/u6DylJBjjfl0TWkPfwTqoS4wCi0GgbSSPny+8viaTcXCYOAqlyDn+WZLlbDpOdfjFtfti4Sc1fY5B2XX3bripcwSdagTL2hjso4w8mMErY+eP0FD5ZY1gfo13njEmG4Yw96iPEE3drY577kndTRzrW+4tvteVe9DXGc15O4YQJ2MEl99WklAHXNmrCWrD6lTMg3kkOf5s4QyK1/zdKG1w==; 5:3EH4/aTD6kpNjX5OBNPJPSbx3LSanTWqu5Dv3V1iR5Og9/7LW+ZPdgcGDaky1uRw9oDy8eLkp/sCDgS7myAdtrX/N1hotH/f7s3VQqZgPhE9u4fNj5aACaT6Jzc0zpZCy94J0gnio5k8Mm4PM/K+FBEUQLfCgpH056F7faG+qlk=; 7:2X29SXa/ptpxj73vFdQ+OpHEqNWfkDQePq29uZ3M7ygRfwICQ95wFb0tUpdvEbtGwts/c3waU8GXDn59yDpJJkeFeT41kgcWfO3b3p7VG1h85JZdhcHnNcmbsYBfOVazTN9435WHQQvh1Lz11qcjhg== x-ms-office365-filtering-correlation-id: 00f219a0-b9b8-4a0b-1206-08d63b8df084 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(8989299)(5600074)(711020)(4618075)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7153060)(7193020); SRVR:DB3PR0502MB3948; x-ms-traffictypediagnostic: DB3PR0502MB3948: 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)(10201501046)(93006095)(93001095)(3002001)(3231355)(944501410)(52105095)(6055026)(148016)(149066)(150057)(6041310)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123564045)(20161123560045)(20161123558120)(20161123562045)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB3948; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB3948; x-forefront-prvs: 083751FCA6 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(366004)(136003)(39860400002)(396003)(346002)(376002)(13464003)(199004)(52314003)(189003)(6436002)(14454004)(8936002)(81156014)(6512007)(99286004)(9686003)(6636002)(68736007)(8676002)(186003)(71200400001)(5250100002)(316002)(71190400001)(54906003)(6486002)(81166006)(66066001)(7736002)(25786009)(2900100001)(229853002)(305945005)(97736004)(5660300001)(26005)(53936002)(2906002)(6246003)(3846002)(6116002)(6862004)(478600001)(486006)(1076002)(256004)(52116002)(86362001)(446003)(11346002)(14444005)(33896004)(476003)(386003)(6506007)(53546011)(76176011)(106356001)(105586002)(33656002)(4326008)(93886005)(102836004)(21314003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB3948; 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: P+NUlbev3bue9TXv6Kkcuw37nPEPRD3r0RXDVNWvsNGnW+FGPCwzj9av7UE6L/84JahIyVSaLJEQ8o2ekglmJaq5ec34DHB44TaL1NRJ24+ZQDO3QovUTpCWeSxN9GNXTLDWAn6OC6B1f7Ee5rZB5tSs33tnvVetJABa/zlMWqvlfeHJBEVqP7bDjS1apszyHAkV1LBpZmd7TU+pRtk8z6t4cjshbLmO9z+5QJUASa4s9+i9X55xsH9NGg99H2Iq8CDE/b5PKgomLtltsvBAErsjICXQJN2MOg0u7yUp1lZhFA2DQbcrl+LVCLvgC0DsmyVVmmINg1Z8nTdaVb3vXXXqKtgdazZQARWxpyJXRqY= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-ID: Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: 00f219a0-b9b8-4a0b-1206-08d63b8df084 X-MS-Exchange-CrossTenant-originalarrivaltime: 26 Oct 2018 21:56:53.4701 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB3948 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 21:56:55 -0000 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 validation > > routine > >=20 > > 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 validatio= n > > > > routine > > > > > > > > On Mon, Oct 15, 2018 at 02:13:30PM +0000, Viacheslav Ovsiienko wrot= e: > > [...] > > > > > @@ -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 (inner + = outer) for > > them. > > > There is just no TCA_FLOWER_KEY_xxx attributes for specifying inner > > items > > > to match by flower. > >=20 > > 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. >=20 > Hm. Interesting. I will check. >=20 > > > 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, an= d > > > should be NULL-ed if multiple items are found in the items list. > > > > > > 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. > >=20 > > You can't use flags at this point. It is validate() so prepare() might = not be > > preceded. > >=20 > > > > 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 Adrie= n'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 red= undant > > > > outer items. > > > > Did I miss something? > > > > > > Interesting, besides rule has correct syntax, I'm not sure whether it= can be > > applied w/o errors. > >=20 > > 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 i= f it is > > support by tcf as this whole patchset itself is pretty huge enough and = we > > don't > > have much time. > >=20 > > > At least our current flow_tcf_translate() implementation does not sup= port > > any INNERs. > > > But it seems the flow_tcf_validate() does, it's subject to recheck - = we > > should not allow > > > unsupported items to pass the validation. I'll check and provide the > > separate bugfix patch > > > (if any). > >=20 > > Neither has tunnel support. It is the first time to add tunnel support = to TCF. > > If it was needed, you should've added it, not skipping it. > >=20 > > You can check how MLX5_FLOW_LAYER_TUNNEL is used in Verbs/DV as a > > reference. >=20 > Yes. I understood your point. Will check and add tunnel support for TCF r= ules. > Anyway, inner MAC addresses are supported for VXLAN decap, I think we sho= uld > specify these ones in the rule as inners (after VNI item), definitely > some tunnel support in validate/parse/translate should be added. >=20 > >=20 > > > > 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. > >=20 > > 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 fruits if= any. > >=20 > > > > 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_deca= p(), > > 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 prio= r to > > > > outer header item validation, you can relocate the code - 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 preliminary > > gathered, > > > to check them in action's specific fashion and to check action itself= . > > > 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 d= oes > > 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 minim= izes the > > changes > > > of existing code. > > > In your approach we should: > > > - scan actions, w/o full checking, just action_flags gathering and ch= ecking > > > - scan items, performing variating check (depending on gathered actio= n > > flags) > > > - scan actions again, performing full check with params (at least for= now > > > check whether all params gathered) > >=20 > > Disagree. flow_tcf_validate_vxlan_encap() doesn't even need any info 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: > >=20 > > { > > 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; > > } > > } > >=20 > > Curretly you are doing, > >=20 > > - validate items > > - validate actions > > - validate items again if decap. > >=20 > > But this can simply be > >=20 > > - validate actions > How we could validate VXLAN decap at this stage?=20 > As we do not have item_flags set yet? > Do I miss something? 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. Thanks, Yongseok >=20 > > - validate items > >=20