[dpdk-dev] [PATCH v2 3/6] ethdev: synchronize port allocation

Matan Azrad matan at mellanox.com
Sun Jan 7 10:58:17 CET 2018


Self-review.

> -----Original Message-----
> From: dev [mailto:dev-bounces at dpdk.org] On Behalf Of Matan Azrad
> Sent: Sunday, January 7, 2018 11:46 AM
> To: Thomas Monjalon <thomas at monjalon.net>; Gaetan Rivet
> <gaetan.rivet at 6wind.com>; Jingjing Wu <jingjing.wu at intel.com>
> Cc: dev at dpdk.org; Neil Horman <nhorman at tuxdriver.com>; Bruce
> Richardson <bruce.richardson at intel.com>; Konstantin Ananyev
> <konstantin.ananyev at intel.com>
> Subject: [dpdk-dev] [PATCH v2 3/6] ethdev: synchronize port allocation
> 
> Ethernet port allocation was not thread safe, means 2 threads which tried to
> allocate a new port at the same time might get an identical port identifier and
> caused to memory overwrite.
> Actually, all the port configurations were not thread safe from ethdev point
> of view.
> 
> The port ownership mechanism added to the ethdev is a good point to
> redefine the synchronization rules in ethdev:
> 
> 1. The port allocation and port release synchronization will be
>    managed by ethdev.
> 2. The port usage synchronization will be managed by the port owner.
> 3. The port ownership synchronization will be managed by ethdev.
> 
> Add port allocation synchronization to complete the new rules.
> 
> Signed-off-by: Matan Azrad <matan at mellanox.com>
> ---
>  config/common_base            |  4 ++--
>  lib/librte_ether/rte_ethdev.c | 39 ++++++++++++++++++++++++++++------
> -----
>  2 files changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/config/common_base b/config/common_base index
> b8ee8f9..980ae3b 100644
> --- a/config/common_base
> +++ b/config/common_base
> @@ -94,7 +94,7 @@ CONFIG_RTE_MAX_MEMSEG=256
>  CONFIG_RTE_MAX_MEMZONE=2560
>  CONFIG_RTE_MAX_TAILQ=32
>  CONFIG_RTE_ENABLE_ASSERT=n
> -CONFIG_RTE_LOG_LEVEL=RTE_LOG_INFO
> +CONFIG_RTE_LOG_LEVEL=RTE_LOG_DEBUG
Wrong change. Will fix in the next version.
>  CONFIG_RTE_LOG_DP_LEVEL=RTE_LOG_INFO
>  CONFIG_RTE_LOG_HISTORY=256
>  CONFIG_RTE_BACKTRACE=y
> @@ -136,7 +136,7 @@ CONFIG_RTE_LIBRTE_KVARGS=y  # Compile generic
> ethernet library  #  CONFIG_RTE_LIBRTE_ETHER=y -
> CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=n
> +CONFIG_RTE_LIBRTE_ETHDEV_DEBUG=y
Wrong change. Will fix in the next version.
>  CONFIG_RTE_MAX_ETHPORTS=32
>  CONFIG_RTE_MAX_QUEUES_PER_PORT=1024
>  CONFIG_RTE_LIBRTE_IEEE1588=n
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0e12452..d30d61f 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -85,6 +85,9 @@
>  /* spinlock for add/remove tx callbacks */  static rte_spinlock_t
> rte_eth_tx_cb_lock = RTE_SPINLOCK_INITIALIZER;
> 
> +/* spinlock for shared data allocation */ static rte_spinlock_t
> +rte_eth_share_data_alloc = RTE_SPINLOCK_INITIALIZER;
> +
>  /* spinlock for eth device ownership management stored in shared memory
> */  static rte_spinlock_t *rte_eth_dev_ownership_lock;
> 
> @@ -234,21 +237,27 @@ struct rte_eth_dev *  rte_eth_dev_allocate(const
> char *name)  {
>  	uint16_t port_id;
> -	struct rte_eth_dev *eth_dev;
> +	struct rte_eth_dev *eth_dev = NULL;
> +
> +	/* Synchronize share data one time allocation between local threads.
> */
> +	rte_spinlock_lock(&rte_eth_share_data_alloc);
> +	if (rte_eth_dev_data == NULL)
> +		rte_eth_dev_share_data_alloc();
> +	rte_spinlock_unlock(&rte_eth_share_data_alloc);
> +
> +	/* Synchronize port creation between primary and secondary
> threads. */
> +	rte_spinlock_lock(rte_eth_dev_ownership_lock);
> 
>  	port_id = rte_eth_dev_find_free_port();
>  	if (port_id == RTE_MAX_ETHPORTS) {
>  		RTE_PMD_DEBUG_TRACE("Reached maximum number of
> Ethernet ports\n");
> -		return NULL;
> +		goto unlock;
>  	}
> 
> -	if (rte_eth_dev_data == NULL)
> -		rte_eth_dev_share_data_alloc();
> -
>  	if (rte_eth_dev_allocated(name) != NULL) {
>  		RTE_PMD_DEBUG_TRACE("Ethernet Device with name %s
> already allocated!\n",
>  				name);
> -		return NULL;
> +		goto unlock;
>  	}
> 
>  	eth_dev = eth_dev_get(port_id);
> @@ -256,6 +265,8 @@ struct rte_eth_dev *
>  	eth_dev->data->port_id = port_id;
>  	eth_dev->data->mtu = ETHER_MTU;
> 
> +unlock:
> +	rte_spinlock_unlock(rte_eth_dev_ownership_lock);
>  	return eth_dev;
>  }
> 
> @@ -268,10 +279,16 @@ struct rte_eth_dev *
> rte_eth_dev_attach_secondary(const char *name)  {
>  	uint16_t i;
> -	struct rte_eth_dev *eth_dev;
> +	struct rte_eth_dev *eth_dev = NULL;
> 
> +	/* Synchronize share data one time attachment between local
> threads. */
> +	rte_spinlock_lock(&rte_eth_share_data_alloc);
>  	if (rte_eth_dev_data == NULL)
>  		rte_eth_dev_share_data_alloc();
> +	rte_spinlock_unlock(&rte_eth_share_data_alloc);
> +
> +	/* Synchronize port attachment to primary port creation and release.
> */
> +	rte_spinlock_lock(rte_eth_dev_ownership_lock);
> 
>  	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
>  		if (strcmp(rte_eth_dev_data[i].name, name) == 0) @@ -
> 281,12 +298,12 @@ struct rte_eth_dev *
>  		RTE_PMD_DEBUG_TRACE(
>  			"device %s is not driven by the primary process\n",
>  			name);
> -		return NULL;
> +	} else {
> +		eth_dev = eth_dev_get(i);
> +		RTE_ASSERT(eth_dev->data->port_id == i);
>  	}
> 
> -	eth_dev = eth_dev_get(i);
> -	RTE_ASSERT(eth_dev->data->port_id == i);
> -
> +	rte_spinlock_unlock(rte_eth_dev_ownership_lock);
>  	return eth_dev;
>  }
> 
> --
> 1.8.3.1



More information about the dev mailing list