From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f51.google.com (mail-wm0-f51.google.com [74.125.82.51]) by dpdk.org (Postfix) with ESMTP id 43C101B6A0 for ; Fri, 13 Oct 2017 12:43:03 +0200 (CEST) Received: by mail-wm0-f51.google.com with SMTP id t69so20342050wmt.2 for ; Fri, 13 Oct 2017 03:43:03 -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=5M3G2O0CMXSYVnIhnxf8J8mlhQL+VwXnR+gE2sZ/bc4=; b=EKtE8DOGUv/sJmh4uRNXQB9lDz4aKzp+RH07T1pFRIVMdNgndI76dEbEWSnMRf+GvA M0yxe8vo5hmYBrmMmCwB5pFR/sXMELFmRLZR7D0P6eRSXtv6en8R6X1wrmMl0GiqudHn 11I09BOC+g3xnvqMr/MkazyZysyrJ9i6RMHeLnrejX64zyLV8nGIJGZs6Qzt3xHKgceG yM4uh+FyYb4Xj9KX5qdME1r+lAGKHupTsgNmeNV617gzjw75bzDLUrKhWy2Gi8t5eyIF c7euFbqp7HgSWUZ6RNgjuReqZPBTd5ObXdn/h02Rkcp/Te7U4D/NJgMfhuQoV3+KzKQq F70g== 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=5M3G2O0CMXSYVnIhnxf8J8mlhQL+VwXnR+gE2sZ/bc4=; b=TpGBv0hRQkov6M9LaB/g4U/F2GMI4gr+1ebEenJpVxEv97Ae8FvXZ6jydjJsIYDlQD u3A1gJWCqpw23pcbSD9nSTXmUl9BNwd9VSlMihd+S6jKd4IJ3xSz2l9UV/3SlmHIffFR UQNRRzLdhAAsIj7XjicsLB6jyZIwWzHcMlVwCkUpy6+pwPdIcZJK3PUSxtF4Gl5i7c5Q /winx8UNdX/xtDIeTSiC2mA1ARh358P2UgQ3wqsdgCt+hhSlPuoO5LRgAcD3kQE5am3J AfB3ybqI4QriYVO5qJzk0SVzLqgRoefWrCtyzRAnN8C6XTB+FBOJnDw3v8E6kRCL6pkE SYsA== X-Gm-Message-State: AMCzsaXrx5EsqB5Usr1ljl2n7gQny6pbNgrPocYDdOvgnAjOKobTW2GI wOlsL0Ny/UPt8TY23LxhZyUD8/Sl X-Google-Smtp-Source: ABhQp+SWDDZqRHA3cFJQo1XAmfPrLc08qlpo60Ut+dPe9gxK/9YpAO4X7Rwc/uxkCDc2k7kiAeYHIw== X-Received: by 10.28.215.4 with SMTP id o4mr1055562wmg.0.1507891382805; Fri, 13 Oct 2017 03:43:02 -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 m26sm1258854wrb.81.2017.10.13.03.43.01 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 13 Oct 2017 03:43:01 -0700 (PDT) Date: Fri, 13 Oct 2017 12:42:51 +0200 From: Adrien Mazarguil To: Ferruh Yigit Cc: Gaetan Rivet , dev@dpdk.org, Thomas Monjalon , Bruce Richardson , Olivier MATZ Message-ID: <20171013104251.GB13551@6wind.com> References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> <20171011095754.GH13551@6wind.com> <20171012125311.GS13551@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: Fri, 13 Oct 2017 10:43:03 -0000 On Thu, Oct 12, 2017 at 05:37:41PM +0100, Ferruh Yigit wrote: > On 10/12/2017 1:53 PM, Adrien Mazarguil wrote: > > On Wed, Oct 11, 2017 at 07:07:29PM +0100, Ferruh Yigit wrote: > >> On 10/11/2017 10:57 AM, Adrien Mazarguil wrote: > >>> On Tue, Oct 10, 2017 at 07:05:30PM +0100, Ferruh Yigit wrote: > > > >>>> 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. > > > > OK, I will re-submit an updated version later then. > > > >> 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? > > > > Well, that is actually how I originally envisioned this function to be used, > > but for whatever reason I thought a positive return value was the way to go > > at the time. Too bad. > > > >> Would you mind updating those drivers as above in a separate patch? > > > > That would be nice but I'm not familiar with most of these PMDs (I'm > > only updating mlx4 as part of the RSS resurrection series), and there's > > always a risk of breaking something. > > The change looks mechanical, so I believe it won't break anything. > > Not directly related to this one, but including this one, recently we > kind of have an agreement that library changes doesn't have to update > every usage of it, because it is too much work and this is preventing > improvements in libraries. (Because sometimes library implementer > doesn't know PMD internals to update it properly.) > > But currently I have list that PMDs needs to reflect some library work, > and list is growing. > > Let's assume this work go in without updating PMDs, I believe it will > take some time for some PMDs even to recognize this can be changed. > > So we either need to change our approach, or find an effective way to > pass this information into PMDs. But I believe even passing this > information won't be enough, each driver has their own roadmaps, > resource allocation, some may tend to not apply these unless PMD is broken. > > Any comments? I'm generally fine with that, it should likely be considered on a case basis though. Some API changes (e.g. port ID type) also can't avoid modifying all PMDs at once. It's more or less optional for those that do not break existing APIs, depending on their impact. Say, if some new API involves something to be done on the data path, all PMDs must be validated for performance non-regression. This can't be done properly by people unfamiliar with impacted PMDs (or who may benefit from decreasing performance of competing PMDs :). Either way, if a PMD maintainer disagrees with a change in his/her PMD for any reason, this should not block the API change from entering. The patch for that specific PMD should be ignored in the meantime (possibly to be reworked by one of its maintainers). -- Adrien Mazarguil 6WIND