[Qemu-devel] [PATCH v1 0/2] virtio-blk: dataplane: one fix plus one optimization

[Qemu-devel] [PATCH v1 0/2] virtio-blk: dataplane: one fix plus one optimization

[Ming Lei]

Hi,

The first one fixes one problem introduced recently.

The second one suppresses notifications to guest a lot.

V1:
	- use BH to suppress notifications to guest as suggested
	by Paolo


Thanks,
--
Ming Lei

[Qemu-devel] [PATCH v1 1/2] virtio-blk: data-plane: fix save/set .complete_request in start

[Ming Lei]

The callback has to be saved and reset in virtio_blk_data_plane_start(),
otherwise dataplane's requests will be completed in qemu aio context.

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/block/dataplane/virtio-blk.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index 227bb15..e88862d 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -125,7 +125,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
                                   Error **errp)
 {
     VirtIOBlockDataPlane *s;
-    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
     Error *local_err = NULL;
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
@@ -178,8 +177,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
     bdrv_op_block_all(blk->conf.bs, s->blocker);
 
     *dataplane = s;
-    s->saved_complete_request = vblk->complete_request;
-    vblk->complete_request = complete_request_vring;
 }
 
 /* Context: QEMU global mutex held */
@@ -201,6 +198,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
 {
     BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
     VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
+    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
     VirtQueue *vq;
 
     if (s->started) {
@@ -234,6 +232,9 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
     }
     s->host_notifier = *virtio_queue_get_host_notifier(vq);
 
+    s->saved_complete_request = vblk->complete_request;
+    vblk->complete_request = complete_request_vring;
+
     s->starting = false;
     s->started = true;
     trace_virtio_blk_data_plane_start(s);
-- 
1.7.9.5

[Qemu-devel] [PATCH v1 2/2] virtio-blk: dataplane: notify guest as a batch

[Ming Lei]

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>
---
 hw/block/dataplane/virtio-blk.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e88862d..d65e022 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -34,6 +34,7 @@ struct VirtIOBlockDataPlane {
     VirtIODevice *vdev;
     Vring vring;                    /* virtqueue vring */
     EventNotifier *guest_notifier;  /* irq */
+    QEMUBH *bh;                     /* bh for guest notification */
 
     /* Note that these EventNotifiers are assigned by value.  This is
      * fine as long as you do not call event_notifier_cleanup on them
@@ -61,13 +62,28 @@ static void notify_guest(VirtIOBlockDataPlane *s)
     event_notifier_set(s->guest_notifier);
 }
 
+static void notify_guest_bh(void *opaque)
+{
+    VirtIOBlockDataPlane *s = opaque;
+
+    notify_guest(s);
+}
+
 static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
 {
+    VirtIOBlockDataPlane *s = req->dev->dataplane;
     stb_p(&req->in->status, status);
 
-    vring_push(&req->dev->dataplane->vring, &req->elem,
+    vring_push(&s->vring, &req->elem,
                req->qiov.size + sizeof(*req->in));
-    notify_guest(req->dev->dataplane);
+
+    /* Suppress notification to guest by BH and its scheduled
+     * flag because requests are completed as a batch after io
+     * plug & unplug is introduced, and the BH can still be
+     * executed in dataplane aio context even after it is
+     * stopped, so needn't worry about notification loss with BH.
+     */
+    qemu_bh_schedule(s->bh);
 }
 
 static void handle_notify(EventNotifier *e)
@@ -172,6 +188,7 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
         s->iothread = &s->internal_iothread_obj;
     }
     s->ctx = iothread_get_aio_context(s->iothread);
+    s->bh = aio_bh_new(s->ctx, notify_guest_bh, s);
 
     error_setg(&s->blocker, "block device is in use by data plane");
     bdrv_op_block_all(blk->conf.bs, s->blocker);
@@ -190,6 +207,7 @@ void virtio_blk_data_plane_destroy(VirtIOBlockDataPlane *s)
     bdrv_op_unblock_all(s->blk->conf.bs, s->blocker);
     error_free(s->blocker);
     object_unref(OBJECT(s->iothread));
+    qemu_bh_delete(s->bh);
     g_free(s);
 }
 
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v1 1/2] virtio-blk: data-plane: fix save/set .complete_request in start

[Fam Zheng]

On Sat, 07/05 12:18, Ming Lei wrote:
> The callback has to be saved and reset in virtio_blk_data_plane_start(),
> otherwise dataplane's requests will be completed in qemu aio context.

Yes, the cb is wrong once virtio_blk_data_plane_stop is called (device reset,
etc.).

Reviewed-by: Fam Zheng <famz@redhat.com>

> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/block/dataplane/virtio-blk.c |    7 ++++---
>  1 file changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index 227bb15..e88862d 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -125,7 +125,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>                                    Error **errp)
>  {
>      VirtIOBlockDataPlane *s;
> -    VirtIOBlock *vblk = VIRTIO_BLK(vdev);
>      Error *local_err = NULL;
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(vdev)));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> @@ -178,8 +177,6 @@ void virtio_blk_data_plane_create(VirtIODevice *vdev, VirtIOBlkConf *blk,
>      bdrv_op_block_all(blk->conf.bs, s->blocker);
>  
>      *dataplane = s;
> -    s->saved_complete_request = vblk->complete_request;
> -    vblk->complete_request = complete_request_vring;
>  }
>  
>  /* Context: QEMU global mutex held */
> @@ -201,6 +198,7 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>  {
>      BusState *qbus = BUS(qdev_get_parent_bus(DEVICE(s->vdev)));
>      VirtioBusClass *k = VIRTIO_BUS_GET_CLASS(qbus);
> +    VirtIOBlock *vblk = VIRTIO_BLK(s->vdev);
>      VirtQueue *vq;
>  
>      if (s->started) {
> @@ -234,6 +232,9 @@ void virtio_blk_data_plane_start(VirtIOBlockDataPlane *s)
>      }
>      s->host_notifier = *virtio_queue_get_host_notifier(vq);
>  
> +    s->saved_complete_request = vblk->complete_request;
> +    vblk->complete_request = complete_request_vring;
> +
>      s->starting = false;
>      s->started = true;
>      trace_virtio_blk_data_plane_start(s);
> -- 
> 1.7.9.5
> 
>