From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga14.intel.com (mga14.intel.com [192.55.52.115]) by dpdk.org (Postfix) with ESMTP id 90E8A1B198 for ; Wed, 11 Oct 2017 20:07:31 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 11 Oct 2017 11:07:30 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,362,1503385200"; d="scan'208";a="908996659" Received: from unknown (HELO [10.241.225.21]) ([10.241.225.21]) by FMSMGA003.fm.intel.com with ESMTP; 11 Oct 2017 11:07:30 -0700 To: Adrien Mazarguil Cc: Gaetan Rivet , dev@dpdk.org References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> <20171011095754.GH13551@6wind.com> From: Ferruh Yigit Message-ID: Date: Wed, 11 Oct 2017 19:07:29 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <20171011095754.GH13551@6wind.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH v1 0/7] Flow API helpers enhancements 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: Wed, 11 Oct 2017 18:07:32 -0000 On 10/11/2017 10:57 AM, Adrien Mazarguil wrote: > On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote: >> On 10/6/2017 9:05 AM, Adrien Mazarguil wrote: >>> On Fri, Oct 06, 2017 at 02:13:14AM +0100, Ferruh Yigit wrote: >>>> On 10/5/2017 10:49 AM, Adrien Mazarguil wrote: >>>>> This series brings enhancements to various rte_flow helpers: >>>>> >>>>> - Allow applications to use rte_flow_error_set() by making it part of the >>>>> public interface and documenting it as such. >>>>> >>>>> - Address rte_flow_copy()'s limitations by replacing it with the more >>>>> versatile rte_flow_conv(). This new function allows retrieving other >>>>> properties such as item/action names, enabling testpmd to finally use it >>>>> and get rid of duplicated code. >>>>> >>>>> - Add a script (gen-rte_flow_conv-h.sh) to help with generating the >>>>> resources used by rte_flow_conv(). Developers should run it when adding or >>>>> modifying pattern items or actions (done as part of this series to add the >>>>> missing "fuzzy" pattern item). >>>>> >>>>> - Future plans for rte_flow_conv() include translating error codes to >>>>> human-readable messages, so applications do not have to make their own. >>>>> >>>>> All these changes address concerns raised a couple of months ago [1]. Work >>>>> on these patches actually started at the time but I was unable to complete >>>>> and clean them up until recently. >>>>> >>>>> [1] http://dpdk.org/ml/archives/dev/2017-July/070492.html >>>>> >>>>> Adrien Mazarguil (7): >>>>> ethdev: expose flow API error helper >>>>> ethdev: replace flow API object copy function >>>>> ethdev: add flow API item/action name conversion >>>>> app/testpmd: rely on flow API conversion function >>>>> ethdev: enhance flow API item/action descriptions >>>>> ethdev: generate flow API conversion header >>>>> ethdev: update flow API conversion header >>>> >>>> Hi Adrien, >>>> >>>> This received too late for this release cycle, and changes in rte_flow >>>> library may effect PMDs. >>>> >>>> I suggest deferring the set to next release, what do you think? >>> >>> Hi Ferruh, >>> >>> My opinion as the author (since you're asking :) is that it would be nice to >>> have it in this release assuming reviewers don't find blocker issues with >>> it. >> >> Review part may be the problem, since this is very short notice before >> release, relevant parties may not review this on time. >> And they will be right to not expect a new feature like this after >> proposal deadline. > > Yes, I generally agree with that. > >>> To summarize the changes from a PMD standpoint: >>> >>> - rte_flow_error() (previously not public) switching from positive to >>> negative return value like the other rte_flow_*() functions. The only PMDs >>> relying on its return value so far are mlx4 and tap. >>> >>> - rte_flow_copy() disappearing. This function was temporary pending a better >>> solution, and so far is only used by fail-safe PMD (modified as part of >>> this series). Besides fail-safe, PMDs did not a have a use case for this >>> function. >> >> Although you update all rte_flow_copy() usage in the DPDK, this is >> public API right, and technically a user code may be using this, can we >> remove this without notice? > > Right, actually rte_flow_copy() was missing the EXPERIMENTAL tag. It's been > present for a single release and wasn't documented as being officially part > of the public rte_flow interface, unlike this series with rte_flow_conv(). > > For various reasons including its lack of flexibility (enforced by the > struct rte_flow_desc format), I honestly believe rte_flow_copy() is only > presently used by the fail-safe PMD. I may be wrong, and in the case of > complaints the plan is to re-add rte_flow_copy() as a wrapper to > rte_flow_conv() later as a standard maintenance fix. > >>> These patches were originally targeted at 17.08, and since the "fuzzy" item >>> is missing from rte_flow_copy() (GTP/GTPU/GTPC are now also missing by the >>> way) and there is currently a lot of redundancy between this function and >>> testpmd's internals, I thought it would be a good time to have everything in >>> a single place. I was also considering using rte_flow_conv() in upcoming >>> mlx4 patches in case it was included. >>> >>> So here's my suggestion: I can track all rte_flow-related changes in PMDs >>> and in rte_flow itself and update this series accordingly until things have >>> settled (e.g. I'll re-submit to rebase and include GTP). Once applied, I >>> will check all new code that relies on these two functions and update it if >>> necessary until the release. How about that? >> >> I have no concern that you will do the necessary updates, and agreed it >> is good to get updates to help maintenance, and it would be nice to have >> these in the this LTS release. >> >> After above said, API changes one week before integration deadline, a >> new script and make target for automated header file, I am a little >> scared :), I will be much relieved to get this in the beginning of the >> next release cycle. > > I can drop the script from this series to speed up inclusion if it there's > any concern about it. It's only a helper to update rte_flow_conv.h after > modifying rte_flow.h, I thought it could be useful to anyone, hence I've > included it but it's pretty much optional. > >> I would like to see more comment on this, specially from PMD maintainers. > > Me too. I don't even mind negative ones! > > Here's what I plan to do regardless, seeing most concerns so far are with > rte_flow_copy()/rte_flow_conv(): > > - Whether this series is included for 17.11 or later, a v2 is already > necessary. > > - I will drop the rte_flow_error() change to submit it instead along another > upcoming series for mlx4 where it's the most needed. > > - We'll then continue to discuss rte_flow_conv() as a something nice to have > but not super urgent to integrate and I'll keep trying to convince > everyone it's safe enough. > > - Once it becomes clear there's no way to have it for 17.11, I'll update > this series as a somewhat late deprecation notice for rte_flow_copy(). > > Sounds good? OK. Lets get rte_flow_error() change and not block mlx4 changes. And I still suggest waiting beginning of the release for rest of the patch. So it can come with optional header auto generation. Related to the rte_flow_error_set() modification, as far as I can see it doesn't effect drivers but following drivers are now using it: drivers/net/bnxt/ drivers/net/e1000/ drivers/net/enic/ drivers/net/i40e/ drivers/net/ixgbe/ drivers/net/mlx4/ drivers/net/mlx5/ drivers/net/sfc/ drivers/net/tap/ There is already tap and mlx4 fixes in the patch to fix "return -rte_flow_error_set(...)" kind of usage. Rest uses rte_flow_error_set() as: " rte_flow_error_set(...); return -rte_errno; " But now it can directly be used as: " return rte_flow_error_set(...); " What do you think updating to that usage? Would you mind updating those drivers as above in a separate patch? Thanks, ferruh