* [PATCH 1/1] staging: hv: Fix error checking in channel.c
From: Haiyang Zhang @ 2010-05-13 15:56 UTC (permalink / raw)
To: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
"'virtualization@lists.osdl.org'" <virtualiz>
[-- Attachment #1: Type: text/plain, Size: 1613 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: staging: hv: Fix error checking in channel.c
Fixed errors in return value checking code, which caused vmbus channel
not functioning.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
index 12c351e..f047c5a 100644
--- a/drivers/staging/hv/channel.c
+++ b/drivers/staging/hv/channel.c
@@ -204,13 +204,13 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize) >> PAGE_SHIFT;
ret = RingBufferInit(&NewChannel->Outbound, out, SendRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
ret = RingBufferInit(&NewChannel->Inbound, in, RecvRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -228,7 +228,7 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize,
&NewChannel->RingBufferGpadlHandle);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -569,7 +569,7 @@ int VmbusChannelEstablishGpadl(struct vmbus_channel *Channel, void *Kbuffer,
ret = VmbusPostMessage(gpadlBody,
subMsgInfo->MessageSize -
sizeof(*subMsgInfo));
- if (!ret)
+ if (ret != 0)
goto Cleanup;
}
--
1.6.3.2
[-- Attachment #2: 0513-Fix-error-checking-in-channel.c.patch --]
[-- Type: application/octet-stream, Size: 1558 bytes --]
From: Haiyang Zhang <haiyangz@microsoft.com>
Subject: staging: hv: Fix error checking in channel.c
Fixed errors in return value checking code, which caused vmbus channel
not functioning.
Cc: Greg Kroah-Hartman <gregkh@suse.de>
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
index 12c351e..f047c5a 100644
--- a/drivers/staging/hv/channel.c
+++ b/drivers/staging/hv/channel.c
@@ -204,13 +204,13 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize) >> PAGE_SHIFT;
ret = RingBufferInit(&NewChannel->Outbound, out, SendRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
ret = RingBufferInit(&NewChannel->Inbound, in, RecvRingBufferSize);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -228,7 +228,7 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
RecvRingBufferSize,
&NewChannel->RingBufferGpadlHandle);
- if (!ret) {
+ if (ret != 0) {
err = ret;
goto errorout;
}
@@ -569,7 +569,7 @@ int VmbusChannelEstablishGpadl(struct vmbus_channel *Channel, void *Kbuffer,
ret = VmbusPostMessage(gpadlBody,
subMsgInfo->MessageSize -
sizeof(*subMsgInfo));
- if (!ret)
+ if (ret != 0)
goto Cleanup;
}
--
1.6.3.2
[-- Attachment #3: Type: text/plain, Size: 184 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH 1/1] staging: hv: Fix error checking in channel.c
From: Greg KH @ 2010-05-13 16:33 UTC (permalink / raw)
To: Haiyang Zhang, Bill Pemberton
Cc: 'linux-kernel@vger.kernel.org',
'devel@driverdev.osuosl.org',
'virtualization@lists.osdl.org', Hank Janssen
In-Reply-To: <1FB5E1D5CA062146B38059374562DF72662AB0C9@TK5EX14MBXC130.redmond.corp.microsoft.com>
On Thu, May 13, 2010 at 03:56:30PM +0000, Haiyang Zhang wrote:
> From: Haiyang Zhang <haiyangz@microsoft.com>
>
> Subject: staging: hv: Fix error checking in channel.c
> Fixed errors in return value checking code, which caused vmbus channel
> not functioning.
Doh, sorry about that. Bill, be a bit more careful in the future
please, this came in on your ASSERT cleanup patch.
thanks,
greg k-h
> ---
> drivers/staging/hv/channel.c | 8 ++++----
> 1 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/staging/hv/channel.c b/drivers/staging/hv/channel.c
> index 12c351e..f047c5a 100644
> --- a/drivers/staging/hv/channel.c
> +++ b/drivers/staging/hv/channel.c
> @@ -204,13 +204,13 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
> RecvRingBufferSize) >> PAGE_SHIFT;
>
> ret = RingBufferInit(&NewChannel->Outbound, out, SendRingBufferSize);
> - if (!ret) {
> + if (ret != 0) {
> err = ret;
> goto errorout;
> }
>
> ret = RingBufferInit(&NewChannel->Inbound, in, RecvRingBufferSize);
> - if (!ret) {
> + if (ret != 0) {
> err = ret;
> goto errorout;
> }
> @@ -228,7 +228,7 @@ int VmbusChannelOpen(struct vmbus_channel *NewChannel, u32 SendRingBufferSize,
> RecvRingBufferSize,
> &NewChannel->RingBufferGpadlHandle);
>
> - if (!ret) {
> + if (ret != 0) {
> err = ret;
> goto errorout;
> }
> @@ -569,7 +569,7 @@ int VmbusChannelEstablishGpadl(struct vmbus_channel *Channel, void *Kbuffer,
> ret = VmbusPostMessage(gpadlBody,
> subMsgInfo->MessageSize -
> sizeof(*subMsgInfo));
> - if (!ret)
> + if (ret != 0)
> goto Cleanup;
>
> }
> --
> 1.6.3.2
>
^ permalink raw reply
* Re: [GIT PULL] last minute vhost-net fix
From: David Miller @ 2010-05-14 11:04 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20100513084433.GA23082@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Thu, 13 May 2010 11:44:34 +0300
> David, if it's not too late, please pull the following
> last minute fix into 2.6.34.
Pulled, thanks.
^ permalink raw reply
* [PATCH RFC] virtio_blk: Use blk-iopoll for host->guest notify
From: Stefan Hajnoczi @ 2010-05-14 20:47 UTC (permalink / raw)
To: virtualization; +Cc: Stefan Hajnoczi, kvm, qemu-devel, linux-kernel, Jens Axboe
This patch adds blk-iopoll interrupt mitigation to virtio-blk. Instead
of processing completed requests inside the virtqueue interrupt handler,
a softirq is scheduled to process up to a maximum number of completed
requests in one go.
If the number of complete requests exceeds the maximum number, then another
softirq is scheduled to continue polling. Otherwise the virtqueue interrupt is
enabled again and we return to interrupt-driven mode.
The patch sets the maximum number of completed requests (aka budget, aka
weight) to 4. This is a low number but reflects the expensive context
switch between guest and host virtio-blk emulation.
The blk-iopoll infrastructure is enabled system-wide by default:
kernel.blk_iopoll = 1
It can be disabled to always use interrupt-driven mode (useful for comparison):
kernel.blk_iopoll = 0
Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
---
No performance figures yet.
drivers/block/virtio_blk.c | 71 ++++++++++++++++++++++++++++++++++++++-----
1 files changed, 62 insertions(+), 9 deletions(-)
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
index 2138a7a..1523895 100644
--- a/drivers/block/virtio_blk.c
+++ b/drivers/block/virtio_blk.c
@@ -6,6 +6,7 @@
#include <linux/virtio.h>
#include <linux/virtio_blk.h>
#include <linux/scatterlist.h>
+#include <linux/blk-iopoll.h>
#define PART_BITS 4
@@ -26,6 +27,9 @@ struct virtio_blk
mempool_t *pool;
+ /* Host->guest notify mitigation */
+ struct blk_iopoll iopoll;
+
/* What host tells us, plus 2 for header & tailer. */
unsigned int sg_elems;
@@ -42,16 +46,18 @@ struct virtblk_req
u8 status;
};
-static void blk_done(struct virtqueue *vq)
+/* Assumes vblk->lock held */
+static int __virtblk_end_requests(struct virtio_blk *vblk, int weight)
{
- struct virtio_blk *vblk = vq->vdev->priv;
struct virtblk_req *vbr;
unsigned int len;
- unsigned long flags;
+ int error;
+ int work = 0;
- spin_lock_irqsave(&vblk->lock, flags);
- while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len)) != NULL) {
- int error;
+ while (!weight || work < weight) {
+ vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len);
+ if (!vbr)
+ break;
switch (vbr->status) {
case VIRTIO_BLK_S_OK:
@@ -74,10 +80,53 @@ static void blk_done(struct virtqueue *vq)
__blk_end_request_all(vbr->req, error);
list_del(&vbr->list);
mempool_free(vbr, vblk->pool);
+ work++;
}
+
/* In case queue is stopped waiting for more buffers. */
blk_start_queue(vblk->disk->queue);
+ return work;
+}
+
+static int virtblk_iopoll(struct blk_iopoll *iopoll, int weight)
+{
+ struct virtio_blk *vblk =
+ container_of(iopoll, struct virtio_blk, iopoll);
+ unsigned long flags;
+ int work;
+
+ spin_lock_irqsave(&vblk->lock, flags);
+
+ work = __virtblk_end_requests(vblk, weight);
+ if (work < weight) {
+ /* Keep polling if there are pending requests. */
+ if (vblk->vq->vq_ops->enable_cb(vblk->vq))
+ __blk_iopoll_complete(&vblk->iopoll);
+ else
+ vblk->vq->vq_ops->disable_cb(vblk->vq);
+ }
+
spin_unlock_irqrestore(&vblk->lock, flags);
+ return work;
+}
+
+static void blk_done(struct virtqueue *vq)
+{
+ struct virtio_blk *vblk = vq->vdev->priv;
+ unsigned long flags;
+
+ if (blk_iopoll_enabled) {
+ if (!blk_iopoll_sched_prep(&vblk->iopoll)) {
+ spin_lock_irqsave(&vblk->lock, flags);
+ vblk->vq->vq_ops->disable_cb(vblk->vq);
+ spin_unlock_irqrestore(&vblk->lock, flags);
+ blk_iopoll_sched(&vblk->iopoll);
+ }
+ } else {
+ spin_lock_irqsave(&vblk->lock, flags);
+ __virtblk_end_requests(vblk, 0);
+ spin_unlock_irqrestore(&vblk->lock, flags);
+ }
}
static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
@@ -289,11 +338,14 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
goto out_free_vq;
}
+ blk_iopoll_init(&vblk->iopoll, 4 /* budget */, virtblk_iopoll);
+ blk_iopoll_enable(&vblk->iopoll);
+
/* FIXME: How many partitions? How long is a piece of string? */
vblk->disk = alloc_disk(1 << PART_BITS);
if (!vblk->disk) {
err = -ENOMEM;
- goto out_mempool;
+ goto out_iopoll;
}
q = vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
@@ -401,13 +453,13 @@ static int __devinit virtblk_probe(struct virtio_device *vdev)
if (!err && opt_io_size)
blk_queue_io_opt(q, blk_size * opt_io_size);
-
add_disk(vblk->disk);
return 0;
out_put_disk:
put_disk(vblk->disk);
-out_mempool:
+out_iopoll:
+ blk_iopoll_disable(&vblk->iopoll);
mempool_destroy(vblk->pool);
out_free_vq:
vdev->config->del_vqs(vdev);
@@ -430,6 +482,7 @@ static void __devexit virtblk_remove(struct virtio_device *vdev)
del_gendisk(vblk->disk);
blk_cleanup_queue(vblk->disk->queue);
put_disk(vblk->disk);
+ blk_iopoll_disable(&vblk->iopoll);
mempool_destroy(vblk->pool);
vdev->config->del_vqs(vdev);
kfree(vblk);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH RFC] virtio_blk: Use blk-iopoll for host->guest notify
From: Brian Jackson @ 2010-05-14 22:30 UTC (permalink / raw)
To: Stefan Hajnoczi; +Cc: kvm, qemu-devel, linux-kernel, Jens Axboe, virtualization
In-Reply-To: <1273870057-4287-1-git-send-email-stefanha@linux.vnet.ibm.com>
On Friday, May 14, 2010 03:47:37 pm Stefan Hajnoczi wrote:
> This patch adds blk-iopoll interrupt mitigation to virtio-blk. Instead
> of processing completed requests inside the virtqueue interrupt handler,
> a softirq is scheduled to process up to a maximum number of completed
> requests in one go.
>
> If the number of complete requests exceeds the maximum number, then another
> softirq is scheduled to continue polling. Otherwise the virtqueue
> interrupt is enabled again and we return to interrupt-driven mode.
>
> The patch sets the maximum number of completed requests (aka budget, aka
> weight) to 4. This is a low number but reflects the expensive context
> switch between guest and host virtio-blk emulation.
>
> The blk-iopoll infrastructure is enabled system-wide by default:
>
> kernel.blk_iopoll = 1
>
> It can be disabled to always use interrupt-driven mode (useful for
> comparison):
>
> kernel.blk_iopoll = 0
Any preliminary numbers? latency, throughput, cpu use? What about comparing
different "weights"?
>
> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> ---
> No performance figures yet.
>
> drivers/block/virtio_blk.c | 71
> ++++++++++++++++++++++++++++++++++++++----- 1 files changed, 62
> insertions(+), 9 deletions(-)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index 2138a7a..1523895 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -6,6 +6,7 @@
> #include <linux/virtio.h>
> #include <linux/virtio_blk.h>
> #include <linux/scatterlist.h>
> +#include <linux/blk-iopoll.h>
>
> #define PART_BITS 4
>
> @@ -26,6 +27,9 @@ struct virtio_blk
>
> mempool_t *pool;
>
> + /* Host->guest notify mitigation */
> + struct blk_iopoll iopoll;
> +
> /* What host tells us, plus 2 for header & tailer. */
> unsigned int sg_elems;
>
> @@ -42,16 +46,18 @@ struct virtblk_req
> u8 status;
> };
>
> -static void blk_done(struct virtqueue *vq)
> +/* Assumes vblk->lock held */
> +static int __virtblk_end_requests(struct virtio_blk *vblk, int weight)
> {
> - struct virtio_blk *vblk = vq->vdev->priv;
> struct virtblk_req *vbr;
> unsigned int len;
> - unsigned long flags;
> + int error;
> + int work = 0;
>
> - spin_lock_irqsave(&vblk->lock, flags);
> - while ((vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len)) != NULL) {
> - int error;
> + while (!weight || work < weight) {
> + vbr = vblk->vq->vq_ops->get_buf(vblk->vq, &len);
> + if (!vbr)
> + break;
>
> switch (vbr->status) {
> case VIRTIO_BLK_S_OK:
> @@ -74,10 +80,53 @@ static void blk_done(struct virtqueue *vq)
> __blk_end_request_all(vbr->req, error);
> list_del(&vbr->list);
> mempool_free(vbr, vblk->pool);
> + work++;
> }
> +
> /* In case queue is stopped waiting for more buffers. */
> blk_start_queue(vblk->disk->queue);
> + return work;
> +}
> +
> +static int virtblk_iopoll(struct blk_iopoll *iopoll, int weight)
> +{
> + struct virtio_blk *vblk =
> + container_of(iopoll, struct virtio_blk, iopoll);
> + unsigned long flags;
> + int work;
> +
> + spin_lock_irqsave(&vblk->lock, flags);
> +
> + work = __virtblk_end_requests(vblk, weight);
> + if (work < weight) {
> + /* Keep polling if there are pending requests. */
> + if (vblk->vq->vq_ops->enable_cb(vblk->vq))
> + __blk_iopoll_complete(&vblk->iopoll);
> + else
> + vblk->vq->vq_ops->disable_cb(vblk->vq);
> + }
> +
> spin_unlock_irqrestore(&vblk->lock, flags);
> + return work;
> +}
> +
> +static void blk_done(struct virtqueue *vq)
> +{
> + struct virtio_blk *vblk = vq->vdev->priv;
> + unsigned long flags;
> +
> + if (blk_iopoll_enabled) {
> + if (!blk_iopoll_sched_prep(&vblk->iopoll)) {
> + spin_lock_irqsave(&vblk->lock, flags);
> + vblk->vq->vq_ops->disable_cb(vblk->vq);
> + spin_unlock_irqrestore(&vblk->lock, flags);
> + blk_iopoll_sched(&vblk->iopoll);
> + }
> + } else {
> + spin_lock_irqsave(&vblk->lock, flags);
> + __virtblk_end_requests(vblk, 0);
> + spin_unlock_irqrestore(&vblk->lock, flags);
> + }
> }
>
> static bool do_req(struct request_queue *q, struct virtio_blk *vblk,
> @@ -289,11 +338,14 @@ static int __devinit virtblk_probe(struct
> virtio_device *vdev) goto out_free_vq;
> }
>
> + blk_iopoll_init(&vblk->iopoll, 4 /* budget */, virtblk_iopoll);
> + blk_iopoll_enable(&vblk->iopoll);
> +
> /* FIXME: How many partitions? How long is a piece of string? */
> vblk->disk = alloc_disk(1 << PART_BITS);
> if (!vblk->disk) {
> err = -ENOMEM;
> - goto out_mempool;
> + goto out_iopoll;
> }
>
> q = vblk->disk->queue = blk_init_queue(do_virtblk_request, &vblk->lock);
> @@ -401,13 +453,13 @@ static int __devinit virtblk_probe(struct
> virtio_device *vdev) if (!err && opt_io_size)
> blk_queue_io_opt(q, blk_size * opt_io_size);
>
> -
> add_disk(vblk->disk);
> return 0;
>
> out_put_disk:
> put_disk(vblk->disk);
> -out_mempool:
> +out_iopoll:
> + blk_iopoll_disable(&vblk->iopoll);
> mempool_destroy(vblk->pool);
> out_free_vq:
> vdev->config->del_vqs(vdev);
> @@ -430,6 +482,7 @@ static void __devexit virtblk_remove(struct
> virtio_device *vdev) del_gendisk(vblk->disk);
> blk_cleanup_queue(vblk->disk->queue);
> put_disk(vblk->disk);
> + blk_iopoll_disable(&vblk->iopoll);
> mempool_destroy(vblk->pool);
> vdev->config->del_vqs(vdev);
> kfree(vblk);
^ permalink raw reply
* [PATCH 1/4] drivers/char: Eliminate use after free
From: Julia Lawall @ 2010-05-15 9:45 UTC (permalink / raw)
To: Amit Shah, virtualization, linux-kernel, kernel-janitors
From: Julia Lawall <julia@diku.dk>
In each case, the first argument to send_control_msg or __send_control_msg,
respectively, has either not been successfully allocated or has been freed
at the point of the call. In the first case, the first argument, port, is
only used to access the portdev and id fields, in order to call
__send_control_msg. Thus it seems possible instead to call
__send_control_msg directly. In the second case, the call to
__send_control_msg is moved up to a place where it seems like the first
argument, portdev, has been initialized sufficiently to make the call to
__send_control_msg meaningful.
This has only been compile tested.
A simplified version of the semantic match that finds this problem is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@free@
expression E;
position p;
@@
kfree@p(E)
@@
expression free.E, subE<=free.E, E1;
position free.p;
@@
kfree@p(E)
...
(
subE = E1
|
* E
)
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/char/virtio_console.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 458d907..8c99bf1 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1090,7 +1090,7 @@ free_port:
kfree(port);
fail:
/* The host might want to notify management sw about port add failure */
- send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
+ __send_control_msg(portdev, id, VIRTIO_CONSOLE_PORT_READY, 0);
return err;
}
@@ -1559,6 +1559,9 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
return 0;
free_vqs:
+ /* The host might want to notify mgmt sw about device add failure */
+ __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
+ VIRTIO_CONSOLE_DEVICE_READY, 0);
vdev->config->del_vqs(vdev);
kfree(portdev->in_vqs);
kfree(portdev->out_vqs);
@@ -1567,9 +1570,6 @@ free_chrdev:
free:
kfree(portdev);
fail:
- /* The host might want to notify mgmt sw about device add failure */
- __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
- VIRTIO_CONSOLE_DEVICE_READY, 0);
return err;
}
^ permalink raw reply related
* [PATCH 4/37] drivers/block: Use kmemdup
From: Julia Lawall @ 2010-05-15 21:14 UTC (permalink / raw)
To: Jeremy Fitzhardinge, Chris Wright, virtualization, xen-devel,
linux-kernel, kernel-jani
From: Julia Lawall <julia@diku.dk>
Use kmemdup when some other buffer is immediately copied into the
allocated region.
A simplified version of the semantic patch that makes this change is as
follows: (http://coccinelle.lip6.fr/)
// <smpl>
@@
expression from,to,size,flag;
statement S;
@@
- to = \(kmalloc\|kzalloc\)(size,flag);
+ to = kmemdup(from,size,flag);
if (to==NULL || ...) S
- memcpy(to, from, size);
// </smpl>
Signed-off-by: Julia Lawall <julia@diku.dk>
---
drivers/block/xen-blkfront.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff -u -p a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
--- a/drivers/block/xen-blkfront.c
+++ b/drivers/block/xen-blkfront.c
@@ -775,11 +775,10 @@ static int blkif_recover(struct blkfront
int j;
/* Stage 1: Make a safe copy of the shadow state. */
- copy = kmalloc(sizeof(info->shadow),
+ copy = kmemdup(info->shadow, sizeof(info->shadow),
GFP_NOIO | __GFP_REPEAT | __GFP_HIGH);
if (!copy)
return -ENOMEM;
- memcpy(copy, info->shadow, sizeof(info->shadow));
/* Stage 2: Set up free list. */
memset(&info->shadow, 0, sizeof(info->shadow));
^ permalink raw reply
* [PATCH 1/1] staging: hv: Added heartbeat functionality to hv_utils
From: Hank Janssen @ 2010-05-15 21:39 UTC (permalink / raw)
To: hjanssen, linux-kernel, devel, virtualization, gregkh, haiyangz
In-Reply-To: <4BE0A5C9.2020409@sailtheuniverse.com>
From: Hank Janssen <hjanssen@microsoft.com>
Subject: [PATCH 1/1] staging: hv: Added heartbeat functionality to hv_utils
Add heartbeat functionality to hv_utils/Hyper-V
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
Signed-off-by: Hank Janssen <hjanssen@microsoft.com>
---
drivers/staging/hv/channel_mgmt.c | 23 ++++++++++++-
drivers/staging/hv/hv_utils.c | 64 +++++++++++++++++++++++++++++++++++++
drivers/staging/hv/utils.h | 5 +++
drivers/staging/hv/version_info.h | 3 +-
4 files changed, 92 insertions(+), 3 deletions(-)
diff --git a/drivers/staging/hv/channel_mgmt.c b/drivers/staging/hv/channel_mgmt.c
index 6877e8e..3f53b4d 100644
--- a/drivers/staging/hv/channel_mgmt.c
+++ b/drivers/staging/hv/channel_mgmt.c
@@ -33,8 +33,8 @@ struct vmbus_channel_message_table_entry {
void (*messageHandler)(struct vmbus_channel_message_header *msg);
};
-#define MAX_MSG_TYPES 2
-#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 6
+#define MAX_MSG_TYPES 3
+#define MAX_NUM_DEVICE_CLASSES_SUPPORTED 7
static const struct hv_guid
gSupportedDeviceClasses[MAX_NUM_DEVICE_CLASSES_SUPPORTED] = {
@@ -89,6 +89,14 @@ static const struct hv_guid
0xad, 0xce, 0xe8, 0x0a, 0xb0, 0x17, 0x5c, 0xaf
}
},
+ /* {57164f39-9115-4e78-ab55-382f3bd5422d} */
+ /* Heartbeat */
+ {
+ .data = {
+ 0x39, 0x4f, 0x16, 0x57, 0x15, 0x91, 0x78, 0x4e,
+ 0xab, 0x55, 0x38, 0x2f, 0x3b, 0xd5, 0x42, 0x2d
+ }
+ },
};
@@ -211,6 +219,17 @@ struct hyperv_service_callback hv_cb_utils[MAX_MSG_TYPES] = {
.callback = chn_cb_negotiate,
.log_msg = "Timesync channel functionality initialized"
},
+ /* {57164f39-9115-4e78-ab55-382f3bd5422d} */
+ /* Heartbeat */
+ {
+ .msg_type = HV_HEARTBEAT_MSG,
+ .data = {
+ 0x39, 0x4f, 0x16, 0x57, 0x15, 0x91, 0x78, 0x4e,
+ 0xab, 0x55, 0x38, 0x2f, 0x3b, 0xd5, 0x42, 0x2d
+ },
+ .callback = chn_cb_negotiate,
+ .log_msg = "Heartbeat channel functionality initialized"
+ },
};
EXPORT_SYMBOL(hv_cb_utils);
diff --git a/drivers/staging/hv/hv_utils.c b/drivers/staging/hv/hv_utils.c
index db45d97..8a49aaf 100644
--- a/drivers/staging/hv/hv_utils.c
+++ b/drivers/staging/hv/hv_utils.c
@@ -194,6 +194,62 @@ static void timesync_onchannelcallback(void *context)
DPRINT_EXIT(VMBUS);
}
+/*
+ * Heartbeat functionality.
+ * Every two seconds, Hyper-V send us a heartbeat request message.
+ * we respond to this message, and Hyper-V knows we are alive.
+ */
+static void heartbeat_onchannelcallback(void *context)
+{
+ struct vmbus_channel *channel = context;
+ u8 *buf;
+ u32 buflen, recvlen;
+ u64 requestid;
+ struct icmsg_hdr *icmsghdrp;
+ struct heartbeat_msg_data *heartbeat_msg;
+
+ DPRINT_ENTER(VMBUS);
+
+ buflen = PAGE_SIZE;
+ buf = kmalloc(buflen, GFP_ATOMIC);
+
+ VmbusChannelRecvPacket(channel, buf, buflen, &recvlen, &requestid);
+
+ if (recvlen > 0) {
+ DPRINT_DBG(VMBUS, "heartbeat packet: len=%d, requestid=%lld",
+ recvlen, requestid);
+
+ icmsghdrp = (struct icmsg_hdr *)&buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ icmsghdrp = (struct icmsg_hdr *)&buf[
+ sizeof(struct vmbuspipe_hdr)];
+
+ if (icmsghdrp->icmsgtype == ICMSGTYPE_NEGOTIATE) {
+ prep_negotiate_resp(icmsghdrp, NULL, buf);
+ } else {
+ heartbeat_msg = (struct heartbeat_msg_data *)&buf[
+ sizeof(struct vmbuspipe_hdr) +
+ sizeof(struct icmsg_hdr)];
+
+ DPRINT_DBG(VMBUS, "heartbeat seq = %lld",
+ heartbeat_msg->seq_num);
+
+ heartbeat_msg->seq_num += 1;
+ }
+
+ icmsghdrp->icflags = ICMSGHDRFLAG_TRANSACTION
+ | ICMSGHDRFLAG_RESPONSE;
+
+ VmbusChannelSendPacket(channel, buf,
+ recvlen, requestid,
+ VmbusPacketTypeDataInBand, 0);
+ }
+
+ kfree(buf);
+
+ DPRINT_EXIT(VMBUS);
+}
static int __init init_hyperv_utils(void)
{
@@ -207,6 +263,10 @@ static int __init init_hyperv_utils(void)
×ync_onchannelcallback;
hv_cb_utils[HV_TIMESYNC_MSG].callback = ×ync_onchannelcallback;
+ hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
+ &heartbeat_onchannelcallback;
+ hv_cb_utils[HV_HEARTBEAT_MSG].callback = &heartbeat_onchannelcallback;
+
return 0;
}
@@ -221,6 +281,10 @@ static void exit_hyperv_utils(void)
hv_cb_utils[HV_TIMESYNC_MSG].channel->OnChannelCallback =
&chn_cb_negotiate;
hv_cb_utils[HV_TIMESYNC_MSG].callback = &chn_cb_negotiate;
+
+ hv_cb_utils[HV_HEARTBEAT_MSG].channel->OnChannelCallback =
+ &chn_cb_negotiate;
+ hv_cb_utils[HV_HEARTBEAT_MSG].callback = &chn_cb_negotiate;
}
module_init(init_hyperv_utils);
diff --git a/drivers/staging/hv/utils.h b/drivers/staging/hv/utils.h
index a4b9fd0..7c07499 100644
--- a/drivers/staging/hv/utils.h
+++ b/drivers/staging/hv/utils.h
@@ -75,6 +75,10 @@ struct shutdown_msg_data {
u8 display_message[2048];
} __attribute__((packed));
+struct heartbeat_msg_data {
+ u64 seq_num;
+ u32 reserved[8];
+} __attribute__((packed));
/* Time Sync IC defs */
#define ICTIMESYNCFLAG_PROBE 0
@@ -97,6 +101,7 @@ struct ictimesync_data{
/* Index for each IC struct in array hv_cb_utils[] */
#define HV_SHUTDOWN_MSG 0
#define HV_TIMESYNC_MSG 1
+#define HV_HEARTBEAT_MSG 2
struct hyperv_service_callback {
u8 msg_type;
diff --git a/drivers/staging/hv/version_info.h b/drivers/staging/hv/version_info.h
index 82e74b1..35178f2 100644
--- a/drivers/staging/hv/version_info.h
+++ b/drivers/staging/hv/version_info.h
@@ -40,8 +40,9 @@
* Minor Number Changes when new functionality is added
* to the Linux IC's that is not a bug fix.
*
+ * 3.1 - Added completed hv_utils driver. Shutdown/Heartbeat/Timesync
*/
-#define HV_DRV_VERSION "3.0"
+#define HV_DRV_VERSION "3.1"
#endif
--
1.6.0.2
^ permalink raw reply related
* Re: [PATCH 1/4] drivers/char: Eliminate use after free
From: Amit Shah @ 2010-05-17 9:01 UTC (permalink / raw)
To: Julia Lawall; +Cc: kernel-janitors, linux-kernel, virtualization
In-Reply-To: <Pine.LNX.4.64.1005151145340.15566@ask.diku.dk>
On (Sat) May 15 2010 [11:45:53], Julia Lawall wrote:
> From: Julia Lawall <julia@diku.dk>
>
> In each case, the first argument to send_control_msg or __send_control_msg,
> respectively, has either not been successfully allocated or has been freed
> at the point of the call. In the first case, the first argument, port, is
> only used to access the portdev and id fields, in order to call
> __send_control_msg. Thus it seems possible instead to call
> __send_control_msg directly. In the second case, the call to
> __send_control_msg is moved up to a place where it seems like the first
> argument, portdev, has been initialized sufficiently to make the call to
> __send_control_msg meaningful.
>
> This has only been compile tested.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> @free@
> expression E;
> position p;
> @@
> kfree@p(E)
>
> @@
> expression free.E, subE<=free.E, E1;
> position free.p;
> @@
>
> kfree@p(E)
> ...
> (
> subE = E1
> |
> * E
> )
> // </smpl>
>
> Signed-off-by: Julia Lawall <julia@diku.dk>
Acked-by: Amit Shah <amit.shah@redhat.com>
Thanks, Julia.
Rusty, please pick this patch. Thanks.
>
> ---
> drivers/char/virtio_console.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
> index 458d907..8c99bf1 100644
> --- a/drivers/char/virtio_console.c
> +++ b/drivers/char/virtio_console.c
> @@ -1090,7 +1090,7 @@ free_port:
> kfree(port);
> fail:
> /* The host might want to notify management sw about port add failure */
> - send_control_msg(port, VIRTIO_CONSOLE_PORT_READY, 0);
> + __send_control_msg(portdev, id, VIRTIO_CONSOLE_PORT_READY, 0);
> return err;
> }
>
> @@ -1559,6 +1559,9 @@ static int __devinit virtcons_probe(struct virtio_device *vdev)
> return 0;
>
> free_vqs:
> + /* The host might want to notify mgmt sw about device add failure */
> + __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> + VIRTIO_CONSOLE_DEVICE_READY, 0);
> vdev->config->del_vqs(vdev);
> kfree(portdev->in_vqs);
> kfree(portdev->out_vqs);
> @@ -1567,9 +1570,6 @@ free_chrdev:
> free:
> kfree(portdev);
> fail:
> - /* The host might want to notify mgmt sw about device add failure */
> - __send_control_msg(portdev, VIRTIO_CONSOLE_BAD_ID,
> - VIRTIO_CONSOLE_DEVICE_READY, 0);
> return err;
> }
>
Amit
^ permalink raw reply
* [PATCH] vhost: Storage class should be before const qualifier
From: Tobias Klauser @ 2010-05-17 13:12 UTC (permalink / raw)
To: mst; +Cc: kvm, virtualization, netdev, Tobias Klauser
The C99 specification states in section 6.11.5:
The placement of a storage-class specifier other than at the beginning
of the declaration specifiers in a declaration is an obsolescent
feature.
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
drivers/vhost/net.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index aa88911..cd36f5f 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -626,7 +626,7 @@ static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
}
#endif
-const static struct file_operations vhost_net_fops = {
+static const struct file_operations vhost_net_fops = {
.owner = THIS_MODULE,
.release = vhost_net_release,
.unlocked_ioctl = vhost_net_ioctl,
--
1.6.3.3
^ permalink raw reply related
* Re: [PATCH] vhost: Storage class should be before const qualifier
From: Michael S. Tsirkin @ 2010-05-17 13:13 UTC (permalink / raw)
To: Tobias Klauser; +Cc: kvm, virtualization, netdev
In-Reply-To: <1274101969-21109-1-git-send-email-tklauser@distanz.ch>
On Mon, May 17, 2010 at 03:12:49PM +0200, Tobias Klauser wrote:
> The C99 specification states in section 6.11.5:
>
> The placement of a storage-class specifier other than at the beginning
> of the declaration specifiers in a declaration is an obsolescent
> feature.
>
> Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
Will apply, thanks!
Just to clarify: does some compiler/checker actually barf on this?
> ---
> drivers/vhost/net.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index aa88911..cd36f5f 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -626,7 +626,7 @@ static long vhost_net_compat_ioctl(struct file *f, unsigned int ioctl,
> }
> #endif
>
> -const static struct file_operations vhost_net_fops = {
> +static const struct file_operations vhost_net_fops = {
> .owner = THIS_MODULE,
> .release = vhost_net_release,
> .unlocked_ioctl = vhost_net_ioctl,
> --
> 1.6.3.3
^ permalink raw reply
* Re: [PATCH] vhost: Storage class should be before const qualifier
From: Tobias Klauser @ 2010-05-17 13:27 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization, netdev
In-Reply-To: <20100517131335.GA12126@redhat.com>
On 2010-05-17 at 15:13:35 +0200, Michael S. Tsirkin <mst@redhat.com> wrote:
> On Mon, May 17, 2010 at 03:12:49PM +0200, Tobias Klauser wrote:
> > The C99 specification states in section 6.11.5:
> >
> > The placement of a storage-class specifier other than at the beginning
> > of the declaration specifiers in a declaration is an obsolescent
> > feature.
> >
> > Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
>
>
> Will apply, thanks!
> Just to clarify: does some compiler/checker actually barf on this?
GCC does emit a warning if the options '-std=c99 -W -Wall' are present.
ICC also does warn about this, though I don't know whether this depends
on any commandline options.
Cheers
Tobias
^ permalink raw reply
* [PATCH] [resend] fix non-mergeable buffers packet too large error handling
From: David L Stevens @ 2010-05-17 21:16 UTC (permalink / raw)
To: mst; +Cc: netdev, kvm, virtualization
This patch enforces single-buffer allocation when
mergeable rx buffers is not enabled.
Reported-by: Michael S. Tsirkin <mst@redhat.com>
Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 309c570..c346304 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -361,13 +361,21 @@ static void handle_rx(struct vhost_net *net)
break;
}
/* TODO: Should check and handle checksum. */
- if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
- memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
- offsetof(typeof(hdr), num_buffers),
- sizeof hdr.num_buffers)) {
- vq_err(vq, "Failed num_buffers write");
+ if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
+ if (memcpy_toiovecend(vq->hdr,
+ (unsigned char *)&headcount,
+ offsetof(typeof(hdr),
+ num_buffers),
+ sizeof hdr.num_buffers)) {
+ vq_err(vq, "Failed num_buffers write");
+ vhost_discard_desc(vq, headcount);
+ break;
+ }
+ } else if (headcount > 1) {
+ vq_err(vq, "rx packet too large (%d) for guest",
+ sock_len);
vhost_discard_desc(vq, headcount);
- break;
+ continue;
}
vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
headcount);
^ permalink raw reply related
* Re: [PATCH 1/4] drivers/char: Eliminate use after free
From: Rusty Russell @ 2010-05-18 0:39 UTC (permalink / raw)
To: Amit Shah; +Cc: kernel-janitors, Julia Lawall, linux-kernel, virtualization
In-Reply-To: <20100517072337.GC3313@amit-laptop.redhat.com>
On Mon, 17 May 2010 06:31:40 pm Amit Shah wrote:
> On (Sat) May 15 2010 [11:45:53], Julia Lawall wrote:
> > From: Julia Lawall <julia@diku.dk>
...
> > Signed-off-by: Julia Lawall <julia@diku.dk>
>
> Acked-by: Amit Shah <amit.shah@redhat.com>
>
> Thanks, Julia.
>
> Rusty, please pick this patch. Thanks.
Done.
Thanks!
Rusty.
^ permalink raw reply
* [PATCH] vhost-net: utilize PUBLISH_USED_IDX feature
From: Michael S. Tsirkin @ 2010-05-18 1:19 UTC (permalink / raw)
To: davem, Juan Quintela, Rusty Russell, Paul E. McKenney
With PUBLISH_USED_IDX, guest tells us which used entries
it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet
consumed the previous entry, or if the guest has already consumed the
new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
Rusty, Dave, this patch depends on the patch
"virtio: put last seen used index into ring itself"
which is currently destined at Rusty's tree.
Rusty, if you are taking that one for 2.6.35, please
take this one as well.
Dave, any objections?
drivers/vhost/vhost.c | 27 +++++++++++++++++++++------
drivers/vhost/vhost.h | 2 ++
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 750effe..2a66cf3 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -278,14 +278,15 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_memory *mem,
return 1;
}
-static int vq_access_ok(unsigned int num,
+static int vq_access_ok(struct vhost_dev *d, unsigned int num,
struct vring_desc __user *desc,
struct vring_avail __user *avail,
struct vring_used __user *used)
{
+ size_t s = vhost_has_feature(d, VIRTIO_RING_F_PUBLISH_USED) ? 2 : 0;
return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
access_ok(VERIFY_READ, avail,
- sizeof *avail + num * sizeof *avail->ring) &&
+ sizeof *avail + num * sizeof *avail->ring + s) &&
access_ok(VERIFY_WRITE, used,
sizeof *used + num * sizeof *used->ring);
}
@@ -312,7 +313,7 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
/* Caller should have vq mutex and device mutex */
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
- return vq_access_ok(vq->num, vq->desc, vq->avail, vq->used) &&
+ return vq_access_ok(vq->dev, vq->num, vq->desc, vq->avail, vq->used,) &&
vq_log_access_ok(vq, vq->log_base);
}
@@ -448,7 +449,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
* If it is not, we don't as size might not have been setup.
* We will verify when backend is configured. */
if (vq->private_data) {
- if (!vq_access_ok(vq->num,
+ if (!vq_access_ok(d, vq->num,
(void __user *)(unsigned long)a.desc_user_addr,
(void __user *)(unsigned long)a.avail_user_addr,
(void __user *)(unsigned long)a.used_user_addr)) {
@@ -473,6 +474,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
+ vq->last_used = (u16 __user *)&vq->avail->ring[vq->num];
vq->log_addr = a.log_guest_addr;
vq->used = (void __user *)(unsigned long)a.used_user_addr;
break;
@@ -993,7 +995,8 @@ void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
/* After we've used one of their buffers, we tell them about it. We'll then
* want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
+static int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head,
+ int len)
{
struct vring_used_elem __user *used;
@@ -1034,9 +1037,10 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
}
/* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+static void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
__u16 flags;
+ __u16 used;
/* Flush out used index updates. This is paired
* with the barrier that the Guest executes when enabling
* interrupts. */
@@ -1053,6 +1057,17 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
!vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
return;
+ if (vhost_has_feature(d, VIRTIO_RING_F_PUBLISH_USED)) {
+ __u16 used;
+ if (get_user(used, vq->last_used)) {
+ vq_err(vq, "Failed to get last used idx");
+ return;
+ }
+
+ if (used != (u16)(vq->last_used_idx - 1))
+ return;
+ }
+
/* Signal the Guest tell them we used something up. */
if (vq->call_ctx)
eventfd_signal(vq->call_ctx, 1);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 44591ba..00e9784 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -52,6 +52,7 @@ struct vhost_virtqueue {
unsigned int num;
struct vring_desc __user *desc;
struct vring_avail __user *avail;
+ u16 __user *last_used;
struct vring_used __user *used;
struct file *kick;
struct file *call;
@@ -148,6 +149,7 @@ void vhost_cleanup(void);
enum {
VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1 << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1 << VIRTIO_RING_F_PUBLISH_USED) |
(1 << VHOST_F_LOG_ALL) |
(1 << VHOST_NET_F_VIRTIO_NET_HDR),
};
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: [PATCH] [resend] fix non-mergeable buffers packet too large error handling
From: Michael S. Tsirkin @ 2010-05-18 1:53 UTC (permalink / raw)
To: David L Stevens; +Cc: netdev, kvm, virtualization
In-Reply-To: <1274130996.8492.7.camel@w-dls.beaverton.ibm.com>
On Mon, May 17, 2010 at 02:16:36PM -0700, David L Stevens wrote:
> This patch enforces single-buffer allocation when
> mergeable rx buffers is not enabled.
>
> Reported-by: Michael S. Tsirkin <mst@redhat.com>
> Signed-off-by: David L Stevens <dlstevens@us.ibm.com>
Thanks! Why are you resending?
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 309c570..c346304 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -361,13 +361,21 @@ static void handle_rx(struct vhost_net *net)
> break;
> }
> /* TODO: Should check and handle checksum. */
> - if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF) &&
> - memcpy_toiovecend(vq->hdr, (unsigned char *)&headcount,
> - offsetof(typeof(hdr), num_buffers),
> - sizeof hdr.num_buffers)) {
> - vq_err(vq, "Failed num_buffers write");
> + if (vhost_has_feature(&net->dev, VIRTIO_NET_F_MRG_RXBUF)) {
> + if (memcpy_toiovecend(vq->hdr,
> + (unsigned char *)&headcount,
> + offsetof(typeof(hdr),
> + num_buffers),
> + sizeof hdr.num_buffers)) {
> + vq_err(vq, "Failed num_buffers write");
> + vhost_discard_desc(vq, headcount);
> + break;
> + }
> + } else if (headcount > 1) {
> + vq_err(vq, "rx packet too large (%d) for guest",
> + sock_len);
> vhost_discard_desc(vq, headcount);
> - break;
> + continue;
> }
> vhost_add_used_and_signal_n(&net->dev, vq, vq->heads,
> headcount);
>
^ permalink raw reply
* [PATCHv2] vhost-net: utilize PUBLISH_USED_IDX feature
From: Michael S. Tsirkin @ 2010-05-18 2:21 UTC (permalink / raw)
To: davem, Juan Quintela, Rusty Russell, Paul E. McKenney
With PUBLISH_USED_IDX, guest tells us which used entries
it has consumed. This can be used to reduce the number
of interrupts: after we write a used entry, if the guest has not yet
consumed the previous entry, or if the guest has already consumed the
new entry, we do not need to interrupt.
This imporves bandwidth by 30% under some workflows.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
---
This is on top of Rusty's tree and depends on the virtio patch.
Changes from v1:
fix build
drivers/vhost/vhost.c | 27 +++++++++++++++++++++------
drivers/vhost/vhost.h | 4 ++--
2 files changed, 23 insertions(+), 8 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 750effe..18c4f6e 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -278,14 +278,15 @@ static int memory_access_ok(struct vhost_dev *d, struct vhost_memory *mem,
return 1;
}
-static int vq_access_ok(unsigned int num,
+static int vq_access_ok(struct vhost_dev *d, unsigned int num,
struct vring_desc __user *desc,
struct vring_avail __user *avail,
struct vring_used __user *used)
{
+ size_t s = vhost_has_feature(d, VIRTIO_RING_F_PUBLISH_USED) ? 2 : 0;
return access_ok(VERIFY_READ, desc, num * sizeof *desc) &&
access_ok(VERIFY_READ, avail,
- sizeof *avail + num * sizeof *avail->ring) &&
+ sizeof *avail + num * sizeof *avail->ring + s) &&
access_ok(VERIFY_WRITE, used,
sizeof *used + num * sizeof *used->ring);
}
@@ -312,7 +313,7 @@ static int vq_log_access_ok(struct vhost_virtqueue *vq, void __user *log_base)
/* Caller should have vq mutex and device mutex */
int vhost_vq_access_ok(struct vhost_virtqueue *vq)
{
- return vq_access_ok(vq->num, vq->desc, vq->avail, vq->used) &&
+ return vq_access_ok(vq->dev, vq->num, vq->desc, vq->avail, vq->used) &&
vq_log_access_ok(vq, vq->log_base);
}
@@ -448,7 +449,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
* If it is not, we don't as size might not have been setup.
* We will verify when backend is configured. */
if (vq->private_data) {
- if (!vq_access_ok(vq->num,
+ if (!vq_access_ok(d, vq->num,
(void __user *)(unsigned long)a.desc_user_addr,
(void __user *)(unsigned long)a.avail_user_addr,
(void __user *)(unsigned long)a.used_user_addr)) {
@@ -473,6 +474,7 @@ static long vhost_set_vring(struct vhost_dev *d, int ioctl, void __user *argp)
vq->log_used = !!(a.flags & (0x1 << VHOST_VRING_F_LOG));
vq->desc = (void __user *)(unsigned long)a.desc_user_addr;
vq->avail = (void __user *)(unsigned long)a.avail_user_addr;
+ vq->last_used = (u16 __user *)&vq->avail->ring[vq->num];
vq->log_addr = a.log_guest_addr;
vq->used = (void __user *)(unsigned long)a.used_user_addr;
break;
@@ -993,7 +995,8 @@ void vhost_discard_vq_desc(struct vhost_virtqueue *vq)
/* After we've used one of their buffers, we tell them about it. We'll then
* want to notify the guest, using eventfd. */
-int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
+static int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head,
+ int len)
{
struct vring_used_elem __user *used;
@@ -1034,9 +1037,10 @@ int vhost_add_used(struct vhost_virtqueue *vq, unsigned int head, int len)
}
/* This actually signals the guest, using eventfd. */
-void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
+static void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
{
__u16 flags;
+ __u16 used;
/* Flush out used index updates. This is paired
* with the barrier that the Guest executes when enabling
* interrupts. */
@@ -1053,6 +1057,17 @@ void vhost_signal(struct vhost_dev *dev, struct vhost_virtqueue *vq)
!vhost_has_feature(dev, VIRTIO_F_NOTIFY_ON_EMPTY)))
return;
+ if (vhost_has_feature(dev, VIRTIO_RING_F_PUBLISH_USED)) {
+ __u16 used;
+ if (get_user(used, vq->last_used)) {
+ vq_err(vq, "Failed to get last used idx");
+ return;
+ }
+
+ if (used != (u16)(vq->last_used_idx - 1))
+ return;
+ }
+
/* Signal the Guest tell them we used something up. */
if (vq->call_ctx)
eventfd_signal(vq->call_ctx, 1);
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 44591ba..bd01aca 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -52,6 +52,7 @@ struct vhost_virtqueue {
unsigned int num;
struct vring_desc __user *desc;
struct vring_avail __user *avail;
+ u16 __user *last_used;
struct vring_used __user *used;
struct file *kick;
struct file *call;
@@ -126,8 +127,6 @@ unsigned vhost_get_vq_desc(struct vhost_dev *, struct vhost_virtqueue *,
struct vhost_log *log, unsigned int *log_num);
void vhost_discard_vq_desc(struct vhost_virtqueue *);
-int vhost_add_used(struct vhost_virtqueue *, unsigned int head, int len);
-void vhost_signal(struct vhost_dev *, struct vhost_virtqueue *);
void vhost_add_used_and_signal(struct vhost_dev *, struct vhost_virtqueue *,
unsigned int head, int len);
void vhost_disable_notify(struct vhost_virtqueue *);
@@ -148,6 +147,7 @@ void vhost_cleanup(void);
enum {
VHOST_FEATURES = (1 << VIRTIO_F_NOTIFY_ON_EMPTY) |
(1 << VIRTIO_RING_F_INDIRECT_DESC) |
+ (1 << VIRTIO_RING_F_PUBLISH_USED) |
(1 << VHOST_F_LOG_ALL) |
(1 << VHOST_NET_F_VIRTIO_NET_HDR),
};
--
1.7.1.12.g42b7f
^ permalink raw reply related
* Re: [PATCH] [resend] fix non-mergeable buffers packet too large error handling
From: Michael S. Tsirkin @ 2010-05-18 3:12 UTC (permalink / raw)
To: David L Stevens; +Cc: netdev, kvm, virtualization, davem
DaveM, just to clarify, this patch is on top of the series
we are working on with David L Stevens. It's not for your net tree.
--
MST
^ permalink raw reply
* Re: [PATCH] vhost-net: utilize PUBLISH_USED_IDX feature
From: David Miller @ 2010-05-18 4:08 UTC (permalink / raw)
To: mst
Cc: quintela, rusty, paulmck, arnd, kvm, virtualization, netdev,
linux-kernel, alex.williamson, amit.shah
In-Reply-To: <20100518011931.GA21918@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 18 May 2010 04:19:31 +0300
> With PUBLISH_USED_IDX, guest tells us which used entries
> it has consumed. This can be used to reduce the number
> of interrupts: after we write a used entry, if the guest has not yet
> consumed the previous entry, or if the guest has already consumed the
> new entry, we do not need to interrupt.
> This imporves bandwidth by 30% under some workflows.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
>
> Rusty, Dave, this patch depends on the patch
> "virtio: put last seen used index into ring itself"
> which is currently destined at Rusty's tree.
> Rusty, if you are taking that one for 2.6.35, please
> take this one as well.
> Dave, any objections?
None:
Acked-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* Re: [PATCH] [resend] fix non-mergeable buffers packet too large error handling
From: David Miller @ 2010-05-18 4:14 UTC (permalink / raw)
To: mst; +Cc: dlstevens, netdev, kvm, virtualization
In-Reply-To: <20100518031252.GA23764@redhat.com>
From: "Michael S. Tsirkin" <mst@redhat.com>
Date: Tue, 18 May 2010 06:12:52 +0300
> DaveM, just to clarify, this patch is on top of the series
> we are working on with David L Stevens. It's not for your net tree.
Understood.
^ permalink raw reply
* [PATCH] fix a code style in drivers/char/virtio_console.c
From: Steven Liu @ 2010-05-18 5:17 UTC (permalink / raw)
To: amit.shah, linux-kernel, virtualization
fix a code style in drivers/char/virtio_console.c
Signed-off-by: Liu Qi <lingjiujianke@gmail.com>
---
drivers/char/virtio_console.c | 7 +------
1 files changed, 1 insertions(+), 6 deletions(-)
diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c
index 196428c..61dad7e 100644
--- a/drivers/char/virtio_console.c
+++ b/drivers/char/virtio_console.c
@@ -1299,36 +1299,31 @@ static int init_vqs(struct ports_device *portdev)
char **io_names;
struct virtqueue **vqs;
u32 i, j, nr_ports, nr_queues;
- int err;
+ int err = -ENOMEM;
nr_ports = portdev->config.max_nr_ports;
nr_queues = use_multiport(portdev) ? (nr_ports + 1) * 2 : 2;
vqs = kmalloc(nr_queues * sizeof(struct virtqueue *), GFP_KERNEL);
if (!vqs) {
- err = -ENOMEM;
goto fail;
}
io_callbacks = kmalloc(nr_queues * sizeof(vq_callback_t *), GFP_KERNEL);
if (!io_callbacks) {
- err = -ENOMEM;
goto free_vqs;
}
io_names = kmalloc(nr_queues * sizeof(char *), GFP_KERNEL);
if (!io_names) {
- err = -ENOMEM;
goto free_callbacks;
}
portdev->in_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
GFP_KERNEL);
if (!portdev->in_vqs) {
- err = -ENOMEM;
goto free_names;
}
portdev->out_vqs = kmalloc(nr_ports * sizeof(struct virtqueue *),
GFP_KERNEL);
if (!portdev->out_vqs) {
- err = -ENOMEM;
goto free_invqs;
}
--
1.7.1
Best regards
^ permalink raw reply related
* Re: [PATCH] fix a code style in drivers/char/virtio_console.c
From: Amit Shah @ 2010-05-18 5:22 UTC (permalink / raw)
To: Steven Liu; +Cc: linux-kernel, virtualization
In-Reply-To: <AANLkTilW8zZxCOrdiefZ4ZtixprI58LzDZ6Cz03z4lFk@mail.gmail.com>
On (Tue) May 18 2010 [13:17:22], Steven Liu wrote:
> fix a code style in drivers/char/virtio_console.c
>
> Signed-off-by: Liu Qi <lingjiujianke@gmail.com>
NACK.
This style is deliberate and follows Rusty's style: if we end up using
'err' uninitialised in some code path, it can get flagged by source
checkers.
Amit
^ permalink raw reply
* Re: [PATCH] fix a code style in drivers/char/virtio_console.c
From: Steven Liu @ 2010-05-18 5:41 UTC (permalink / raw)
To: Amit Shah; +Cc: linux-kernel, virtualization
In-Reply-To: <20100518052230.GA1706@amit-laptop.redhat.com>
Hi, Amit,
if 'err' initialised in this path, it needn't do err = -ENOMEM
after,isn't it?
Best regards
2010/5/18 Amit Shah <amit.shah@redhat.com>:
> On (Tue) May 18 2010 [13:17:22], Steven Liu wrote:
>> fix a code style in drivers/char/virtio_console.c
>>
>> Signed-off-by: Liu Qi <lingjiujianke@gmail.com>
>
> NACK.
>
> This style is deliberate and follows Rusty's style: if we end up using
> 'err' uninitialised in some code path, it can get flagged by source
> checkers.
>
> Amit
>
^ permalink raw reply
* Re: [PATCH] fix a code style in drivers/char/virtio_console.c
From: Amit Shah @ 2010-05-18 6:32 UTC (permalink / raw)
To: Steven Liu; +Cc: linux-kernel, virtualization
In-Reply-To: <AANLkTimXLkzb-CyhbkOaubAOiSsIkGKW0y5Cblrwfvst@mail.gmail.com>
Hello,
On (Tue) May 18 2010 [13:41:29], Steven Liu wrote:
> Hi, Amit,
>
> if 'err' initialised in this path, it needn't do err = -ENOMEM
> after,isn't it?
What I mean is if we later add some code that just does:
if (err)
goto fail;
then 'ret' can be -ENOMEM, as it was initialised to, which would be
fine. But if a later patch adds something like:
+ ret = -EIO;
+ err = ...
+ if (err)
+ goto fail;
+
err = ...
if (err)
goto fail;
In this case, the 2nd if() would now return EIO instead of ENOMEM as
earlier.
Also, this style of coding can prevent uninitialised usage of 'ret', eg:
int ret;
if (err)
goto fail;
fail:
return ret;
In this case, the compiler will warn about 'ret' being used
uninitialised.
This is just a coding style issue. I had initially coded it the way your
patch does, but Rusty asked me to change that and I like this new style
better: there's less scope for surprises.
Amit
^ permalink raw reply
* Re: [PATCH] fix a code style in drivers/char/virtio_console.c
From: Steven Liu @ 2010-05-18 6:41 UTC (permalink / raw)
To: Amit Shah; +Cc: linux-kernel, virtualization
In-Reply-To: <20100518063241.GB27262@amit-laptop.redhat.com>
okay, i see, thanks for your regards :)
2010/5/18 Amit Shah <amit.shah@redhat.com>:
> Hello,
>
> On (Tue) May 18 2010 [13:41:29], Steven Liu wrote:
>> Hi, Amit,
>>
>> if 'err' initialised in this path, it needn't do err = -ENOMEM
>> after,isn't it?
>
> What I mean is if we later add some code that just does:
>
> if (err)
> goto fail;
>
> then 'ret' can be -ENOMEM, as it was initialised to, which would be
> fine. But if a later patch adds something like:
>
> + ret = -EIO;
> + err = ...
> + if (err)
> + goto fail;
> +
> err = ...
> if (err)
> goto fail;
>
> In this case, the 2nd if() would now return EIO instead of ENOMEM as
> earlier.
>
> Also, this style of coding can prevent uninitialised usage of 'ret', eg:
>
>
> int ret;
>
> if (err)
> goto fail;
>
> fail:
> return ret;
>
> In this case, the compiler will warn about 'ret' being used
> uninitialised.
>
> This is just a coding style issue. I had initially coded it the way your
> patch does, but Rusty asked me to change that and I like this new style
> better: there's less scope for surprises.
>
> Amit
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox