From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f49.google.com (mail-wm0-f49.google.com [74.125.82.49]) by dpdk.org (Postfix) with ESMTP id 754D67CB6 for ; Mon, 11 Sep 2017 10:31:29 +0200 (CEST) Received: by mail-wm0-f49.google.com with SMTP id f199so33659648wme.0 for ; Mon, 11 Sep 2017 01:31:29 -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=37ySh+QZQkQHgaZtTiJp9zdEKzqx6bavNnOh3Yuvi4I=; b=u7at3Z4QQLD3h5mjA8E6WVtL28hJWjFYZMkc3EhXW2ueLXxHXRS8BPNs8dM8iXMU0L DfGgjBUEwCZHob57FZWpTHFXsyZqvBZYXkDUrck0e5noAG7GnAv9LJS4AoGkbs7Xhd2J IJ09BuM/ofykA1HCHtNelJUGWIGN7p736+noig9tJwCxMc3xBCOuxA9mVwln2mkFgIPa rQypjufKwF6aDX65uMQ6zJZrJogb9R/EEhHWsyM0Yz2mCzhWMka/pyd9KVLS5zeZTFJW V7E+8Xe3Fy0Bn00cq+wqSqOy8UZP0zTQbCp1Pub27oIn3wgvoCDkTUzOa3Zz6LD0Q1vl BmDA== 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=37ySh+QZQkQHgaZtTiJp9zdEKzqx6bavNnOh3Yuvi4I=; b=laOQ0Hb0gPVSZpOYRkwIeN0ZT6yr3FXcKFZVjBDOZLMAyw+Rbg/pA5NXeNwMqMmAey dUCiZLheze9b2wBrqRMVGw2g7MwBHQ37/kfzFkSN72De5kZhSI4cwDn91NXz5UWVXSnk zxR+UA33Hl5a2AiHMCtA/yJd5XXqvhTRa6ZZGDpUVanmZvOfaMkmaILZThdciKDUuzqD O9aLxmBlvTjhZlwG/FMMuICozFGOQ1QzzQSg6lzXEQBp73YtQjEn5ymscvlyoChKIjJ3 Zyn18lzaoLzYNCGX5FXGff9WNuL+JY8Sxsddg8L8U9mXdC8Pu81D3bbSFmh/0ANK5qtt /ZGw== X-Gm-Message-State: AHPjjUjWAy6dO2B78xIvIqpeywS+YakoFF5TIiybomjSBJfGnD4ZaATa Sa1dNQLJX6DWMxup X-Google-Smtp-Source: AOwi7QDEH9Dgld6aiH5xCzk2/C7fQkD2+z/+ZZo2iyh7iG29K/cuPKTNesEssBETM1plhL7NoRhgjg== X-Received: by 10.28.68.5 with SMTP id r5mr6727345wma.6.1505118688871; Mon, 11 Sep 2017 01:31:28 -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 n6sm5436529wmg.11.2017.09.11.01.31.27 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 11 Sep 2017 01:31:27 -0700 (PDT) Date: Mon, 11 Sep 2017 10:31:18 +0200 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Ophir Munk Cc: Adrien Mazarguil , dev@dpdk.org, Thomas Monjalon , Olga Shern , stable@dpdk.org Message-ID: <20170911083117.GM21444@bidouze.vm.6wind.com> References: <1504985231-21031-1-git-send-email-ophirmu@mellanox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1504985231-21031-1-git-send-email-ophirmu@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH] net/failsafe: fix calling device during RMV events 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, 11 Sep 2017 08:31:29 -0000 Hi Ophir, On Sat, Sep 09, 2017 at 07:27:11PM +0000, Ophir Munk wrote: > This commit prevents control path operations from failing after a sub > device has informed failsafe it has been removed. > > Before this commit if a device was removed and then a control path Here are the steps if I understood correctly: 0. The physical device is removed 1. The interrupt thread flags the device 2. A control lcore initiates a control operation 3. The alarm triggers, waking up the eal-intr-thread, initiating the actual device removal. 4. Race condition occurs between control lcore and interrupt thread. "if a device was removed" is ambiguous I think (are we speaking about the physical port? Is it only flagged? Is it after the removal of the device itself?). From the context I gather that you mean the device is flagged to be removed, but it won't be as clear in a few month when we revisit this bug :) . Could you please rephrase this so that the whole context of the issue is available? > operations was initiated on failsafe - in some cases failsafe called the > sub device operation instead of avoiding it. Such cases could lead to > operations failures. > > This commit fixes failsafe criteria to determine when the device is removed > such that it will avoid calling the sub device operations during that time > and will only call them otherwise. > This commit mitigates the race condition, reducing the probability for it to have an effect. It does not, however, remove this race condition, which is inherent to the DPDK architecture at the moment. A proper fix, a more detailed workaround and additional documentation warning users writing applications to mind their threads could be interesting. But let's focus on this patch for the time being. > Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD") > Cc: stable@dpdk.org > > Signed-off-by: Ophir Munk > --- > drivers/net/failsafe/failsafe_ether.c | 1 + > drivers/net/failsafe/failsafe_ops.c | 52 +++++++++++++++++++++++++++++------ > 2 files changed, 45 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/failsafe/failsafe_ether.c b/drivers/net/failsafe/failsafe_ether.c > index a3a8cce..1def110 100644 > --- a/drivers/net/failsafe/failsafe_ether.c > +++ b/drivers/net/failsafe/failsafe_ether.c > @@ -378,6 +378,7 @@ Could you please generate your patches with the function name in the diff? > i); > goto err_remove; > } > + sdev->remove = 0; You are adding this here, within failsafe_eth_dev_state_sync, and removing it from the dev_configure ops. 10 lines above, the call to dev_configure is done, meaning that the remove flag was resetted at this point. Can you explain why you prefer resetting the flag here? The position of this flag reset will be dependent upon my subsequent remarks anyway, so hold that thought :) . > } > } > /* > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > index ff9ad15..314d53d 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -232,7 +232,6 @@ > dev->data->dev_conf.intr_conf.lsc = 0; > } > DEBUG("Configuring sub-device %d", i); > - sdev->remove = 0; > ret = rte_eth_dev_configure(PORT_ID(sdev), > dev->data->nb_rx_queues, > dev->data->nb_tx_queues, > @@ -311,6 +310,8 @@ > int ret; > > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > + if (sdev->remove) > + continue; > DEBUG("Calling rte_eth_dev_set_link_up on sub_device %d", i); > ret = rte_eth_dev_set_link_up(PORT_ID(sdev)); > if (ret) { > @@ -330,6 +331,8 @@ > int ret; > > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > + if (sdev->remove) > + continue; For this change and all the others: I think it might be best to have this check added to fs_find_next directly. Most of the call to the iterators are done within dev_ops, so it makes sense I think to have it there. But then there'd be an issue with the sub-EAL iterations done on previously-removed ports, as the removed flag is precisely resetted too late. The function failsafe_dev_remove would also need to have a manual iteration upon the sub-devices instead of using the macro. I think you can actually reset this flag within fs_dev_remove, instead of the next plug-in, then having this check within fs_find_next *should* not be a problem. I think you should break up those changes in two: first move the flag reset to fs_dev_remove instead of fs_dev_configure, then add this check to the iterator. This way, a git bisect should allow us to pinpoint more easily any new bug as both changes have the potential to introduce subtle ones. > DEBUG("Calling rte_eth_dev_set_link_down on sub_device %d", i); > ret = rte_eth_dev_set_link_down(PORT_ID(sdev)); > if (ret) { > @@ -517,8 +520,11 @@ > struct sub_device *sdev; > uint8_t i; > > - FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) > + FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > + if (sdev->remove) > + continue; > rte_eth_promiscuous_enable(PORT_ID(sdev)); > + } > } > > static void > -- > 1.8.3.1 > Thanks, -- Gaëtan Rivet 6WIND