[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