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 4805E1B25B for ; Tue, 10 Oct 2017 20:05:32 +0200 (CEST) Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by fmsmga103.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 10 Oct 2017 11:05:31 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,359,1503385200"; d="scan'208";a="161553894" Received: from unknown (HELO [10.241.225.83]) ([10.241.225.83]) by fmsmga006.fm.intel.com with ESMTP; 10 Oct 2017 11:05:31 -0700 To: Adrien Mazarguil Cc: Gaetan Rivet , dev@dpdk.org References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> From: Ferruh Yigit Message-ID: Date: Tue, 10 Oct 2017 19:05:30 +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: <20171006080550.GB3871@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: Tue, 10 Oct 2017 18:05:32 -0000 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. > > 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? > > 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 would like to see more comment on this, specially from PMD maintainers.