From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by dpdk.org (Postfix) with ESMTP id 332FD1B3AE for ; Thu, 12 Oct 2017 18:37:44 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga102.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 12 Oct 2017 09:37:42 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.43,366,1503385200"; d="scan'208";a="909338852" Received: from unknown (HELO [10.241.225.21]) ([10.241.225.21]) by FMSMGA003.fm.intel.com with ESMTP; 12 Oct 2017 09:37:41 -0700 To: Adrien Mazarguil Cc: Gaetan Rivet , dev@dpdk.org, Thomas Monjalon , Bruce Richardson , Olivier MATZ References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> <20171011095754.GH13551@6wind.com> <20171012125311.GS13551@6wind.com> From: Ferruh Yigit Message-ID: Date: Thu, 12 Oct 2017 17:37:41 +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: <20171012125311.GS13551@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: Thu, 12 Oct 2017 16:37:45 -0000 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? > > The current approach of relying on rte_errno is OK since this function > updates it. I'll put it on my TODO list anyway but given its current size, > it's going to be low priority. >