[dpdk-dev] [PATCH 1/3] vhost: get rid of linked list dev

[dpdk-dev] [PATCH 1/3] vhost: get rid of linked list dev

[Yuanhan Liu]

While we use a single linked list to maintain all devices, we could
use a static array to achieve the same goal, just like what we did
to maintain the eth devices with rte_eth_devices array. This could
simplifies the code a bit.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

Note that there is a slight functional change to the old code: this
patch limits the vhost devices to 1024. We could either make it
configurable to increase the limit, or dynamically re-allocate a
bigger array when necessary to totally get rid of the limit.

Need a bit thoughts on that.
---
 lib/librte_vhost/virtio-net.c | 209 ++++++++++--------------------------------
 1 file changed, 50 insertions(+), 159 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index de78a0f..2f83438 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -55,18 +55,11 @@
 #include "vhost-net.h"
 #include "virtio-net.h"
 
-/*
- * Device linked list structure for configuration.
- */
-struct virtio_net_config_ll {
-	struct virtio_net dev;			/* Virtio device.*/
-	struct virtio_net_config_ll *next;	/* Next dev on linked list.*/
-};
+#define MAX_VHOST_DEVICE	1024
+static struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
 
 /* device ops to add/remove device to/from data core. */
 struct virtio_net_device_ops const *notify_ops;
-/* root address of the linked list of managed virtio devices */
-static struct virtio_net_config_ll *ll_root;
 
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
 
@@ -108,77 +101,17 @@ qva_to_vva(struct virtio_net *dev, uint64_t qemu_va)
 }
 
 
-/*
- * Retrieves an entry from the devices configuration linked list.
- */
-static struct virtio_net_config_ll *
-get_config_ll_entry(struct vhost_device_ctx ctx)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* Loop through linked list until the device_fh is found. */
-	while (ll_dev != NULL) {
-		if (ll_dev->dev.device_fh == ctx.fh)
-			return ll_dev;
-		ll_dev = ll_dev->next;
-	}
-
-	return NULL;
-}
-
-/*
- * Searches the configuration core linked list and
- * retrieves the device if it exists.
- */
 struct virtio_net *
 get_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev;
-
-	ll_dev = get_config_ll_entry(ctx);
-
-	if (ll_dev)
-		return &ll_dev->dev;
+	struct virtio_net *dev = vhost_devices[ctx.fh];
 
-	RTE_LOG(ERR, VHOST_CONFIG,
-		"(%"PRIu64") Device not found in linked list.\n", ctx.fh);
-	return NULL;
-}
-
-/*
- * Add entry containing a device to the device configuration linked list.
- */
-static void
-add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* If ll_dev == NULL then this is the first device so go to else */
-	if (ll_dev) {
-		/* If the 1st device_fh != 0 then we insert our device here. */
-		if (ll_dev->dev.device_fh != 0) {
-			new_ll_dev->dev.device_fh = 0;
-			new_ll_dev->next = ll_dev;
-			ll_root = new_ll_dev;
-		} else {
-			/*
-			 * Increment through the ll until we find un unused
-			 * device_fh. Insert the device at that entry.
-			 */
-			while ((ll_dev->next != NULL) &&
-				(ll_dev->dev.device_fh ==
-					(ll_dev->next->dev.device_fh - 1)))
-				ll_dev = ll_dev->next;
-
-			new_ll_dev->dev.device_fh = ll_dev->dev.device_fh + 1;
-			new_ll_dev->next = ll_dev->next;
-			ll_dev->next = new_ll_dev;
-		}
-	} else {
-		ll_root = new_ll_dev;
-		ll_root->dev.device_fh = 0;
+	if (unlikely(!dev)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"(%"PRIu64") device not found.\n", ctx.fh);
 	}
 
+	return dev;
 }
 
 static void
@@ -217,43 +150,14 @@ cleanup_device(struct virtio_net *dev, int destroy)
  * Release virtqueues and device memory.
  */
 static void
-free_device(struct virtio_net_config_ll *ll_dev)
+free_device(struct virtio_net *dev)
 {
 	uint32_t i;
 
-	for (i = 0; i < ll_dev->dev.virt_qp_nb; i++)
-		rte_free(ll_dev->dev.virtqueue[i * VIRTIO_QNUM]);
-
-	rte_free(ll_dev);
-}
+	for (i = 0; i < dev->virt_qp_nb; i++)
+		rte_free(dev->virtqueue[i * VIRTIO_QNUM]);
 
-/*
- * Remove an entry from the device configuration linked list.
- */
-static struct virtio_net_config_ll *
-rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
-	struct virtio_net_config_ll *ll_dev_last)
-{
-	/* First remove the device and then clean it up. */
-	if (ll_dev == ll_root) {
-		ll_root = ll_dev->next;
-		cleanup_device(&ll_dev->dev, 1);
-		free_device(ll_dev);
-		return ll_root;
-	} else {
-		if (likely(ll_dev_last != NULL)) {
-			ll_dev_last->next = ll_dev->next;
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			return ll_dev_last->next;
-		} else {
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			RTE_LOG(ERR, VHOST_CONFIG,
-				"Remove entry from config_ll failed\n");
-			return NULL;
-		}
-	}
+	rte_free(dev);
 }
 
 static void
@@ -351,23 +255,31 @@ reset_device(struct virtio_net *dev)
 static int
 new_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *new_ll_dev;
+	struct virtio_net *dev;
+	int i;
 
-	/* Setup device and virtqueues. */
-	new_ll_dev = rte_zmalloc(NULL, sizeof(struct virtio_net_config_ll), 0);
-	if (new_ll_dev == NULL) {
+	dev = rte_zmalloc(NULL, sizeof(struct virtio_net), 0);
+	if (dev == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%"PRIu64") Failed to allocate memory for dev.\n",
 			ctx.fh);
 		return -1;
 	}
 
-	new_ll_dev->next = NULL;
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		if (vhost_devices[i] == NULL)
+			break;
+	}
+	if (i == MAX_VHOST_DEVICE) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Failed to find a free slot for new device.\n");
+		return -1;
+	}
 
-	/* Add entry to device configuration linked list. */
-	add_config_ll_entry(new_ll_dev);
+	vhost_devices[i] = dev;
+	dev->device_fh   = i;
 
-	return new_ll_dev->dev.device_fh;
+	return i;
 }
 
 /*
@@ -377,30 +289,15 @@ new_device(struct vhost_device_ctx ctx)
 static void
 destroy_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;
-	struct virtio_net_config_ll *ll_dev_cur = ll_root;
-
-	/* Find the linked list entry for the device to be removed. */
-	ll_dev_cur_ctx = get_config_ll_entry(ctx);
-	while (ll_dev_cur != NULL) {
-		/*
-		 * If the device is found or
-		 * a device that doesn't exist is found then it is removed.
-		 */
-		if (ll_dev_cur == ll_dev_cur_ctx) {
-			/*
-			 * If the device is running on a data core then call
-			 * the function to remove it from the data core.
-			 */
-			if ((ll_dev_cur->dev.flags & VIRTIO_DEV_RUNNING))
-				notify_ops->destroy_device(&(ll_dev_cur->dev));
-			ll_dev_cur = rm_config_ll_entry(ll_dev_cur,
-					ll_dev_last);
-		} else {
-			ll_dev_last = ll_dev_cur;
-			ll_dev_cur = ll_dev_cur->next;
-		}
-	}
+	struct virtio_net *dev = get_device(ctx);
+
+	if (dev->flags & VIRTIO_DEV_RUNNING)
+		notify_ops->destroy_device(dev);
+
+	cleanup_device(dev, 1);
+	free_device(dev);
+
+	vhost_devices[ctx.fh] = NULL;
 }
 
 static void
@@ -544,17 +441,17 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net_config_ll *old_ll_dev, *new_ll_dev = NULL;
+	struct virtio_net *old_dev, *new_dev = NULL;
 	struct vhost_virtqueue *old_vq, *new_vq = NULL;
 	int ret;
 	int realloc_dev = 0, realloc_vq = 0;
 
-	old_ll_dev = (struct virtio_net_config_ll *)dev;
-	old_vq = dev->virtqueue[index];
+	old_dev = dev;
+	old_vq  = dev->virtqueue[index];
 
 	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
 			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_ll_dev,
+	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
 			MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
@@ -578,36 +475,30 @@ numa_realloc(struct virtio_net *dev, int index)
 		return dev;
 
 	if (realloc_dev)
-		new_ll_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net_config_ll), 0, newnode);
+		new_dev = rte_malloc_socket(NULL,
+			sizeof(struct virtio_net), 0, newnode);
 	if (realloc_vq)
 		new_vq = rte_malloc_socket(NULL,
 			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_ll_dev && !new_vq)
+	if (!new_dev && !new_vq)
 		return dev;
 
 	if (realloc_vq)
 		memcpy(new_vq, old_vq, sizeof(*new_vq));
 	if (realloc_dev)
-		memcpy(new_ll_dev, old_ll_dev, sizeof(*new_ll_dev));
-	(new_ll_dev ? new_ll_dev : old_ll_dev)->dev.virtqueue[index] =
+		memcpy(new_dev, old_dev, sizeof(*new_dev));
+
+	(new_dev ? new_dev : old_dev)->virtqueue[index] =
 		new_vq ? new_vq : old_vq;
 	if (realloc_vq)
 		rte_free(old_vq);
 	if (realloc_dev) {
-		if (ll_root == old_ll_dev)
-			ll_root = new_ll_dev;
-		else {
-			struct virtio_net_config_ll *prev = ll_root;
-			while (prev->next != old_ll_dev)
-				prev = prev->next;
-			prev->next = new_ll_dev;
-			new_ll_dev->next = old_ll_dev->next;
-		}
-		rte_free(old_ll_dev);
+		rte_free(old_dev);
+
+		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? &new_ll_dev->dev : dev;
+	return realloc_dev ? new_dev: dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH 2/3] vhost: simplify numa_realloc

[Yuanhan Liu]

We could first check if we need realloc vq or not, if so,
reallocate it. We then do similar to vhost dev realloc.

This could get rid of the tons of repeated "if (realloc_dev)"
and "if (realloc_vq)" statements, therefore, makes code
a bit more readable.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/virtio-net.c | 77 ++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 2f83438..31ca4f7 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -441,64 +441,59 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net *old_dev, *new_dev = NULL;
-	struct vhost_virtqueue *old_vq, *new_vq = NULL;
+	struct virtio_net *old_dev;
+	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
-	int realloc_dev = 0, realloc_vq = 0;
 
 	old_dev = dev;
-	old_vq  = dev->virtqueue[index];
+	vq = old_vq = dev->virtqueue[index];
 
-	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
-			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
+	ret = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
 			MPOL_F_NODE | MPOL_F_ADDR);
+
+	/* check if we need to reallocate vq */
+	ret = get_mempolicy(&oldnode, NULL, 0, old_vq, MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vring desc or dev numa information.\n");
+			"Unable to get vq numa information.\n");
 		return dev;
 	}
-	if (oldnode != newnode)
-		realloc_dev = 1;
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate vq from %d to %d node\n", oldnode, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		if (!vq)
+			return dev;
+
+		memcpy(vq, old_vq, sizeof(*vq));
+		rte_free(old_vq);
+	}
 
-	ret = get_mempolicy(&oldnode, NULL, 0, old_vq,
-			MPOL_F_NODE | MPOL_F_ADDR);
+	/* check if we need to reallocate dev */
+	ret = get_mempolicy(&oldnode, NULL, 0, old_dev, MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vq numa information.\n");
-		return dev;
+			"Unable to get vring desc or dev numa information.\n");
+		goto out;
 	}
-	if (oldnode != newnode)
-		realloc_vq = 1;
-
-	if (realloc_dev == 0 && realloc_vq == 0)
-		return dev;
-
-	if (realloc_dev)
-		new_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net), 0, newnode);
-	if (realloc_vq)
-		new_vq = rte_malloc_socket(NULL,
-			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_dev && !new_vq)
-		return dev;
-
-	if (realloc_vq)
-		memcpy(new_vq, old_vq, sizeof(*new_vq));
-	if (realloc_dev)
-		memcpy(new_dev, old_dev, sizeof(*new_dev));
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate dev from %d to %d node\n", oldnode, newnode);
+		dev = rte_malloc_socket(NULL, sizeof(*dev), 0, newnode);
+		if (!dev) {
+			dev = old_dev;
+			goto out;
+		}
 
-	(new_dev ? new_dev : old_dev)->virtqueue[index] =
-		new_vq ? new_vq : old_vq;
-	if (realloc_vq)
-		rte_free(old_vq);
-	if (realloc_dev) {
+		memcpy(dev, old_dev, sizeof(*dev));
 		rte_free(old_dev);
-
-		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? new_dev: dev;
+out:
+	dev->virtqueue[index] = vq;
+	vhost_devices[dev->device_fh] = dev;
+
+	return dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH 3/3] vhost: fix vq realloc at numa_realloc

[Yuanhan Liu]

vq is allocated on pairs, hence we should do pair reallocation
at numa_realloc() as well, otherwise an error like following
occurs while do numa reallocation:

    VHOST_CONFIG: reallocate vq from 0 to 1 node
    PANIC in rte_free():
    Fatal error: Invalid memory

The reason we don't catch it is because numa_realloc() will
not take effect when RTE_LIBRTE_VHOST_NUMA is not enabled,
which is the default case.

Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---
 lib/librte_vhost/virtio-net.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 31ca4f7..a962667 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -445,6 +445,13 @@ numa_realloc(struct virtio_net *dev, int index)
 	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
 
+	/*
+	 * vq is allocated on pairs, we should try to do realloc
+	 * on first queue of one queue pair only.
+	 */
+	if (index % VIRTIO_QNUM != 0)
+		return dev;
+
 	old_dev = dev;
 	vq = old_vq = dev->virtqueue[index];
 
@@ -461,11 +468,12 @@ numa_realloc(struct virtio_net *dev, int index)
 	if (oldnode != newnode) {
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"reallocate vq from %d to %d node\n", oldnode, newnode);
-		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq) * VIRTIO_QNUM, 0,
+				       newnode);
 		if (!vq)
 			return dev;
 
-		memcpy(vq, old_vq, sizeof(*vq));
+		memcpy(vq, old_vq, sizeof(*vq) * VIRTIO_QNUM);
 		rte_free(old_vq);
 	}
 
@@ -491,6 +499,7 @@ numa_realloc(struct virtio_net *dev, int index)
 
 out:
 	dev->virtqueue[index] = vq;
+	dev->virtqueue[index + 1] = vq + 1;
 	vhost_devices[dev->device_fh] = dev;
 
 	return dev;
-- 
1.9.0

Re: [dpdk-dev] [PATCH 1/3] vhost: get rid of linked list dev

[Xie, Huawei]

On 12/18/2015 3:03 PM, Yuanhan Liu wrote:
> While we use a single linked list to maintain all devices, we could
> use a static array to achieve the same goal, just like what we did
> to maintain the eth devices with rte_eth_devices array. This could
> simplifies the code a bit.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

This does simplify the implementation. Cced Tommy.
[...]

Re: [dpdk-dev] [PATCH 1/3] vhost: get rid of linked list dev

[Xie, Huawei]

On 12/18/2015 3:03 PM, Yuanhan Liu wrote:
> While we use a single linked list to maintain all devices, we could
> use a static array to achieve the same goal, just like what we did
> to maintain the eth devices with rte_eth_devices array. This could
> simplifies the code a bit.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Acked-by: Huawei Xie <huawei.xie@intel.com>
[...]

Re: [dpdk-dev] [PATCH 2/3] vhost: simplify numa_realloc

[Xie, Huawei]

On 12/18/2015 3:03 PM, Yuanhan Liu wrote:
> We could first check if we need realloc vq or not, if so,
> reallocate it. We then do similar to vhost dev realloc.
>
> This could get rid of the tons of repeated "if (realloc_dev)"
> and "if (realloc_vq)" statements, therefore, makes code
> a bit more readable.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> ---
>  lib/librte_vhost/virtio-net.c | 77 ++++++++++++++++++++-----------------------
>  1 file changed, 36 insertions(+), 41 deletions(-)
>
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 2f83438..31ca4f7 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -441,64 +441,59 @@ static struct virtio_net*
>  numa_realloc(struct virtio_net *dev, int index)
>  {
>  	int oldnode, newnode;
> -	struct virtio_net *old_dev, *new_dev = NULL;
> -	struct vhost_virtqueue *old_vq, *new_vq = NULL;
> +	struct virtio_net *old_dev;
> +	struct vhost_virtqueue *old_vq, *vq;
>  	int ret;
> -	int realloc_dev = 0, realloc_vq = 0;
>  
>  	old_dev = dev;
> -	old_vq  = dev->virtqueue[index];
> +	vq = old_vq = dev->virtqueue[index];
>  
> -	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
> -			MPOL_F_NODE | MPOL_F_ADDR);
> -	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
> +	ret = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
>  			MPOL_F_NODE | MPOL_F_ADDR);
> +
> +	/* check if we need to reallocate vq */
> +	ret = get_mempolicy(&oldnode, NULL, 0, old_vq, MPOL_F_NODE | MPOL_F_ADDR);

Why remove the ret = ret | ? Both get_mempolicy could fail.

>  	if (ret) {
>  		RTE_LOG(ERR, VHOST_CONFIG,
> -			"Unable to get vring desc or dev numa information.\n");
> +			"Unable to get vq numa information.\n");
>  		return dev;
>  	}
> -	if (oldnode != newnode)
> -		realloc_dev = 1;
> +	if (oldnode != newnode) {
> +		RTE_LOG(INFO, VHOST_CONFIG,
> +			"reallocate vq from %d to %d node\n", oldnode, newnode);
> +		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
> +		if (!vq)
> +			return dev;
> +
> +		memcpy(vq, old_vq, sizeof(*vq));
> +		rte_free(old_vq);
> +	}
>  
> -	ret = get_mempolicy(&oldnode, NULL, 0, old_vq,
> -			MPOL_F_NODE | MPOL_F_ADDR);
> +	/* check if we need to reallocate dev */
> +	ret = get_mempolicy(&oldnode, NULL, 0, old_dev, MPOL_F_NODE | MPOL_F_ADDR);
>  	if (ret) {
>  		RTE_LOG(ERR, VHOST_CONFIG,
> -			"Unable to get vq numa information.\n");
> -		return dev;
> +			"Unable to get vring desc or dev numa information.\n");
> +		goto out;
>  	}

Why vring desc in the err message?
-huawei
[...]

Re: [dpdk-dev] [PATCH 2/3] vhost: simplify numa_realloc

[Yuanhan Liu]

On Tue, Dec 22, 2015 at 06:46:32AM +0000, Xie, Huawei wrote:
> On 12/18/2015 3:03 PM, Yuanhan Liu wrote:
> > We could first check if we need realloc vq or not, if so,
> > reallocate it. We then do similar to vhost dev realloc.
> >
> > This could get rid of the tons of repeated "if (realloc_dev)"
> > and "if (realloc_vq)" statements, therefore, makes code
> > a bit more readable.
> >
> > Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> > ---
> >  lib/librte_vhost/virtio-net.c | 77 ++++++++++++++++++++-----------------------
> >  1 file changed, 36 insertions(+), 41 deletions(-)
> >
> > diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> > index 2f83438..31ca4f7 100644
> > --- a/lib/librte_vhost/virtio-net.c
> > +++ b/lib/librte_vhost/virtio-net.c
> > @@ -441,64 +441,59 @@ static struct virtio_net*
> >  numa_realloc(struct virtio_net *dev, int index)
> >  {
> >  	int oldnode, newnode;
> > -	struct virtio_net *old_dev, *new_dev = NULL;
> > -	struct vhost_virtqueue *old_vq, *new_vq = NULL;
> > +	struct virtio_net *old_dev;
> > +	struct vhost_virtqueue *old_vq, *vq;
> >  	int ret;
> > -	int realloc_dev = 0, realloc_vq = 0;
> >  
> >  	old_dev = dev;
> > -	old_vq  = dev->virtqueue[index];
> > +	vq = old_vq = dev->virtqueue[index];
> >  
> > -	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
> > -			MPOL_F_NODE | MPOL_F_ADDR);
> > -	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
> > +	ret = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
> >  			MPOL_F_NODE | MPOL_F_ADDR);
> > +
> > +	/* check if we need to reallocate vq */
> > +	ret = get_mempolicy(&oldnode, NULL, 0, old_vq, MPOL_F_NODE | MPOL_F_ADDR);
> 
> Why remove the ret = ret | ? Both get_mempolicy could fail.

Right, will fix it.

> 
> >  	if (ret) {
> >  		RTE_LOG(ERR, VHOST_CONFIG,
> > -			"Unable to get vring desc or dev numa information.\n");
> > +			"Unable to get vq numa information.\n");
> >  		return dev;
> >  	}
> > -	if (oldnode != newnode)
> > -		realloc_dev = 1;
> > +	if (oldnode != newnode) {
> > +		RTE_LOG(INFO, VHOST_CONFIG,
> > +			"reallocate vq from %d to %d node\n", oldnode, newnode);
> > +		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
> > +		if (!vq)
> > +			return dev;
> > +
> > +		memcpy(vq, old_vq, sizeof(*vq));
> > +		rte_free(old_vq);
> > +	}
> >  
> > -	ret = get_mempolicy(&oldnode, NULL, 0, old_vq,
> > -			MPOL_F_NODE | MPOL_F_ADDR);
> > +	/* check if we need to reallocate dev */
> > +	ret = get_mempolicy(&oldnode, NULL, 0, old_dev, MPOL_F_NODE | MPOL_F_ADDR);
> >  	if (ret) {
> >  		RTE_LOG(ERR, VHOST_CONFIG,
> > -			"Unable to get vq numa information.\n");
> > -		return dev;
> > +			"Unable to get vring desc or dev numa information.\n");
> > +		goto out;
> >  	}
> 
> Why vring desc in the err message?

Oops, no idea why I did that. Will fix it.

	--yliu

Re: [dpdk-dev] [PATCH 3/3] vhost: fix vq realloc at numa_realloc

[Xie, Huawei]

On 12/18/2015 3:03 PM, Yuanhan Liu wrote:
> vq is allocated on pairs, hence we should do pair reallocation
> at numa_realloc() as well, otherwise an error like following
> occurs while do numa reallocation:
>
>     VHOST_CONFIG: reallocate vq from 0 to 1 node
>     PANIC in rte_free():
>     Fatal error: Invalid memory
>
> The reason we don't catch it is because numa_realloc() will
looks to me, but
s /because/that/
s /on pairs/in pairs/ ? :).
> not take effect when RTE_LIBRTE_VHOST_NUMA is not enabled,
> which is the default case.
>
> Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Acked-by: Huawei Xie <huawei.xie@intel.com>

[dpdk-dev] [PATCH v2 1/3] vhost: get rid of linked list dev

[Yuanhan Liu]

While we use a single linked list to maintain all devices, we could
use a static array to achieve the same goal, just like what we did
to maintain the eth devices with rte_eth_devices array. This could
simplifies the code a bit.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
---

Note that there is a slight functional change to the old code: this
patch limits the vhost devices to 1024. We could either make it
configurable to increase the limit, or dynamically re-allocate a
bigger array when necessary to totally get rid of the limit.

Need comments on that.
---
 lib/librte_vhost/virtio-net.c | 209 ++++++++++--------------------------------
 1 file changed, 50 insertions(+), 159 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index de78a0f..2f83438 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -55,18 +55,11 @@
 #include "vhost-net.h"
 #include "virtio-net.h"
 
-/*
- * Device linked list structure for configuration.
- */
-struct virtio_net_config_ll {
-	struct virtio_net dev;			/* Virtio device.*/
-	struct virtio_net_config_ll *next;	/* Next dev on linked list.*/
-};
+#define MAX_VHOST_DEVICE	1024
+static struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
 
 /* device ops to add/remove device to/from data core. */
 struct virtio_net_device_ops const *notify_ops;
-/* root address of the linked list of managed virtio devices */
-static struct virtio_net_config_ll *ll_root;
 
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
 
@@ -108,77 +101,17 @@ qva_to_vva(struct virtio_net *dev, uint64_t qemu_va)
 }
 
 
-/*
- * Retrieves an entry from the devices configuration linked list.
- */
-static struct virtio_net_config_ll *
-get_config_ll_entry(struct vhost_device_ctx ctx)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* Loop through linked list until the device_fh is found. */
-	while (ll_dev != NULL) {
-		if (ll_dev->dev.device_fh == ctx.fh)
-			return ll_dev;
-		ll_dev = ll_dev->next;
-	}
-
-	return NULL;
-}
-
-/*
- * Searches the configuration core linked list and
- * retrieves the device if it exists.
- */
 struct virtio_net *
 get_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev;
-
-	ll_dev = get_config_ll_entry(ctx);
-
-	if (ll_dev)
-		return &ll_dev->dev;
+	struct virtio_net *dev = vhost_devices[ctx.fh];
 
-	RTE_LOG(ERR, VHOST_CONFIG,
-		"(%"PRIu64") Device not found in linked list.\n", ctx.fh);
-	return NULL;
-}
-
-/*
- * Add entry containing a device to the device configuration linked list.
- */
-static void
-add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* If ll_dev == NULL then this is the first device so go to else */
-	if (ll_dev) {
-		/* If the 1st device_fh != 0 then we insert our device here. */
-		if (ll_dev->dev.device_fh != 0) {
-			new_ll_dev->dev.device_fh = 0;
-			new_ll_dev->next = ll_dev;
-			ll_root = new_ll_dev;
-		} else {
-			/*
-			 * Increment through the ll until we find un unused
-			 * device_fh. Insert the device at that entry.
-			 */
-			while ((ll_dev->next != NULL) &&
-				(ll_dev->dev.device_fh ==
-					(ll_dev->next->dev.device_fh - 1)))
-				ll_dev = ll_dev->next;
-
-			new_ll_dev->dev.device_fh = ll_dev->dev.device_fh + 1;
-			new_ll_dev->next = ll_dev->next;
-			ll_dev->next = new_ll_dev;
-		}
-	} else {
-		ll_root = new_ll_dev;
-		ll_root->dev.device_fh = 0;
+	if (unlikely(!dev)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"(%"PRIu64") device not found.\n", ctx.fh);
 	}
 
+	return dev;
 }
 
 static void
@@ -217,43 +150,14 @@ cleanup_device(struct virtio_net *dev, int destroy)
  * Release virtqueues and device memory.
  */
 static void
-free_device(struct virtio_net_config_ll *ll_dev)
+free_device(struct virtio_net *dev)
 {
 	uint32_t i;
 
-	for (i = 0; i < ll_dev->dev.virt_qp_nb; i++)
-		rte_free(ll_dev->dev.virtqueue[i * VIRTIO_QNUM]);
-
-	rte_free(ll_dev);
-}
+	for (i = 0; i < dev->virt_qp_nb; i++)
+		rte_free(dev->virtqueue[i * VIRTIO_QNUM]);
 
-/*
- * Remove an entry from the device configuration linked list.
- */
-static struct virtio_net_config_ll *
-rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
-	struct virtio_net_config_ll *ll_dev_last)
-{
-	/* First remove the device and then clean it up. */
-	if (ll_dev == ll_root) {
-		ll_root = ll_dev->next;
-		cleanup_device(&ll_dev->dev, 1);
-		free_device(ll_dev);
-		return ll_root;
-	} else {
-		if (likely(ll_dev_last != NULL)) {
-			ll_dev_last->next = ll_dev->next;
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			return ll_dev_last->next;
-		} else {
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			RTE_LOG(ERR, VHOST_CONFIG,
-				"Remove entry from config_ll failed\n");
-			return NULL;
-		}
-	}
+	rte_free(dev);
 }
 
 static void
@@ -351,23 +255,31 @@ reset_device(struct virtio_net *dev)
 static int
 new_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *new_ll_dev;
+	struct virtio_net *dev;
+	int i;
 
-	/* Setup device and virtqueues. */
-	new_ll_dev = rte_zmalloc(NULL, sizeof(struct virtio_net_config_ll), 0);
-	if (new_ll_dev == NULL) {
+	dev = rte_zmalloc(NULL, sizeof(struct virtio_net), 0);
+	if (dev == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%"PRIu64") Failed to allocate memory for dev.\n",
 			ctx.fh);
 		return -1;
 	}
 
-	new_ll_dev->next = NULL;
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		if (vhost_devices[i] == NULL)
+			break;
+	}
+	if (i == MAX_VHOST_DEVICE) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Failed to find a free slot for new device.\n");
+		return -1;
+	}
 
-	/* Add entry to device configuration linked list. */
-	add_config_ll_entry(new_ll_dev);
+	vhost_devices[i] = dev;
+	dev->device_fh   = i;
 
-	return new_ll_dev->dev.device_fh;
+	return i;
 }
 
 /*
@@ -377,30 +289,15 @@ new_device(struct vhost_device_ctx ctx)
 static void
 destroy_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;
-	struct virtio_net_config_ll *ll_dev_cur = ll_root;
-
-	/* Find the linked list entry for the device to be removed. */
-	ll_dev_cur_ctx = get_config_ll_entry(ctx);
-	while (ll_dev_cur != NULL) {
-		/*
-		 * If the device is found or
-		 * a device that doesn't exist is found then it is removed.
-		 */
-		if (ll_dev_cur == ll_dev_cur_ctx) {
-			/*
-			 * If the device is running on a data core then call
-			 * the function to remove it from the data core.
-			 */
-			if ((ll_dev_cur->dev.flags & VIRTIO_DEV_RUNNING))
-				notify_ops->destroy_device(&(ll_dev_cur->dev));
-			ll_dev_cur = rm_config_ll_entry(ll_dev_cur,
-					ll_dev_last);
-		} else {
-			ll_dev_last = ll_dev_cur;
-			ll_dev_cur = ll_dev_cur->next;
-		}
-	}
+	struct virtio_net *dev = get_device(ctx);
+
+	if (dev->flags & VIRTIO_DEV_RUNNING)
+		notify_ops->destroy_device(dev);
+
+	cleanup_device(dev, 1);
+	free_device(dev);
+
+	vhost_devices[ctx.fh] = NULL;
 }
 
 static void
@@ -544,17 +441,17 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net_config_ll *old_ll_dev, *new_ll_dev = NULL;
+	struct virtio_net *old_dev, *new_dev = NULL;
 	struct vhost_virtqueue *old_vq, *new_vq = NULL;
 	int ret;
 	int realloc_dev = 0, realloc_vq = 0;
 
-	old_ll_dev = (struct virtio_net_config_ll *)dev;
-	old_vq = dev->virtqueue[index];
+	old_dev = dev;
+	old_vq  = dev->virtqueue[index];
 
 	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
 			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_ll_dev,
+	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
 			MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
@@ -578,36 +475,30 @@ numa_realloc(struct virtio_net *dev, int index)
 		return dev;
 
 	if (realloc_dev)
-		new_ll_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net_config_ll), 0, newnode);
+		new_dev = rte_malloc_socket(NULL,
+			sizeof(struct virtio_net), 0, newnode);
 	if (realloc_vq)
 		new_vq = rte_malloc_socket(NULL,
 			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_ll_dev && !new_vq)
+	if (!new_dev && !new_vq)
 		return dev;
 
 	if (realloc_vq)
 		memcpy(new_vq, old_vq, sizeof(*new_vq));
 	if (realloc_dev)
-		memcpy(new_ll_dev, old_ll_dev, sizeof(*new_ll_dev));
-	(new_ll_dev ? new_ll_dev : old_ll_dev)->dev.virtqueue[index] =
+		memcpy(new_dev, old_dev, sizeof(*new_dev));
+
+	(new_dev ? new_dev : old_dev)->virtqueue[index] =
 		new_vq ? new_vq : old_vq;
 	if (realloc_vq)
 		rte_free(old_vq);
 	if (realloc_dev) {
-		if (ll_root == old_ll_dev)
-			ll_root = new_ll_dev;
-		else {
-			struct virtio_net_config_ll *prev = ll_root;
-			while (prev->next != old_ll_dev)
-				prev = prev->next;
-			prev->next = new_ll_dev;
-			new_ll_dev->next = old_ll_dev->next;
-		}
-		rte_free(old_ll_dev);
+		rte_free(old_dev);
+
+		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? &new_ll_dev->dev : dev;
+	return realloc_dev ? new_dev: dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH v2 2/3] vhost: simplify numa_realloc

[Yuanhan Liu]

We could first check if we need realloc vq or not, if so,
reallocate it. We then do similar to vhost dev realloc.

This could get rid of the tons of repeated "if (realloc_dev)"
and "if (realloc_vq)" statements, therefore, makes code
a bit more readable.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
---

v2: fix debug message and check return value of all get_mempolicy()
---
 lib/librte_vhost/virtio-net.c | 77 ++++++++++++++++++++-----------------------
 1 file changed, 36 insertions(+), 41 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 2f83438..1566c93 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -441,64 +441,59 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net *old_dev, *new_dev = NULL;
-	struct vhost_virtqueue *old_vq, *new_vq = NULL;
+	struct virtio_net *old_dev;
+	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
-	int realloc_dev = 0, realloc_vq = 0;
 
 	old_dev = dev;
-	old_vq  = dev->virtqueue[index];
+	vq = old_vq = dev->virtqueue[index];
 
-	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
-			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
+	ret = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
 			MPOL_F_NODE | MPOL_F_ADDR);
+
+	/* check if we need to reallocate vq */
+	ret |= get_mempolicy(&oldnode, NULL, 0, old_vq, MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vring desc or dev numa information.\n");
+			"Unable to get vq numa information.\n");
 		return dev;
 	}
-	if (oldnode != newnode)
-		realloc_dev = 1;
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate vq from %d to %d node\n", oldnode, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		if (!vq)
+			return dev;
+
+		memcpy(vq, old_vq, sizeof(*vq));
+		rte_free(old_vq);
+	}
 
-	ret = get_mempolicy(&oldnode, NULL, 0, old_vq,
-			MPOL_F_NODE | MPOL_F_ADDR);
+	/* check if we need to reallocate dev */
+	ret = get_mempolicy(&oldnode, NULL, 0, old_dev, MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vq numa information.\n");
-		return dev;
+			"Unable to get dev numa information.\n");
+		goto out;
 	}
-	if (oldnode != newnode)
-		realloc_vq = 1;
-
-	if (realloc_dev == 0 && realloc_vq == 0)
-		return dev;
-
-	if (realloc_dev)
-		new_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net), 0, newnode);
-	if (realloc_vq)
-		new_vq = rte_malloc_socket(NULL,
-			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_dev && !new_vq)
-		return dev;
-
-	if (realloc_vq)
-		memcpy(new_vq, old_vq, sizeof(*new_vq));
-	if (realloc_dev)
-		memcpy(new_dev, old_dev, sizeof(*new_dev));
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate dev from %d to %d node\n", oldnode, newnode);
+		dev = rte_malloc_socket(NULL, sizeof(*dev), 0, newnode);
+		if (!dev) {
+			dev = old_dev;
+			goto out;
+		}
 
-	(new_dev ? new_dev : old_dev)->virtqueue[index] =
-		new_vq ? new_vq : old_vq;
-	if (realloc_vq)
-		rte_free(old_vq);
-	if (realloc_dev) {
+		memcpy(dev, old_dev, sizeof(*dev));
 		rte_free(old_dev);
-
-		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? new_dev: dev;
+out:
+	dev->virtqueue[index] = vq;
+	vhost_devices[dev->device_fh] = dev;
+
+	return dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc

[Yuanhan Liu]

vq is allocated on pairs, hence we should do pair reallocation
at numa_realloc() as well, otherwise an error like following
occurs while do numa reallocation:

    VHOST_CONFIG: reallocate vq from 0 to 1 node
    PANIC in rte_free():
    Fatal error: Invalid memory

The reason we don't catch it is because numa_realloc() will
not take effect when RTE_LIBRTE_VHOST_NUMA is not enabled,
which is the default case.

Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/virtio-net.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 1566c93..7469312 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -445,6 +445,13 @@ numa_realloc(struct virtio_net *dev, int index)
 	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
 
+	/*
+	 * vq is allocated on pairs, we should try to do realloc
+	 * on first queue of one queue pair only.
+	 */
+	if (index % VIRTIO_QNUM != 0)
+		return dev;
+
 	old_dev = dev;
 	vq = old_vq = dev->virtqueue[index];
 
@@ -461,11 +468,12 @@ numa_realloc(struct virtio_net *dev, int index)
 	if (oldnode != newnode) {
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"reallocate vq from %d to %d node\n", oldnode, newnode);
-		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq) * VIRTIO_QNUM, 0,
+				       newnode);
 		if (!vq)
 			return dev;
 
-		memcpy(vq, old_vq, sizeof(*vq));
+		memcpy(vq, old_vq, sizeof(*vq) * VIRTIO_QNUM);
 		rte_free(old_vq);
 	}
 
@@ -491,6 +499,7 @@ numa_realloc(struct virtio_net *dev, int index)
 
 out:
 	dev->virtqueue[index] = vq;
+	dev->virtqueue[index + 1] = vq + 1;
 	vhost_devices[dev->device_fh] = dev;
 
 	return dev;
-- 
1.9.0

Re: [dpdk-dev] [PATCH v2 2/3] vhost: simplify numa_realloc

[Xie, Huawei]

On 12/22/2015 3:27 PM, Yuanhan Liu wrote:
> We could first check if we need realloc vq or not, if so,
> reallocate it. We then do similar to vhost dev realloc.
>
> This could get rid of the tons of repeated "if (realloc_dev)"
> and "if (realloc_vq)" statements, therefore, makes code
> a bit more readable.
>
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>

Acked-by: Huawei Xie <huawei.xie@intel.com>
[...]

Re: [dpdk-dev] [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc

[Loftus, Ciara]

> 
> vq is allocated on pairs, hence we should do pair reallocation
> at numa_realloc() as well, otherwise an error like following
> occurs while do numa reallocation:
> 
>     VHOST_CONFIG: reallocate vq from 0 to 1 node
>     PANIC in rte_free():
>     Fatal error: Invalid memory
> 
> The reason we don't catch it is because numa_realloc() will
> not take effect when RTE_LIBRTE_VHOST_NUMA is not enabled,
> which is the default case.
> 
> Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")
> 
> Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
> Acked-by: Huawei Xie <huawei.xie@intel.com>
> ---
>  lib/librte_vhost/virtio-net.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
> index 1566c93..7469312 100644
> --- a/lib/librte_vhost/virtio-net.c
> +++ b/lib/librte_vhost/virtio-net.c
> @@ -445,6 +445,13 @@ numa_realloc(struct virtio_net *dev, int index)
>  	struct vhost_virtqueue *old_vq, *vq;
>  	int ret;
> 
> +	/*
> +	 * vq is allocated on pairs, we should try to do realloc
> +	 * on first queue of one queue pair only.
> +	 */
> +	if (index % VIRTIO_QNUM != 0)
> +		return dev;
> +
>  	old_dev = dev;
>  	vq = old_vq = dev->virtqueue[index];
> 
> @@ -461,11 +468,12 @@ numa_realloc(struct virtio_net *dev, int index)
>  	if (oldnode != newnode) {
>  		RTE_LOG(INFO, VHOST_CONFIG,
>  			"reallocate vq from %d to %d node\n", oldnode,
> newnode);
> -		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
> +		vq = rte_malloc_socket(NULL, sizeof(*vq) * VIRTIO_QNUM,
> 0,
> +				       newnode);
>  		if (!vq)
>  			return dev;
> 
> -		memcpy(vq, old_vq, sizeof(*vq));
> +		memcpy(vq, old_vq, sizeof(*vq) * VIRTIO_QNUM);
>  		rte_free(old_vq);
>  	}
> 
> @@ -491,6 +499,7 @@ numa_realloc(struct virtio_net *dev, int index)
> 
>  out:
>  	dev->virtqueue[index] = vq;
> +	dev->virtqueue[index + 1] = vq + 1;
>  	vhost_devices[dev->device_fh] = dev;
> 
>  	return dev;
> --
> 1.9.0

I encountered the " PANIC in rte_free():" error when using RTE_LIBRTE_VHOST_NUMA too, and applying this series resolved the issue. Thanks for the patches.

Tested-by: Ciara Loftus <ciara.loftus@intel.com>

Thanks,
Ciara

Re: [dpdk-dev] [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc

[Yuanhan Liu]

On Mon, Mar 07, 2016 at 01:49:26PM +0000, Loftus, Ciara wrote:
> 
> I encountered the " PANIC in rte_free():" error when using RTE_LIBRTE_VHOST_NUMA too, and applying this series resolved the issue. Thanks for the patches.
> 
> Tested-by: Ciara Loftus <ciara.loftus@intel.com>

Thanks for testing. A new version (just about rebase) will hopefully be
sent out in one or two days.

	--yliu

[dpdk-dev] [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes

[Yuanhan Liu]

This short series includes two cleanup patch, and one bug fix patch.

v3: - code rebase

---
Yuanhan Liu (3):
  vhost: get rid of linked list dev
  vhost: simplify numa_realloc
  vhost: fix vq realloc at numa_realloc

 lib/librte_vhost/virtio-net.c | 276 +++++++++++++-----------------------------
 1 file changed, 87 insertions(+), 189 deletions(-)

-- 
1.9.0

[dpdk-dev] [PATCH v3 1/3] vhost: get rid of linked list dev

[Yuanhan Liu]

While we use a single linked list to maintain all devices, we could
use a static array to achieve the same goal, just like what we did
to maintain the eth devices with rte_eth_devices array. This could
simplifies the code a bit.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
---
 lib/librte_vhost/virtio-net.c | 209 ++++++++++--------------------------------
 1 file changed, 50 insertions(+), 159 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index fe1a77e..be91e5f 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -55,18 +55,11 @@
 #include "vhost-net.h"
 #include "virtio-net.h"
 
-/*
- * Device linked list structure for configuration.
- */
-struct virtio_net_config_ll {
-	struct virtio_net dev;			/* Virtio device.*/
-	struct virtio_net_config_ll *next;	/* Next dev on linked list.*/
-};
+#define MAX_VHOST_DEVICE	1024
+static struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
 
 /* device ops to add/remove device to/from data core. */
 struct virtio_net_device_ops const *notify_ops;
-/* root address of the linked list of managed virtio devices */
-static struct virtio_net_config_ll *ll_root;
 
 #define VHOST_USER_F_PROTOCOL_FEATURES	30
 
@@ -116,77 +109,17 @@ qva_to_vva(struct virtio_net *dev, uint64_t qemu_va)
 }
 
 
-/*
- * Retrieves an entry from the devices configuration linked list.
- */
-static struct virtio_net_config_ll *
-get_config_ll_entry(struct vhost_device_ctx ctx)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* Loop through linked list until the device_fh is found. */
-	while (ll_dev != NULL) {
-		if (ll_dev->dev.device_fh == ctx.fh)
-			return ll_dev;
-		ll_dev = ll_dev->next;
-	}
-
-	return NULL;
-}
-
-/*
- * Searches the configuration core linked list and
- * retrieves the device if it exists.
- */
 struct virtio_net *
 get_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev;
+	struct virtio_net *dev = vhost_devices[ctx.fh];
 
-	ll_dev = get_config_ll_entry(ctx);
-
-	if (ll_dev)
-		return &ll_dev->dev;
-
-	RTE_LOG(ERR, VHOST_CONFIG,
-		"(%"PRIu64") Device not found in linked list.\n", ctx.fh);
-	return NULL;
-}
-
-/*
- * Add entry containing a device to the device configuration linked list.
- */
-static void
-add_config_ll_entry(struct virtio_net_config_ll *new_ll_dev)
-{
-	struct virtio_net_config_ll *ll_dev = ll_root;
-
-	/* If ll_dev == NULL then this is the first device so go to else */
-	if (ll_dev) {
-		/* If the 1st device_fh != 0 then we insert our device here. */
-		if (ll_dev->dev.device_fh != 0) {
-			new_ll_dev->dev.device_fh = 0;
-			new_ll_dev->next = ll_dev;
-			ll_root = new_ll_dev;
-		} else {
-			/*
-			 * Increment through the ll until we find un unused
-			 * device_fh. Insert the device at that entry.
-			 */
-			while ((ll_dev->next != NULL) &&
-				(ll_dev->dev.device_fh ==
-					(ll_dev->next->dev.device_fh - 1)))
-				ll_dev = ll_dev->next;
-
-			new_ll_dev->dev.device_fh = ll_dev->dev.device_fh + 1;
-			new_ll_dev->next = ll_dev->next;
-			ll_dev->next = new_ll_dev;
-		}
-	} else {
-		ll_root = new_ll_dev;
-		ll_root->dev.device_fh = 0;
+	if (unlikely(!dev)) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"(%"PRIu64") device not found.\n", ctx.fh);
 	}
 
+	return dev;
 }
 
 static void
@@ -219,43 +152,14 @@ cleanup_device(struct virtio_net *dev, int destroy)
  * Release virtqueues and device memory.
  */
 static void
-free_device(struct virtio_net_config_ll *ll_dev)
+free_device(struct virtio_net *dev)
 {
 	uint32_t i;
 
-	for (i = 0; i < ll_dev->dev.virt_qp_nb; i++)
-		rte_free(ll_dev->dev.virtqueue[i * VIRTIO_QNUM]);
-
-	rte_free(ll_dev);
-}
+	for (i = 0; i < dev->virt_qp_nb; i++)
+		rte_free(dev->virtqueue[i * VIRTIO_QNUM]);
 
-/*
- * Remove an entry from the device configuration linked list.
- */
-static struct virtio_net_config_ll *
-rm_config_ll_entry(struct virtio_net_config_ll *ll_dev,
-	struct virtio_net_config_ll *ll_dev_last)
-{
-	/* First remove the device and then clean it up. */
-	if (ll_dev == ll_root) {
-		ll_root = ll_dev->next;
-		cleanup_device(&ll_dev->dev, 1);
-		free_device(ll_dev);
-		return ll_root;
-	} else {
-		if (likely(ll_dev_last != NULL)) {
-			ll_dev_last->next = ll_dev->next;
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			return ll_dev_last->next;
-		} else {
-			cleanup_device(&ll_dev->dev, 1);
-			free_device(ll_dev);
-			RTE_LOG(ERR, VHOST_CONFIG,
-				"Remove entry from config_ll failed\n");
-			return NULL;
-		}
-	}
+	rte_free(dev);
 }
 
 static void
@@ -353,23 +257,31 @@ reset_device(struct virtio_net *dev)
 int
 vhost_new_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *new_ll_dev;
+	struct virtio_net *dev;
+	int i;
 
-	/* Setup device and virtqueues. */
-	new_ll_dev = rte_zmalloc(NULL, sizeof(struct virtio_net_config_ll), 0);
-	if (new_ll_dev == NULL) {
+	dev = rte_zmalloc(NULL, sizeof(struct virtio_net), 0);
+	if (dev == NULL) {
 		RTE_LOG(ERR, VHOST_CONFIG,
 			"(%"PRIu64") Failed to allocate memory for dev.\n",
 			ctx.fh);
 		return -1;
 	}
 
-	new_ll_dev->next = NULL;
+	for (i = 0; i < MAX_VHOST_DEVICE; i++) {
+		if (vhost_devices[i] == NULL)
+			break;
+	}
+	if (i == MAX_VHOST_DEVICE) {
+		RTE_LOG(ERR, VHOST_CONFIG,
+			"Failed to find a free slot for new device.\n");
+		return -1;
+	}
 
-	/* Add entry to device configuration linked list. */
-	add_config_ll_entry(new_ll_dev);
+	vhost_devices[i] = dev;
+	dev->device_fh   = i;
 
-	return new_ll_dev->dev.device_fh;
+	return i;
 }
 
 /*
@@ -379,30 +291,15 @@ vhost_new_device(struct vhost_device_ctx ctx)
 void
 vhost_destroy_device(struct vhost_device_ctx ctx)
 {
-	struct virtio_net_config_ll *ll_dev_cur_ctx, *ll_dev_last = NULL;
-	struct virtio_net_config_ll *ll_dev_cur = ll_root;
-
-	/* Find the linked list entry for the device to be removed. */
-	ll_dev_cur_ctx = get_config_ll_entry(ctx);
-	while (ll_dev_cur != NULL) {
-		/*
-		 * If the device is found or
-		 * a device that doesn't exist is found then it is removed.
-		 */
-		if (ll_dev_cur == ll_dev_cur_ctx) {
-			/*
-			 * If the device is running on a data core then call
-			 * the function to remove it from the data core.
-			 */
-			if ((ll_dev_cur->dev.flags & VIRTIO_DEV_RUNNING))
-				notify_ops->destroy_device(&(ll_dev_cur->dev));
-			ll_dev_cur = rm_config_ll_entry(ll_dev_cur,
-					ll_dev_last);
-		} else {
-			ll_dev_last = ll_dev_cur;
-			ll_dev_cur = ll_dev_cur->next;
-		}
-	}
+	struct virtio_net *dev = get_device(ctx);
+
+	if (dev->flags & VIRTIO_DEV_RUNNING)
+		notify_ops->destroy_device(dev);
+
+	cleanup_device(dev, 1);
+	free_device(dev);
+
+	vhost_devices[ctx.fh] = NULL;
 }
 
 void
@@ -547,17 +444,17 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net_config_ll *old_ll_dev, *new_ll_dev = NULL;
+	struct virtio_net *old_dev, *new_dev = NULL;
 	struct vhost_virtqueue *old_vq, *new_vq = NULL;
 	int ret;
 	int realloc_dev = 0, realloc_vq = 0;
 
-	old_ll_dev = (struct virtio_net_config_ll *)dev;
-	old_vq = dev->virtqueue[index];
+	old_dev = dev;
+	old_vq  = dev->virtqueue[index];
 
 	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
 			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_ll_dev,
+	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
 			MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
@@ -581,36 +478,30 @@ numa_realloc(struct virtio_net *dev, int index)
 		return dev;
 
 	if (realloc_dev)
-		new_ll_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net_config_ll), 0, newnode);
+		new_dev = rte_malloc_socket(NULL,
+			sizeof(struct virtio_net), 0, newnode);
 	if (realloc_vq)
 		new_vq = rte_malloc_socket(NULL,
 			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_ll_dev && !new_vq)
+	if (!new_dev && !new_vq)
 		return dev;
 
 	if (realloc_vq)
 		memcpy(new_vq, old_vq, sizeof(*new_vq));
 	if (realloc_dev)
-		memcpy(new_ll_dev, old_ll_dev, sizeof(*new_ll_dev));
-	(new_ll_dev ? new_ll_dev : old_ll_dev)->dev.virtqueue[index] =
+		memcpy(new_dev, old_dev, sizeof(*new_dev));
+
+	(new_dev ? new_dev : old_dev)->virtqueue[index] =
 		new_vq ? new_vq : old_vq;
 	if (realloc_vq)
 		rte_free(old_vq);
 	if (realloc_dev) {
-		if (ll_root == old_ll_dev)
-			ll_root = new_ll_dev;
-		else {
-			struct virtio_net_config_ll *prev = ll_root;
-			while (prev->next != old_ll_dev)
-				prev = prev->next;
-			prev->next = new_ll_dev;
-			new_ll_dev->next = old_ll_dev->next;
-		}
-		rte_free(old_ll_dev);
+		rte_free(old_dev);
+
+		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? &new_ll_dev->dev : dev;
+	return realloc_dev ? new_dev : dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH v3 2/3] vhost: simplify numa_realloc

[Yuanhan Liu]

We could first check if we need realloc vq or not, if so,
reallocate it. We then do similar to vhost dev realloc.

This could get rid of the tons of repeated "if (realloc_dev)"
and "if (realloc_vq)" statements, therefore, makes code
a bit more readable.

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
---

v2: fix debug message and check return value of all get_mempolicy()
---
 lib/librte_vhost/virtio-net.c | 82 +++++++++++++++++++++----------------------
 1 file changed, 40 insertions(+), 42 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index be91e5f..764810e 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -444,64 +444,62 @@ static struct virtio_net*
 numa_realloc(struct virtio_net *dev, int index)
 {
 	int oldnode, newnode;
-	struct virtio_net *old_dev, *new_dev = NULL;
-	struct vhost_virtqueue *old_vq, *new_vq = NULL;
+	struct virtio_net *old_dev;
+	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
-	int realloc_dev = 0, realloc_vq = 0;
 
 	old_dev = dev;
-	old_vq  = dev->virtqueue[index];
+	vq = old_vq = dev->virtqueue[index];
 
-	ret  = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
-			MPOL_F_NODE | MPOL_F_ADDR);
-	ret = ret | get_mempolicy(&oldnode, NULL, 0, old_dev,
-			MPOL_F_NODE | MPOL_F_ADDR);
+	ret = get_mempolicy(&newnode, NULL, 0, old_vq->desc,
+			    MPOL_F_NODE | MPOL_F_ADDR);
+
+	/* check if we need to reallocate vq */
+	ret |= get_mempolicy(&oldnode, NULL, 0, old_vq,
+			     MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vring desc or dev numa information.\n");
+			"Unable to get vq numa information.\n");
 		return dev;
 	}
-	if (oldnode != newnode)
-		realloc_dev = 1;
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate vq from %d to %d node\n", oldnode, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		if (!vq)
+			return dev;
+
+		memcpy(vq, old_vq, sizeof(*vq));
+		rte_free(old_vq);
+	}
 
-	ret = get_mempolicy(&oldnode, NULL, 0, old_vq,
-			MPOL_F_NODE | MPOL_F_ADDR);
+	/* check if we need to reallocate dev */
+	ret = get_mempolicy(&oldnode, NULL, 0, old_dev,
+			    MPOL_F_NODE | MPOL_F_ADDR);
 	if (ret) {
 		RTE_LOG(ERR, VHOST_CONFIG,
-			"Unable to get vq numa information.\n");
-		return dev;
+			"Unable to get dev numa information.\n");
+		goto out;
 	}
-	if (oldnode != newnode)
-		realloc_vq = 1;
-
-	if (realloc_dev == 0 && realloc_vq == 0)
-		return dev;
-
-	if (realloc_dev)
-		new_dev = rte_malloc_socket(NULL,
-			sizeof(struct virtio_net), 0, newnode);
-	if (realloc_vq)
-		new_vq = rte_malloc_socket(NULL,
-			sizeof(struct vhost_virtqueue), 0, newnode);
-	if (!new_dev && !new_vq)
-		return dev;
-
-	if (realloc_vq)
-		memcpy(new_vq, old_vq, sizeof(*new_vq));
-	if (realloc_dev)
-		memcpy(new_dev, old_dev, sizeof(*new_dev));
+	if (oldnode != newnode) {
+		RTE_LOG(INFO, VHOST_CONFIG,
+			"reallocate dev from %d to %d node\n",
+			oldnode, newnode);
+		dev = rte_malloc_socket(NULL, sizeof(*dev), 0, newnode);
+		if (!dev) {
+			dev = old_dev;
+			goto out;
+		}
 
-	(new_dev ? new_dev : old_dev)->virtqueue[index] =
-		new_vq ? new_vq : old_vq;
-	if (realloc_vq)
-		rte_free(old_vq);
-	if (realloc_dev) {
+		memcpy(dev, old_dev, sizeof(*dev));
 		rte_free(old_dev);
-
-		vhost_devices[new_dev->device_fh] = new_dev;
 	}
 
-	return realloc_dev ? new_dev : dev;
+out:
+	dev->virtqueue[index] = vq;
+	vhost_devices[dev->device_fh] = dev;
+
+	return dev;
 }
 #else
 static struct virtio_net*
-- 
1.9.0

[dpdk-dev] [PATCH v3 3/3] vhost: fix vq realloc at numa_realloc

[Yuanhan Liu]

vq is allocated on pairs, hence we should do pair reallocation
at numa_realloc() as well, otherwise an error like following
occurs while do numa reallocation:

    VHOST_CONFIG: reallocate vq from 0 to 1 node
    PANIC in rte_free():
    Fatal error: Invalid memory

The reason we don't catch it is because numa_realloc() will
not take effect when RTE_LIBRTE_VHOST_NUMA is not enabled,
which is the default case.

Fixes: e049ca6d10e0 ("vhost-user: prepare multiple queue setup")

Signed-off-by: Yuanhan Liu <yuanhan.liu@linux.intel.com>
Acked-by: Huawei Xie <huawei.xie@intel.com>
Tested-by: Ciara Loftus <ciara.loftus@intel.com>
---
 lib/librte_vhost/virtio-net.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/librte_vhost/virtio-net.c b/lib/librte_vhost/virtio-net.c
index 764810e..28c3912 100644
--- a/lib/librte_vhost/virtio-net.c
+++ b/lib/librte_vhost/virtio-net.c
@@ -448,6 +448,13 @@ numa_realloc(struct virtio_net *dev, int index)
 	struct vhost_virtqueue *old_vq, *vq;
 	int ret;
 
+	/*
+	 * vq is allocated on pairs, we should try to do realloc
+	 * on first queue of one queue pair only.
+	 */
+	if (index % VIRTIO_QNUM != 0)
+		return dev;
+
 	old_dev = dev;
 	vq = old_vq = dev->virtqueue[index];
 
@@ -465,11 +472,12 @@ numa_realloc(struct virtio_net *dev, int index)
 	if (oldnode != newnode) {
 		RTE_LOG(INFO, VHOST_CONFIG,
 			"reallocate vq from %d to %d node\n", oldnode, newnode);
-		vq = rte_malloc_socket(NULL, sizeof(*vq), 0, newnode);
+		vq = rte_malloc_socket(NULL, sizeof(*vq) * VIRTIO_QNUM, 0,
+				       newnode);
 		if (!vq)
 			return dev;
 
-		memcpy(vq, old_vq, sizeof(*vq));
+		memcpy(vq, old_vq, sizeof(*vq) * VIRTIO_QNUM);
 		rte_free(old_vq);
 	}
 
@@ -497,6 +505,7 @@ numa_realloc(struct virtio_net *dev, int index)
 
 out:
 	dev->virtqueue[index] = vq;
+	dev->virtqueue[index + 1] = vq + 1;
 	vhost_devices[dev->device_fh] = dev;
 
 	return dev;
-- 
1.9.0

Re: [dpdk-dev] [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes

[Thomas Monjalon]

> Yuanhan Liu (3):
>   vhost: get rid of linked list dev
>   vhost: simplify numa_realloc
>   vhost: fix vq realloc at numa_realloc

Applied, thanks