From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-wm0-f68.google.com (mail-wm0-f68.google.com [74.125.82.68]) by dpdk.org (Postfix) with ESMTP id E08CC1B292 for ; Mon, 6 Nov 2017 09:19:25 +0100 (CET) Received: by mail-wm0-f68.google.com with SMTP id y83so12200748wmc.4 for ; Mon, 06 Nov 2017 00:19:25 -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=tnF5gN+76xYKsQ7zh5EjJr04BC14g4LqmfSLIyC3RRY=; b=rk9Bm/tON64kXh3FbxJ3fgMHCzreZrcc3+Z1qWa55BiOFjQVDWmR+ml2/WiC22MKvB rZDQErSJYLScQY8Lc991PDaFKsMudq7X78Dch5Nzdl45Voklk9AIgQRsN6T1Rw1J7pE3 XJN6icLb5/ChQ9d4xYPEs4pMxsL8/bJOz/tUNQjlCc5zXowz/1Y1Yq0ux70F+7OfbNVm 9fk/5RUiuu56G9G01+YOtiCPcASsTLfbtC8ve+Ot18EVEcqu5q9EDeim1i2UVGYqH9MM tnhTcej5WdQCs4AKxBK5PYCSH/VhMnrMMvcjMH+84DVd2NbpA/Ov+TXAPpbY5D83R1TU Sdcg== 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=tnF5gN+76xYKsQ7zh5EjJr04BC14g4LqmfSLIyC3RRY=; b=ImTY0PmQUmgnDVnWZlDbHN8c3KVxwORIOm6Wx3UHv1kQrAdleOELUURF2vZHMve670 43sKbgKOx6m6N/va5dmmhceVXChsAzPIc8nNAHvfzpK4dScIGJnGhjGudgUfDG2OuNmU 6MPGPL38dQ+yZA4119ejpAAmKvNv3COXM7oXZBAdRaEN/0lCsAwaawT1hKvxRRRvljH7 tJPTr+Gja3H/Wl8pultIrWHvqiIShgaJgHoy+L+vYnhMcW+HOTXNRd6hLBwfWD97ii7x UTInQtNNK3aRKWh3GZVMqTSz7XQuUSQXl5FVtdjSwzZ6zebXqED7DbGKX0PWy8axdDKS BTXQ== X-Gm-Message-State: AMCzsaW0OJEN1sPqcjJtq7RadXUe5icdwMSYV2H+lz/ZKH8iN5iLNSMM mYQUJQYniPMlD2rRA+xYePRtmA== X-Google-Smtp-Source: ABhQp+RCM1rCNAjy6ADj/bv8gokT/0HcdN/O5sSIgAvVlRJfoSQFV3CbJeN+9fGa9kpGkWByAGYt6g== X-Received: by 10.80.201.12 with SMTP id o12mr18832609edh.98.1509956365333; Mon, 06 Nov 2017 00:19:25 -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 r4sm12984484edd.4.2017.11.06.00.19.24 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Mon, 06 Nov 2017 00:19:24 -0800 (PST) Date: Mon, 6 Nov 2017 09:19:12 +0100 From: =?iso-8859-1?Q?Ga=EBtan?= Rivet To: Matan Azrad Cc: Adrien Mazarguil , dev@dpdk.org, stable@dpdk.org Message-ID: <20171106081912.GM10890@bidouze.vm.6wind.com> References: <1509637324-13525-1-git-send-email-matan@mellanox.com> <1509637324-13525-2-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: <1509637324-13525-2-git-send-email-matan@mellanox.com> User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH 1/3] net/failsafe: fix removal handling lack 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, 06 Nov 2017 08:19:26 -0000 Hello Matan, On Thu, Nov 02, 2017 at 03:42:02PM +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. > > Define a removal error that each sub-device PMD should return in case > of an error caused by removal event; The special error is -ENODEV. > > Add an error check in each relevant control command error flow and > prevent an error report to application when its value is -ENODEV. > > Fixes: a46f8d5 ("net/failsafe: add fail-safe PMD") > Fixes: b737a1e ("net/failsafe: support flow API") > Cc: stable@dpdk.org > This is not a fix. This would be useless backported in stable without the related mlx4 and mlx5 changes. The related mlx4 and mlx5 patches are themselves not marked as fixes and won't be backported. > Signed-off-by: Matan Azrad > --- > doc/guides/nics/fail_safe.rst | 7 +++++++ > doc/guides/prog_guide/env_abstraction_layer.rst | 3 +++ > drivers/net/failsafe/failsafe_flow.c | 16 +++++++++------ > drivers/net/failsafe/failsafe_ops.c | 27 ++++++++++++++++--------- > drivers/net/failsafe/failsafe_private.h | 8 ++++++++ > 5 files changed, 45 insertions(+), 16 deletions(-) > > diff --git a/doc/guides/nics/fail_safe.rst b/doc/guides/nics/fail_safe.rst > index c4e3d2e..5023fc4 100644 > --- a/doc/guides/nics/fail_safe.rst > +++ b/doc/guides/nics/fail_safe.rst > @@ -193,6 +193,13 @@ any time. The fail-safe PMD will register a callback for such event and react > accordingly. It will try to safely stop, close and uninit the sub-device having > emitted this event, allowing it to free its eventual resources. > > +When fail-safe PMD gets -ENODEV error from control command sent to removable > +sub-devices, it assumes that the error reason is device removal. In this case > +fail-safe returns success value to application. The PMD controlling the > +sub-device is still responsible to emit a removal event (RMV) in addition to > +returning -ENODEV from control operations after the device has been physically > +removed. Only the reception of this event unregisters it on the fail-safe side. > + > Fail-safe glossary > ------------------ > > diff --git a/doc/guides/prog_guide/env_abstraction_layer.rst b/doc/guides/prog_guide/env_abstraction_layer.rst > index 4775eb3..bd2fd87 100644 > --- a/doc/guides/prog_guide/env_abstraction_layer.rst > +++ b/doc/guides/prog_guide/env_abstraction_layer.rst > @@ -213,6 +213,9 @@ device having emitted a Device Removal Event. In such case, calling > callback. Care must be taken not to close the device from the interrupt handler > context. It is necessary to reschedule such closing operation. > > +Unsuccessful control operations (for those that return errors) may return > +-ENODEV after the device is physically unplugged. > + I think I should be neither ack-ing nor nack-ing this change. Could you propose it on its own, so that people ignoring fail-safe related matters could look into it as well? > Blacklisting > ~~~~~~~~~~~~ > > diff --git a/drivers/net/failsafe/failsafe_flow.c b/drivers/net/failsafe/failsafe_flow.c > index 153ceee..ce9b769 100644 > --- a/drivers/net/failsafe/failsafe_flow.c > +++ b/drivers/net/failsafe/failsafe_flow.c > @@ -87,7 +87,7 @@ > DEBUG("Calling rte_flow_validate on sub_device %d", i); > ret = rte_flow_validate(PORT_ID(sdev), > attr, patterns, actions, error); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { Here and for subsequent checks, there should be an explicit check against zero instead of using unary !. > ERROR("Operation rte_flow_validate failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -111,7 +111,8 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > flow->flows[i] = rte_flow_create(PORT_ID(sdev), > attr, patterns, actions, error); > - if (flow->flows[i] == NULL) { > + if (flow->flows[i] == NULL && > + !SUBDEV_REMOVED(sdev, -rte_errno)) { > ERROR("Failed to create flow on sub_device %d", > i); > goto err; > @@ -150,7 +151,7 @@ > continue; > local_ret = rte_flow_destroy(PORT_ID(sdev), > flow->flows[i], error); > - if (local_ret) { > + if (local_ret && !SUBDEV_REMOVED(sdev, local_ret)) { > ERROR("Failed to destroy flow on sub_device %d: %d", > i, local_ret); > if (ret == 0) > @@ -175,7 +176,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling rte_flow_flush on sub_device %d", i); > ret = rte_flow_flush(PORT_ID(sdev), error); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_flow_flush failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -199,8 +200,11 @@ > > sdev = TX_SUBDEV(dev); > if (sdev != NULL) { > - return rte_flow_query(PORT_ID(sdev), > + int ret = rte_flow_query(PORT_ID(sdev), > flow->flows[SUB_ID(sdev)], type, arg, error); > + > + if (!SUBDEV_REMOVED(sdev, ret)) > + return ret; > } > WARN("No active sub_device to query about its flow"); > return -1; > @@ -223,7 +227,7 @@ > WARN("flow isolation mode of sub_device %d in incoherent state.", > i); > ret = rte_flow_isolate(PORT_ID(sdev), set, error); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_flow_isolate failed for sub_device %d" > " with error %d", i, ret); > return ret; > diff --git a/drivers/net/failsafe/failsafe_ops.c b/drivers/net/failsafe/failsafe_ops.c > index f460551..cc7ab7f 100644 > --- a/drivers/net/failsafe/failsafe_ops.c > +++ b/drivers/net/failsafe/failsafe_ops.c > @@ -314,7 +314,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > 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) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_set_link_up failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -333,7 +333,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > 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) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_set_link_down failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -418,7 +418,7 @@ > rx_queue_id, > nb_rx_desc, socket_id, > rx_conf, mb_pool); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("RX queue setup failed for sub_device %d", i); > goto free_rxq; > } > @@ -484,7 +484,7 @@ > tx_queue_id, > nb_tx_desc, socket_id, > tx_conf); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("TX queue setup failed for sub_device %d", i); > goto free_txq; > } > @@ -563,7 +563,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling link_update on sub_device %d", i); > ret = (SUBOPS(sdev, link_update))(ETH(sdev), wait_to_complete); > - if (ret && ret != -1) { > + if (ret && ret != -1 && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Link update failed for sub_device %d with error %d", > i, ret); > return ret; > @@ -587,6 +587,7 @@ > fs_stats_get(struct rte_eth_dev *dev, > struct rte_eth_stats *stats) > { > + struct rte_eth_stats backup; > struct sub_device *sdev; > uint8_t i; > int ret; > @@ -596,14 +597,20 @@ > struct rte_eth_stats *snapshot = &sdev->stats_snapshot.stats; > uint64_t *timestamp = &sdev->stats_snapshot.timestamp; > > + rte_memcpy(&backup, snapshot, sizeof(backup)); > ret = rte_eth_stats_get(PORT_ID(sdev), snapshot); > if (ret) { > + if (SUBDEV_REMOVED(sdev, ret)) { > + rte_memcpy(snapshot, &backup, sizeof(backup)); > + goto inc; > + } > ERROR("Operation rte_eth_stats_get failed for sub_device %d with error %d", > i, ret); > *timestamp = 0; > return ret; > } > *timestamp = rte_rdtsc(); > +inc: > failsafe_stats_increment(stats, snapshot); > } > return 0; > @@ -716,7 +723,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling rte_eth_dev_set_mtu on sub_device %d", i); > ret = rte_eth_dev_set_mtu(PORT_ID(sdev), mtu); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_set_mtu failed for sub_device %d with error %d", > i, ret); > return ret; > @@ -735,7 +742,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling rte_eth_dev_vlan_filter on sub_device %d", i); > ret = rte_eth_dev_vlan_filter(PORT_ID(sdev), vlan_id, on); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_vlan_filter failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -769,7 +776,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling rte_eth_dev_flow_ctrl_set on sub_device %d", i); > ret = rte_eth_dev_flow_ctrl_set(PORT_ID(sdev), fc_conf); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_flow_ctrl_set failed for sub_device %d" > " with error %d", i, ret); > return ret; > @@ -806,7 +813,7 @@ > RTE_ASSERT(index < FAILSAFE_MAX_ETHADDR); > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > ret = rte_eth_dev_mac_addr_add(PORT_ID(sdev), mac_addr, vmdq); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_mac_addr_add failed for sub_device %" > PRIu8 " with error %d", i, ret); > return ret; > @@ -848,7 +855,7 @@ > FOREACH_SUBDEV_STATE(sdev, i, dev, DEV_ACTIVE) { > DEBUG("Calling rte_eth_dev_filter_ctrl on sub_device %d", i); > ret = rte_eth_dev_filter_ctrl(PORT_ID(sdev), type, op, arg); > - if (ret) { > + if (ret && !SUBDEV_REMOVED(sdev, ret)) { > ERROR("Operation rte_eth_dev_filter_ctrl failed for sub_device %d" > " with error %d", i, ret); > return ret; > diff --git a/drivers/net/failsafe/failsafe_private.h b/drivers/net/failsafe/failsafe_private.h > index d81cc3c..ee81b70 100644 > --- a/drivers/net/failsafe/failsafe_private.h > +++ b/drivers/net/failsafe/failsafe_private.h > @@ -262,6 +262,14 @@ int failsafe_eth_lsc_event_callback(uint16_t port_id, > (ETH(s)->dev_ops->ops) > > /** > + * s: (struct sub_device *) > + * e: (int) error > + */ > +#define SUBDEV_REMOVED(s, e) \ > + (s->remove || \ > + (((e) == -ENODEV) && (ETH(s)->data->dev_flags & RTE_ETH_DEV_INTR_RMV))) > + > +/** > * Atomic guard > */ > > -- > 1.8.3.1 > -- Gaëtan Rivet 6WIND