From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f54.google.com (mail-wm0-f54.google.com [74.125.82.54]) by dpdk.org (Postfix) with ESMTP id AE95C1B1B2 for ; Mon, 8 Jan 2018 14:47:07 +0100 (CET) Received: by mail-wm0-f54.google.com with SMTP id 9so14179065wme.4 for ; Mon, 08 Jan 2018 05:47:07 -0800 (PST) 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:content-transfer-encoding:in-reply-to :user-agent; bh=eC9xrbCqCmrzO7Rr77sRBxBUbn47LtIKArFvPeuQDIQ=; b=X39iEuacAhtS492z5s2k7HmkPz1qLZ2MNYBrIRfuccR9oOUIg3dL8UGeiAWtUN3BIw Jbc8k9a9U6v/+v8po2MW4B1d0zVwzN/47CFT4U3pJySw8hTM7Ji42ENsWD0T2P0y8H82 M8ir28gspGptp1voxoUvJaP6jXaReY1mFtnPOm+qwkaHltSb9p2920Uo31l1vaIXKwdC IDFFLteECrfX2XOkFZBtDo3oGa4acwVKrvmET2VQ+PoH6eNDW4yV6I+bwkoVpKmld57i Oa8JEx4WyeDGZxs/WaLWdm9Z68EtV/QUgFWQIo/Lf6MJMa+Pk6ImcnRgZQjndb0EiLWR QkVQ== 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:content-transfer-encoding :in-reply-to:user-agent; bh=eC9xrbCqCmrzO7Rr77sRBxBUbn47LtIKArFvPeuQDIQ=; b=f54/QicRtXHvigENV5DhBZ0+EoebYiac5viztZOlAskuMSZcwl2TTowbZMcJYHJZNY KT7pkoOSs4hnkkJML6xbktzsAWu/ckQvpbNO6UcuagGpAaW9lTtqO1Pl3GY6Laf2v4iF Z3SUhrOPr3mXd/HyfVPwBFDs0uzNlY85FBwjwqVzLyIe9K4DR2wBB/jk3jC76Wvtx511 Dp5uNCwGeJw84qU+7NBFFqRh66rdLJuM3ypomVHtRA//c2tL2U5M0rGxYEaaY6kjZqQ5 NxZ2jzHAqyluAtaeKA2jbJkQ3Qx/sg9EaI6oWDs4/UwkBMWi2F058fvZV95aRI6NaQHC k2OA== X-Gm-Message-State: AKGB3mIEv5+nbMx/SRWbI1dYTjE6PMkg8naGPZWjOaCIrJWCjgp4s7N2 j09ObN3IOYvSCaZuvnTEAPKx75c+ X-Google-Smtp-Source: ACJfBotbjnkwpkKShCTmt55jZFJoLLjoPtQvoe9DUVj+ppwnkU7qppvIkkLe7ZmTZhH6MUM9H7qmxw== X-Received: by 10.28.169.151 with SMTP id s145mr8498633wme.102.1515419227278; Mon, 08 Jan 2018 05:47:07 -0800 (PST) Received: from bidouze.vm.6wind.com (host.78.145.23.62.rev.coltfrance.com. [62.23.145.78]) by smtp.gmail.com with ESMTPSA id r82sm9746367wme.31.2018.01.08.05.47.06 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 08 Jan 2018 05:47:06 -0800 (PST) Date: Mon, 8 Jan 2018 14:46:54 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: Adrien Mazarguil , Thomas Monjalon , "dev@dpdk.org" Message-ID: <20180108134654.wb7svquzhuuvvmh6@bidouze.vm.6wind.com> References: <1513175370-16583-1-git-send-email-matan@mellanox.com> <1513703415-29145-1-git-send-email-matan@mellanox.com> <1513703415-29145-7-git-send-email-matan@mellanox.com> <20171219222131.plcfn5wqggyn5znw@bidouze.vm.6wind.com> <20180108105739.qkyejshupojkwyv2@bidouze.vm.6wind.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: User-Agent: NeoMutt/20170113 (1.7.2) Subject: Re: [dpdk-dev] [PATCH v3 6/6] net/failsafe: fix removed device handling 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: Mon, 08 Jan 2018 13:47:07 -0000 On Mon, Jan 08, 2018 at 12:55:49PM +0000, Matan Azrad wrote: > Hi Gaetan > > From: Gaëtan Rivet, Monday, January 8, 2018 12:58 PM > > Hi Matan, > > > > Sorry for the delay on this. > > > > It's OK in spite of I need to fetch it back :) > > > On Wed, Dec 20, 2017 at 10:58:29AM +0000, Matan Azrad wrote: > > > Hi Gaetan > > > > > > > -----Original Message----- > > > > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com] > > > > Sent: Wednesday, December 20, 2017 12:22 AM > > > > To: Matan Azrad > > > > Cc: Adrien Mazarguil ; Thomas Monjalon > > > > ; dev@dpdk.org > > > > Subject: Re: [PATCH v3 6/6] net/failsafe: fix removed device > > > > handling > > > > > > > > Hi Matan, > > > > > > > > On Tue, Dec 19, 2017 at 05:10:15PM +0000, Matan Azrad wrote: > > > > > There is time between the physical removal of the device until > > > > > sub-device PMDs get a RMV interrupt. At this time DPDK PMDs and > > > > > applications still don't know about the removal and may call > > > > > sub-device control operation which should return an error. > > > > > > > > > > In previous code this error is reported to the application > > > > > contrary to fail-safe principle that the app should not be aware of > > device removal. > > > > > > > > > > Add an removal check in each relevant control command error flow > > > > > and prevent an error report to application when the sub-device is > > removed. > > > > > > > > > > Fixes: a46f8d5 ("net/failsafe: add fail-safe PMD") > > > > > Fixes: b737a1e ("net/failsafe: support flow API") > > > > As stated previously, please do not include those fixes lines. > > > > > > > > > > > > Signed-off-by: Matan Azrad > > > > > --- > > > > > > > > > > > > > > > > > +/* > > > > > + * Check if error should be reported to the user. > > > > > + */ > > > > > +static inline bool > > > > > +fs_is_error(struct sub_device *sdev, int err) { > > > > > + /* A device removal shouldn't be reported as an error. */ > > > > > + if (err == 0 || sdev->remove == 1 || err == -EIO) > > > > > + return false; > > > > > + return true; > > > > > +} > > > > > > > > This is better, thanks. > > > > > > > > However is there a reason you did not follow the same pattern as > > > > ethdev with eth_err? I see the two functions as similar in their > > > > intent, making them close to each other would be clearer to a reader > > > > being familiar with the ethdev API and that would be interested in fail- > > safe. > > > > > > > > What do you think? > > > > > > > > > > I think that there is a real different between eth_err function to > > fs_is_error: > > > ethdev uses eth_err function to adjust removal return value to be -EIO. > > > fail-safe uses fs_is_error function to check if an error should be reported to > > the user to save the fail-safe principle that the app should not be aware of > > device removal - this is the main idea that also causes me to change the > > name from fs_is_removed to fs_is_error. > > > > I would have preferred if it followed the same pattern as ethdev (that > > function be used to adjust the return value, not performing a flag check). > > > > While better on its own, the pattern: > > > > if (fs_is_error(sdev, err)) { > > ERROR("xxxx"); > > return err; > > } > > > > is dangerous, as then the author is forbidden from returning err, assuming > > err could be -EIO. He or she would be forced to return an explicit "0". > > To be clear, here would be an easy mistake to do: > > > > if (fs_is_error(sdev, err)) { > > ERROR("xxxx"); > > } > > return err; > > > > And this kind of code-flow is not unusual, or even unwanted. > > I dislike having this kind of implicit rule derived from using a helper such as > > fs_is_error(). > > > > The alternative > > > > if ((err = fs_err(sdev, err))) { > > ERROR("xxxx"); > > return err; > > } > > > > Forces the value err to be set to the correct one. > > > Good point, will change it. > > > This mistake can already be found in your patch: > > > > > @@ -150,7 +150,7 @@ > > > continue; > > > local_ret = rte_flow_destroy(PORT_ID(sdev), > > > flow->flows[i], error); > > > - if (local_ret) { > > > + if (fs_is_error(sdev, local_ret)) { > > > ERROR("Failed to destroy flow on sub_device %d: %d", > > > i, local_ret); > > > if (ret == 0) > > > > Sorry, I can't see any issue here. > You're right, actually the code would still be correct. I checked again the rest of the edit, there shouldn't be any issue, usually "0" is explicitly returned. Still, the point stands. > > Your environment does not include the function, but this is within > > fs_flow_destroy (please update to include the context by the way it helps a > > lot the review :). Afterward, line 162 ret is directly used as return value. > > > I don't understand what do you mean. > > > Also, fs_err() would need to transform rte_errno when relevant (mostly in > > failsafe_flow.c I think). > > > Your suggestion is always to update rte_errno to 0 in case the error is because of removal? > If the error is indeed due to the device being absent, then rte_errno should be set back to its previous value I think. -- Gaëtan Rivet 6WIND