Re: [dpdk-dev] [PATCH 1/3] net/failsafe: fix removal handling lack

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

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 <matan@mellanox.com>
> ---
>  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