From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f44.google.com (mail-wm0-f44.google.com [74.125.82.44]) by dpdk.org (Postfix) with ESMTP id 9DE499171 for ; Wed, 16 Aug 2017 10:46:40 +0200 (CEST) Received: by mail-wm0-f44.google.com with SMTP id m85so25284612wma.0 for ; Wed, 16 Aug 2017 01:46:40 -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:content-transfer-encoding:in-reply-to :user-agent; bh=vfoTPvUSMXRygV/abonBKHZiLtJkstLwfUiWTJ67UgU=; b=fJ7/PNfhOoKp9pIxTJ+bvhC2x93sOHHLsj27QfSDzMBLQFQ6vjFGoYaIVluElTngdH LWqy42LR5uG0X/zvPcGe+y+V1l/leH27ZPhcatvsM4kCMKUxCTXpoTp6wLHgNYAgefqq MCY9LjwP4l6Ok44x76jSqmsJfvxyb83wydKStXYw6Z7Hag9aB2rnbsB6LErb7u5LqaGs LLNMMfaF+3xb7H+uv7wJXYhKgRi559ZAOAp6hNaXS6TMK/XeAfcD/tkfapcOCojrxc+t 2Suzjvq4DTu8Rdm+DToE4rs29WlLUS1DTCnzVjZ1V2fKo3fX0+r9u+ql4Bcd90KAOQUH OuCg== 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=vfoTPvUSMXRygV/abonBKHZiLtJkstLwfUiWTJ67UgU=; b=E/KjIMxEXcJbuhQyz0epVxro4mhFIVUhTMjnbpNad+YUFsNxl2s0ID+HhjXYVjO7YL 9eY2LHWhSURB3+qxSfNuL91VIxtrawSbr1dhkRGzFV7Z4TmEADgD69Bk3ysTYY0Jt+xx byJL7hJIIvn00Xlp1PCtaBm+RjU2PTnrrIC8Hbnt2JcWNYawycTQI8OZPrqxZfdeXkYH tE7MKy6KEdQ6/IEF/0uIVITDloa6nkF6uCHSCpWAhlHMJXGISL+e54CgyB+zuyPSbWz8 nlbf/euaKFzniuHlrXB9hM9T5rxIJPM3hNYNX39b9ObrRCUaIUd53cKNzuF2Ms4wclvx TSqw== X-Gm-Message-State: AHYfb5j+TUQiwI5IA4QdJhpp2dD2JHLDeFex4ML1Leh33uCLpH39bjM3 MaTbUfgHX2/QhqqO X-Received: by 10.28.134.201 with SMTP id i192mr735874wmd.124.1502873200131; Wed, 16 Aug 2017 01:46:40 -0700 (PDT) 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 r79sm344911wrb.29.2017.08.16.01.46.38 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 16 Aug 2017 01:46:39 -0700 (PDT) Date: Wed, 16 Aug 2017 10:46:30 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: dev@dpdk.org, stable@dpdk.org Message-ID: <20170816084629.GK8124@bidouze.vm.6wind.com> References: <1502780359-60553-1-git-send-email-matan@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1502780359-60553-1-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] net/failsafe: fix tx sub device deactivating 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: Wed, 16 Aug 2017 08:46:40 -0000 Hi Matan, Thanks for spotting this, a few nits below. On Tue, Aug 15, 2017 at 09:59:19AM +0300, Matan Azrad wrote: > The corrupted code couldn't recognize that all sub devices > were not ready for tx traffic when failsafe PMD was trying > to switch device because of an unreachable condition using. > > Hence, the current tx sub device variable was not updated > correctly. > > The fix removed the unreachable condition and adds condition > in the right place to handle non tx device ready scenario. > It should be reworded as Make the condition reachable by moving it in the right place to handle the scenario when no TX device is ready. If the condition is removed and then added, I find it clearer to say that it was moved. > Fixes: ebea83f899d8 ("net/failsafe: add plug-in support") > Fixes: 598fb8aec6f6 ("net/failsafe: support device removal") > The root commit introducing the issue is the first one, but this fix only applies to the second. So I don't know which commit is actually fixed by this, but I find peculiar to have two commits targeted by a fix. In doubt, probably leave both, but maybe someone has a better idea about it? > Signed-off-by: Matan Azrad > Cc: stable@dpdk.org The Cc: stable line should immediately follow the Fixes: line. > --- > drivers/net/failsafe/failsafe_private.h | 7 ++++--- > 1 file changed, 4 insertions(+), 3 deletions(-) > > Hi Gaetan > I didn't find any real scenario which cause to problematic > behavior because of the previous code. > But it may be. > > diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h > index 0361cf4..dc97aec 100644 > --- a/drivers/net/failsafe/failsafe_private.h > +++ b/drivers/net/failsafe/failsafe_private.h > @@ -346,9 +346,10 @@ fs_switch_dev(struct rte_eth_dev *dev, > PRIV(dev)->subs_tx = i; > break; > } > - } else if (txd && txd->state < req_state) { > - DEBUG("No device ready, deactivating tx_dev"); > - PRIV(dev)->subs_tx = PRIV(dev)->subs_tail; > + if (i >= PRIV(dev)->subs_tail || !sdev) { `!sdev` should be `sdev == NULL`, see [1]. > + DEBUG("No device ready, deactivating tx_dev"); > + PRIV(dev)->subs_tx = PRIV(dev)->subs_tail; > + } > } else { > return; > } > -- > 2.7.4 > With these changes, Acked-by: Gaetan Rivet [1]: http://dpdk.org/doc/guides/contributing/coding_style.html#c-statement-style-and-conventions -- Gaëtan Rivet 6WIND