From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wr0-f174.google.com (mail-wr0-f174.google.com [209.85.128.174]) by dpdk.org (Postfix) with ESMTP id 71F561B1C1 for ; Wed, 11 Oct 2017 11:58:07 +0200 (CEST) Received: by mail-wr0-f174.google.com with SMTP id r79so841574wrb.13 for ; Wed, 11 Oct 2017 02:58:07 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind-com.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=HvMw2vAyxQaN3XRNqptwDIsCWTfWhfrL0DRDiZHVGv0=; b=tXhiq9UZ424cWM2sqC58KXGE1BtS+y9Tf/caatUa00WxhegPXqNws4d9brL/97PuPh mHtDsXTL/CzKIZFFMvOi+yxcVnl/eJ16i8WDOlyKaQDMsiiNV3Kpf37wQEI13y/iSqL9 /yAmyFJmXskR845+5CgkyitMGIHIG6QaX2He2ekkbQLTtz6xhEQ5CXncYBFJjM/RKFt6 vrMyWPhmVQk1g9mvcasc9Eql+Iz3H1lUNmjpLVlOuvEq3wOyF2mavZPofjER2hDYqsyQ Vl7K6iGcktUyjxQaEKK6AWK2dqmw/MfqV/D4i5+6VxNYyoK2+FFgK3WLsfJel9+2L2ai Mxaw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=HvMw2vAyxQaN3XRNqptwDIsCWTfWhfrL0DRDiZHVGv0=; b=VBbpx6KxxM1nwizlVY4Uk6tgTeyTjNHE0Gvuj5hwP+ta5fFqaZbCANs8TP287t+ofH 8QFWwMhCCNXWXYFzTLCwHIfo9spM7LGrBzSzTG9cKML8l3oFqpJDfNdiLb750bMgjPty kkERsncYplunfHKH/wan+a+mkvEsPyHt1f07+/Gzc7QknZLIakBOENxIoVEiZhJg9ZnS 8Yl1lwwTlYZw2ds7TL+619c7vxsDWWAPL02pCDdfeqy734lMz+Bgnw3lqKXu5+DwSNQ3 XSVu5X2UmJ9YR0Icf1SDBdBf7tCX6VFe2eXGUEZqwPqr+6VoewDK1Y7mAZdV7DOF7RIH +fRQ== X-Gm-Message-State: AMCzsaVUaeNhPkIE6RUCybvyorTG1OcgWZ4v3rgiMozTJy/tVC336AiW ZUlVofP3RfGgzH6+UTzvWH8naQ== X-Google-Smtp-Source: AOwi7QDW6YfKs13MzR6cbwwVK11BjBsrD+1XftWTuP0ndIPK7XLqmM6Wv3QOkF11T4O1BEjASNHqLA== X-Received: by 10.223.134.68 with SMTP id 4mr16655874wrw.67.1507715887024; Wed, 11 Oct 2017 02:58:07 -0700 (PDT) Received: from 6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id y144sm10430940wmd.18.2017.10.11.02.58.05 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 11 Oct 2017 02:58:05 -0700 (PDT) Date: Wed, 11 Oct 2017 11:57:54 +0200 From: Adrien Mazarguil To: Ferruh Yigit Cc: Gaetan Rivet , dev@dpdk.org Message-ID: <20171011095754.GH13551@6wind.com> References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 09:58:07 -0000 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? -- Adrien Mazarguil 6WIND