From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f42.google.com (mail-wm0-f42.google.com [74.125.82.42]) by dpdk.org (Postfix) with ESMTP id 4482F1B2CE for ; Thu, 12 Oct 2017 14:53:23 +0200 (CEST) Received: by mail-wm0-f42.google.com with SMTP id q124so12945410wmb.0 for ; Thu, 12 Oct 2017 05:53:23 -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=7GyrUJjCKI0loP9BUoH55uufDRtbKCllRibci7gpQ3I=; b=X/wEEw8tBKXGoWvv/NK9M87dC9+PPjhQt8Ei8Vrn20vmjLMcqtZJTLchsRcsGDfy+M rUlnV/fVwxm9TvEXRvSSH2dfzWd3VNjd3IbndTUsVO7b623E2qkJAesTWuBMtb2pWZco BWrodkpVAAcjgYrKfkJ2ffszf907CxVuM3GFXuZzYmTu1or7W5PwyXCnMP1LednPdVWv 94LGbFq0zyPP2GcQUh47KluduLhgLfhSZocZeNfDkW0MFml1w5u3sWg/UQnfc5mNGacv xRCt7vG8JyIf0z3ZrCGYfOQMzchVESv38vdTjXrDCnD+ZYZ9TcYA5GCVaU6HRyPZgBwl QrFA== 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=7GyrUJjCKI0loP9BUoH55uufDRtbKCllRibci7gpQ3I=; b=ePPCbtMOWGvroku600v5sufZTAOZonhiEOksqxTp3IRuV0Ed0auvQI/PGQo4wWyrBG 11bEBFy/i5txPmnDYcgttJ3CuSlV/nvcGt6o9p4CM0WZweddYepT2uIY/woTs6dgCACm Tn9eNi0qK5KAGBPZaV+f71G7Wj3sxBFP0Ci6PCWC/NlojLp18AjQB4oZMAwn4tQLc4yP YJi4EElw4DndbdSH/M76lkAlfu4cVzyFM4i9RI8d4twI3XoHOuu8cjePNZbJzPT0CzDE 73dd6qgx1YdtVC/y4M+4iO9muXieRfHs98HAJMn/TRBQnr+slJtrAPzt3i52tz8mKGsx +A3w== X-Gm-Message-State: AMCzsaUgdLnRoQRGA6SRjFkiowOTP9ZKN1vlsv/RNWnK9NNzuDA6IKXp ATCXhlQbcEf8iEuFONhgKPtP4Q== X-Google-Smtp-Source: AOwi7QBhoIWmX+fwRkWYfHomyjZkYru3Bdddshv1A1j3ammrcjBR2/D5oDpBPepuESZ6mE+Mi1S8uA== X-Received: by 10.223.195.147 with SMTP id p19mr1968065wrf.176.1507812802885; Thu, 12 Oct 2017 05:53:22 -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 b72sm210473wmf.30.2017.10.12.05.53.21 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 12 Oct 2017 05:53:21 -0700 (PDT) Date: Thu, 12 Oct 2017 14:53:11 +0200 From: Adrien Mazarguil To: Ferruh Yigit Cc: Gaetan Rivet , dev@dpdk.org Message-ID: <20171012125311.GS13551@6wind.com> References: <919ab2dc-1081-d139-50a3-c797fbeb7284@intel.com> <20171006080550.GB3871@6wind.com> <20171011095754.GH13551@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: Thu, 12 Oct 2017 12:53:23 -0000 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 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. -- Adrien Mazarguil 6WIND