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@xxxxxxxxxxxxxxxxxx>
> ---
> I am not yet 100% happy with this patch which aims to reduce guest CPU
> consumption related to vblk->lock contention. Although this patch reduces
> wait/hold times it does not affect I/O throughput or guest CPU utilization.
> More investigation is required to get to the bottom of why guest CPU
> utilization does not decrease when a lock bottleneck has been removed.
>
> Performance figures:
>
> Host: 2.6.34 upstream kernel, qemu-kvm-0.12.4 if=virtio,cache=none
> Guest: 2.6.35-rc3-kvm.git upstream kernel
> Storage: 12 disks as striped LVM volume
> Benchmark: 4 concurrent dd bs=4k iflag=direct
>
> Lockstat data for &vblk->lock:
>
> test con-bounces contentions waittime-min waittime-max waittime-total
> unmodified 7097 7108 0.31 956.09 161165.4
> patched 11484 11550 0.30 411.80 50245.83
>
> The maximum wait time went down by 544.29 us (-57%) and the total wait time
> decreased by 69%. This shows that the virtqueue kick is indeed hogging the
> lock.
>
> The patched version actually has higher contention than the unmodified version.
> I think the reason for this is that each virtqueue kick now includes a short
> release and reacquire. This short release gives other vcpus a chance to
> acquire the lock and progress, hence more contention but overall better wait
> time numbers.
>
> name acq-bounces acquisitions holdtime-min holdtime-max holdtime-total
> unmodified 10771 5038346 0.00 3271.81 59016905.47
> patched 31594 5857813 0.00 219.76 24104915.55
>
> Here we see the full impact of this patch: hold time reduced to 219.76 us
> (-93%).
>
> Again the acquisitions have increased since we're now doing an extra
> unlock+lock per virtqueue kick.
>
> Testing, ideas, and comments appreciated.
>
> drivers/block/virtio_blk.c | 2 +-
> drivers/char/hw_random/virtio-rng.c | 2 +-
> drivers/char/virtio_console.c | 6 +++---
> drivers/net/virtio_net.c | 6 +++---
> drivers/virtio/virtio_balloon.c | 6 +++---
> drivers/virtio/virtio_ring.c | 13 +++++++++++--
> include/linux/virtio.h | 3 ++-
> net/9p/trans_virtio.c | 2 +-
> 8 files changed, 25 insertions(+), 15 deletions(-)
>
> 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);
> + }
> +
The queue lock requires irq's to be disabled in addition to lock
acquision.
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
