Re: [dpdk-dev] [PATCH 07/11] ethdev: add lock to port allocation check

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

Hi,

On Wed, May 09, 2018 at 11:43:33AM +0200, Thomas Monjalon wrote:
> From: Matan Azrad <matan@mellanox.com>
> 
> When comparing the port name, there can be a race condition with
> a thread allocating a new port and writing the name at the same time.
> It can lead to match with a partial name by error.
> 
> The check of the port is now considered as a critical section
> protected with locks.
> 
> This fix will be even more required for multi-process when the
> port availability will rely only on the name, in a following patch.
> 
> Fixes: 84934303a17c ("ethdev: synchronize port allocation")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Matan Azrad <matan@mellanox.com>
> Acked-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>  lib/librte_ethdev/rte_ethdev.c | 22 +++++++++++++++++++---
>  1 file changed, 19 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/librte_ethdev/rte_ethdev.c b/lib/librte_ethdev/rte_ethdev.c
> index ae86d0ba7..357be2dca 100644
> --- a/lib/librte_ethdev/rte_ethdev.c
> +++ b/lib/librte_ethdev/rte_ethdev.c
> @@ -227,8 +227,8 @@ rte_eth_dev_shared_data_prepare(void)
>  	rte_spinlock_unlock(&rte_eth_shared_data_lock);
>  }
>  
> -struct rte_eth_dev *
> -rte_eth_dev_allocated(const char *name)
> +static struct rte_eth_dev *
> +rte_eth_dev_allocated_lock_free(const char *name)

A suggestion about the naming here.
Reading subsequent patches, we can see this function being used during
ethdev allocation routines. The _lock_free suffix is a little
misleading, as for an instant one can think that there is something
being freed about an allocated ethdev lock.

I would suggest

  * rte_eth_dev_allocated_nolock
    or
  * rte_eth_dev_allocated_lockless
    (or even rte_eth_lockless_dev_allocated)

instead.

>  {
>  	unsigned i;
>  
> @@ -240,6 +240,22 @@ rte_eth_dev_allocated(const char *name)
>  	return NULL;
>  }
>  
> +struct rte_eth_dev *
> +rte_eth_dev_allocated(const char *name)
> +{
> +	struct rte_eth_dev *ethdev;
> +
> +	rte_eth_dev_shared_data_prepare();
> +
> +	rte_spinlock_lock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	ethdev = rte_eth_dev_allocated_lock_free(name);
> +
> +	rte_spinlock_unlock(&rte_eth_dev_shared_data->ownership_lock);
> +
> +	return ethdev;
> +}
> +
>  static uint16_t
>  rte_eth_dev_find_free_port(void)
>  {
> @@ -286,7 +302,7 @@ rte_eth_dev_allocate(const char *name)
>  		goto unlock;
>  	}
>  
> -	if (rte_eth_dev_allocated(name) != NULL) {
> +	if (rte_eth_dev_allocated_lock_free(name) != NULL) {
>  		ethdev_log(ERR,
>  			"Ethernet Device with name %s already allocated!",
>  			name);
> -- 
> 2.16.2
> 

-- 
Gaëtan Rivet
6WIND