| Message ID | 1277328242-10685-1-git-send-email-stefanha@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 23, 2010 at 10:24:02PM +0100, Stefan Hajnoczi wrote: > The virtio block device holds a lock during I/O request processing. > Kicking the virtqueue while the lock is held results in long lock hold > times and increases contention for the lock. > > This patch modifies virtqueue_kick() to optionally release a lock while > notifying the host. Virtio block is modified to pass in its lock. This > allows other vcpus to queue I/O requests during the time spent servicing > the virtqueue notify in the host. > > The virtqueue_kick() function is modified to know about locking because > it changes the state of the virtqueue and should execute with the lock > held (it would not be correct for virtio block to release the lock > before calling virtqueue_kick()). > > Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> While the optimization makes sense, the API's pretty hairy IMHO. Why don't we split the kick functionality instead? E.g. /* Report whether host notification is necessary. */ bool virtqueue_kick_prepare(struct virtqueue *vq) /* Can be done in parallel with add_buf/get_buf */ void virtqueue_kick_notify(struct virtqueue *vq) And users can r = virtqueue_kick_prepare(vq); spin_unlock_irqrestore(...); if (r) virtqueue_kick_notify(struct virtqueue *vq)
On Sun, Jun 19, 2011 at 8:14 AM, Michael S. Tsirkin <mst@redhat.com> wrote: > On Wed, Jun 23, 2010 at 10:24:02PM +0100, Stefan Hajnoczi wrote: >> The virtio block device holds a lock during I/O request processing. >> Kicking the virtqueue while the lock is held results in long lock hold >> times and increases contention for the lock. >> >> This patch modifies virtqueue_kick() to optionally release a lock while >> notifying the host. Virtio block is modified to pass in its lock. This >> allows other vcpus to queue I/O requests during the time spent servicing >> the virtqueue notify in the host. >> >> The virtqueue_kick() function is modified to know about locking because >> it changes the state of the virtqueue and should execute with the lock >> held (it would not be correct for virtio block to release the lock >> before calling virtqueue_kick()). >> >> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> > > While the optimization makes sense, the API's pretty hairy IMHO. > Why don't we split the kick functionality instead? > E.g. > /* Report whether host notification is necessary. */ > bool virtqueue_kick_prepare(struct virtqueue *vq) > /* Can be done in parallel with add_buf/get_buf */ > void virtqueue_kick_notify(struct virtqueue *vq) This is a nice idea, it makes the code cleaner. I am testing patches that implement this and after Khoa has measured the performance I will send them out. Stefan -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 20, 2011 at 4:27 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Sun, Jun 19, 2011 at 8:14 AM, Michael S. Tsirkin <mst@redhat.com> wrote: >> On Wed, Jun 23, 2010 at 10:24:02PM +0100, Stefan Hajnoczi wrote: >>> The virtio block device holds a lock during I/O request processing. >>> Kicking the virtqueue while the lock is held results in long lock hold >>> times and increases contention for the lock. >>> >>> This patch modifies virtqueue_kick() to optionally release a lock while >>> notifying the host. Virtio block is modified to pass in its lock. This >>> allows other vcpus to queue I/O requests during the time spent servicing >>> the virtqueue notify in the host. >>> >>> The virtqueue_kick() function is modified to know about locking because >>> it changes the state of the virtqueue and should execute with the lock >>> held (it would not be correct for virtio block to release the lock >>> before calling virtqueue_kick()). >>> >>> Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com> >> >> While the optimization makes sense, the API's pretty hairy IMHO. >> Why don't we split the kick functionality instead? >> E.g. >> /* Report whether host notification is necessary. */ >> bool virtqueue_kick_prepare(struct virtqueue *vq) >> /* Can be done in parallel with add_buf/get_buf */ >> void virtqueue_kick_notify(struct virtqueue *vq) > > This is a nice idea, it makes the code cleaner. I am testing patches > that implement this and after Khoa has measured the performance I will > send them out. Just an update that benchmarks are being run. Will send out patches and results as soon as they are in. Stefan -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Any progress on these patches? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 10, 2011 at 09:18:01AM -0400, Christoph Hellwig wrote:
> Any progress on these patches?
Khoa ran ffsb benchmarks on his rig and we didn't see any benefit. I
have not started investigating yet, been working on other things.
It will be necessary to compare against the old patches which did show
an improvment last year when we ran them. Then we'll know if I broke
something in the new patches.
If you like I can send the latest patches for you to take a look. I
will not get a chance to play with this until after LinuxCon.
Stefan
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c index 258bc2a..de033bf 100644 --- a/drivers/block/virtio_blk.c +++ b/drivers/block/virtio_blk.c @@ -187,7 +187,7 @@ static void do_virtblk_request(struct request_queue *q) } if (issued) - virtqueue_kick(vblk->vq); + virtqueue_kick(vblk->vq, &vblk->lock); } static void virtblk_prepare_flush(struct request_queue *q, struct request *req) diff --git a/drivers/char/hw_random/virtio-rng.c b/drivers/char/hw_random/virtio-rng.c index 75f1cbd..852d563 100644 --- a/drivers/char/hw_random/virtio-rng.c +++ b/drivers/char/hw_random/virtio-rng.c @@ -49,7 +49,7 @@ static void register_buffer(u8 *buf, size_t size) if (virtqueue_add_buf(vq, &sg, 0, 1, buf) < 0) BUG(); - virtqueue_kick(vq); + virtqueue_kick(vq, NULL); } static int virtio_read(struct hwrng *rng, void *buf, size_t size, bool wait) diff --git a/drivers/char/virtio_console.c b/drivers/char/virtio_console.c index 942a982..677714d 100644 --- a/drivers/char/virtio_console.c +++ b/drivers/char/virtio_console.c @@ -328,7 +328,7 @@ static int add_inbuf(struct virtqueue *vq, struct port_buffer *buf) sg_init_one(sg, buf->buf, buf->size); ret = virtqueue_add_buf(vq, sg, 0, 1, buf); - virtqueue_kick(vq); + virtqueue_kick(vq, NULL); return ret; } @@ -400,7 +400,7 @@ static ssize_t __send_control_msg(struct ports_device *portdev, u32 port_id, sg_init_one(sg, &cpkt, sizeof(cpkt)); if (virtqueue_add_buf(vq, sg, 1, 0, &cpkt) >= 0) { - virtqueue_kick(vq); + virtqueue_kick(vq, NULL); while (!virtqueue_get_buf(vq, &len)) cpu_relax(); } @@ -444,7 +444,7 @@ static ssize_t send_buf(struct port *port, void *in_buf, size_t in_count, ret = virtqueue_add_buf(out_vq, sg, 1, 0, in_buf); /* Tell Host to go! */ - virtqueue_kick(out_vq); + virtqueue_kick(out_vq, NULL); if (ret < 0) { in_count = 0; diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index 1edb7a6..6a837b3 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -433,7 +433,7 @@ static bool try_fill_recv(struct virtnet_info *vi, gfp_t gfp) } while (err > 0); if (unlikely(vi->num > vi->max)) vi->max = vi->num; - virtqueue_kick(vi->rvq); + virtqueue_kick(vi->rvq, NULL); return !oom; } @@ -581,7 +581,7 @@ again: } return NETDEV_TX_BUSY; } - virtqueue_kick(vi->svq); + virtqueue_kick(vi->svq, NULL); /* Don't wait up for transmitted skbs to be freed. */ skb_orphan(skb); @@ -680,7 +680,7 @@ static bool virtnet_send_command(struct virtnet_info *vi, u8 class, u8 cmd, BUG_ON(virtqueue_add_buf(vi->cvq, sg, out, in, vi) < 0); - virtqueue_kick(vi->cvq); + virtqueue_kick(vi->cvq, NULL); /* * Spin for a response, the kick causes an ioport write, trapping diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c index 0f1da45..c9c5c4a 100644 --- a/drivers/virtio/virtio_balloon.c +++ b/drivers/virtio/virtio_balloon.c @@ -91,7 +91,7 @@ static void tell_host(struct virtio_balloon *vb, struct virtqueue *vq) /* We should always be able to add one buffer to an empty queue. */ if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0) BUG(); - virtqueue_kick(vq); + virtqueue_kick(vq, NULL); /* When host has read buffer, this completes via balloon_ack */ wait_for_completion(&vb->acked); @@ -223,7 +223,7 @@ static void stats_handle_request(struct virtio_balloon *vb) sg_init_one(&sg, vb->stats, sizeof(vb->stats)); if (virtqueue_add_buf(vq, &sg, 1, 0, vb) < 0) BUG(); - virtqueue_kick(vq); + virtqueue_kick(vq, NULL); } static void virtballoon_changed(struct virtio_device *vdev) @@ -316,7 +316,7 @@ static int virtballoon_probe(struct virtio_device *vdev) sg_init_one(&sg, vb->stats, sizeof vb->stats); if (virtqueue_add_buf(vb->stats_vq, &sg, 1, 0, vb) < 0) BUG(); - virtqueue_kick(vb->stats_vq); + virtqueue_kick(vb->stats_vq, NULL); } vb->thread = kthread_run(balloon, vb, "vballoon"); diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 1ca8890..163a237 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -236,7 +236,7 @@ add_head: } EXPORT_SYMBOL_GPL(virtqueue_add_buf_gfp); -void virtqueue_kick(struct virtqueue *_vq) +void virtqueue_kick(struct virtqueue *_vq, spinlock_t *lock) { struct vring_virtqueue *vq = to_vvq(_vq); START_USE(vq); @@ -250,10 +250,19 @@ void virtqueue_kick(struct virtqueue *_vq) /* Need to update avail index before checking if we should notify */ virtio_mb(); - if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) + if (!(vq->vring.used->flags & VRING_USED_F_NO_NOTIFY)) { + /* Release lock while doing the kick because the guest should + * not exit with the lock held. */ + if (lock) + spin_unlock(lock); + /* Prod other side to tell it about changes. */ vq->notify(&vq->vq); + if (lock) + spin_lock(lock); + } + END_USE(vq); } EXPORT_SYMBOL_GPL(virtqueue_kick); diff --git a/include/linux/virtio.h b/include/linux/virtio.h index aff5b4f..1561c86 100644 --- a/include/linux/virtio.h +++ b/include/linux/virtio.h @@ -37,6 +37,7 @@ struct virtqueue { * Returns remaining capacity of queue (sg segments) or a negative error. * virtqueue_kick: update after add_buf * vq: the struct virtqueue + * lock: spinlock to release during kick (may be NULL). * After one or more add_buf calls, invoke this to kick the other side. * virtqueue_get_buf: get the next used buffer * vq: the struct virtqueue we're talking about. @@ -78,7 +79,7 @@ static inline int virtqueue_add_buf(struct virtqueue *vq, return virtqueue_add_buf_gfp(vq, sg, out_num, in_num, data, GFP_ATOMIC); } -void virtqueue_kick(struct virtqueue *vq); +void virtqueue_kick(struct virtqueue *vq, spinlock_t *lock); void *virtqueue_get_buf(struct virtqueue *vq, unsigned int *len); diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c index dcfbe99..ccf17dc 100644 --- a/net/9p/trans_virtio.c +++ b/net/9p/trans_virtio.c @@ -215,7 +215,7 @@ p9_virtio_request(struct p9_client *client, struct p9_req_t *req) return -EIO; } - virtqueue_kick(chan->vq); + virtqueue_kick(chan->vq, NULL); P9_DPRINTK(P9_DEBUG_TRANS, "9p debug: virtio request kicked\n"); return 0;