* [PATCH 2/3] vhost: simplify numa_realloc
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
@ 2015-12-18 7:04 ` Yuanhan Liu
2015-12-22 6:46 ` Xie, Huawei
2015-12-18 7:04 ` [PATCH 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-18 7:04 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 2/3] vhost: simplify numa_realloc
2015-12-18 7:04 ` [PATCH 2/3] vhost: simplify numa_realloc Yuanhan Liu
@ 2015-12-22 6:46 ` Xie, Huawei
2015-12-22 6:52 ` Yuanhan Liu
0 siblings, 1 reply; 19+ messages in thread
From: Xie, Huawei @ 2015-12-22 6:46 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org
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
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 2/3] vhost: simplify numa_realloc
2015-12-22 6:46 ` Xie, Huawei
@ 2015-12-22 6:52 ` Yuanhan Liu
0 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-22 6:52 UTC (permalink / raw)
To: Xie, Huawei; +Cc: dev@dpdk.org
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 3/3] vhost: fix vq realloc at numa_realloc
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
2015-12-18 7:04 ` [PATCH 2/3] vhost: simplify numa_realloc Yuanhan Liu
@ 2015-12-18 7:04 ` Yuanhan Liu
2015-12-22 6:56 ` Xie, Huawei
2015-12-22 3:33 ` [PATCH 1/3] vhost: get rid of linked list dev Xie, Huawei
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-18 7:04 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH 3/3] vhost: fix vq realloc at numa_realloc
2015-12-18 7:04 ` [PATCH 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
@ 2015-12-22 6:56 ` Xie, Huawei
0 siblings, 0 replies; 19+ messages in thread
From: Xie, Huawei @ 2015-12-22 6:56 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org
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>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] vhost: get rid of linked list dev
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
2015-12-18 7:04 ` [PATCH 2/3] vhost: simplify numa_realloc Yuanhan Liu
2015-12-18 7:04 ` [PATCH 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
@ 2015-12-22 3:33 ` Xie, Huawei
2015-12-22 6:21 ` Xie, Huawei
` (2 subsequent siblings)
5 siblings, 0 replies; 19+ messages in thread
From: Xie, Huawei @ 2015-12-22 3:33 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org, Long, Thomas
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.
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH 1/3] vhost: get rid of linked list dev
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
` (2 preceding siblings ...)
2015-12-22 3:33 ` [PATCH 1/3] vhost: get rid of linked list dev Xie, Huawei
@ 2015-12-22 6:21 ` Xie, Huawei
2015-12-22 7:28 ` [PATCH v2 " Yuanhan Liu
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
5 siblings, 0 replies; 19+ messages in thread
From: Xie, Huawei @ 2015-12-22 6:21 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org
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>
[...]
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v2 1/3] vhost: get rid of linked list dev
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
` (3 preceding siblings ...)
2015-12-22 6:21 ` Xie, Huawei
@ 2015-12-22 7:28 ` Yuanhan Liu
2015-12-22 7:28 ` [PATCH v2 2/3] vhost: simplify numa_realloc Yuanhan Liu
2015-12-22 7:28 ` [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
5 siblings, 2 replies; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-22 7:28 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 2/3] vhost: simplify numa_realloc
2015-12-22 7:28 ` [PATCH v2 " Yuanhan Liu
@ 2015-12-22 7:28 ` Yuanhan Liu
2015-12-22 14:40 ` Xie, Huawei
2015-12-22 7:28 ` [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
1 sibling, 1 reply; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-22 7:28 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc
2015-12-22 7:28 ` [PATCH v2 " Yuanhan Liu
2015-12-22 7:28 ` [PATCH v2 2/3] vhost: simplify numa_realloc Yuanhan Liu
@ 2015-12-22 7:28 ` Yuanhan Liu
2016-03-07 13:49 ` Loftus, Ciara
1 sibling, 1 reply; 19+ messages in thread
From: Yuanhan Liu @ 2015-12-22 7:28 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc
2015-12-22 7:28 ` [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
@ 2016-03-07 13:49 ` Loftus, Ciara
2016-03-08 11:54 ` Yuanhan Liu
0 siblings, 1 reply; 19+ messages in thread
From: Loftus, Ciara @ 2016-03-07 13:49 UTC (permalink / raw)
To: Yuanhan Liu, dev@dpdk.org
>
> 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
^ permalink raw reply [flat|nested] 19+ messages in thread* Re: [PATCH v2 3/3] vhost: fix vq realloc at numa_realloc
2016-03-07 13:49 ` Loftus, Ciara
@ 2016-03-08 11:54 ` Yuanhan Liu
0 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2016-03-08 11:54 UTC (permalink / raw)
To: Loftus, Ciara; +Cc: dev@dpdk.org
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
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes
2015-12-18 7:04 [PATCH 1/3] vhost: get rid of linked list dev Yuanhan Liu
` (4 preceding siblings ...)
2015-12-22 7:28 ` [PATCH v2 " Yuanhan Liu
@ 2016-03-10 4:19 ` Yuanhan Liu
2016-03-10 4:19 ` [PATCH v3 1/3] vhost: get rid of linked list dev Yuanhan Liu
` (3 more replies)
5 siblings, 4 replies; 19+ messages in thread
From: Yuanhan Liu @ 2016-03-10 4:19 UTC (permalink / raw)
To: dev
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
^ permalink raw reply [flat|nested] 19+ messages in thread* [PATCH v3 1/3] vhost: get rid of linked list dev
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
@ 2016-03-10 4:19 ` Yuanhan Liu
2016-03-10 4:20 ` [PATCH v3 2/3] vhost: simplify numa_realloc Yuanhan Liu
` (2 subsequent siblings)
3 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2016-03-10 4:19 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 2/3] vhost: simplify numa_realloc
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
2016-03-10 4:19 ` [PATCH v3 1/3] vhost: get rid of linked list dev Yuanhan Liu
@ 2016-03-10 4:20 ` Yuanhan Liu
2016-03-10 4:20 ` [PATCH v3 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
2016-03-11 15:35 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Thomas Monjalon
3 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2016-03-10 4:20 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* [PATCH v3 3/3] vhost: fix vq realloc at numa_realloc
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
2016-03-10 4:19 ` [PATCH v3 1/3] vhost: get rid of linked list dev Yuanhan Liu
2016-03-10 4:20 ` [PATCH v3 2/3] vhost: simplify numa_realloc Yuanhan Liu
@ 2016-03-10 4:20 ` Yuanhan Liu
2016-03-11 15:35 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Thomas Monjalon
3 siblings, 0 replies; 19+ messages in thread
From: Yuanhan Liu @ 2016-03-10 4:20 UTC (permalink / raw)
To: dev
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
^ permalink raw reply related [flat|nested] 19+ messages in thread* Re: [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes
2016-03-10 4:19 ` [PATCH v3 0/3] vhost: virtio-net.c cleanups and fixes Yuanhan Liu
` (2 preceding siblings ...)
2016-03-10 4:20 ` [PATCH v3 3/3] vhost: fix vq realloc at numa_realloc Yuanhan Liu
@ 2016-03-11 15:35 ` Thomas Monjalon
3 siblings, 0 replies; 19+ messages in thread
From: Thomas Monjalon @ 2016-03-11 15:35 UTC (permalink / raw)
To: Yuanhan Liu; +Cc: dev
> Yuanhan Liu (3):
> vhost: get rid of linked list dev
> vhost: simplify numa_realloc
> vhost: fix vq realloc at numa_realloc
Applied, thanks
^ permalink raw reply [flat|nested] 19+ messages in thread