[Patch v2] net/mana: use mana_local_data for tracking usage data for primary process

[Patch v2] net/mana: use mana_local_data for tracking usage data for primary process

[longli]

From: Long Li <longli@microsoft.com>

The driver uses mana_shared_data for tracking usage count for primary
process. This is not correct as the mana_shared_data is allocated
by the primary and is meant to track usage of secondary process by the
primary process. And it creates a race condition when the device is
removed because the counter is no longer available if this shared
memory is freed.

Move the usage count tracking to mana_local_data and fix the race
condition in mana_pci_remove().

Fixes: 517ed6e2d590 ("net/mana: add basic driver with build environment")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes:
v2: use atomic variable to track the secondary_cnt in shared memory, remove the spinlock for shared memory

 drivers/net/mana/mana.c | 99 +++++++++++++++++++++++------------------
 drivers/net/mana/mana.h |  6 +--
 drivers/net/mana/mp.c   |  2 +-
 3 files changed, 57 insertions(+), 50 deletions(-)

diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c
index c37c4e3444..69cb7ab13f 100644
--- a/drivers/net/mana/mana.c
+++ b/drivers/net/mana/mana.c
@@ -23,9 +23,14 @@
 #include "mana.h"
 
 /* Shared memory between primary/secondary processes, per driver */
-/* Data to track primary/secondary usage */
 struct mana_shared_data *mana_shared_data;
-static struct mana_shared_data mana_local_data;
+
+/* Local data to track device instance usage for primary/secondary processes */
+static struct mana_local_data {
+	int init_done;
+	unsigned int primary_cnt;
+	unsigned int secondary_cnt;
+} mana_local_data;
 
 /* The memory region for the above data */
 static const struct rte_memzone *mana_shared_mz;
@@ -1167,8 +1172,12 @@ mana_init_shared_data(void)
 	rte_spinlock_lock(&mana_shared_data_lock);
 
 	/* Skip if shared data is already initialized */
-	if (mana_shared_data)
+	if (mana_shared_data) {
+		DRV_LOG(INFO, "shared data is already initialized");
 		goto exit;
+	}
+
+	memset(&mana_local_data, 0, sizeof(mana_local_data));
 
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
 		mana_shared_mz = rte_memzone_reserve(MZ_MANA_SHARED_DATA,
@@ -1181,8 +1190,7 @@ mana_init_shared_data(void)
 		}
 
 		mana_shared_data = mana_shared_mz->addr;
-		memset(mana_shared_data, 0, sizeof(*mana_shared_data));
-		rte_spinlock_init(&mana_shared_data->lock);
+		rte_atomic32_set(&mana_shared_data->secondary_cnt, 0);
 	} else {
 		secondary_mz = rte_memzone_lookup(MZ_MANA_SHARED_DATA);
 		if (!secondary_mz) {
@@ -1192,7 +1200,6 @@ mana_init_shared_data(void)
 		}
 
 		mana_shared_data = secondary_mz->addr;
-		memset(&mana_local_data, 0, sizeof(mana_local_data));
 	}
 
 exit:
@@ -1213,11 +1220,11 @@ mana_init_once(void)
 	if (ret)
 		return ret;
 
-	rte_spinlock_lock(&mana_shared_data->lock);
+	rte_spinlock_lock(&mana_shared_data_lock);
 
 	switch (rte_eal_process_type()) {
 	case RTE_PROC_PRIMARY:
-		if (mana_shared_data->init_done)
+		if (mana_local_data.init_done)
 			break;
 
 		ret = mana_mp_init_primary();
@@ -1225,7 +1232,7 @@ mana_init_once(void)
 			break;
 		DRV_LOG(ERR, "MP INIT PRIMARY");
 
-		mana_shared_data->init_done = 1;
+		mana_local_data.init_done = 1;
 		break;
 
 	case RTE_PROC_SECONDARY:
@@ -1248,7 +1255,7 @@ mana_init_once(void)
 		break;
 	}
 
-	rte_spinlock_unlock(&mana_shared_data->lock);
+	rte_spinlock_unlock(&mana_shared_data_lock);
 
 	return ret;
 }
@@ -1319,11 +1326,6 @@ mana_probe_port(struct ibv_device *ibdev, struct ibv_device_attr_ex *dev_attr,
 		eth_dev->tx_pkt_burst = mana_tx_burst;
 		eth_dev->rx_pkt_burst = mana_rx_burst;
 
-		rte_spinlock_lock(&mana_shared_data->lock);
-		mana_shared_data->secondary_cnt++;
-		mana_local_data.secondary_cnt++;
-		rte_spinlock_unlock(&mana_shared_data->lock);
-
 		rte_eth_copy_pci_info(eth_dev, pci_dev);
 		rte_eth_dev_probing_finish(eth_dev);
 
@@ -1406,10 +1408,6 @@ mana_probe_port(struct ibv_device *ibdev, struct ibv_device_attr_ex *dev_attr,
 		goto failed;
 	}
 
-	rte_spinlock_lock(&mana_shared_data->lock);
-	mana_shared_data->primary_cnt++;
-	rte_spinlock_unlock(&mana_shared_data->lock);
-
 	eth_dev->device = &pci_dev->device;
 
 	DRV_LOG(INFO, "device %s at port %u", name, eth_dev->data->port_id);
@@ -1552,13 +1550,37 @@ mana_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
 		count = mana_pci_probe_mac(pci_dev, NULL);
 	}
 
+	/* If no device is found, clean up resources if this is the last one */
 	if (!count) {
-		rte_memzone_free(mana_shared_mz);
-		mana_shared_mz = NULL;
-		ret = -ENODEV;
+		rte_spinlock_lock(&mana_shared_data_lock);
+		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+			if (!mana_local_data.primary_cnt) {
+				mana_mp_uninit_primary();
+				rte_memzone_free(mana_shared_mz);
+				mana_shared_mz = NULL;
+				mana_shared_data = NULL;
+			}
+		} else {
+			if (!mana_local_data.secondary_cnt) {
+				mana_mp_uninit_secondary();
+				mana_shared_data = NULL;
+			}
+		}
+		rte_spinlock_unlock(&mana_shared_data_lock);
+		return -ENODEV;
 	}
 
-	return ret;
+	/* At least one eth_dev is probed, increase counter for shared data */
+	rte_spinlock_lock(&mana_shared_data_lock);
+	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
+		mana_local_data.primary_cnt++;
+	} else {
+		rte_atomic32_inc(&mana_shared_data->secondary_cnt);
+		mana_local_data.secondary_cnt++;
+	}
+	rte_spinlock_unlock(&mana_shared_data_lock);
+
+	return 0;
 }
 
 static int
@@ -1573,45 +1595,34 @@ mana_dev_uninit(struct rte_eth_dev *dev)
 static int
 mana_pci_remove(struct rte_pci_device *pci_dev)
 {
+	rte_spinlock_lock(&mana_shared_data_lock);
 	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
-		rte_spinlock_lock(&mana_shared_data_lock);
-
-		rte_spinlock_lock(&mana_shared_data->lock);
+		RTE_VERIFY(mana_local_data.primary_cnt > 0);
+		mana_local_data.primary_cnt--;
 
-		RTE_VERIFY(mana_shared_data->primary_cnt > 0);
-		mana_shared_data->primary_cnt--;
-		if (!mana_shared_data->primary_cnt) {
+		if (!mana_local_data.primary_cnt) {
 			DRV_LOG(DEBUG, "mp uninit primary");
 			mana_mp_uninit_primary();
-		}
-
-		rte_spinlock_unlock(&mana_shared_data->lock);
 
-		/* Also free the shared memory if this is the last */
-		if (!mana_shared_data->primary_cnt) {
+			/* Also free the shared memory if this is the last */
 			DRV_LOG(DEBUG, "free shared memezone data");
 			rte_memzone_free(mana_shared_mz);
 			mana_shared_mz = NULL;
+			mana_shared_data = NULL;
 		}
-
-		rte_spinlock_unlock(&mana_shared_data_lock);
 	} else {
-		rte_spinlock_lock(&mana_shared_data_lock);
-
-		rte_spinlock_lock(&mana_shared_data->lock);
-		RTE_VERIFY(mana_shared_data->secondary_cnt > 0);
-		mana_shared_data->secondary_cnt--;
-		rte_spinlock_unlock(&mana_shared_data->lock);
+		rte_atomic32_dec(&mana_shared_data->secondary_cnt);
+		RTE_VERIFY(rte_atomic32_read(&mana_shared_data->secondary_cnt) >= 0);
 
 		RTE_VERIFY(mana_local_data.secondary_cnt > 0);
 		mana_local_data.secondary_cnt--;
 		if (!mana_local_data.secondary_cnt) {
 			DRV_LOG(DEBUG, "mp uninit secondary");
 			mana_mp_uninit_secondary();
+			mana_shared_data = NULL;
 		}
-
-		rte_spinlock_unlock(&mana_shared_data_lock);
 	}
+	rte_spinlock_unlock(&mana_shared_data_lock);
 
 	return rte_eth_dev_pci_generic_remove(pci_dev, mana_dev_uninit);
 }
diff --git a/drivers/net/mana/mana.h b/drivers/net/mana/mana.h
index 41a0ca6dfe..b60b1f1977 100644
--- a/drivers/net/mana/mana.h
+++ b/drivers/net/mana/mana.h
@@ -8,12 +8,8 @@
 #define	PCI_VENDOR_ID_MICROSOFT		0x1414
 #define PCI_DEVICE_ID_MICROSOFT_MANA	0x00ba
 
-/* Shared data between primary/secondary processes */
 struct mana_shared_data {
-	rte_spinlock_t lock;
-	int init_done;
-	unsigned int primary_cnt;
-	unsigned int secondary_cnt;
+	rte_atomic32_t secondary_cnt;
 };
 
 #define MANA_MAX_MTU	9000
diff --git a/drivers/net/mana/mp.c b/drivers/net/mana/mp.c
index 34b45ed832..e7eecfcc30 100644
--- a/drivers/net/mana/mp.c
+++ b/drivers/net/mana/mp.c
@@ -306,7 +306,7 @@ mana_mp_req_on_rxtx(struct rte_eth_dev *dev, enum mana_mp_req_type type)
 		return;
 	}
 
-	if (!mana_shared_data->secondary_cnt)
+	if (rte_atomic32_read(&mana_shared_data->secondary_cnt) <= 0)
 		return;
 
 	mp_init_msg(&mp_req, type, dev->data->port_id);
-- 
2.34.1

RE: [EXTERNAL] [Patch v2] net/mana: use mana_local_data for tracking usage data for primary process

[Wei Hu]

> -----Original Message-----
> From: longli@linuxonhyperv.com <longli@linuxonhyperv.com>
> Sent: Wednesday, February 19, 2025 4:59 AM
> To: Stephen Hemminger <stephen@networkplumber.org>; Wei Hu
> <weh@microsoft.com>
> Cc: dev@dpdk.org; stable@dpdk.org; Long Li <longli@microsoft.com>
> Subject: [EXTERNAL] [Patch v2] net/mana: use mana_local_data for tracking
> usage data for primary process
> 
> From: Long Li <longli@microsoft.com>
> 
> The driver uses mana_shared_data for tracking usage count for primary
> process. This is not correct as the mana_shared_data is allocated by the
> primary and is meant to track usage of secondary process by the primary
> process. And it creates a race condition when the device is removed because
> the counter is no longer available if this shared memory is freed.
> 
> Move the usage count tracking to mana_local_data and fix the race condition
> in mana_pci_remove().
> 
> Fixes: 517ed6e2d590 ("net/mana: add basic driver with build environment")
> Signed-off-by: Long Li <longli@microsoft.com>

Reviewed-by: Wei Hu <weh@linux.microsoft.com>

> ---
> Changes:
> v2: use atomic variable to track the secondary_cnt in shared memory, remove
> the spinlock for shared memory
> 
>  drivers/net/mana/mana.c | 99 +++++++++++++++++++++++------------------
>  drivers/net/mana/mana.h |  6 +--
>  drivers/net/mana/mp.c   |  2 +-
>  3 files changed, 57 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/net/mana/mana.c b/drivers/net/mana/mana.c index
> c37c4e3444..69cb7ab13f 100644
> --- a/drivers/net/mana/mana.c
> +++ b/drivers/net/mana/mana.c
> @@ -23,9 +23,14 @@
>  #include "mana.h"
> 
>  /* Shared memory between primary/secondary processes, per driver */
> -/* Data to track primary/secondary usage */  struct mana_shared_data
> *mana_shared_data; -static struct mana_shared_data mana_local_data;
> +
> +/* Local data to track device instance usage for primary/secondary
> +processes */ static struct mana_local_data {
> +	int init_done;
> +	unsigned int primary_cnt;
> +	unsigned int secondary_cnt;
> +} mana_local_data;
> 
>  /* The memory region for the above data */  static const struct rte_memzone
> *mana_shared_mz; @@ -1167,8 +1172,12 @@
> mana_init_shared_data(void)
>  	rte_spinlock_lock(&mana_shared_data_lock);
> 
>  	/* Skip if shared data is already initialized */
> -	if (mana_shared_data)
> +	if (mana_shared_data) {
> +		DRV_LOG(INFO, "shared data is already initialized");
>  		goto exit;
> +	}
> +
> +	memset(&mana_local_data, 0, sizeof(mana_local_data));
> 
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
>  		mana_shared_mz =
> rte_memzone_reserve(MZ_MANA_SHARED_DATA,
> @@ -1181,8 +1190,7 @@ mana_init_shared_data(void)
>  		}
> 
>  		mana_shared_data = mana_shared_mz->addr;
> -		memset(mana_shared_data, 0, sizeof(*mana_shared_data));
> -		rte_spinlock_init(&mana_shared_data->lock);
> +		rte_atomic32_set(&mana_shared_data->secondary_cnt, 0);
>  	} else {
>  		secondary_mz =
> rte_memzone_lookup(MZ_MANA_SHARED_DATA);
>  		if (!secondary_mz) {
> @@ -1192,7 +1200,6 @@ mana_init_shared_data(void)
>  		}
> 
>  		mana_shared_data = secondary_mz->addr;
> -		memset(&mana_local_data, 0, sizeof(mana_local_data));
>  	}
> 
>  exit:
> @@ -1213,11 +1220,11 @@ mana_init_once(void)
>  	if (ret)
>  		return ret;
> 
> -	rte_spinlock_lock(&mana_shared_data->lock);
> +	rte_spinlock_lock(&mana_shared_data_lock);
> 
>  	switch (rte_eal_process_type()) {
>  	case RTE_PROC_PRIMARY:
> -		if (mana_shared_data->init_done)
> +		if (mana_local_data.init_done)
>  			break;
> 
>  		ret = mana_mp_init_primary();
> @@ -1225,7 +1232,7 @@ mana_init_once(void)
>  			break;
>  		DRV_LOG(ERR, "MP INIT PRIMARY");
> 
> -		mana_shared_data->init_done = 1;
> +		mana_local_data.init_done = 1;
>  		break;
> 
>  	case RTE_PROC_SECONDARY:
> @@ -1248,7 +1255,7 @@ mana_init_once(void)
>  		break;
>  	}
> 
> -	rte_spinlock_unlock(&mana_shared_data->lock);
> +	rte_spinlock_unlock(&mana_shared_data_lock);
> 
>  	return ret;
>  }
> @@ -1319,11 +1326,6 @@ mana_probe_port(struct ibv_device *ibdev,
> struct ibv_device_attr_ex *dev_attr,
>  		eth_dev->tx_pkt_burst = mana_tx_burst;
>  		eth_dev->rx_pkt_burst = mana_rx_burst;
> 
> -		rte_spinlock_lock(&mana_shared_data->lock);
> -		mana_shared_data->secondary_cnt++;
> -		mana_local_data.secondary_cnt++;
> -		rte_spinlock_unlock(&mana_shared_data->lock);
> -
>  		rte_eth_copy_pci_info(eth_dev, pci_dev);
>  		rte_eth_dev_probing_finish(eth_dev);
> 
> @@ -1406,10 +1408,6 @@ mana_probe_port(struct ibv_device *ibdev,
> struct ibv_device_attr_ex *dev_attr,
>  		goto failed;
>  	}
> 
> -	rte_spinlock_lock(&mana_shared_data->lock);
> -	mana_shared_data->primary_cnt++;
> -	rte_spinlock_unlock(&mana_shared_data->lock);
> -
>  	eth_dev->device = &pci_dev->device;
> 
>  	DRV_LOG(INFO, "device %s at port %u", name, eth_dev->data-
> >port_id); @@ -1552,13 +1550,37 @@ mana_pci_probe(struct rte_pci_driver
> *pci_drv __rte_unused,
>  		count = mana_pci_probe_mac(pci_dev, NULL);
>  	}
> 
> +	/* If no device is found, clean up resources if this is the last one
> +*/
>  	if (!count) {
> -		rte_memzone_free(mana_shared_mz);
> -		mana_shared_mz = NULL;
> -		ret = -ENODEV;
> +		rte_spinlock_lock(&mana_shared_data_lock);
> +		if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +			if (!mana_local_data.primary_cnt) {
> +				mana_mp_uninit_primary();
> +				rte_memzone_free(mana_shared_mz);
> +				mana_shared_mz = NULL;
> +				mana_shared_data = NULL;
> +			}
> +		} else {
> +			if (!mana_local_data.secondary_cnt) {
> +				mana_mp_uninit_secondary();
> +				mana_shared_data = NULL;
> +			}
> +		}
> +		rte_spinlock_unlock(&mana_shared_data_lock);
> +		return -ENODEV;
>  	}
> 
> -	return ret;
> +	/* At least one eth_dev is probed, increase counter for shared data */
> +	rte_spinlock_lock(&mana_shared_data_lock);
> +	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> +		mana_local_data.primary_cnt++;
> +	} else {
> +		rte_atomic32_inc(&mana_shared_data->secondary_cnt);
> +		mana_local_data.secondary_cnt++;
> +	}
> +	rte_spinlock_unlock(&mana_shared_data_lock);
> +
> +	return 0;
>  }
> 
>  static int
> @@ -1573,45 +1595,34 @@ mana_dev_uninit(struct rte_eth_dev *dev)
> static int  mana_pci_remove(struct rte_pci_device *pci_dev)  {
> +	rte_spinlock_lock(&mana_shared_data_lock);
>  	if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
> -		rte_spinlock_lock(&mana_shared_data_lock);
> -
> -		rte_spinlock_lock(&mana_shared_data->lock);
> +		RTE_VERIFY(mana_local_data.primary_cnt > 0);
> +		mana_local_data.primary_cnt--;
> 
> -		RTE_VERIFY(mana_shared_data->primary_cnt > 0);
> -		mana_shared_data->primary_cnt--;
> -		if (!mana_shared_data->primary_cnt) {
> +		if (!mana_local_data.primary_cnt) {
>  			DRV_LOG(DEBUG, "mp uninit primary");
>  			mana_mp_uninit_primary();
> -		}
> -
> -		rte_spinlock_unlock(&mana_shared_data->lock);
> 
> -		/* Also free the shared memory if this is the last */
> -		if (!mana_shared_data->primary_cnt) {
> +			/* Also free the shared memory if this is the last */
>  			DRV_LOG(DEBUG, "free shared memezone data");
>  			rte_memzone_free(mana_shared_mz);
>  			mana_shared_mz = NULL;
> +			mana_shared_data = NULL;
>  		}
> -
> -		rte_spinlock_unlock(&mana_shared_data_lock);
>  	} else {
> -		rte_spinlock_lock(&mana_shared_data_lock);
> -
> -		rte_spinlock_lock(&mana_shared_data->lock);
> -		RTE_VERIFY(mana_shared_data->secondary_cnt > 0);
> -		mana_shared_data->secondary_cnt--;
> -		rte_spinlock_unlock(&mana_shared_data->lock);
> +		rte_atomic32_dec(&mana_shared_data->secondary_cnt);
> +		RTE_VERIFY(rte_atomic32_read(&mana_shared_data-
> >secondary_cnt) >= 0);
> 
>  		RTE_VERIFY(mana_local_data.secondary_cnt > 0);
>  		mana_local_data.secondary_cnt--;
>  		if (!mana_local_data.secondary_cnt) {
>  			DRV_LOG(DEBUG, "mp uninit secondary");
>  			mana_mp_uninit_secondary();
> +			mana_shared_data = NULL;
>  		}
> -
> -		rte_spinlock_unlock(&mana_shared_data_lock);
>  	}
> +	rte_spinlock_unlock(&mana_shared_data_lock);
> 
>  	return rte_eth_dev_pci_generic_remove(pci_dev,
> mana_dev_uninit);  } diff --git a/drivers/net/mana/mana.h
> b/drivers/net/mana/mana.h index 41a0ca6dfe..b60b1f1977 100644
> --- a/drivers/net/mana/mana.h
> +++ b/drivers/net/mana/mana.h
> @@ -8,12 +8,8 @@
>  #define	PCI_VENDOR_ID_MICROSOFT		0x1414
>  #define PCI_DEVICE_ID_MICROSOFT_MANA	0x00ba
> 
> -/* Shared data between primary/secondary processes */  struct
> mana_shared_data {
> -	rte_spinlock_t lock;
> -	int init_done;
> -	unsigned int primary_cnt;
> -	unsigned int secondary_cnt;
> +	rte_atomic32_t secondary_cnt;
>  };
> 
>  #define MANA_MAX_MTU	9000
> diff --git a/drivers/net/mana/mp.c b/drivers/net/mana/mp.c index
> 34b45ed832..e7eecfcc30 100644
> --- a/drivers/net/mana/mp.c
> +++ b/drivers/net/mana/mp.c
> @@ -306,7 +306,7 @@ mana_mp_req_on_rxtx(struct rte_eth_dev *dev,
> enum mana_mp_req_type type)
>  		return;
>  	}
> 
> -	if (!mana_shared_data->secondary_cnt)
> +	if (rte_atomic32_read(&mana_shared_data->secondary_cnt) <= 0)
>  		return;
> 
>  	mp_init_msg(&mp_req, type, dev->data->port_id);
> --
> 2.34.1

Re: [EXTERNAL] [Patch v2] net/mana: use mana_local_data for tracking usage data for primary process

[Stephen Hemminger]

On Wed, 19 Feb 2025 12:47:11 +0000
Wei Hu <weh@microsoft.com> wrote:

> > 
> > -/* Shared data between primary/secondary processes */  struct
> > mana_shared_data {
> > -	rte_spinlock_t lock;
> > -	int init_done;
> > -	unsigned int primary_cnt;
> > -	unsigned int secondary_cnt;
> > +	rte_atomic32_t secondary_cnt;
> >  };

The DPDK has converted to using the C11 atomic's so this should use
that (i.e RTE_ATOMIC() etc) instead of the older atomic primitives.
	

RE: [EXTERNAL] [Patch v2] net/mana: use mana_local_data for tracking usage data for primary process

[Long Li]

> The DPDK has converted to using the C11 atomic's so this should use that (i.e
> RTE_ATOMIC() etc) instead of the older atomic primitives.
> 

I have sent v3.