diff mbox series

[v2,09/14] vhost: use linked-list for vDPA devices

Message ID 20200624122701.1369327-10-maxime.coquelin@redhat.com (mailing list archive)
State Superseded, archived
Delegated to: Maxime Coquelin
Headers show
Series vDPA API and framework rework | expand

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Maxime Coquelin June 24, 2020, 12:26 p.m. UTC
There is no more notion of device ID outside of vdpa.c.
We can now move from array to linked-list model for keeping
track of the vDPA devices.

There is no point in using array here, as all vDPA API are
used from the control path, so no performance concerns.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/rte_vdpa.h |   1 +
 lib/librte_vhost/vdpa.c     | 134 +++++++++++++++++++-----------------
 2 files changed, 70 insertions(+), 65 deletions(-)

Comments

Adrian Moreno June 26, 2020, 11:03 a.m. UTC | #1
On 6/24/20 2:26 PM, Maxime Coquelin wrote:
> There is no more notion of device ID outside of vdpa.c.
> We can now move from array to linked-list model for keeping
> track of the vDPA devices.
> 
> There is no point in using array here, as all vDPA API are
> used from the control path, so no performance concerns.
> 
> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/librte_vhost/rte_vdpa.h |   1 +
>  lib/librte_vhost/vdpa.c     | 134 +++++++++++++++++++-----------------
>  2 files changed, 70 insertions(+), 65 deletions(-)
> 
> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
> index da8ee33ab1..dbdc273702 100644
> --- a/lib/librte_vhost/rte_vdpa.h
> +++ b/lib/librte_vhost/rte_vdpa.h
> @@ -105,6 +105,7 @@ struct rte_vdpa_dev_ops {
>   * vdpa device structure includes device address and device operations.
>   */
>  struct rte_vdpa_device {
> +	TAILQ_ENTRY(rte_vdpa_device) next;
>  	/** Generic device information */
>  	struct rte_device *device;
>  	/** vdpa device operations */
> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
> index d7965b8854..dc2a138009 100644
> --- a/lib/librte_vhost/vdpa.c
> +++ b/lib/librte_vhost/vdpa.c
> @@ -9,35 +9,54 @@
>   */
>  
>  #include <stdbool.h>
> +#include <sys/queue.h>
>  
>  #include <rte_class.h>
>  #include <rte_malloc.h>
> +#include <rte_spinlock.h>
> +#include <rte_tailq.h>
> +

I'm curious why we need both sys/queue.h and rte_tailq.h
>  #include "rte_vdpa.h"
>  #include "vhost.h"
>  
> -static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
> +/** Double linked list of vDPA devices. */
> +TAILQ_HEAD(vdpa_device_list, rte_vdpa_device);
> +
> +static struct vdpa_device_list vdpa_device_list =
> +		TAILQ_HEAD_INITIALIZER(vdpa_device_list);
> +static rte_spinlock_t vdpa_device_list_lock = RTE_SPINLOCK_INITIALIZER;
>  static uint32_t vdpa_device_num;
>  
>  
> -struct rte_vdpa_device *
> -rte_vdpa_find_device_by_name(const char *name)
> +/* Unsafe, needs to be called with vdpa_device_list_lock held */
> +static struct rte_vdpa_device *
> +__vdpa_find_device_by_name(const char *name)
>  {
> -	struct rte_vdpa_device *dev;
> -	int i;
> +	struct rte_vdpa_device *dev, *ret = NULL;
>  
>  	if (name == NULL)
>  		return NULL;
>  
> -	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
> -		dev = &vdpa_devices[i];
> -		if (dev->ops == NULL)
> -			continue;
> -
> -		if (strcmp(dev->device->name, name) == 0)
> -			return dev;
> +	TAILQ_FOREACH(dev, &vdpa_device_list, next) {
> +		if (strcmp(dev->device->name, name) == 0) {
> +			ret = dev;
> +			break;
> +		}
>  	}
>  
> -	return NULL;
> +	return ret;
> +}
> +
> +struct rte_vdpa_device *
> +rte_vdpa_find_device_by_name(const char *name)
> +{
> +	struct rte_vdpa_device *dev;
> +
> +	rte_spinlock_lock(&vdpa_device_list_lock);
> +	dev = __vdpa_find_device_by_name(name);
> +	rte_spinlock_unlock(&vdpa_device_list_lock);
> +
> +	return dev;
>  }
>  
>  struct rte_device *
> @@ -54,52 +73,52 @@ rte_vdpa_register_device(struct rte_device *rte_dev,
>  		struct rte_vdpa_dev_ops *ops)
>  {
>  	struct rte_vdpa_device *dev;
> -	int i;
>  
> -	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
> +	if (ops == NULL)
>  		return NULL;
>  
> -	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> -		dev = &vdpa_devices[i];
> -		if (dev->ops == NULL)
> -			continue;
> -
> -		if (dev->device == rte_dev)
> -			return NULL;
> -	}
> -
> -	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> -		if (vdpa_devices[i].ops == NULL)
> -			break;
> +	rte_spinlock_lock(&vdpa_device_list_lock);
> +	/* Check the device hasn't been register already */
> +	dev = __vdpa_find_device_by_name(rte_dev->name);
> +	if (dev) {
> +		dev = NULL;
> +		goto out_unlock;
>  	}
>  
> -	if (i == MAX_VHOST_DEVICE)
> -		return NULL;
> +	dev = rte_zmalloc(NULL, sizeof(*dev), 0);
> +	if (!dev)
> +		goto out_unlock;
>  
> -	dev = &vdpa_devices[i];
>  	dev->device = rte_dev;
>  	dev->ops = ops;
> +	TAILQ_INSERT_TAIL(&vdpa_device_list, dev, next);
>  	vdpa_device_num++;
> +out_unlock:
> +	rte_spinlock_unlock(&vdpa_device_list_lock);
>  
>  	return dev;
>  }
>  
>  int
> -rte_vdpa_unregister_device(struct rte_vdpa_device *vdev)
> +rte_vdpa_unregister_device(struct rte_vdpa_device *dev)
>  {
> -	int i;
> +	struct rte_vdpa_device *cur_dev, *tmp_dev;
> +	int ret = -1;
>  
> -	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
> -		if (vdev != &vdpa_devices[i])
> +	rte_spinlock_lock(&vdpa_device_list_lock);
> +	TAILQ_FOREACH_SAFE(cur_dev, &vdpa_device_list, next, tmp_dev) {
> +		if (dev != cur_dev)
>  			continue;
>  
> -		memset(vdev, 0, sizeof(struct rte_vdpa_device));
> +		TAILQ_REMOVE(&vdpa_device_list, dev, next);
> +		rte_free(dev);
>  		vdpa_device_num--;
> -
> -		return 0;
> +		ret = 0;
> +		break;
>  	}
> +	rte_spinlock_lock(&vdpa_device_list_lock);
>  
You mean rte_spinlock_unlock?

> -	return -1;
> +	return ret;
>  }
>  
>  int
> @@ -246,19 +265,6 @@ rte_vdpa_reset_stats(struct rte_vdpa_device *dev, uint16_t qid)
>  	return dev->ops->reset_stats(dev, qid);
>  }
>  
> -static uint16_t
> -vdpa_dev_to_id(const struct rte_vdpa_device *dev)
> -{
> -	if (dev == NULL)
> -		return MAX_VHOST_DEVICE;
> -
> -	if (dev < &vdpa_devices[0] ||
> -			dev >= &vdpa_devices[MAX_VHOST_DEVICE])
> -		return MAX_VHOST_DEVICE;
> -
> -	return (uint16_t)(dev - vdpa_devices);
> -}
> -
>  static int
>  vdpa_dev_match(struct rte_vdpa_device *dev,
>  	      const struct rte_device *rte_dev)
> @@ -278,24 +284,22 @@ vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
>  		struct rte_device *rte_dev)
>  {
>  	struct rte_vdpa_device *dev;
> -	uint16_t idx;
>  
> -	if (start != NULL)
> -		idx = vdpa_dev_to_id(start) + 1;
> +	rte_spinlock_lock(&vdpa_device_list_lock);
> +	if (start == NULL)
> +		dev = TAILQ_FIRST(&vdpa_device_list);
>  	else
> -		idx = 0;
> -	for (; idx < MAX_VHOST_DEVICE; idx++) {
> -		dev = &vdpa_devices[idx];
> -		/*
> -		 * ToDo: Certainly better to introduce a state field,
> -		 * but rely on ops being set for now.
> -		 */
> -		if (dev->ops == NULL)
> -			continue;
> +		dev = TAILQ_NEXT(start, next);
> +
> +	while (dev != NULL) {
>  		if (cmp(dev, rte_dev) == 0)
> -			return dev;
> +			break;
> +
> +		dev = TAILQ_NEXT(dev, next);
>  	}
> -	return NULL;
> +	rte_spinlock_unlock(&vdpa_device_list_lock);
> +
> +	return dev;
>  }
>  
>  static void *
>
Maxime Coquelin June 26, 2020, 11:20 a.m. UTC | #2
On 6/26/20 1:03 PM, Adrian Moreno wrote:
> 
> On 6/24/20 2:26 PM, Maxime Coquelin wrote:
>> There is no more notion of device ID outside of vdpa.c.
>> We can now move from array to linked-list model for keeping
>> track of the vDPA devices.
>>
>> There is no point in using array here, as all vDPA API are
>> used from the control path, so no performance concerns.
>>
>> Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>> ---
>>  lib/librte_vhost/rte_vdpa.h |   1 +
>>  lib/librte_vhost/vdpa.c     | 134 +++++++++++++++++++-----------------
>>  2 files changed, 70 insertions(+), 65 deletions(-)
>>
>> diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
>> index da8ee33ab1..dbdc273702 100644
>> --- a/lib/librte_vhost/rte_vdpa.h
>> +++ b/lib/librte_vhost/rte_vdpa.h
>> @@ -105,6 +105,7 @@ struct rte_vdpa_dev_ops {
>>   * vdpa device structure includes device address and device operations.
>>   */
>>  struct rte_vdpa_device {
>> +	TAILQ_ENTRY(rte_vdpa_device) next;
>>  	/** Generic device information */
>>  	struct rte_device *device;
>>  	/** vdpa device operations */
>> diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
>> index d7965b8854..dc2a138009 100644
>> --- a/lib/librte_vhost/vdpa.c
>> +++ b/lib/librte_vhost/vdpa.c
>> @@ -9,35 +9,54 @@
>>   */
>>  
>>  #include <stdbool.h>
>> +#include <sys/queue.h>
>>  
>>  #include <rte_class.h>
>>  #include <rte_malloc.h>
>> +#include <rte_spinlock.h>
>> +#include <rte_tailq.h>
>> +
> I'm curious why we need both sys/queue.h and rte_tailq.h

That's because I use TAILQ_FOREACH_SAFE, the TAILQ_FOREACH variant that
is safe against removal. This one is not part of sys/queue.h

>>  #include "rte_vdpa.h"
>>  #include "vhost.h"
>>  

Thanks,
Maxime
diff mbox series

Patch

diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
index da8ee33ab1..dbdc273702 100644
--- a/lib/librte_vhost/rte_vdpa.h
+++ b/lib/librte_vhost/rte_vdpa.h
@@ -105,6 +105,7 @@  struct rte_vdpa_dev_ops {
  * vdpa device structure includes device address and device operations.
  */
 struct rte_vdpa_device {
+	TAILQ_ENTRY(rte_vdpa_device) next;
 	/** Generic device information */
 	struct rte_device *device;
 	/** vdpa device operations */
diff --git a/lib/librte_vhost/vdpa.c b/lib/librte_vhost/vdpa.c
index d7965b8854..dc2a138009 100644
--- a/lib/librte_vhost/vdpa.c
+++ b/lib/librte_vhost/vdpa.c
@@ -9,35 +9,54 @@ 
  */
 
 #include <stdbool.h>
+#include <sys/queue.h>
 
 #include <rte_class.h>
 #include <rte_malloc.h>
+#include <rte_spinlock.h>
+#include <rte_tailq.h>
+
 #include "rte_vdpa.h"
 #include "vhost.h"
 
-static struct rte_vdpa_device vdpa_devices[MAX_VHOST_DEVICE];
+/** Double linked list of vDPA devices. */
+TAILQ_HEAD(vdpa_device_list, rte_vdpa_device);
+
+static struct vdpa_device_list vdpa_device_list =
+		TAILQ_HEAD_INITIALIZER(vdpa_device_list);
+static rte_spinlock_t vdpa_device_list_lock = RTE_SPINLOCK_INITIALIZER;
 static uint32_t vdpa_device_num;
 
 
-struct rte_vdpa_device *
-rte_vdpa_find_device_by_name(const char *name)
+/* Unsafe, needs to be called with vdpa_device_list_lock held */
+static struct rte_vdpa_device *
+__vdpa_find_device_by_name(const char *name)
 {
-	struct rte_vdpa_device *dev;
-	int i;
+	struct rte_vdpa_device *dev, *ret = NULL;
 
 	if (name == NULL)
 		return NULL;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; ++i) {
-		dev = &vdpa_devices[i];
-		if (dev->ops == NULL)
-			continue;
-
-		if (strcmp(dev->device->name, name) == 0)
-			return dev;
+	TAILQ_FOREACH(dev, &vdpa_device_list, next) {
+		if (strcmp(dev->device->name, name) == 0) {
+			ret = dev;
+			break;
+		}
 	}
 
-	return NULL;
+	return ret;
+}
+
+struct rte_vdpa_device *
+rte_vdpa_find_device_by_name(const char *name)
+{
+	struct rte_vdpa_device *dev;
+
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	dev = __vdpa_find_device_by_name(name);
+	rte_spinlock_unlock(&vdpa_device_list_lock);
+
+	return dev;
 }
 
 struct rte_device *
@@ -54,52 +73,52 @@  rte_vdpa_register_device(struct rte_device *rte_dev,
 		struct rte_vdpa_dev_ops *ops)
 {
 	struct rte_vdpa_device *dev;
-	int i;
 
-	if (vdpa_device_num >= MAX_VHOST_DEVICE || ops == NULL)
+	if (ops == NULL)
 		return NULL;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		dev = &vdpa_devices[i];
-		if (dev->ops == NULL)
-			continue;
-
-		if (dev->device == rte_dev)
-			return NULL;
-	}
-
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdpa_devices[i].ops == NULL)
-			break;
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	/* Check the device hasn't been register already */
+	dev = __vdpa_find_device_by_name(rte_dev->name);
+	if (dev) {
+		dev = NULL;
+		goto out_unlock;
 	}
 
-	if (i == MAX_VHOST_DEVICE)
-		return NULL;
+	dev = rte_zmalloc(NULL, sizeof(*dev), 0);
+	if (!dev)
+		goto out_unlock;
 
-	dev = &vdpa_devices[i];
 	dev->device = rte_dev;
 	dev->ops = ops;
+	TAILQ_INSERT_TAIL(&vdpa_device_list, dev, next);
 	vdpa_device_num++;
+out_unlock:
+	rte_spinlock_unlock(&vdpa_device_list_lock);
 
 	return dev;
 }
 
 int
-rte_vdpa_unregister_device(struct rte_vdpa_device *vdev)
+rte_vdpa_unregister_device(struct rte_vdpa_device *dev)
 {
-	int i;
+	struct rte_vdpa_device *cur_dev, *tmp_dev;
+	int ret = -1;
 
-	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
-		if (vdev != &vdpa_devices[i])
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	TAILQ_FOREACH_SAFE(cur_dev, &vdpa_device_list, next, tmp_dev) {
+		if (dev != cur_dev)
 			continue;
 
-		memset(vdev, 0, sizeof(struct rte_vdpa_device));
+		TAILQ_REMOVE(&vdpa_device_list, dev, next);
+		rte_free(dev);
 		vdpa_device_num--;
-
-		return 0;
+		ret = 0;
+		break;
 	}
+	rte_spinlock_lock(&vdpa_device_list_lock);
 
-	return -1;
+	return ret;
 }
 
 int
@@ -246,19 +265,6 @@  rte_vdpa_reset_stats(struct rte_vdpa_device *dev, uint16_t qid)
 	return dev->ops->reset_stats(dev, qid);
 }
 
-static uint16_t
-vdpa_dev_to_id(const struct rte_vdpa_device *dev)
-{
-	if (dev == NULL)
-		return MAX_VHOST_DEVICE;
-
-	if (dev < &vdpa_devices[0] ||
-			dev >= &vdpa_devices[MAX_VHOST_DEVICE])
-		return MAX_VHOST_DEVICE;
-
-	return (uint16_t)(dev - vdpa_devices);
-}
-
 static int
 vdpa_dev_match(struct rte_vdpa_device *dev,
 	      const struct rte_device *rte_dev)
@@ -278,24 +284,22 @@  vdpa_find_device(const struct rte_vdpa_device *start, rte_vdpa_cmp_t cmp,
 		struct rte_device *rte_dev)
 {
 	struct rte_vdpa_device *dev;
-	uint16_t idx;
 
-	if (start != NULL)
-		idx = vdpa_dev_to_id(start) + 1;
+	rte_spinlock_lock(&vdpa_device_list_lock);
+	if (start == NULL)
+		dev = TAILQ_FIRST(&vdpa_device_list);
 	else
-		idx = 0;
-	for (; idx < MAX_VHOST_DEVICE; idx++) {
-		dev = &vdpa_devices[idx];
-		/*
-		 * ToDo: Certainly better to introduce a state field,
-		 * but rely on ops being set for now.
-		 */
-		if (dev->ops == NULL)
-			continue;
+		dev = TAILQ_NEXT(start, next);
+
+	while (dev != NULL) {
 		if (cmp(dev, rte_dev) == 0)
-			return dev;
+			break;
+
+		dev = TAILQ_NEXT(dev, next);
 	}
-	return NULL;
+	rte_spinlock_unlock(&vdpa_device_list_lock);
+
+	return dev;
 }
 
 static void *