[PATCH 0/2] Release ethdev shared memory on port cleanup

[PATCH 0/2] Release ethdev shared memory on port cleanup

[David Marchand]

This series was triggered after investigating why the
eal_flags_file_prefix_autotest unit test was failing in the case of
statically built binaries [1]).

For now, I went with a simple (naive) approach and put all accesses to the
shared data under a single lock: ethdev maintainers, it is your turn to
shine and give me reasons why we should keep the locks the way they
were ;-).
And let's see what the CI reports...

1: https://inbox.dpdk.org/dev/20230816153439.551501-12-bruce.richardson@intel.com/T/#m0e4c23f7be80bbdac076a387f4a2f9094dd07e0a


-- 
David Marchand

David Marchand (2):
  ethdev: protect shared memory accesses under one lock
  ethdev: cleanup shared data with the last port

 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 36 ++++++++++-------
 lib/ethdev/ethdev_private.c         | 23 ++++++-----
 lib/ethdev/ethdev_private.h         | 12 ++++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 91 insertions(+), 52 deletions(-)

-- 
2.41.0

[PATCH 1/2] ethdev: protect shared memory accesses under one lock

[David Marchand]

ethdev currently uses two locks to protect access around
eth_dev_shared_data:
- one (process local) for avoiding multiple threads to reserve/lookup
  the eth_dev_shared_data memzone,
- one (shared with other processes) for protecting port
  allocation/destruction,

Simplify the logic and put everything under a single lock in DPDK shared
memory config.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 30 ++++++++-------
 lib/ethdev/ethdev_private.c         |  9 -----
 lib/ethdev/ethdev_private.h         |  9 +++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/lib/eal/common/eal_common_mcfg.c b/lib/eal/common/eal_common_mcfg.c
index b60d41f7b6..2a785e74c4 100644
--- a/lib/eal/common/eal_common_mcfg.c
+++ b/lib/eal/common/eal_common_mcfg.c
@@ -177,6 +177,12 @@ rte_mcfg_timer_unlock(void)
 	rte_spinlock_unlock(rte_mcfg_timer_get_lock());
 }
 
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void)
+{
+	return &rte_eal_get_configuration()->mem_config->ethdev_lock;
+}
+
 bool
 rte_mcfg_get_single_file_segments(void)
 {
diff --git a/lib/eal/common/eal_memcfg.h b/lib/eal/common/eal_memcfg.h
index 8889ba063f..d5c63e2f4d 100644
--- a/lib/eal/common/eal_memcfg.h
+++ b/lib/eal/common/eal_memcfg.h
@@ -37,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
 	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
+	rte_spinlock_t ethdev_lock; /**< used by ethdev library. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/eal/include/rte_eal_memconfig.h b/lib/eal/include/rte_eal_memconfig.h
index c527f9aa29..0b1d0d4ff0 100644
--- a/lib/eal/include/rte_eal_memconfig.h
+++ b/lib/eal/include/rte_eal_memconfig.h
@@ -39,6 +39,10 @@ __rte_internal
 rte_spinlock_t *
 rte_mcfg_timer_get_lock(void);
 
+__rte_internal
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void);
+
 /**
  * Lock the internal EAL shared memory configuration for shared access.
  */
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7940431e5a..0029db3c73 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -456,6 +456,7 @@ INTERNAL {
 	rte_intr_vec_list_free;
 	rte_intr_vec_list_index_get;
 	rte_intr_vec_list_index_set;
+	rte_mcfg_ethdev_get_lock;
 	rte_mcfg_mem_get_lock;
 	rte_mcfg_mempool_get_lock;
 	rte_mcfg_tailq_get_lock;
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 0be1e8ca04..5bb9c3f97c 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -44,6 +44,7 @@ eth_dev_allocated(const char *name)
 
 static uint16_t
 eth_dev_find_free_port(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	uint16_t i;
 
@@ -60,6 +61,7 @@ eth_dev_find_free_port(void)
 
 static struct rte_eth_dev *
 eth_dev_get(uint16_t port_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
@@ -86,10 +88,10 @@ rte_eth_dev_allocate(const char *name)
 		return NULL;
 	}
 
-	eth_dev_shared_data_prepare();
+	/* Synchronize port creation between primary and secondary processes. */
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	/* Synchronize port creation between primary and secondary threads. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -113,7 +115,7 @@ rte_eth_dev_allocate(const char *name)
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
 
 unlock:
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return eth_dev;
 }
@@ -123,13 +125,13 @@ rte_eth_dev_allocated(const char *name)
 {
 	struct rte_eth_dev *ethdev;
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	ethdev = eth_dev_allocated(name);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return ethdev;
 }
@@ -145,10 +147,10 @@ rte_eth_dev_attach_secondary(const char *name)
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	eth_dev_shared_data_prepare();
-
 	/* Synchronize port attachment to primary port creation and release. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+
+	eth_dev_shared_data_prepare();
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -163,7 +165,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
 
@@ -219,7 +221,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	eth_dev_shared_data_prepare();
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
@@ -227,7 +231,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id);
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	eth_dev->device = NULL;
@@ -251,7 +255,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return 0;
 }
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 14ec8c6ccf..6756625729 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,12 +11,8 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
-/* Shared memory between primary and secondary processes. */
 struct eth_dev_shared *eth_dev_shared_data;
 
-/* spinlock for shared data allocation */
-static rte_spinlock_t eth_dev_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
-
 /* spinlock for eth device callbacks */
 rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
 
@@ -328,8 +324,6 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	rte_spinlock_lock(&eth_dev_shared_data_lock);
-
 	if (eth_dev_shared_data == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
@@ -345,13 +339,10 @@ eth_dev_shared_data_prepare(void)
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
-			rte_spinlock_init(&eth_dev_shared_data->ownership_lock);
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
-
-	rte_spinlock_unlock(&eth_dev_shared_data_lock);
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index acb4b335c8..f7706e6a95 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -7,6 +7,7 @@
 
 #include <sys/queue.h>
 
+#include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_os_shim.h>
 
@@ -14,11 +15,12 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
-	rte_spinlock_t ownership_lock;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
-extern struct eth_dev_shared *eth_dev_shared_data;
+/* Shared memory between primary and secondary processes. */
+extern struct eth_dev_shared *eth_dev_shared_data
+	__rte_guarded_by(rte_mcfg_ethdev_get_lock());
 
 /**
  * The user application callback description.
@@ -65,7 +67,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void);
+void eth_dev_shared_data_prepare(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0840d2b594..b91191b554 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -409,6 +409,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
 
 static int
 eth_is_valid_owner_id(uint64_t owner_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
 	    eth_dev_shared_data->next_owner_id <= owner_id)
@@ -437,13 +438,12 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	*owner_id = eth_dev_shared_data->next_owner_id++;
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_new(*owner_id);
 
@@ -453,6 +453,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 static int
 eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		       const struct rte_eth_dev_owner *new_owner)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
@@ -503,13 +504,12 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 {
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_set(port_id, owner, ret);
 
@@ -523,13 +523,12 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 			{.id = RTE_ETH_DEV_NO_OWNER, .name = ""};
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_unset(port_id, owner_id, ret);
 
@@ -542,10 +541,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 	uint16_t port_id;
 	int ret = 0;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
@@ -564,7 +562,7 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		ret = -EINVAL;
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_delete(owner_id, ret);
 
@@ -591,11 +589,12 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_get(port_id, owner);
 
@@ -675,9 +674,12 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 		return -EINVAL;
 	}
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	/* shouldn't check 'rte_eth_devices[i].data',
 	 * because it might be overwritten by VDEV PMD */
 	tmp = eth_dev_shared_data->data[port_id].name;
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+
 	strcpy(name, tmp);
 
 	rte_ethdev_trace_get_name_by_port(port_id, name);
@@ -688,6 +690,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
+	int ret = -ENODEV;
 	uint16_t pid;
 
 	if (name == NULL) {
@@ -701,16 +704,19 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 
-	RTE_ETH_FOREACH_VALID_DEV(pid)
-		if (!strcmp(name, eth_dev_shared_data->data[pid].name)) {
-			*port_id = pid;
-
-			rte_ethdev_trace_get_port_by_name(name, *port_id);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+	RTE_ETH_FOREACH_VALID_DEV(pid) {
+		if (strcmp(name, eth_dev_shared_data->data[pid].name) != 0)
+			continue;
 
-			return 0;
-		}
+		*port_id = pid;
+		rte_ethdev_trace_get_port_by_name(name, *port_id);
+		ret = 0;
+		break;
+	}
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	return -ENODEV;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH 2/2] ethdev: cleanup shared data with the last port

[David Marchand]

If no port is allocated, ethdev (from a primary process) can release the
memzone used to store port data.
This makes it possible for the DPDK memory allocator to release
associated resources back to the OS.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/ethdev_driver.c  |  6 ++++++
 lib/ethdev/ethdev_private.c | 16 +++++++++++++++-
 lib/ethdev/ethdev_private.h |  3 +++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 5bb9c3f97c..c650421c3a 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -113,6 +113,8 @@ rte_eth_dev_allocate(const char *name)
 	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
 	eth_dev->data->mtu = RTE_ETHER_MTU;
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	eth_dev_shared_data->allocated_count++;
 
 unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
@@ -253,6 +255,10 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		rte_free(eth_dev->data->dev_private);
 		pthread_mutex_destroy(&eth_dev->data->flow_ops_mutex);
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+
+		eth_dev_shared_data->allocated_count--;
+		if (eth_dev_shared_data->allocated_count == 0)
+			eth_dev_shared_data_release();
 	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 6756625729..6fe2e37c56 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,6 +11,7 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
+static const struct rte_memzone *eth_dev_shared_mz;
 struct eth_dev_shared *eth_dev_shared_data;
 
 /* spinlock for eth device callbacks */
@@ -324,7 +325,7 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	if (eth_dev_shared_data == NULL) {
+	if (eth_dev_shared_mz == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
 			mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
@@ -335,16 +336,29 @@ eth_dev_shared_data_prepare(void)
 		if (mz == NULL)
 			rte_panic("Cannot allocate ethdev shared data\n");
 
+		eth_dev_shared_mz = mz;
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
+			eth_dev_shared_data->allocated_count = 0;
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
 }
 
+void
+eth_dev_shared_data_release(void)
+{
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	if (eth_dev_shared_mz != NULL) {
+		rte_memzone_free(eth_dev_shared_mz);
+		eth_dev_shared_mz = NULL;
+		eth_dev_shared_data = NULL;
+	}
+}
+
 void
 eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid)
 {
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index f7706e6a95..c456ba9a50 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -15,6 +15,7 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
+	uint64_t allocated_count;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
@@ -69,6 +70,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 
 void eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
+void eth_dev_shared_data_release(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
-- 
2.41.0

RE: [PATCH 0/2] Release ethdev shared memory on port cleanup

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Friday, 18 August 2023 11.13
> 
> This series was triggered after investigating why the
> eal_flags_file_prefix_autotest unit test was failing in the case of
> statically built binaries [1]).
> 
> For now, I went with a simple (naive) approach and put all accesses to the
> shared data under a single lock: ethdev maintainers, it is your turn to
> shine and give me reasons why we should keep the locks the way they
> were ;-).

This looks like a better solution to me. Perhaps because I'm not an ethdev maintainer. ;-)

> And let's see what the CI reports...
> 
> 1: https://inbox.dpdk.org/dev/20230816153439.551501-12-
> bruce.richardson@intel.com/T/#m0e4c23f7be80bbdac076a387f4a2f9094dd07e0a

Series-acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 2/2] ethdev: cleanup shared data with the last port

[David Marchand]

On Fri, Aug 18, 2023 at 11:13 AM David Marchand
<david.marchand@redhat.com> wrote:
> @@ -253,6 +255,10 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
>                 rte_free(eth_dev->data->dev_private);
>                 pthread_mutex_destroy(&eth_dev->data->flow_ops_mutex);
>                 memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));

At device cleanup stage (rte_eal_cleanup -> bus -> device),
eth_dev_allocated() may still be called and use a leftover reference
to (freed) data.
So here, we need to reset it to NULL (caught by ASan with test-null.sh
in the CI).

This will be in v2.


> +
> +               eth_dev_shared_data->allocated_count--;
> +               if (eth_dev_shared_data->allocated_count == 0)
> +                       eth_dev_shared_data_release();
>         }
>
>         rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());


-- 
David Marchand

[PATCH v2 0/2] Release ethdev shared memory on port cleanup

[David Marchand]

This series was triggered after investigating why the
eal_flags_file_prefix_autotest unit test was failing in the case of
statically built binaries [1]).

For now, I went with a simple (naive) approach and put all accesses to the
shared data under a single lock: ethdev maintainers, it is your turn to
shine and give me reasons why we should keep the locks the way they
were ;-).
And let's see what the CI reports...

1: https://inbox.dpdk.org/dev/20230816153439.551501-12-bruce.richardson@intel.com/T/#m0e4c23f7be80bbdac076a387f4a2f9094dd07e0a


-- 
David Marchand

Changes since v1:
- fixed uaf in port cleanup,

David Marchand (2):
  ethdev: protect shared memory accesses under one lock
  ethdev: cleanup shared data with the last port

 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 37 +++++++++++-------
 lib/ethdev/ethdev_private.c         | 23 ++++++-----
 lib/ethdev/ethdev_private.h         | 12 ++++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 92 insertions(+), 52 deletions(-)

-- 
2.41.0

[PATCH v2 1/2] ethdev: protect shared memory accesses under one lock

[David Marchand]

ethdev currently uses two locks to protect access around
eth_dev_shared_data:
- one (process local) for avoiding multiple threads to reserve/lookup
  the eth_dev_shared_data memzone,
- one (shared with other processes) for protecting port
  allocation/destruction,

Simplify the logic and put everything under a single lock in DPDK shared
memory config.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 30 ++++++++-------
 lib/ethdev/ethdev_private.c         |  9 -----
 lib/ethdev/ethdev_private.h         |  9 +++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/lib/eal/common/eal_common_mcfg.c b/lib/eal/common/eal_common_mcfg.c
index b60d41f7b6..2a785e74c4 100644
--- a/lib/eal/common/eal_common_mcfg.c
+++ b/lib/eal/common/eal_common_mcfg.c
@@ -177,6 +177,12 @@ rte_mcfg_timer_unlock(void)
 	rte_spinlock_unlock(rte_mcfg_timer_get_lock());
 }
 
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void)
+{
+	return &rte_eal_get_configuration()->mem_config->ethdev_lock;
+}
+
 bool
 rte_mcfg_get_single_file_segments(void)
 {
diff --git a/lib/eal/common/eal_memcfg.h b/lib/eal/common/eal_memcfg.h
index 8889ba063f..d5c63e2f4d 100644
--- a/lib/eal/common/eal_memcfg.h
+++ b/lib/eal/common/eal_memcfg.h
@@ -37,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
 	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
+	rte_spinlock_t ethdev_lock; /**< used by ethdev library. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/eal/include/rte_eal_memconfig.h b/lib/eal/include/rte_eal_memconfig.h
index c527f9aa29..0b1d0d4ff0 100644
--- a/lib/eal/include/rte_eal_memconfig.h
+++ b/lib/eal/include/rte_eal_memconfig.h
@@ -39,6 +39,10 @@ __rte_internal
 rte_spinlock_t *
 rte_mcfg_timer_get_lock(void);
 
+__rte_internal
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void);
+
 /**
  * Lock the internal EAL shared memory configuration for shared access.
  */
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7940431e5a..0029db3c73 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -456,6 +456,7 @@ INTERNAL {
 	rte_intr_vec_list_free;
 	rte_intr_vec_list_index_get;
 	rte_intr_vec_list_index_set;
+	rte_mcfg_ethdev_get_lock;
 	rte_mcfg_mem_get_lock;
 	rte_mcfg_mempool_get_lock;
 	rte_mcfg_tailq_get_lock;
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 0be1e8ca04..5bb9c3f97c 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -44,6 +44,7 @@ eth_dev_allocated(const char *name)
 
 static uint16_t
 eth_dev_find_free_port(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	uint16_t i;
 
@@ -60,6 +61,7 @@ eth_dev_find_free_port(void)
 
 static struct rte_eth_dev *
 eth_dev_get(uint16_t port_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
@@ -86,10 +88,10 @@ rte_eth_dev_allocate(const char *name)
 		return NULL;
 	}
 
-	eth_dev_shared_data_prepare();
+	/* Synchronize port creation between primary and secondary processes. */
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	/* Synchronize port creation between primary and secondary threads. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -113,7 +115,7 @@ rte_eth_dev_allocate(const char *name)
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
 
 unlock:
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return eth_dev;
 }
@@ -123,13 +125,13 @@ rte_eth_dev_allocated(const char *name)
 {
 	struct rte_eth_dev *ethdev;
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	ethdev = eth_dev_allocated(name);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return ethdev;
 }
@@ -145,10 +147,10 @@ rte_eth_dev_attach_secondary(const char *name)
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	eth_dev_shared_data_prepare();
-
 	/* Synchronize port attachment to primary port creation and release. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+
+	eth_dev_shared_data_prepare();
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -163,7 +165,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
 
@@ -219,7 +221,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	eth_dev_shared_data_prepare();
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
@@ -227,7 +231,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id);
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	eth_dev->device = NULL;
@@ -251,7 +255,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return 0;
 }
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 14ec8c6ccf..6756625729 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,12 +11,8 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
-/* Shared memory between primary and secondary processes. */
 struct eth_dev_shared *eth_dev_shared_data;
 
-/* spinlock for shared data allocation */
-static rte_spinlock_t eth_dev_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
-
 /* spinlock for eth device callbacks */
 rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
 
@@ -328,8 +324,6 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	rte_spinlock_lock(&eth_dev_shared_data_lock);
-
 	if (eth_dev_shared_data == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
@@ -345,13 +339,10 @@ eth_dev_shared_data_prepare(void)
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
-			rte_spinlock_init(&eth_dev_shared_data->ownership_lock);
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
-
-	rte_spinlock_unlock(&eth_dev_shared_data_lock);
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index acb4b335c8..f7706e6a95 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -7,6 +7,7 @@
 
 #include <sys/queue.h>
 
+#include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_os_shim.h>
 
@@ -14,11 +15,12 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
-	rte_spinlock_t ownership_lock;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
-extern struct eth_dev_shared *eth_dev_shared_data;
+/* Shared memory between primary and secondary processes. */
+extern struct eth_dev_shared *eth_dev_shared_data
+	__rte_guarded_by(rte_mcfg_ethdev_get_lock());
 
 /**
  * The user application callback description.
@@ -65,7 +67,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void);
+void eth_dev_shared_data_prepare(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0840d2b594..b91191b554 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -409,6 +409,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
 
 static int
 eth_is_valid_owner_id(uint64_t owner_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
 	    eth_dev_shared_data->next_owner_id <= owner_id)
@@ -437,13 +438,12 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	*owner_id = eth_dev_shared_data->next_owner_id++;
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_new(*owner_id);
 
@@ -453,6 +453,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 static int
 eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		       const struct rte_eth_dev_owner *new_owner)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
@@ -503,13 +504,12 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 {
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_set(port_id, owner, ret);
 
@@ -523,13 +523,12 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 			{.id = RTE_ETH_DEV_NO_OWNER, .name = ""};
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_unset(port_id, owner_id, ret);
 
@@ -542,10 +541,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 	uint16_t port_id;
 	int ret = 0;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
@@ -564,7 +562,7 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		ret = -EINVAL;
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_delete(owner_id, ret);
 
@@ -591,11 +589,12 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_get(port_id, owner);
 
@@ -675,9 +674,12 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 		return -EINVAL;
 	}
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	/* shouldn't check 'rte_eth_devices[i].data',
 	 * because it might be overwritten by VDEV PMD */
 	tmp = eth_dev_shared_data->data[port_id].name;
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+
 	strcpy(name, tmp);
 
 	rte_ethdev_trace_get_name_by_port(port_id, name);
@@ -688,6 +690,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
+	int ret = -ENODEV;
 	uint16_t pid;
 
 	if (name == NULL) {
@@ -701,16 +704,19 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 
-	RTE_ETH_FOREACH_VALID_DEV(pid)
-		if (!strcmp(name, eth_dev_shared_data->data[pid].name)) {
-			*port_id = pid;
-
-			rte_ethdev_trace_get_port_by_name(name, *port_id);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+	RTE_ETH_FOREACH_VALID_DEV(pid) {
+		if (strcmp(name, eth_dev_shared_data->data[pid].name) != 0)
+			continue;
 
-			return 0;
-		}
+		*port_id = pid;
+		rte_ethdev_trace_get_port_by_name(name, *port_id);
+		ret = 0;
+		break;
+	}
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	return -ENODEV;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH v2 2/2] ethdev: cleanup shared data with the last port

[David Marchand]

If no port is allocated, ethdev (from a primary process) can release the
memzone used to store port data.
This makes it possible for the DPDK memory allocator to release
associated resources back to the OS.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/ethdev_driver.c  |  7 +++++++
 lib/ethdev/ethdev_private.c | 16 +++++++++++++++-
 lib/ethdev/ethdev_private.h |  3 +++
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 5bb9c3f97c..340e448c1d 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -113,6 +113,8 @@ rte_eth_dev_allocate(const char *name)
 	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
 	eth_dev->data->mtu = RTE_ETHER_MTU;
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	eth_dev_shared_data->allocated_count++;
 
 unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
@@ -253,6 +255,11 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		rte_free(eth_dev->data->dev_private);
 		pthread_mutex_destroy(&eth_dev->data->flow_ops_mutex);
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+		eth_dev->data = NULL;
+
+		eth_dev_shared_data->allocated_count--;
+		if (eth_dev_shared_data->allocated_count == 0)
+			eth_dev_shared_data_release();
 	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 6756625729..6fe2e37c56 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,6 +11,7 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
+static const struct rte_memzone *eth_dev_shared_mz;
 struct eth_dev_shared *eth_dev_shared_data;
 
 /* spinlock for eth device callbacks */
@@ -324,7 +325,7 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	if (eth_dev_shared_data == NULL) {
+	if (eth_dev_shared_mz == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
 			mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
@@ -335,16 +336,29 @@ eth_dev_shared_data_prepare(void)
 		if (mz == NULL)
 			rte_panic("Cannot allocate ethdev shared data\n");
 
+		eth_dev_shared_mz = mz;
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
+			eth_dev_shared_data->allocated_count = 0;
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
 }
 
+void
+eth_dev_shared_data_release(void)
+{
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	if (eth_dev_shared_mz != NULL) {
+		rte_memzone_free(eth_dev_shared_mz);
+		eth_dev_shared_mz = NULL;
+		eth_dev_shared_data = NULL;
+	}
+}
+
 void
 eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid)
 {
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index f7706e6a95..c456ba9a50 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -15,6 +15,7 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
+	uint64_t allocated_count;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
@@ -69,6 +70,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 
 void eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
+void eth_dev_shared_data_release(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
-- 
2.41.0

[PATCH v3 0/3] Release ethdev shared memory on port cleanup

[David Marchand]

This series was triggered after investigating why the
eal_flags_file_prefix_autotest unit test was failing in the case of
statically built binaries [1]).

For now, I went with a simple (naive) approach and put all accesses to the
shared data under a single lock: ethdev maintainers, it is your turn to
shine and give me reasons why we should keep the locks the way they
were ;-).
And let's see what the CI reports...

1: https://inbox.dpdk.org/dev/20230816153439.551501-12-bruce.richardson@intel.com/T/#m0e4c23f7be80bbdac076a387f4a2f9094dd07e0a


-- 
David Marchand

Changes since v2:
- fixed multiprocess via patch 2,
- fixed "ownership" history (not releasing shared mem if some owner is
  registered),

Changes since v1:
- fixed uaf in port cleanup,

David Marchand (3):
  ethdev: protect shared memory accesses under one lock
  ethdev: avoid panicking in absence of ethdev shared data
  ethdev: cleanup shared data with the last port

 lib/eal/common/eal_common_mcfg.c    |  6 ++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 53 ++++++++++-----
 lib/ethdev/ethdev_private.c         | 38 +++++++----
 lib/ethdev/ethdev_private.h         | 13 +++-
 lib/ethdev/ethdev_trace.h           |  6 +-
 lib/ethdev/rte_ethdev.c             | 99 ++++++++++++++++++-----------
 9 files changed, 151 insertions(+), 70 deletions(-)

-- 
2.41.0

[PATCH v3 1/3] ethdev: protect shared memory accesses under one lock

[David Marchand]

ethdev currently uses two locks to protect access around
eth_dev_shared_data:
- one (process local) for avoiding multiple threads to reserve/lookup
  the eth_dev_shared_data memzone,
- one (shared with other processes) for protecting port
  allocation/destruction,

Simplify the logic and put everything under a single lock in DPDK shared
memory config.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 30 ++++++++-------
 lib/ethdev/ethdev_private.c         |  9 -----
 lib/ethdev/ethdev_private.h         |  9 +++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/lib/eal/common/eal_common_mcfg.c b/lib/eal/common/eal_common_mcfg.c
index b60d41f7b6..2a785e74c4 100644
--- a/lib/eal/common/eal_common_mcfg.c
+++ b/lib/eal/common/eal_common_mcfg.c
@@ -177,6 +177,12 @@ rte_mcfg_timer_unlock(void)
 	rte_spinlock_unlock(rte_mcfg_timer_get_lock());
 }
 
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void)
+{
+	return &rte_eal_get_configuration()->mem_config->ethdev_lock;
+}
+
 bool
 rte_mcfg_get_single_file_segments(void)
 {
diff --git a/lib/eal/common/eal_memcfg.h b/lib/eal/common/eal_memcfg.h
index 8889ba063f..d5c63e2f4d 100644
--- a/lib/eal/common/eal_memcfg.h
+++ b/lib/eal/common/eal_memcfg.h
@@ -37,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
 	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
+	rte_spinlock_t ethdev_lock; /**< used by ethdev library. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/eal/include/rte_eal_memconfig.h b/lib/eal/include/rte_eal_memconfig.h
index c527f9aa29..0b1d0d4ff0 100644
--- a/lib/eal/include/rte_eal_memconfig.h
+++ b/lib/eal/include/rte_eal_memconfig.h
@@ -39,6 +39,10 @@ __rte_internal
 rte_spinlock_t *
 rte_mcfg_timer_get_lock(void);
 
+__rte_internal
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void);
+
 /**
  * Lock the internal EAL shared memory configuration for shared access.
  */
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 7940431e5a..0029db3c73 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -456,6 +456,7 @@ INTERNAL {
 	rte_intr_vec_list_free;
 	rte_intr_vec_list_index_get;
 	rte_intr_vec_list_index_set;
+	rte_mcfg_ethdev_get_lock;
 	rte_mcfg_mem_get_lock;
 	rte_mcfg_mempool_get_lock;
 	rte_mcfg_tailq_get_lock;
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 0be1e8ca04..5bb9c3f97c 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -44,6 +44,7 @@ eth_dev_allocated(const char *name)
 
 static uint16_t
 eth_dev_find_free_port(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	uint16_t i;
 
@@ -60,6 +61,7 @@ eth_dev_find_free_port(void)
 
 static struct rte_eth_dev *
 eth_dev_get(uint16_t port_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
@@ -86,10 +88,10 @@ rte_eth_dev_allocate(const char *name)
 		return NULL;
 	}
 
-	eth_dev_shared_data_prepare();
+	/* Synchronize port creation between primary and secondary processes. */
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	/* Synchronize port creation between primary and secondary threads. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -113,7 +115,7 @@ rte_eth_dev_allocate(const char *name)
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
 
 unlock:
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return eth_dev;
 }
@@ -123,13 +125,13 @@ rte_eth_dev_allocated(const char *name)
 {
 	struct rte_eth_dev *ethdev;
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	ethdev = eth_dev_allocated(name);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return ethdev;
 }
@@ -145,10 +147,10 @@ rte_eth_dev_attach_secondary(const char *name)
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	eth_dev_shared_data_prepare();
-
 	/* Synchronize port attachment to primary port creation and release. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+
+	eth_dev_shared_data_prepare();
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -163,7 +165,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
 
@@ -219,7 +221,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	eth_dev_shared_data_prepare();
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
@@ -227,7 +231,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id);
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	eth_dev->device = NULL;
@@ -251,7 +255,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return 0;
 }
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 14ec8c6ccf..6756625729 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,12 +11,8 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
-/* Shared memory between primary and secondary processes. */
 struct eth_dev_shared *eth_dev_shared_data;
 
-/* spinlock for shared data allocation */
-static rte_spinlock_t eth_dev_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
-
 /* spinlock for eth device callbacks */
 rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
 
@@ -328,8 +324,6 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	rte_spinlock_lock(&eth_dev_shared_data_lock);
-
 	if (eth_dev_shared_data == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
@@ -345,13 +339,10 @@ eth_dev_shared_data_prepare(void)
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
-			rte_spinlock_init(&eth_dev_shared_data->ownership_lock);
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
-
-	rte_spinlock_unlock(&eth_dev_shared_data_lock);
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index acb4b335c8..f7706e6a95 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -7,6 +7,7 @@
 
 #include <sys/queue.h>
 
+#include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_os_shim.h>
 
@@ -14,11 +15,12 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
-	rte_spinlock_t ownership_lock;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
-extern struct eth_dev_shared *eth_dev_shared_data;
+/* Shared memory between primary and secondary processes. */
+extern struct eth_dev_shared *eth_dev_shared_data
+	__rte_guarded_by(rte_mcfg_ethdev_get_lock());
 
 /**
  * The user application callback description.
@@ -65,7 +67,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void);
+void eth_dev_shared_data_prepare(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 0840d2b594..b91191b554 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -409,6 +409,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
 
 static int
 eth_is_valid_owner_id(uint64_t owner_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
 	    eth_dev_shared_data->next_owner_id <= owner_id)
@@ -437,13 +438,12 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	*owner_id = eth_dev_shared_data->next_owner_id++;
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_new(*owner_id);
 
@@ -453,6 +453,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 static int
 eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		       const struct rte_eth_dev_owner *new_owner)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
@@ -503,13 +504,12 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 {
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_set(port_id, owner, ret);
 
@@ -523,13 +523,12 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 			{.id = RTE_ETH_DEV_NO_OWNER, .name = ""};
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_unset(port_id, owner_id, ret);
 
@@ -542,10 +541,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 	uint16_t port_id;
 	int ret = 0;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
@@ -564,7 +562,7 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		ret = -EINVAL;
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_delete(owner_id, ret);
 
@@ -591,11 +589,12 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_get(port_id, owner);
 
@@ -675,9 +674,12 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 		return -EINVAL;
 	}
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	/* shouldn't check 'rte_eth_devices[i].data',
 	 * because it might be overwritten by VDEV PMD */
 	tmp = eth_dev_shared_data->data[port_id].name;
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+
 	strcpy(name, tmp);
 
 	rte_ethdev_trace_get_name_by_port(port_id, name);
@@ -688,6 +690,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
+	int ret = -ENODEV;
 	uint16_t pid;
 
 	if (name == NULL) {
@@ -701,16 +704,19 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 
-	RTE_ETH_FOREACH_VALID_DEV(pid)
-		if (!strcmp(name, eth_dev_shared_data->data[pid].name)) {
-			*port_id = pid;
-
-			rte_ethdev_trace_get_port_by_name(name, *port_id);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+	RTE_ETH_FOREACH_VALID_DEV(pid) {
+		if (strcmp(name, eth_dev_shared_data->data[pid].name) != 0)
+			continue;
 
-			return 0;
-		}
+		*port_id = pid;
+		rte_ethdev_trace_get_port_by_name(name, *port_id);
+		ret = 0;
+		break;
+	}
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	return -ENODEV;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH v3 2/3] ethdev: avoid panicking in absence of ethdev shared data

[David Marchand]

This is a preparation step before freeing the ethdev shared data
memzone.

Previously, because the primary process never freed the memzone, a
secondary process could assume this memzone was present.
But in the next commit, this will change.
Make eth_dev_shared_data_prepare() report whether the memzone is
available so that upper level API can react accordingly.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/ethdev_driver.c  | 23 ++++++++++++++-----
 lib/ethdev/ethdev_private.c | 10 +++++---
 lib/ethdev/ethdev_private.h |  2 +-
 lib/ethdev/ethdev_trace.h   |  6 +++--
 lib/ethdev/rte_ethdev.c     | 46 +++++++++++++++++++++++++------------
 5 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 5bb9c3f97c..f04666f3a2 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -91,7 +91,8 @@ rte_eth_dev_allocate(const char *name)
 	/* Synchronize port creation between primary and secondary processes. */
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		goto unlock;
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -127,9 +128,10 @@ rte_eth_dev_allocated(const char *name)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-
-	ethdev = eth_dev_allocated(name);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ethdev = eth_dev_allocated(name);
+	else
+		ethdev = NULL;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -150,7 +152,8 @@ rte_eth_dev_attach_secondary(const char *name)
 	/* Synchronize port attachment to primary port creation and release. */
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		goto unlock;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -165,6 +168,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
+unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
@@ -218,12 +222,19 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 int
 rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
+	int ret;
+
 	if (eth_dev == NULL)
 		return -EINVAL;
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		ret = -EINVAL;
+	else
+		ret = 0;
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+	if (ret != 0)
+		return ret;
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 6756625729..911de1e595 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -318,7 +318,7 @@ rte_eth_call_tx_callbacks(uint16_t port_id, uint16_t queue_id,
 	return nb_pkts;
 }
 
-void
+void *
 eth_dev_shared_data_prepare(void)
 {
 	const unsigned int flags = 0;
@@ -332,8 +332,10 @@ eth_dev_shared_data_prepare(void)
 					rte_socket_id(), flags);
 		} else
 			mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
-		if (mz == NULL)
-			rte_panic("Cannot allocate ethdev shared data\n");
+		if (mz == NULL) {
+			RTE_ETHDEV_LOG(ERR, "Cannot allocate ethdev shared data\n");
+			goto out;
+		}
 
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
@@ -343,6 +345,8 @@ eth_dev_shared_data_prepare(void)
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
+out:
+	return eth_dev_shared_data;
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index f7706e6a95..1572da7b48 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -67,7 +67,7 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void)
+void *eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 423e71236e..e367d29c3a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -112,8 +112,9 @@ RTE_TRACE_POINT(
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_owner_new,
-	RTE_TRACE_POINT_ARGS(uint64_t owner_id),
+	RTE_TRACE_POINT_ARGS(uint64_t owner_id, int ret),
 	rte_trace_point_emit_u64(owner_id);
+	rte_trace_point_emit_int(ret);
 )
 
 RTE_TRACE_POINT(
@@ -377,10 +378,11 @@ RTE_TRACE_POINT(
 RTE_TRACE_POINT(
 	rte_ethdev_trace_owner_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
-		const struct rte_eth_dev_owner *owner),
+		const struct rte_eth_dev_owner *owner, int ret),
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u64(owner->id);
 	rte_trace_point_emit_string(owner->name);
+	rte_trace_point_emit_int(ret);
 )
 
 RTE_TRACE_POINT(
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index b91191b554..ea430d8bab 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -388,7 +388,7 @@ rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id)
 static bool
 eth_dev_is_allocated(const struct rte_eth_dev *ethdev)
 {
-	return ethdev->data->name[0] != '\0';
+	return ethdev->data != NULL && ethdev->data->name[0] != '\0';
 }
 
 int
@@ -433,6 +433,8 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 int
 rte_eth_dev_owner_new(uint64_t *owner_id)
 {
+	int ret;
+
 	if (owner_id == NULL) {
 		RTE_ETHDEV_LOG(ERR, "Cannot get new owner ID to NULL\n");
 		return -EINVAL;
@@ -440,14 +442,18 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	*owner_id = eth_dev_shared_data->next_owner_id++;
+	if (eth_dev_shared_data_prepare() != NULL) {
+		*owner_id = eth_dev_shared_data->next_owner_id++;
+		ret = 0;
+	} else {
+		ret = -ENOMEM;
+	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	rte_ethdev_trace_owner_new(*owner_id);
+	rte_ethdev_trace_owner_new(*owner_id, ret);
 
-	return 0;
+	return ret;
 }
 
 static int
@@ -506,8 +512,10 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
+	else
+		ret = -ENOMEM;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -525,8 +533,10 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
+	else
+		ret = -ENOMEM;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -543,8 +553,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	if (eth_is_valid_owner_id(owner_id)) {
+	if (eth_dev_shared_data_prepare() == NULL) {
+		ret = -ENOMEM;
+	} else if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
 				rte_eth_devices[port_id].data;
@@ -573,6 +584,7 @@ int
 rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 {
 	struct rte_eth_dev *ethdev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	ethdev = &rte_eth_devices[port_id];
@@ -591,14 +603,18 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
+	if (eth_dev_shared_data_prepare() != NULL) {
+		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
+		ret = 0;
+	} else {
+		ret = -ENOMEM;
+	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	rte_ethdev_trace_owner_get(port_id, owner);
+	rte_ethdev_trace_owner_get(port_id, owner, ret);
 
-	return 0;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH v3 3/3] ethdev: cleanup shared data with the last port

[David Marchand]

If no port is allocated and no port owner is still registered,
ethdev from a primary process may release the memzone used to store
port data.
This makes it possible for the DPDK memory allocator to release
associated resources back to the OS.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
Changes since v2:
- tracked owners count and and avoided releasing shared mem if some
  owner is still registered,

---
 lib/ethdev/ethdev_driver.c  |  6 ++++++
 lib/ethdev/ethdev_private.c | 21 ++++++++++++++++++++-
 lib/ethdev/ethdev_private.h |  4 ++++
 lib/ethdev/rte_ethdev.c     |  3 +++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index f04666f3a2..c30cb33963 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -114,6 +114,8 @@ rte_eth_dev_allocate(const char *name)
 	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
 	eth_dev->data->mtu = RTE_ETHER_MTU;
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	eth_dev_shared_data->allocated_ports++;
 
 unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
@@ -264,6 +266,10 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		rte_free(eth_dev->data->dev_private);
 		pthread_mutex_destroy(&eth_dev->data->flow_ops_mutex);
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+		eth_dev->data = NULL;
+
+		eth_dev_shared_data->allocated_ports--;
+		eth_dev_shared_data_release();
 	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 911de1e595..aea9112cc2 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,6 +11,7 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
+static const struct rte_memzone *eth_dev_shared_mz;
 struct eth_dev_shared *eth_dev_shared_data;
 
 /* spinlock for eth device callbacks */
@@ -324,7 +325,7 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	if (eth_dev_shared_data == NULL) {
+	if (eth_dev_shared_mz == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
 			mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
@@ -337,10 +338,13 @@ eth_dev_shared_data_prepare(void)
 			goto out;
 		}
 
+		eth_dev_shared_mz = mz;
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+			eth_dev_shared_data->allocated_owners = 0;
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
+			eth_dev_shared_data->allocated_ports = 0;
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
@@ -349,6 +353,21 @@ eth_dev_shared_data_prepare(void)
 	return eth_dev_shared_data;
 }
 
+void
+eth_dev_shared_data_release(void)
+{
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+
+	if (eth_dev_shared_data->allocated_ports != 0)
+		return;
+	if (eth_dev_shared_data->allocated_owners != 0)
+		return;
+
+	rte_memzone_free(eth_dev_shared_mz);
+	eth_dev_shared_mz = NULL;
+	eth_dev_shared_data = NULL;
+}
+
 void
 eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid)
 {
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index 1572da7b48..0d36b9c30f 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -14,7 +14,9 @@
 #include "rte_ethdev.h"
 
 struct eth_dev_shared {
+	uint64_t allocated_owners;
 	uint64_t next_owner_id;
+	uint64_t allocated_ports;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
@@ -69,6 +71,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 
 void *eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
+void eth_dev_shared_data_release(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index ea430d8bab..35925a403a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -444,6 +444,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 
 	if (eth_dev_shared_data_prepare() != NULL) {
 		*owner_id = eth_dev_shared_data->next_owner_id++;
+		eth_dev_shared_data->allocated_owners++;
 		ret = 0;
 	} else {
 		ret = -ENOMEM;
@@ -566,6 +567,8 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		RTE_ETHDEV_LOG(NOTICE,
 			"All port owners owned by %016"PRIx64" identifier have removed\n",
 			owner_id);
+		eth_dev_shared_data->allocated_owners--;
+		eth_dev_shared_data_release();
 	} else {
 		RTE_ETHDEV_LOG(ERR,
 			       "Invalid owner ID=%016"PRIx64"\n",
-- 
2.41.0

Re: [PATCH 0/2] Release ethdev shared memory on port cleanup

[Thomas Monjalon]

18/08/2023 12:18, Morten Brørup:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Friday, 18 August 2023 11.13
> > 
> > This series was triggered after investigating why the
> > eal_flags_file_prefix_autotest unit test was failing in the case of
> > statically built binaries [1]).
> > 
> > For now, I went with a simple (naive) approach and put all accesses to the
> > shared data under a single lock: ethdev maintainers, it is your turn to
> > shine and give me reasons why we should keep the locks the way they
> > were ;-).
> 
> This looks like a better solution to me. Perhaps because I'm not an ethdev maintainer. ;-)

Yes Morten, you didn't get the beauty of ethdev complexity :)

Re: [PATCH v3 0/3] Release ethdev shared memory on port cleanup

[Thomas Monjalon]

21/08/2023 10:58, David Marchand:
> This series was triggered after investigating why the
> eal_flags_file_prefix_autotest unit test was failing in the case of
> statically built binaries [1]).
> 
> For now, I went with a simple (naive) approach and put all accesses to the
> shared data under a single lock: ethdev maintainers, it is your turn to
> shine and give me reasons why we should keep the locks the way they
> were ;-).

I think the reasons are:
- we wanted to call rte_spinlock_init()
- we didn't want to allocate an ethdev lock in EAL memory config

How eliminating a lock is making the last patch easier exactly?

Re: [PATCH v3 0/3] Release ethdev shared memory on port cleanup

[David Marchand]

On Thu, Aug 31, 2023 at 6:14 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> 21/08/2023 10:58, David Marchand:
> > This series was triggered after investigating why the
> > eal_flags_file_prefix_autotest unit test was failing in the case of
> > statically built binaries [1]).
> >
> > For now, I went with a simple (naive) approach and put all accesses to the
> > shared data under a single lock: ethdev maintainers, it is your turn to
> > shine and give me reasons why we should keep the locks the way they
> > were ;-).
>
> I think the reasons are:
> - we wanted to call rte_spinlock_init()
> - we didn't want to allocate an ethdev lock in EAL memory config

Hum, it is a choice of implementation, not a list of locking requirements.

As to why we would not want ethdev lock in EAL, I can understand the concern.
This could be enhanced with a new service provided by EAL to register
some space in the shared configuration for use by libraries.
However, seeing how the mempool and timer libraries already had one
lock stored there, I assumed it was ok to do the same.


> How eliminating a lock is making the last patch easier exactly?

Let's say that my goal is to cleanup resources once a DPDK application
exits (no hugepage files left).
Here, it is a first step in that direction, with releasing ethdev
shared mem data.
It is not a complete solution, as other device classes probably have
the same issues of leaving some shared data behind.


The ethdev shared memory hosts ethdev ports, and the ownership notion.
The current code implicitly relies on the assumption that the shared
memory will never go away.
So I had to revisit and protect places accessing this shared memory,
and having one shared lock for protecting access was necessary.
But doing so, the ownership lock would be nested in this global lock
while doing nothing more that the ethdev shared data lock.

I will enhance the patches description.


-- 
David Marchand

[PATCH v4 0/3] Release ethdev shared memory on port cleanup

[David Marchand]

This series was triggered after investigating why the
eal_flags_file_prefix_autotest unit test was failing in the case of
statically built binaries [1]).

For now, I went with a simple (naive) approach and put all accesses to the
shared data under a single lock: ethdev maintainers, it is your turn to
shine and give me reasons why we should keep the locks the way they
were ;-).
And let's see what the CI reports...

1: https://inbox.dpdk.org/dev/20230816153439.551501-12-bruce.richardson@intel.com/T/#m0e4c23f7be80bbdac076a387f4a2f9094dd07e0a


-- 
David Marchand

Changes since v3:
- updated commitlogs,

Changes since v2:
- fixed multiprocess via patch 2,
- fixed "ownership" history (not releasing shared mem if some owner is
  registered),

Changes since v1:
- fixed uaf in port cleanup,

David Marchand (3):
  ethdev: protect shared memory accesses under one lock
  ethdev: avoid panicking in absence of ethdev shared data
  ethdev: cleanup shared data with the last port

 lib/eal/common/eal_common_mcfg.c    |  6 ++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 53 ++++++++++-----
 lib/ethdev/ethdev_private.c         | 38 +++++++----
 lib/ethdev/ethdev_private.h         | 13 +++-
 lib/ethdev/ethdev_trace.h           |  6 +-
 lib/ethdev/rte_ethdev.c             | 99 ++++++++++++++++++-----------
 9 files changed, 151 insertions(+), 70 deletions(-)

-- 
2.41.0

[PATCH v4 1/3] ethdev: protect shared memory accesses under one lock

[David Marchand]

ethdev currently uses two locks to protect access around
eth_dev_shared_data:
- one (process local) for avoiding multiple threads to reserve/lookup
  the eth_dev_shared_data memzone,
- one (shared with other processes) for protecting port
  allocation/destruction,

A next change will make it possible for eth_dev_shared_data memzone to
be freed during a DPDK application lifetime. Accessing its content must
then be protected for concurrent accesses and with this new protection,
existing locks become unneeded.

Simplify the logic and put everything under a single lock in DPDK shared
memory config (which cannot be freed during a DPDK application lifetime).

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
Changes since v3:
- updated commitlog,

---
 lib/eal/common/eal_common_mcfg.c    |  6 +++
 lib/eal/common/eal_memcfg.h         |  1 +
 lib/eal/include/rte_eal_memconfig.h |  4 ++
 lib/eal/version.map                 |  1 +
 lib/ethdev/ethdev_driver.c          | 30 ++++++++-------
 lib/ethdev/ethdev_private.c         |  9 -----
 lib/ethdev/ethdev_private.h         |  9 +++--
 lib/ethdev/rte_ethdev.c             | 60 ++++++++++++++++-------------
 8 files changed, 68 insertions(+), 52 deletions(-)

diff --git a/lib/eal/common/eal_common_mcfg.c b/lib/eal/common/eal_common_mcfg.c
index b60d41f7b6..2a785e74c4 100644
--- a/lib/eal/common/eal_common_mcfg.c
+++ b/lib/eal/common/eal_common_mcfg.c
@@ -177,6 +177,12 @@ rte_mcfg_timer_unlock(void)
 	rte_spinlock_unlock(rte_mcfg_timer_get_lock());
 }
 
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void)
+{
+	return &rte_eal_get_configuration()->mem_config->ethdev_lock;
+}
+
 bool
 rte_mcfg_get_single_file_segments(void)
 {
diff --git a/lib/eal/common/eal_memcfg.h b/lib/eal/common/eal_memcfg.h
index 8889ba063f..d5c63e2f4d 100644
--- a/lib/eal/common/eal_memcfg.h
+++ b/lib/eal/common/eal_memcfg.h
@@ -37,6 +37,7 @@ struct rte_mem_config {
 	rte_rwlock_t qlock;   /**< used by tailqs for thread safety. */
 	rte_rwlock_t mplock;  /**< used by mempool library for thread safety. */
 	rte_spinlock_t tlock; /**< used by timer library for thread safety. */
+	rte_spinlock_t ethdev_lock; /**< used by ethdev library. */
 
 	rte_rwlock_t memory_hotplug_lock;
 	/**< Indicates whether memory hotplug request is in progress. */
diff --git a/lib/eal/include/rte_eal_memconfig.h b/lib/eal/include/rte_eal_memconfig.h
index c527f9aa29..0b1d0d4ff0 100644
--- a/lib/eal/include/rte_eal_memconfig.h
+++ b/lib/eal/include/rte_eal_memconfig.h
@@ -39,6 +39,10 @@ __rte_internal
 rte_spinlock_t *
 rte_mcfg_timer_get_lock(void);
 
+__rte_internal
+rte_spinlock_t *
+rte_mcfg_ethdev_get_lock(void);
+
 /**
  * Lock the internal EAL shared memory configuration for shared access.
  */
diff --git a/lib/eal/version.map b/lib/eal/version.map
index 915057b325..e00a844805 100644
--- a/lib/eal/version.map
+++ b/lib/eal/version.map
@@ -452,6 +452,7 @@ INTERNAL {
 	rte_intr_vec_list_free;
 	rte_intr_vec_list_index_get;
 	rte_intr_vec_list_index_set;
+	rte_mcfg_ethdev_get_lock;
 	rte_mcfg_mem_get_lock;
 	rte_mcfg_mempool_get_lock;
 	rte_mcfg_tailq_get_lock;
diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index 30db839a77..c92cd4b947 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -45,6 +45,7 @@ eth_dev_allocated(const char *name)
 
 static uint16_t
 eth_dev_find_free_port(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	uint16_t i;
 
@@ -61,6 +62,7 @@ eth_dev_find_free_port(void)
 
 static struct rte_eth_dev *
 eth_dev_get(uint16_t port_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *eth_dev = &rte_eth_devices[port_id];
 
@@ -87,10 +89,10 @@ rte_eth_dev_allocate(const char *name)
 		return NULL;
 	}
 
-	eth_dev_shared_data_prepare();
+	/* Synchronize port creation between primary and secondary processes. */
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	/* Synchronize port creation between primary and secondary threads. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -114,7 +116,7 @@ rte_eth_dev_allocate(const char *name)
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
 
 unlock:
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return eth_dev;
 }
@@ -124,13 +126,13 @@ rte_eth_dev_allocated(const char *name)
 {
 	struct rte_eth_dev *ethdev;
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 
 	ethdev = eth_dev_allocated(name);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return ethdev;
 }
@@ -146,10 +148,10 @@ rte_eth_dev_attach_secondary(const char *name)
 	uint16_t i;
 	struct rte_eth_dev *eth_dev = NULL;
 
-	eth_dev_shared_data_prepare();
-
 	/* Synchronize port attachment to primary port creation and release. */
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+
+	eth_dev_shared_data_prepare();
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -164,7 +166,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
 
@@ -220,7 +222,9 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 	if (eth_dev == NULL)
 		return -EINVAL;
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	eth_dev_shared_data_prepare();
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
@@ -228,7 +232,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 
 	eth_dev_fp_ops_reset(rte_eth_fp_ops + eth_dev->data->port_id);
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
 	eth_dev->state = RTE_ETH_DEV_UNUSED;
 	eth_dev->device = NULL;
@@ -252,7 +256,7 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	return 0;
 }
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 14ec8c6ccf..6756625729 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,12 +11,8 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
-/* Shared memory between primary and secondary processes. */
 struct eth_dev_shared *eth_dev_shared_data;
 
-/* spinlock for shared data allocation */
-static rte_spinlock_t eth_dev_shared_data_lock = RTE_SPINLOCK_INITIALIZER;
-
 /* spinlock for eth device callbacks */
 rte_spinlock_t eth_dev_cb_lock = RTE_SPINLOCK_INITIALIZER;
 
@@ -328,8 +324,6 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	rte_spinlock_lock(&eth_dev_shared_data_lock);
-
 	if (eth_dev_shared_data == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
@@ -345,13 +339,10 @@ eth_dev_shared_data_prepare(void)
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
-			rte_spinlock_init(&eth_dev_shared_data->ownership_lock);
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
-
-	rte_spinlock_unlock(&eth_dev_shared_data_lock);
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index acb4b335c8..f7706e6a95 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -7,6 +7,7 @@
 
 #include <sys/queue.h>
 
+#include <rte_eal_memconfig.h>
 #include <rte_malloc.h>
 #include <rte_os_shim.h>
 
@@ -14,11 +15,12 @@
 
 struct eth_dev_shared {
 	uint64_t next_owner_id;
-	rte_spinlock_t ownership_lock;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
-extern struct eth_dev_shared *eth_dev_shared_data;
+/* Shared memory between primary and secondary processes. */
+extern struct eth_dev_shared *eth_dev_shared_data
+	__rte_guarded_by(rte_mcfg_ethdev_get_lock());
 
 /**
  * The user application callback description.
@@ -65,7 +67,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void);
+void eth_dev_shared_data_prepare(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 46eaed6467..e0d22c27c6 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -409,6 +409,7 @@ rte_eth_dev_is_valid_port(uint16_t port_id)
 
 static int
 eth_is_valid_owner_id(uint64_t owner_id)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	if (owner_id == RTE_ETH_DEV_NO_OWNER ||
 	    eth_dev_shared_data->next_owner_id <= owner_id)
@@ -437,13 +438,12 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	*owner_id = eth_dev_shared_data->next_owner_id++;
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_new(*owner_id);
 
@@ -453,6 +453,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 static int
 eth_dev_owner_set(const uint16_t port_id, const uint64_t old_owner_id,
 		       const struct rte_eth_dev_owner *new_owner)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock())
 {
 	struct rte_eth_dev *ethdev = &rte_eth_devices[port_id];
 	struct rte_eth_dev_owner *port_owner;
@@ -503,13 +504,12 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 {
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_set(port_id, owner, ret);
 
@@ -523,13 +523,12 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 			{.id = RTE_ETH_DEV_NO_OWNER, .name = ""};
 	int ret;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_unset(port_id, owner_id, ret);
 
@@ -542,10 +541,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 	uint16_t port_id;
 	int ret = 0;
 
-	eth_dev_shared_data_prepare();
-
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
+	eth_dev_shared_data_prepare();
 	if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
@@ -564,7 +562,7 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		ret = -EINVAL;
 	}
 
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_delete(owner_id, ret);
 
@@ -591,11 +589,12 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 		return -EINVAL;
 	}
 
-	eth_dev_shared_data_prepare();
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	rte_spinlock_lock(&eth_dev_shared_data->ownership_lock);
+	eth_dev_shared_data_prepare();
 	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
-	rte_spinlock_unlock(&eth_dev_shared_data->ownership_lock);
+
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
 	rte_ethdev_trace_owner_get(port_id, owner);
 
@@ -675,9 +674,12 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 		return -EINVAL;
 	}
 
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 	/* shouldn't check 'rte_eth_devices[i].data',
 	 * because it might be overwritten by VDEV PMD */
 	tmp = eth_dev_shared_data->data[port_id].name;
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+
 	strcpy(name, tmp);
 
 	rte_ethdev_trace_get_name_by_port(port_id, name);
@@ -688,6 +690,7 @@ rte_eth_dev_get_name_by_port(uint16_t port_id, char *name)
 int
 rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 {
+	int ret = -ENODEV;
 	uint16_t pid;
 
 	if (name == NULL) {
@@ -701,16 +704,19 @@ rte_eth_dev_get_port_by_name(const char *name, uint16_t *port_id)
 		return -EINVAL;
 	}
 
-	RTE_ETH_FOREACH_VALID_DEV(pid)
-		if (!strcmp(name, eth_dev_shared_data->data[pid].name)) {
-			*port_id = pid;
-
-			rte_ethdev_trace_get_port_by_name(name, *port_id);
+	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
+	RTE_ETH_FOREACH_VALID_DEV(pid) {
+		if (strcmp(name, eth_dev_shared_data->data[pid].name) != 0)
+			continue;
 
-			return 0;
-		}
+		*port_id = pid;
+		rte_ethdev_trace_get_port_by_name(name, *port_id);
+		ret = 0;
+		break;
+	}
+	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	return -ENODEV;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH v4 2/3] ethdev: avoid panicking in absence of ethdev shared data

[David Marchand]

This is a preparation step before freeing the ethdev shared data
memzone.

Previously, because the primary process never freed the memzone, a
secondary process could assume this memzone was present.
But in the next commit, this will change.

Make eth_dev_shared_data_prepare() report whether the memzone is
available so that upper level API can react accordingly.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/ethdev/ethdev_driver.c  | 23 ++++++++++++++-----
 lib/ethdev/ethdev_private.c | 10 +++++---
 lib/ethdev/ethdev_private.h |  2 +-
 lib/ethdev/ethdev_trace.h   |  6 +++--
 lib/ethdev/rte_ethdev.c     | 46 +++++++++++++++++++++++++------------
 5 files changed, 60 insertions(+), 27 deletions(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index c92cd4b947..b339e325a0 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -92,7 +92,8 @@ rte_eth_dev_allocate(const char *name)
 	/* Synchronize port creation between primary and secondary processes. */
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		goto unlock;
 
 	if (eth_dev_allocated(name) != NULL) {
 		RTE_ETHDEV_LOG(ERR,
@@ -128,9 +129,10 @@ rte_eth_dev_allocated(const char *name)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-
-	ethdev = eth_dev_allocated(name);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ethdev = eth_dev_allocated(name);
+	else
+		ethdev = NULL;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -151,7 +153,8 @@ rte_eth_dev_attach_secondary(const char *name)
 	/* Synchronize port attachment to primary port creation and release. */
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		goto unlock;
 
 	for (i = 0; i < RTE_MAX_ETHPORTS; i++) {
 		if (strcmp(eth_dev_shared_data->data[i].name, name) == 0)
@@ -166,6 +169,7 @@ rte_eth_dev_attach_secondary(const char *name)
 		RTE_ASSERT(eth_dev->data->port_id == i);
 	}
 
+unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 	return eth_dev;
 }
@@ -219,12 +223,19 @@ rte_eth_dev_probing_finish(struct rte_eth_dev *dev)
 int
 rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 {
+	int ret;
+
 	if (eth_dev == NULL)
 		return -EINVAL;
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
-	eth_dev_shared_data_prepare();
+	if (eth_dev_shared_data_prepare() == NULL)
+		ret = -EINVAL;
+	else
+		ret = 0;
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
+	if (ret != 0)
+		return ret;
 
 	if (eth_dev->state != RTE_ETH_DEV_UNUSED)
 		rte_eth_dev_callback_process(eth_dev,
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 6756625729..911de1e595 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -318,7 +318,7 @@ rte_eth_call_tx_callbacks(uint16_t port_id, uint16_t queue_id,
 	return nb_pkts;
 }
 
-void
+void *
 eth_dev_shared_data_prepare(void)
 {
 	const unsigned int flags = 0;
@@ -332,8 +332,10 @@ eth_dev_shared_data_prepare(void)
 					rte_socket_id(), flags);
 		} else
 			mz = rte_memzone_lookup(MZ_RTE_ETH_DEV_DATA);
-		if (mz == NULL)
-			rte_panic("Cannot allocate ethdev shared data\n");
+		if (mz == NULL) {
+			RTE_ETHDEV_LOG(ERR, "Cannot allocate ethdev shared data\n");
+			goto out;
+		}
 
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
@@ -343,6 +345,8 @@ eth_dev_shared_data_prepare(void)
 			       sizeof(eth_dev_shared_data->data));
 		}
 	}
+out:
+	return eth_dev_shared_data;
 }
 
 void
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index f7706e6a95..1572da7b48 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -67,7 +67,7 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 		const struct rte_eth_dev *dev);
 
 
-void eth_dev_shared_data_prepare(void)
+void *eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/ethdev_trace.h b/lib/ethdev/ethdev_trace.h
index 423e71236e..e367d29c3a 100644
--- a/lib/ethdev/ethdev_trace.h
+++ b/lib/ethdev/ethdev_trace.h
@@ -112,8 +112,9 @@ RTE_TRACE_POINT(
 
 RTE_TRACE_POINT(
 	rte_ethdev_trace_owner_new,
-	RTE_TRACE_POINT_ARGS(uint64_t owner_id),
+	RTE_TRACE_POINT_ARGS(uint64_t owner_id, int ret),
 	rte_trace_point_emit_u64(owner_id);
+	rte_trace_point_emit_int(ret);
 )
 
 RTE_TRACE_POINT(
@@ -377,10 +378,11 @@ RTE_TRACE_POINT(
 RTE_TRACE_POINT(
 	rte_ethdev_trace_owner_get,
 	RTE_TRACE_POINT_ARGS(uint16_t port_id,
-		const struct rte_eth_dev_owner *owner),
+		const struct rte_eth_dev_owner *owner, int ret),
 	rte_trace_point_emit_u16(port_id);
 	rte_trace_point_emit_u64(owner->id);
 	rte_trace_point_emit_string(owner->name);
+	rte_trace_point_emit_int(ret);
 )
 
 RTE_TRACE_POINT(
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index e0d22c27c6..5c9495ecfe 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -388,7 +388,7 @@ rte_eth_find_next_sibling(uint16_t port_id, uint16_t ref_port_id)
 static bool
 eth_dev_is_allocated(const struct rte_eth_dev *ethdev)
 {
-	return ethdev->data->name[0] != '\0';
+	return ethdev->data != NULL && ethdev->data->name[0] != '\0';
 }
 
 int
@@ -433,6 +433,8 @@ rte_eth_find_next_owned_by(uint16_t port_id, const uint64_t owner_id)
 int
 rte_eth_dev_owner_new(uint64_t *owner_id)
 {
+	int ret;
+
 	if (owner_id == NULL) {
 		RTE_ETHDEV_LOG(ERR, "Cannot get new owner ID to NULL\n");
 		return -EINVAL;
@@ -440,14 +442,18 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	*owner_id = eth_dev_shared_data->next_owner_id++;
+	if (eth_dev_shared_data_prepare() != NULL) {
+		*owner_id = eth_dev_shared_data->next_owner_id++;
+		ret = 0;
+	} else {
+		ret = -ENOMEM;
+	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	rte_ethdev_trace_owner_new(*owner_id);
+	rte_ethdev_trace_owner_new(*owner_id, ret);
 
-	return 0;
+	return ret;
 }
 
 static int
@@ -506,8 +512,10 @@ rte_eth_dev_owner_set(const uint16_t port_id,
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ret = eth_dev_owner_set(port_id, RTE_ETH_DEV_NO_OWNER, owner);
+	else
+		ret = -ENOMEM;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -525,8 +533,10 @@ rte_eth_dev_owner_unset(const uint16_t port_id, const uint64_t owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
+	if (eth_dev_shared_data_prepare() != NULL)
+		ret = eth_dev_owner_set(port_id, owner_id, &new_owner);
+	else
+		ret = -ENOMEM;
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
@@ -543,8 +553,9 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	if (eth_is_valid_owner_id(owner_id)) {
+	if (eth_dev_shared_data_prepare() == NULL) {
+		ret = -ENOMEM;
+	} else if (eth_is_valid_owner_id(owner_id)) {
 		for (port_id = 0; port_id < RTE_MAX_ETHPORTS; port_id++) {
 			struct rte_eth_dev_data *data =
 				rte_eth_devices[port_id].data;
@@ -573,6 +584,7 @@ int
 rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 {
 	struct rte_eth_dev *ethdev;
+	int ret;
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
 	ethdev = &rte_eth_devices[port_id];
@@ -591,14 +603,18 @@ rte_eth_dev_owner_get(const uint16_t port_id, struct rte_eth_dev_owner *owner)
 
 	rte_spinlock_lock(rte_mcfg_ethdev_get_lock());
 
-	eth_dev_shared_data_prepare();
-	rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
+	if (eth_dev_shared_data_prepare() != NULL) {
+		rte_memcpy(owner, &ethdev->data->owner, sizeof(*owner));
+		ret = 0;
+	} else {
+		ret = -ENOMEM;
+	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
 
-	rte_ethdev_trace_owner_get(port_id, owner);
+	rte_ethdev_trace_owner_get(port_id, owner, ret);
 
-	return 0;
+	return ret;
 }
 
 int
-- 
2.41.0

[PATCH v4 3/3] ethdev: cleanup shared data with the last port

[David Marchand]

If no port is allocated and no port owner is still registered,
ethdev from a primary process may release the memzone used to store
port data.
This makes it possible for the DPDK memory allocator to release
associated resources back to the OS.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
Changes since v2:
- tracked owners count and and avoided releasing shared mem if some
  owner is still registered,

---
 lib/ethdev/ethdev_driver.c  |  6 ++++++
 lib/ethdev/ethdev_private.c | 21 ++++++++++++++++++++-
 lib/ethdev/ethdev_private.h |  4 ++++
 lib/ethdev/rte_ethdev.c     |  3 +++
 4 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/lib/ethdev/ethdev_driver.c b/lib/ethdev/ethdev_driver.c
index b339e325a0..fff4b7b4cd 100644
--- a/lib/ethdev/ethdev_driver.c
+++ b/lib/ethdev/ethdev_driver.c
@@ -115,6 +115,8 @@ rte_eth_dev_allocate(const char *name)
 	eth_dev->data->backer_port_id = RTE_MAX_ETHPORTS;
 	eth_dev->data->mtu = RTE_ETHER_MTU;
 	pthread_mutex_init(&eth_dev->data->flow_ops_mutex, NULL);
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+	eth_dev_shared_data->allocated_ports++;
 
 unlock:
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
@@ -265,6 +267,10 @@ rte_eth_dev_release_port(struct rte_eth_dev *eth_dev)
 		rte_free(eth_dev->data->dev_private);
 		pthread_mutex_destroy(&eth_dev->data->flow_ops_mutex);
 		memset(eth_dev->data, 0, sizeof(struct rte_eth_dev_data));
+		eth_dev->data = NULL;
+
+		eth_dev_shared_data->allocated_ports--;
+		eth_dev_shared_data_release();
 	}
 
 	rte_spinlock_unlock(rte_mcfg_ethdev_get_lock());
diff --git a/lib/ethdev/ethdev_private.c b/lib/ethdev/ethdev_private.c
index 911de1e595..aea9112cc2 100644
--- a/lib/ethdev/ethdev_private.c
+++ b/lib/ethdev/ethdev_private.c
@@ -11,6 +11,7 @@
 
 static const char *MZ_RTE_ETH_DEV_DATA = "rte_eth_dev_data";
 
+static const struct rte_memzone *eth_dev_shared_mz;
 struct eth_dev_shared *eth_dev_shared_data;
 
 /* spinlock for eth device callbacks */
@@ -324,7 +325,7 @@ eth_dev_shared_data_prepare(void)
 	const unsigned int flags = 0;
 	const struct rte_memzone *mz;
 
-	if (eth_dev_shared_data == NULL) {
+	if (eth_dev_shared_mz == NULL) {
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 			/* Allocate port data and ownership shared memory. */
 			mz = rte_memzone_reserve(MZ_RTE_ETH_DEV_DATA,
@@ -337,10 +338,13 @@ eth_dev_shared_data_prepare(void)
 			goto out;
 		}
 
+		eth_dev_shared_mz = mz;
 		eth_dev_shared_data = mz->addr;
 		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+			eth_dev_shared_data->allocated_owners = 0;
 			eth_dev_shared_data->next_owner_id =
 					RTE_ETH_DEV_NO_OWNER + 1;
+			eth_dev_shared_data->allocated_ports = 0;
 			memset(eth_dev_shared_data->data, 0,
 			       sizeof(eth_dev_shared_data->data));
 		}
@@ -349,6 +353,21 @@ eth_dev_shared_data_prepare(void)
 	return eth_dev_shared_data;
 }
 
+void
+eth_dev_shared_data_release(void)
+{
+	RTE_ASSERT(rte_eal_process_type() == RTE_PROC_PRIMARY);
+
+	if (eth_dev_shared_data->allocated_ports != 0)
+		return;
+	if (eth_dev_shared_data->allocated_owners != 0)
+		return;
+
+	rte_memzone_free(eth_dev_shared_mz);
+	eth_dev_shared_mz = NULL;
+	eth_dev_shared_data = NULL;
+}
+
 void
 eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid)
 {
diff --git a/lib/ethdev/ethdev_private.h b/lib/ethdev/ethdev_private.h
index 1572da7b48..0d36b9c30f 100644
--- a/lib/ethdev/ethdev_private.h
+++ b/lib/ethdev/ethdev_private.h
@@ -14,7 +14,9 @@
 #include "rte_ethdev.h"
 
 struct eth_dev_shared {
+	uint64_t allocated_owners;
 	uint64_t next_owner_id;
+	uint64_t allocated_ports;
 	struct rte_eth_dev_data data[RTE_MAX_ETHPORTS];
 };
 
@@ -69,6 +71,8 @@ void eth_dev_fp_ops_setup(struct rte_eth_fp_ops *fpo,
 
 void *eth_dev_shared_data_prepare(void)
 	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
+void eth_dev_shared_data_release(void)
+	__rte_exclusive_locks_required(rte_mcfg_ethdev_get_lock());
 
 void eth_dev_rxq_release(struct rte_eth_dev *dev, uint16_t qid);
 void eth_dev_txq_release(struct rte_eth_dev *dev, uint16_t qid);
diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 5c9495ecfe..61572d0cd1 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -444,6 +444,7 @@ rte_eth_dev_owner_new(uint64_t *owner_id)
 
 	if (eth_dev_shared_data_prepare() != NULL) {
 		*owner_id = eth_dev_shared_data->next_owner_id++;
+		eth_dev_shared_data->allocated_owners++;
 		ret = 0;
 	} else {
 		ret = -ENOMEM;
@@ -566,6 +567,8 @@ rte_eth_dev_owner_delete(const uint64_t owner_id)
 		RTE_ETHDEV_LOG(NOTICE,
 			"All port owners owned by %016"PRIx64" identifier have removed\n",
 			owner_id);
+		eth_dev_shared_data->allocated_owners--;
+		eth_dev_shared_data_release();
 	} else {
 		RTE_ETHDEV_LOG(ERR,
 			       "Invalid owner ID=%016"PRIx64"\n",
-- 
2.41.0

Re: [PATCH v4 0/3] Release ethdev shared memory on port cleanup

[Thomas Monjalon]

27/09/2023 13:45, David Marchand:
> This series was triggered after investigating why the
> eal_flags_file_prefix_autotest unit test was failing in the case of
> statically built binaries [1]).
> 
> For now, I went with a simple (naive) approach and put all accesses to the
> shared data under a single lock: ethdev maintainers, it is your turn to
> shine and give me reasons why we should keep the locks the way they
> were ;-).
> And let's see what the CI reports...

Applied, thanks.