From mboxrd@z Thu Jan 1 00:00:00 1970 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) 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=NKrjMqvZHFuZXuOf8N80huEZgOa6wN1QsJHexNC/v8g=; b=dnbY/FIkAkW9Nt+S62D9yLbvAX54ww3k0oMfsTZ/3gjPe3FR4WPKdMBd3YpApQExhrFx68hIYiQMGGDIqDVmTWdQo1jDcVwoMKC7Y1JPwYoNBzPZDBV0AgKc5dqMYgvY5aufyiaIZa0Bfg+cPpjcfixTf0KHhmcXo6A2MhVwWUI= Received: from DB3PR0502MB3980.eurprd05.prod.outlook.com (52.134.72.27) by DB3PR0502MB4011.eurprd05.prod.outlook.com (52.134.68.139) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1273.25; Mon, 29 Oct 2018 18:26:58 +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.027; Mon, 29 Oct 2018 18:26:58 +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+cFNxcMrsUaUsMREAgAPZ7oCAAGiIgIAA0jIAgABpXwCABFyMgIAAlSKA 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> In-Reply-To: Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-clientproxiedby: BYAPR05CA0035.namprd05.prod.outlook.com (2603:10b6:a03:c0::48) 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; DB3PR0502MB4011; 6:Y4tRy2XRq0SSPsDx+1DDcOn666635PkYo55j5KxOojZc/Cucvw/bqvj6hjFmtHZLFrRnDPqpG6/d8AALW4gW00XQ+tYMcRGXsn51f2wJb8vdyknRcHWyU8k1qLKMI9cxT8e1S7WqAAWMscPIlLs3G8mdmKTAfG4fpzMM4BjSRvfwMke1YSNkRgafDTZUCOqaq5or+1hxRvFX4VmDRsNodTRH03uOhkPlIUjG0JEW69oPsxq/Opvu5gLhhiMa4+BolG2sMrQ6/cQKZUKsKlUpkxD5oRyxJOCXjxx+7YGpZnCpBKWXVo9htkRRakRrM5g5/g369PgcLOVsl1Ngk1ZNOc8JBaU/3GGbzgT2jEJ5oa/dy/28nq/VJIfMSEYKqdRjCEXn6wd3dgm8bNHtkTYDsfcx9ywE8YYnd1lz8pgEgLHmo9WSl9cZPbYRMB6JS2zVz+2P4tqtqCm9y4HaUlZymg==; 5:wSWZaYL4x1yWNhDhvTvFuOXhNXFzBh7im3VJf1OXAlk2YlrOYTCjaH+IeXVdSoRz24X51o1cmKRE30H0g7E09fC9YQVxwFN4o1tj51KGDHOsyCvzzQmurs/io+qU62zkQzTcW1NKIeCYPjkeZ1nisYeoiK7xpzLIKc1hmnl+vfs=; 7:o0D1h/I1Obzqd6o2RXPVU1ClJynWKSWWnOGHjDCbNDCLpYvDCrqZBhBKuf6RlJXB3kZ5we5vjl8n2kUbbJOPBGRwOeJUb0OTKcKlNdKhB6bYmojtNtQqDWK+Z2tr8BdL7kZCJtbPa7vSl1NjLyqyFA== x-ms-office365-filtering-correlation-id: 763868b0-eb33-4077-4282-08d63dcc1c80 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:DB3PR0502MB4011; x-ms-traffictypediagnostic: DB3PR0502MB4011: 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)(8121501046)(5005006)(10201501046)(3002001)(93006095)(93001095)(3231382)(944501410)(52105095)(6055026)(148016)(149066)(150057)(6041310)(20161123564045)(20161123562045)(20161123558120)(20161123560045)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051)(76991095); SRVR:DB3PR0502MB4011; BCL:0; PCL:0; RULEID:; SRVR:DB3PR0502MB4011; x-forefront-prvs: 084080FC15 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(396003)(39860400002)(136003)(366004)(346002)(376002)(52314003)(199004)(189003)(13464003)(11346002)(102836004)(54906003)(316002)(53936002)(478600001)(256004)(14444005)(6246003)(5250100002)(1076002)(68736007)(33896004)(3846002)(6116002)(9686003)(93886005)(6512007)(53546011)(6506007)(386003)(76176011)(52116002)(66066001)(14454004)(446003)(476003)(26005)(186003)(2906002)(6486002)(5660300001)(81166006)(97736004)(81156014)(4744004)(6436002)(8936002)(4326008)(305945005)(6862004)(2900100001)(7736002)(8676002)(486006)(229853002)(86362001)(99286004)(33656002)(25786009)(105586002)(71190400001)(6636002)(71200400001)(106356001)(21314003); DIR:OUT; SFP:1101; SCL:1; SRVR:DB3PR0502MB4011; 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: WLEhmM8u9P6MFQUNA4ioocr6tejgbYf3GhSsBjnZtYWBuipxhzBTva7qlFts9h/x7O9xFWBQKaFnvgadkIfJUEj2HlULlc9PQsZuRAS1I0fiZXFEo8LyA0QP7OEEMi5y601DBGYlEupRI6sWVFXFY8J+ZqKhbAvnkg/kcMfyGI/gLE3PK8QCM6SVphYyPBasrebj2TQsGzBCL+MhyQHRnfApymhQ1zGOUkpewqDvG1Ungf9KKh3FbZ8LDv7UizVmmFlqjMDMR0OT2oiR6VKKKRUbZKqaHLFrjJOQduqyx71lnMogEBpHdkSxdalHUJepJdm9V7qLrFdKmNLxZ2207J4fWl6j5w7cKYVaPFPug9A= 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: 763868b0-eb33-4077-4282-08d63dcc1c80 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Oct 2018 18:26:58.4046 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: DB3PR0502MB4011 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: Mon, 29 Oct 2018 18:27:00 -0000 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