diff mbox

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

Message ID 1450422247-6814-2-git-send-email-yuanhan.liu@linux.intel.com (mailing list archive)
State Superseded, archived
Headers

Commit Message

Yuanhan Liu Dec. 18, 2015, 7:04 a.m. UTC 
  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(-)

Comments

Huawei Xie Dec. 22, 2015, 6:46 a.m. UTC | #1 
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
[...]
Yuanhan Liu Dec. 22, 2015, 6:52 a.m. UTC | #2 
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
diff mbox

Patch

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*