From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet Subject: Re: [PATCH v2 4/4] net/failsafe: fix removed device handling Date: Thu, 14 Dec 2017 11:48:56 +0100 Message-ID: <20171214104856.d5qgnawuzb54l36z@bidouze.vm.6wind.com> References: <1509637324-13525-1-git-send-email-matan@mellanox.com> <1513175370-16583-1-git-send-email-matan@mellanox.com> <1513175370-16583-5-git-send-email-matan@mellanox.com> <20171213151641.g42zr7zupbsdgxsv@bidouze.vm.6wind.com> <20171213160916.e3rmxmhfhqz72wco@bidouze.vm.6wind.com> <20171213215545.kywwximn2g5xm5x5@bidouze.vm.6wind.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Cc: Adrien Mazarguil , Thomas Monjalon , "dev@dpdk.org" , "stable@dpdk.org" To: Matan Azrad Return-path: Received: from mail-wm0-f45.google.com (mail-wm0-f45.google.com [74.125.82.45]) by dpdk.org (Postfix) with ESMTP id 6C6F71D90 for ; Thu, 14 Dec 2017 11:49:10 +0100 (CET) Received: by mail-wm0-f45.google.com with SMTP id t8so10400317wmc.3 for ; Thu, 14 Dec 2017 02:49:10 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Dec 14, 2017 at 10:40:22AM +0000, Matan Azrad wrote: > Hi Gaetan > > > > > > > If you add this check in the iterator itself, you would skip removed > > > devices before attempting operating upon them, right? > > > > > > Then it should probably help with your issue, unless you tested it and > > > verified that it didnt? > > > > > > Something like this: > > > > > > ---8<--- > > > > > > diff --git a/drivers/net/failsafe/failsafe_private.h > > > b/drivers/net/failsafe/failsafe_private.h > > > index d81cc3ca6..62ddc0689 100644 > > > --- a/drivers/net/failsafe/failsafe_private.h > > > +++ b/drivers/net/failsafe/failsafe_private.h > > > @@ -316,8 +316,12 @@ fs_find_next(struct rte_eth_dev *dev, > > > subs = PRIV(dev)->subs; > > > tail = PRIV(dev)->subs_tail; > > > while (sid < tail) { > > > + if (min_state > DEV_PROBED && > > > + fs_is_removed(&sub[sid])) > > > + goto next; > > > if (subs[sid].state >= min_state) > > > break; > > > +next: > > > sid++; > > > } > > > *sid_out = sid; > > > > > > --->8--- > > > > > > Only issue being that it is completely racy, but as this MT-unsafe > > > property is inescapable we might as well ignore it and go for KISS. > > > > > > If that's enough, I would prefer instead of having this additional > > > check added to all rte_eth operations. > > > > > > > Ok, actually you were right here to do it this way. The "is_removed" > > check needs to happen after the operation attempt to effectively mitigate > > the possible race. Checking before attempting the call will be much less > > effective. > > > > That being said, would it be cleaner to have eth_dev ops return -ENODEV > > directly, and check against it within fail-safe? > > > > I think that according to "is_removed" semantic we must return a Boolean value (Each value different from '0' means that the device is removed) like other functions in c library (for example isspace()). > Sure, I wasn't discussing the interface proposed by rte_eth_dev_is_removed(). What I meant was to ask whether checking rte_eth_dev_is_removed() would be more interesting in the ethdev layer, making the eth_dev_ops return -ENODEV regardless of the previous error if this check is supported by the driver and signal that the port is removed. I think this information could be interesting to other systems, not just fail-safe. -- Gaëtan Rivet 6WIND