From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from EUR04-VI1-obe.outbound.protection.outlook.com (mail-eopbgr80084.outbound.protection.outlook.com [40.107.8.84]) by dpdk.org (Postfix) with ESMTP id 75C7B1C01 for ; Mon, 29 Oct 2018 10:33:06 +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=gETitogEK3Ni19S/BcZzPN48McEt9cd6sTqEcyiNh1I=; b=njyt3rxD35Lsb6W0a69SeQgfZD58+ruiWkQyA8XuUmdMaHjG0N28VQcvQPqbvWrb5wQ6ZZWkLM1jVZu2uX9OFCR231ViI0i3MephY8lMofgBTL0v8xfvNEon6UBegYPYYgS0Ke5L19szKPzfFVJRhLz9GOFSv2u3PH/G5iHgcKw= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.186.150) by AM4PR05MB3315.eurprd05.prod.outlook.com (10.171.187.12) 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 09:33:03 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::544b:a68d:e6a5:ba6e]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::544b:a68d:e6a5:ba6e%2]) with mapi id 15.20.1273.027; Mon, 29 Oct 2018 09:33:03 +0000 From: Slava Ovsiienko To: Yongseok Koh CC: Shahaf Shuler , "dev@dpdk.org" Thread-Topic: [PATCH v2 2/7] net/mlx5: e-switch VXLAN flow validation routine Thread-Index: AQHUZJFYi3kRmdGok0OzngUkDwlPjaUspnMAgANTC8CAAO9tAIAAVSbQgADmaICAA+XaQA== Date: Mon, 29 Oct 2018 09:33:03 +0000 Message-ID: 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: <20181026215646.GC13615@mtidpdk.mti.labs.mlnx> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=viacheslavo@mellanox.com; x-originating-ip: [95.67.35.250] x-ms-publictraffictype: Email x-microsoft-exchange-diagnostics: 1; AM4PR05MB3315; 6:5J5a0qBe9tDpnGFw8VCUZaaok+29qIlBpTsi68UoMsDxetk/Oo1EcKmEGGD1pj4X+W1WZCXYqM9GeBgA6L0WIDkW0PF50BL3wNDxNyX7n9nanfr4U8DkHoZCicaLLv6Vk2JlorX9if6gz4UMjuSwFfvBv0iV+nvMeZk6iAZuklc8KbH+JMLp+U9APOzOS5QFZPCVtIlPGYcaXlPUoaoha1OnP+2W1gSXpQgSxEULCILTlJZSQd2B8rdy72g+OWycxFY+0vuGGVDNkomUSY6Rfk4AFKnwjjtAfMWxxFtmE9zCXEguRLSzCMSbr7SN7RaYtMCloF/ENhMdQUh808X5EkJePKol+uA/ITKJ0SIqsNZBlxuLeqSSVlbZJGeBAQk+pjQmaO6Q0KjQNkMu9cm+Psne7oY8DdPACpVYwnBrypJj02vh8kOKmlGW0sgKiA2b03/Ruak6f67l4oi3K++0oQ==; 5:HwiqO8Z8pazr0j2tuaAwou7vRo2WdpL9JlQRpbEEEKyLTJGDCaa6Cyu2ZlrSTefWIqS0yq7987GbanD4wB0eCJQwm3LszFsRLlObqRcqEbCeaqUzZYBqOF0W/URwyaoDPTXQyf6lr3toORIcPaqpjFlZIjHIQAuJ4PINhn9/0nI=; 7:TSwNezW2N78KaWa/urbSF0bSub4vq33CEdAS7mKhFmqDqIMbJsYRNzPqsLv1rsVqOIteShCptIc7iYhXxk4wxN88eMpPyfN/LDd2a+z2Lenhjvbd+IibEhz2dfCxpIK9GpNorlcoDtNlPPEOsLmnIQ== x-ms-exchange-antispam-srfa-diagnostics: SOS; x-ms-office365-filtering-correlation-id: fa68a76d-36b9-4275-8ce1-08d63d8186a0 x-ms-office365-filtering-ht: Tenant x-microsoft-antispam: BCL:0; PCL:0; RULEID:(7020095)(4652040)(4534185)(4627221)(201703031133081)(201702281549075)(5600074)(711020)(4618075)(2017052603328)(7153060)(7193020); SRVR:AM4PR05MB3315; x-ms-traffictypediagnostic: AM4PR05MB3315: 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)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(20161123562045)(20161123564045)(20161123560045)(20161123558120)(201708071742011)(7699051)(76991095); SRVR:AM4PR05MB3315; BCL:0; PCL:0; RULEID:; SRVR:AM4PR05MB3315; x-forefront-prvs: 084080FC15 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(346002)(136003)(376002)(39860400002)(396003)(366004)(52314003)(189003)(13464003)(199004)(186003)(74316002)(93886005)(305945005)(7736002)(86362001)(26005)(8676002)(81156014)(81166006)(25786009)(2900100001)(6506007)(4326008)(256004)(102836004)(229853002)(106356001)(6636002)(105586002)(68736007)(316002)(8936002)(11346002)(446003)(486006)(476003)(53546011)(14444005)(54906003)(478600001)(66066001)(33656002)(14454004)(53936002)(55016002)(76176011)(7696005)(6862004)(5250100002)(6436002)(71200400001)(3846002)(6116002)(71190400001)(5660300001)(97736004)(99286004)(6246003)(2906002)(9686003)(21314003); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3315; H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; MX:1; A:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-microsoft-antispam-message-info: VRsIVjsLQ8uaIw0wdTgYuY9ZXXd+Ju19Wd2itKB+IW0Ioap6uwrb28xfDlhFLXayXiPWx3q5MhBX73FAzeSbKCxv5FmkGopzDNkWzJ3uDeKgNJDeCwV+N254WUSFmfnxHUxnzeb7pk++yeXFzExwJf+13aZzc9T78VpNt62EKmnjSmbpTskbP3VNygEz15QENbdsuB17TCco+RujfAnGr10xQQ2Ca/qHo5LcBJ/HoBsy8AcwUal1kbiXfMYxei6yVXipa81FQqz56HHpbsNfw1nmgJlcfpnxz8gNe9MCeWYYOOm+UVXWr1NjgbvUpyoweLkuZOtP7d9h9eHi5viGQZUlpPLZnOCJQyNwAT3QWj4= spamdiagnosticoutput: 1:99 spamdiagnosticmetadata: NSPM Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: fa68a76d-36b9-4275-8ce1-08d63d8186a0 X-MS-Exchange-CrossTenant-originalarrivaltime: 29 Oct 2018 09:33:03.6841 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3315 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 09:33:06 -0000 > -----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 validation > > > 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 (inner > > > > + 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 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. > > > > > > 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 whether > > > > 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 recheck > > > > - 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 suppor= t 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 TCF > 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 ad= ded. > > > > > > > > > > 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 fruits = 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 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 itse= lf. > > > > 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. > > > > > > > > > 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 > > > > 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 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: > > > > > > { > > > 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 f= or > 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 assign= 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) How do you propose check these issues in your approach? With best regards, Slava > Thanks, > Yongseok >=20 > > > > > - validate items > > >