Hi,
On Wed, May 09, 2018 at 11:43:33AM +0200, Thomas Monjalon wrote:
> From: Matan Azrad <ma...@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: sta...@dpdk.org
>
> Signed-off-by: Matan Azrad <ma...@mellanox.com>
> Acked-by: Thomas Monjalon <tho...@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
