Re: [dpdk-stable] [dpdk-dev] [PATCH 10/11] net/failsafe: fix sub-device ownership race

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

Hello Matan,

Two nitpicks below:

On Wed, May 09, 2018 at 11:43:36AM +0200, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> There is time between the sub-device port probing by the sub-device PMD
> to the sub-device port ownership taking by a fail-safe port.
> 
> In this time, the port is available for the application usage. For
> example, the port will be exposed to the applications which use
> RTE_ETH_FOREACH_DEV iterator.
> 
> Thus, ownership unaware applications may manage the port in this time
> what may cause a lot of problematic behaviors in the fail-safe
> sub-device initialization.
> 
> Register to the ethdev NEW event to take the sub-device port ownership
> before it becomes exposed to the application.
> 
> Fixes: a46f8d584eb8 ("net/failsafe: add fail-safe PMD")

This fix is relying on the RTE_ETH_EVENT_NEW, an API that I think is not
meant to be backported in the stable release that would be targetted by
this commit id.

I think this fix is useless without the rest of this series, so I don't
know what is exactly planned about the rest (whether it is backported,
and where), but I would only CC stable if this is planned, and only as
soon as the relevant APIs are introduced.

> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/net/failsafe/failsafe.c         | 22 ++++++++++---
>  drivers/net/failsafe/failsafe_eal.c     | 58 +++++++++++++++++++++------------
>  drivers/net/failsafe/failsafe_ether.c   | 23 +++++++++++++
>  drivers/net/failsafe/failsafe_private.h |  4 +++
>  4 files changed, 83 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/net/failsafe/failsafe.c b/drivers/net/failsafe/failsafe.c
> index fc989c4f5..c9d128de3 100644
> --- a/drivers/net/failsafe/failsafe.c
> +++ b/drivers/net/failsafe/failsafe.c
> @@ -204,16 +204,25 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  	}
>  	snprintf(priv->my_owner.name, sizeof(priv->my_owner.name),
>  		 FAILSAFE_OWNER_NAME);
> +	DEBUG("Failsafe port %u owner info: %s_%016"PRIX64, dev->data->port_id,
> +	      priv->my_owner.name, priv->my_owner.id);
> +	ret = rte_eth_dev_callback_register(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					    failsafe_eth_new_event_callback,
> +					    dev);
> +	if (ret) {
> +		ERROR("Failed to register NEW callback");
> +		goto free_args;
> +	}
>  	ret = failsafe_eal_init(dev);
>  	if (ret)
> -		goto free_args;
> +		goto unregister_new_callback;
>  	ret = fs_mutex_init(priv);
>  	if (ret)
> -		goto free_args;
> +		goto unregister_new_callback;
>  	ret = failsafe_hotplug_alarm_install(dev);
>  	if (ret) {
>  		ERROR("Could not set up plug-in event detection");
> -		goto free_args;
> +		goto unregister_new_callback;
>  	}
>  	mac = &dev->data->mac_addrs[0];
>  	if (mac_from_arg) {
> @@ -226,7 +235,7 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  							       mac);
>  			if (ret) {
>  				ERROR("Failed to set default MAC address");
> -				goto free_args;
> +				goto unregister_new_callback;
>  			}
>  		}
>  	} else {
> @@ -261,6 +270,9 @@ fs_eth_dev_create(struct rte_vdev_device *vdev)
>  	};
>  	rte_eth_dev_probing_finish(dev);
>  	return 0;
> +unregister_new_callback:
> +	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					failsafe_eth_new_event_callback, dev);
>  free_args:
>  	failsafe_args_free(dev);
>  free_subs:
> @@ -280,6 +292,8 @@ fs_rte_eth_free(const char *name)
>  	dev = rte_eth_dev_allocated(name);
>  	if (dev == NULL)
>  		return -ENODEV;
> +	rte_eth_dev_callback_unregister(RTE_ETH_ALL, RTE_ETH_EVENT_NEW,
> +					failsafe_eth_new_event_callback, dev);
>  	ret = failsafe_eal_uninit(dev);
>  	if (ret)
>  		ERROR("Error while uninitializing sub-EAL");
> diff --git a/drivers/net/failsafe/failsafe_eal.c b/drivers/net/failsafe/failsafe_eal.c
> index ce767703f..8f1b9d845 100644
> --- a/drivers/net/failsafe/failsafe_eal.c
> +++ b/drivers/net/failsafe/failsafe_eal.c
> @@ -10,7 +10,7 @@
>  static int
>  fs_ethdev_portid_get(const char *name, uint16_t *port_id)
>  {
> -	uint16_t pid;
> +	uint32_t pid;

I do not see why the port_id is made uint32_t? Is there a reason?

Otherwise all seems fine. With the proper justification or with uin16_t
pid, and with a second pass on the backport tagging,

Acked-by: Gaetan Rivet <gaetan.rivet@6wind.com>

Thanks,
-- 
Gaëtan Rivet
6WIND