Re: [Qemu-devel] [PATCH] virtio-blk: notify guest directly

[Paolo Bonzini <pbonzini@redhat.com>]

On 19/12/2017 14:33, sochin.jiang wrote:
> From: "sochin.jiang" <sochin.jiang@huawei.com>
> 
>  Till now, we've already notify guest as a batch mostly, an
>  extra BH won't decrease guest interrupts much, but cause a
>  significant notification loss. Generally, we could have 15%
>  or so performance lost in single queue IO models, as I tested.

Interesting, this was indeed done to decrease interrupt overhead:

    commit 5b2ffbe4d99843fd8305c573a100047a8c962327
    Author: Ming Lei <tom.leiming@gmail.com>
    Date:   Sat Jul 12 12:08:53 2014 +0800

    virtio-blk: dataplane: notify guest as a batch

    Now requests are submitted as a batch, so it is natural
    to notify guest as a batch too.

    This may suppress interrupt notification to VM a lot:

            - in my test, decreased by ~13K/sec

    Signed-off-by: Ming Lei <ming.lei@canonical.com>
    Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Can you explain your benchmark setup?

Paolo


> Signed-off-by: sochin.jiang <sochin.jiang@huawei.com>
> ---
>  hw/block/dataplane/virtio-blk.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 5556f0e..a261a1d 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -32,7 +32,6 @@ struct VirtIOBlockDataPlane {
>  
>      VirtIOBlkConf *conf;
>      VirtIODevice *vdev;
> -    QEMUBH *bh;                     /* bh for guest notification */
>      unsigned long *batch_notify_vqs;
>  
>      /* Note that these EventNotifiers are assigned by value.  This is
> @@ -44,14 +43,7 @@ struct VirtIOBlockDataPlane {
>      AioContext *ctx;
>  };
>  
> -/* Raise an interrupt to signal guest, if necessary */
> -void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq)
> -{
> -    set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs);
> -    qemu_bh_schedule(s->bh);
> -}
> -
> -static void notify_guest_bh(void *opaque)
> +static void notify_guest(void *opaque)
>  {
>      VirtIOBlockDataPlane *s = opaque;
>      unsigned nvqs = s->conf->num_queues;
> @@ -75,7 +67,12 @@ static void notify_guest_bh(void *opaque)
>      }
>  }
>  
> -/* Context: QEMU global mutex held */
> +/* Raise an interrupt to signal guest, if necessary */
> +void virtio_blk_data_plane_notify(VirtIOBlockDataPlane *s, VirtQueue *vq)
> +{
> +    set_bit(virtio_get_queue_index(vq), s->batch_notify_vqs);
> +    notify_guest(s);
> +}
>  void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>                                    VirtIOBlockDataPlane **dataplane,
>                                    Error **errp)
> @@ -122,7 +119,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *conf,
>      } else {
>          s->ctx = qemu_get_aio_context();
>      }
> -    s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
>      s->batch_notify_vqs = bitmap_new(conf->num_queues);
>  
>      *dataplane = s;
> @@ -140,7 +136,6 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
>      vblk = VIRTIO_BLK(s->vdev);
>      assert(!vblk->dataplane_started);
>      g_free(s->batch_notify_vqs);
> -    qemu_bh_delete(s->bh);
>      if (s->iothread) {
>          object_unref(OBJECT(s->iothread));
>      }
>