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

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

[Ming Lei]

Hi,

The first one fixes one problem introduced recently.

The second one supresses notifications to guest.

Thanks,
--
Ming Lei 

[Qemu-devel] [PATCH 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 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 supress interrupt  notification to VM:

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

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/block/dataplane/virtio-blk.c |   13 ++++++++++++-
 hw/block/virtio-blk.c           |    1 +
 include/hw/virtio/virtio-blk.h  |    1 +
 3 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
index e88862d..9d36659 100644
--- a/hw/block/dataplane/virtio-blk.c
+++ b/hw/block/dataplane/virtio-blk.c
@@ -45,6 +45,8 @@ struct VirtIOBlockDataPlane {
     AioContext *ctx;
     EventNotifier host_notifier;    /* doorbell */
 
+    VirtIOBlockReq *last_submit;
+
     /* Operation blocker on BDS */
     Error *blocker;
     void (*saved_complete_request)(struct VirtIOBlockReq *req,
@@ -67,7 +69,10 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
 
     vring_push(&req->dev->dataplane->vring, &req->elem,
                req->qiov.size + sizeof(*req->in));
-    notify_guest(req->dev->dataplane);
+
+    if (req->last) {
+        notify_guest(req->dev->dataplane);
+    }
 }
 
 static void handle_notify(EventNotifier *e)
@@ -101,6 +106,8 @@ static void handle_notify(EventNotifier *e)
                                                         req->elem.index);
 
             virtio_blk_handle_request(req, &mrb);
+
+            s->last_submit = req;
         }
 
         virtio_submit_multiwrite(s->blk->conf.bs, &mrb);
@@ -116,6 +123,10 @@ static void handle_notify(EventNotifier *e)
             break;
         }
     }
+
+    if (s->last_submit) {
+        s->last_submit->last = true;
+    }
     bdrv_io_unplug(s->blk->conf.bs);
 }
 
diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
index 02cd6b0..86b37f7 100644
--- a/hw/block/virtio-blk.c
+++ b/hw/block/virtio-blk.c
@@ -35,6 +35,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
     req->dev = s;
     req->qiov.size = 0;
     req->next = NULL;
+    req->last = false;
     return req;
 }
 
diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
index 992da26..07659c3 100644
--- a/include/hw/virtio/virtio-blk.h
+++ b/include/hw/virtio/virtio-blk.h
@@ -150,6 +150,7 @@ typedef struct VirtIOBlockReq {
     QEMUIOVector qiov;
     struct VirtIOBlockReq *next;
     BlockAcctCookie acct;
+    bool last;
 } VirtIOBlockReq;
 
 #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
-- 
1.7.9.5

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

[Paolo Bonzini]

Il 04/07/2014 14:27, Ming Lei ha scritto:
> Now requests are submitted as a batch, so it is natural
> to notify guest as a batch too.
>
> This may supress interrupt  notification to VM:
>
> 	- in my test, decreased by ~13K/sec

I don't think this can work.  Requests are not completed FIFO.

What you can do is change notify_guest to something like

     qemu_bh_schedule(req->dev->dataplane->notify_guest_bh);

and do the actual notification in the bottom half.  This should ensure 
that multiple notifications are coalesced, but it may also introduce new 
aio_notify calls even with my patch (a BH scheduled from a BH currently 
does an aio_notify; this can be fixed).

Paolo

> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/block/dataplane/virtio-blk.c |   13 ++++++++++++-
>  hw/block/virtio-blk.c           |    1 +
>  include/hw/virtio/virtio-blk.h  |    1 +
>  3 files changed, 14 insertions(+), 1 deletion(-)
>
> diff --git a/hw/block/dataplane/virtio-blk.c b/hw/block/dataplane/virtio-blk.c
> index e88862d..9d36659 100644
> --- a/hw/block/dataplane/virtio-blk.c
> +++ b/hw/block/dataplane/virtio-blk.c
> @@ -45,6 +45,8 @@ struct VirtIOBlockDataPlane {
>      AioContext *ctx;
>      EventNotifier host_notifier;    /* doorbell */
>
> +    VirtIOBlockReq *last_submit;
> +
>      /* Operation blocker on BDS */
>      Error *blocker;
>      void (*saved_complete_request)(struct VirtIOBlockReq *req,
> @@ -67,7 +69,10 @@ static void complete_request_vring(VirtIOBlockReq *req, unsigned char status)
>
>      vring_push(&req->dev->dataplane->vring, &req->elem,
>                 req->qiov.size + sizeof(*req->in));
> -    notify_guest(req->dev->dataplane);
> +
> +    if (req->last) {
> +        notify_guest(req->dev->dataplane);
> +    }
>  }
>
>  static void handle_notify(EventNotifier *e)
> @@ -101,6 +106,8 @@ static void handle_notify(EventNotifier *e)
>                                                          req->elem.index);
>
>              virtio_blk_handle_request(req, &mrb);
> +
> +            s->last_submit = req;
>          }
>
>          virtio_submit_multiwrite(s->blk->conf.bs, &mrb);
> @@ -116,6 +123,10 @@ static void handle_notify(EventNotifier *e)
>              break;
>          }
>      }
> +
> +    if (s->last_submit) {
> +        s->last_submit->last = true;
> +    }
>      bdrv_io_unplug(s->blk->conf.bs);
>  }
>
> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
> index 02cd6b0..86b37f7 100644
> --- a/hw/block/virtio-blk.c
> +++ b/hw/block/virtio-blk.c
> @@ -35,6 +35,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
>      req->dev = s;
>      req->qiov.size = 0;
>      req->next = NULL;
> +    req->last = false;
>      return req;
>  }
>
> diff --git a/include/hw/virtio/virtio-blk.h b/include/hw/virtio/virtio-blk.h
> index 992da26..07659c3 100644
> --- a/include/hw/virtio/virtio-blk.h
> +++ b/include/hw/virtio/virtio-blk.h
> @@ -150,6 +150,7 @@ typedef struct VirtIOBlockReq {
>      QEMUIOVector qiov;
>      struct VirtIOBlockReq *next;
>      BlockAcctCookie acct;
> +    bool last;
>  } VirtIOBlockReq;
>
>  #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>

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

[Ming Lei]

On Fri, Jul 4, 2014 at 8:55 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/07/2014 14:27, Ming Lei ha scritto:
>
>> Now requests are submitted as a batch, so it is natural
>> to notify guest as a batch too.
>>
>> This may supress interrupt  notification to VM:
>>
>>         - in my test, decreased by ~13K/sec
>
>
> I don't think this can work.  Requests are not completed FIFO.

Yes, you are right.

What I want to do is to keep old dataplane's behavior: only call
notify_guest() one time for one IO completion.

Switching to QEMU block makes it a bit difficult to implement, especially
with io plug & unplug.

One approach I thought of is to introduce a notifier in bs->file, and
write it inside linux aio completion handler, which looks a bit ugly, but
shouldn't have performance effect because io completion becomes
less frequent with io plug.

>
> What you can do is change notify_guest to something like
>
>     qemu_bh_schedule(req->dev->dataplane->notify_guest_bh);
>
> and do the actual notification in the bottom half.  This should ensure that
> multiple notifications are coalesced, but it may also introduce new
> aio_notify calls even with my patch (a BH scheduled from a BH currently does
> an aio_notify; this can be fixed).

I think we can do better than above because one aio IO completes lots of
requests, and we just need to notify guest for all these requests.

Thanks,


>
> Paolo
>
>
>> Signed-off-by: Ming Lei <ming.lei@canonical.com>
>> ---
>>  hw/block/dataplane/virtio-blk.c |   13 ++++++++++++-
>>  hw/block/virtio-blk.c           |    1 +
>>  include/hw/virtio/virtio-blk.h  |    1 +
>>  3 files changed, 14 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/block/dataplane/virtio-blk.c
>> b/hw/block/dataplane/virtio-blk.c
>> index e88862d..9d36659 100644
>> --- a/hw/block/dataplane/virtio-blk.c
>> +++ b/hw/block/dataplane/virtio-blk.c
>> @@ -45,6 +45,8 @@ struct VirtIOBlockDataPlane {
>>      AioContext *ctx;
>>      EventNotifier host_notifier;    /* doorbell */
>>
>> +    VirtIOBlockReq *last_submit;
>> +
>>      /* Operation blocker on BDS */
>>      Error *blocker;
>>      void (*saved_complete_request)(struct VirtIOBlockReq *req,
>> @@ -67,7 +69,10 @@ static void complete_request_vring(VirtIOBlockReq *req,
>> unsigned char status)
>>
>>      vring_push(&req->dev->dataplane->vring, &req->elem,
>>                 req->qiov.size + sizeof(*req->in));
>> -    notify_guest(req->dev->dataplane);
>> +
>> +    if (req->last) {
>> +        notify_guest(req->dev->dataplane);
>> +    }
>>  }
>>
>>  static void handle_notify(EventNotifier *e)
>> @@ -101,6 +106,8 @@ static void handle_notify(EventNotifier *e)
>>                                                          req->elem.index);
>>
>>              virtio_blk_handle_request(req, &mrb);
>> +
>> +            s->last_submit = req;
>>          }
>>
>>          virtio_submit_multiwrite(s->blk->conf.bs, &mrb);
>> @@ -116,6 +123,10 @@ static void handle_notify(EventNotifier *e)
>>              break;
>>          }
>>      }
>> +
>> +    if (s->last_submit) {
>> +        s->last_submit->last = true;
>> +    }
>>      bdrv_io_unplug(s->blk->conf.bs);
>>  }
>>
>> diff --git a/hw/block/virtio-blk.c b/hw/block/virtio-blk.c
>> index 02cd6b0..86b37f7 100644
>> --- a/hw/block/virtio-blk.c
>> +++ b/hw/block/virtio-blk.c
>> @@ -35,6 +35,7 @@ VirtIOBlockReq *virtio_blk_alloc_request(VirtIOBlock *s)
>>      req->dev = s;
>>      req->qiov.size = 0;
>>      req->next = NULL;
>> +    req->last = false;
>>      return req;
>>  }
>>
>> diff --git a/include/hw/virtio/virtio-blk.h
>> b/include/hw/virtio/virtio-blk.h
>> index 992da26..07659c3 100644
>> --- a/include/hw/virtio/virtio-blk.h
>> +++ b/include/hw/virtio/virtio-blk.h
>> @@ -150,6 +150,7 @@ typedef struct VirtIOBlockReq {
>>      QEMUIOVector qiov;
>>      struct VirtIOBlockReq *next;
>>      BlockAcctCookie acct;
>> +    bool last;
>>  } VirtIOBlockReq;
>>
>>  #define DEFINE_VIRTIO_BLK_FEATURES(_state, _field) \
>>
>
>

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

[Paolo Bonzini]

Il 04/07/2014 16:52, Ming Lei ha scritto:
>> > What you can do is change notify_guest to something like
>> >
>> >     qemu_bh_schedule(req->dev->dataplane->notify_guest_bh);
>> >
>> > and do the actual notification in the bottom half.  This should ensure that
>> > multiple notifications are coalesced, but it may also introduce new
>> > aio_notify calls even with my patch (a BH scheduled from a BH currently does
>> > an aio_notify; this can be fixed).
> I think we can do better than above because one aio IO completes lots of
> requests, and we just need to notify guest for all these requests.

Exactly, there would be just one BH and thus just one notify.

Paolo

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

[Ming Lei]

On Fri, Jul 4, 2014 at 11:48 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/07/2014 16:52, Ming Lei ha scritto:
>
>>> > What you can do is change notify_guest to something like
>>> >
>>> >     qemu_bh_schedule(req->dev->dataplane->notify_guest_bh);
>>> >
>>> > and do the actual notification in the bottom half.  This should ensure
>>> > that
>>> > multiple notifications are coalesced, but it may also introduce new
>>> > aio_notify calls even with my patch (a BH scheduled from a BH currently
>>> > does
>>> > an aio_notify; this can be fixed).
>>
>> I think we can do better than above because one aio IO completes lots of
>> requests, and we just need to notify guest for all these requests.
>
>
> Exactly, there would be just one BH and thus just one notify.

But we have two cases to consider:

- one submitted IO includes requests from multi vq(virtio-blk or
virtio-scsi maybe),
and each vq has to notify guest

- one submitted IO includes requests from multi bs for scsi device

The 2nd case should be easy to handle, but I am a bit headache for the 1st case.



Thanks,
--
Ming Lei

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

[Paolo Bonzini]

Il 04/07/2014 17:57, Ming Lei ha scritto:
> But we have two cases to consider:
>
> - one submitted IO includes requests from multi vq(virtio-blk or
> virtio-scsi maybe),
> and each vq has to notify guest
>
> - one submitted IO includes requests from multi bs for scsi device
>
> The 2nd case should be easy to handle, but I am a bit headache for the 1st case.

You can just have one bottom half per virtqueue. :)

Paolo

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

[Ming Lei]

On Sat, Jul 5, 2014 at 12:09 AM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 04/07/2014 17:57, Ming Lei ha scritto:
>
>> But we have two cases to consider:
>>
>> - one submitted IO includes requests from multi vq(virtio-blk or
>> virtio-scsi maybe),
>> and each vq has to notify guest
>>
>> - one submitted IO includes requests from multi bs for scsi device
>>
>> The 2nd case should be easy to handle, but I am a bit headache for the 1st
>> case.
>
>
> You can just have one bottom half per virtqueue. :)

Exactly, or one BH plus per virtqueue bit flag should be enough too, :-)

Thanks,
--
Ming Lei