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 <address@hidden> 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 <address@hidden>
>> ---
>>  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) \
>>
>
>