From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from rcdn-iport-4.cisco.com (rcdn-iport-4.cisco.com [173.37.86.75]) by dpdk.org (Postfix) with ESMTP id DFF81D1EA for ; Fri, 24 Mar 2017 18:23:26 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=7049; q=dns/txt; s=iport; t=1490376206; x=1491585806; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-transfer-encoding:mime-version; bh=yYQqedL9E07LXv0L+I1Ru6kelFFpFttZRSSpw9GB2+8=; b=B7SnB5RUusiClyvhGh8KWLnxZQJkQLbP2iDooEUBE16uRv5YOagd9VzI 7Vqo1/+MMEP5ZgfUawVhewfutrtbrGXaEx5hs215fR0G2/MlQqred/t68 6nZ1HpeZyE1YIgLnNc46x/MgtMuvfy+OPfbXSecTRX96MZyIMxwOKHSX6 k=; X-IronPort-Anti-Spam-Filtered: true X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0ASAQDiVNVY/5FdJa1dGQEBAQEBAQEBA?= =?us-ascii?q?QEBBwEBAQEBg1SBbAeNapFOkHKCSIIPgg6GIgKDKT8YAQIBAQEBAQEBayiFFQE?= =?us-ascii?q?BAQECATo/BQcEAgEIEQQBAQEeCQcyFAkIAgQOBQiJdwisSYpFAQEBAQEBAQEBA?= =?us-ascii?q?QEBAQEBAQEBAQEBHYZOhG+EMAGGCAWcWQGKI4geggWFKooKSJMcAR84gQRZFYc?= =?us-ascii?q?ZdYcxAQEkB4EDgQ0BAQE?= X-IronPort-AV: E=Sophos;i="5.36,215,1486425600"; d="scan'208";a="224483104" Received: from rcdn-core-9.cisco.com ([173.37.93.145]) by rcdn-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 24 Mar 2017 17:23:24 +0000 Received: from XCH-RCD-010.cisco.com (xch-rcd-010.cisco.com [173.37.102.20]) by rcdn-core-9.cisco.com (8.14.5/8.14.5) with ESMTP id v2OHNOWt022116 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Fri, 24 Mar 2017 17:23:24 GMT Received: from xch-rcd-007.cisco.com (173.37.102.17) by XCH-RCD-010.cisco.com (173.37.102.20) with Microsoft SMTP Server (TLS) id 15.0.1210.3; Fri, 24 Mar 2017 12:23:23 -0500 Received: from xch-rcd-007.cisco.com ([173.37.102.17]) by XCH-RCD-007.cisco.com ([173.37.102.17]) with mapi id 15.00.1210.000; Fri, 24 Mar 2017 12:23:23 -0500 From: "John Daley (johndale)" To: Adrien Mazarguil CC: "dev@dpdk.org" Thread-Topic: [PATCH 0/1] proposed minor change in rte_flow_validate semantics Thread-Index: AQHSpEeF9wud5P6ZaE+BkyTAFsTE+qGkEk2AgAAdxZA= Date: Fri, 24 Mar 2017 17:23:23 +0000 Message-ID: <6861ef3c7d4a475da48158417c115314@XCH-RCD-007.cisco.com> References: <20170324023659.28099-1-johndale@cisco.com> <20170324094633.GS3790@6wind.com> In-Reply-To: <20170324094633.GS3790@6wind.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.19.145.150] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Subject: Re: [dpdk-dev] [PATCH 0/1] proposed minor change in rte_flow_validate semantics 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, 24 Mar 2017 17:23:27 -0000 Hi Adrien, > -----Original Message----- > From: Adrien Mazarguil [mailto:adrien.mazarguil@6wind.com] > Sent: Friday, March 24, 2017 2:47 AM > To: John Daley (johndale) > Cc: dev@dpdk.org > Subject: Re: [PATCH 0/1] proposed minor change in rte_flow_validate > semantics >=20 > Hi John, >=20 > On Thu, Mar 23, 2017 at 07:36:58PM -0700, John Daley wrote: > > Hi, > > > > In implementing rte_flow_validate() for the Cisco enic, I got to > > wondering if the semantics might be slightly off given how I see apps u= sing > it. > > > > Please forgive me if this has already been discussed, but during > > runtime, I can't see any reason why rte_flow_validate() would be > > called when > > rte_flow_create() returns the same errors as validate, but also > > creates the filter if all is good (calling validate first is just extra= overhead). > > What are the use cases for calling validate when the app is up and runn= ing? >=20 > In short (I'll elaborate below), to get a rough idea of the capabilities = of a given > port at initialization time. One should use rte_flow_validate() with > nonspecific rules to determine if the PMD understands them at all *given = the > circumstances* (the current configuration state of the device). >=20 > Nothing prevents one from using it just before calling rte_flow_create(), > however doing so is indeed redundant. >=20 > > I see rte_flow_validate() being run at startup for answering these 2 > questions: > > 1.) Given enough resources, is the device capable of handling flows a, > > b, c, .., which the app may want to create during runtime? >=20 > Yes, breaking this down means all the following: >=20 > 1. Recognize all pattern items and actions. > 2. Make sure items are properly stacked and actions can be combined. > 3. Check for pattern item specification structures (e.g. limits, masks, > ranges, and so on). > 4. Check for actions configuration structures (e.g. target queue exists). >=20 > Depending on the flow rule, 3. and 4. may have to check the current devic= e > configuration in order to validate the rule. Agreed. You didn't include checking for duplicate patterns with conflicting= actions. Is that a requirement also? I hope some of that and some of 1, 2 = and 3 can be pushed up into rte_ether/flow someday. I.e. only present flows= to the PMDs which are already valid, so the PMD just needs to see if it is= acceptable to the device. >=20 > > 2.) How many concurrent flows of type a, b, c, .. can the device > > handle? To answer this app needs to do a bunch of rte_flow_create()'s > (followed by a bunch of rte_flow_destroy()s). >=20 > Hehe, this is the brute force approach, actually the rte_flow API in its = current > form is not supposed to answer this question, and I don't think we should > document it that way (note you could use rte_flow_flush() to destroy all > flows at once faster). >=20 > Some devices (like mlx5) do not really have an upper limit on the number = of > possible flows one can configure, they're basically limited by the availa= ble > memory on the host system. >=20 > > The answer to these questions will allow the app to decide beforehand > > which filters to offload (if any) and set up function pointers or > > whatever to run as efficiently as possible on the target device. > > > > rte_flow_validate() has comments that imply it should be run on the > > fly,like "the flow rule is validated against its current configuration > > state and the returned value should be considered valid by the caller f= or > that state only". > > The only real way to implement this is for enic (and I think other > > nics) is to actually do a flow create/destroy within > > rte_flow_validate(). But, see paragraph 2 above- why bother? I looked > > at the i40e and mlx5 implementations and I didn't see any calls to the > > nics to check a flow against current state so they might not be > implemented as the comment imply they should be either. >=20 > Then you're probably right there is an issue with the documentation, it's= not > the intent (more below). >=20 > > If there are no use cases for calling validate at runtime, I think we > > ought to change the semantics of rte_flow_validate() a little to > > define success to mean that the device will accept the flow IF there > > are enough resources, rather than it will accept the flow given the > > current state. We can safely remove the -EEXIST return code since no > > other drivers set them yet. This makes the function easier to > > implement correctly without trying to do something that's not useful > anyway. > > > > The following patch just changes comments but I think that is all that > > is required. >=20 > rte_flow_validate() and rte_flow_create() share the same error codes to > make their implementation simpler. rte_flow_validate() basically exposes > the validation part necessarily present in rte_flow_create(). >=20 > EEXIST and other error codes *may* be returned by rte_flow_validate(), bu= t > this does not force PMDs to check possible collisions during the validati= on > step, I'll take documentation is not clear enough regarding this. My scan of the other PMDs didn't turn up any EEXIXT or EBUSY return codes f= rom rte_flow_validate(). I suppose future implementation could, but I doubt= they will since intel/mlx PMDs set the standard and they do not. >=20 > So to be completely clear, rte_flow_validate() already does *not* guarant= ee > a rule can be created afterward, only that rte_flow_create() will underst= and > that rule. >=20 > The other reason "configuration state" is mentioned in the documentation > (besides target queues should exist) is that existing rules may have swit= ched > the device in a mode that does not allow different rule types. >=20 > If after initialization, matching TCP, UDP and ICMP is possible, creating= a UDP > rule might subsequently prevent the creation of otherwise valid TCP and > ICMP rules. rte_flow_validate() should (but is not forced to) check for t= hat. >=20 > What do you think about keeping the defined error codes as is and merging > somehow my above statements in the documentation instead? I guess I still don't see much value of checking a rule against the current= state and no other PMDs are doing this. I still think just having a rte_fl= ow_validate that answers my question 1 above has a lot of value and implyin= g it does more (and having different PMDs do different things) will just co= nfuse users (not to mention PMD maintainers ;). Maybe just cleaning up the = documentations as you say is good enough. I think removing return error cod= es that can't be counted on is even better though. Otherwise the documentat= ion gets harder, right? We would have to basically say "don't count on ENOM= EM and EXIST doing what you think because they probably won't...." . My 2 c= ents, thanks! >=20 > -- > Adrien Mazarguil > 6WIND