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

[Ming Lei]

On Tue, Nov 11, 2014 at 9:52 AM, Fam Zheng <address@hidden> wrote:
> On Tue, 11/11 09:17, Ming Lei wrote:
>> It isn't necessery to notify guest each time when one request
>> is completed, and it should be enough to just notify one time
>> for each running of virtio_scsi_iothread_handle_cmd().
>>
>> This patch supresses about 30K/sec write on eventfd.
>>
>> Signed-off-by: Ming Lei <address@hidden>
>> ---
>>  hw/scsi/virtio-scsi-dataplane.c |    4 +++-
>>  hw/scsi/virtio-scsi.c           |   26 +++++++++++++++++++++++++-
>>  include/hw/virtio/virtio-scsi.h |    4 ++++
>>  3 files changed, 32 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/scsi/virtio-scsi-dataplane.c 
>> b/hw/scsi/virtio-scsi-dataplane.c
>> index df17229..294515a 100644
>> --- a/hw/scsi/virtio-scsi-dataplane.c
>> +++ b/hw/scsi/virtio-scsi-dataplane.c
>> @@ -62,6 +62,7 @@ static VirtIOSCSIVring *virtio_scsi_vring_init(VirtIOSCSI 
>> *s,
>>      aio_set_event_notifier(s->ctx, &r->host_notifier, handler);
>>
>>      r->parent = s;
>> +    r->qid = n;
>>
>>      if (!vring_setup(&r->vring, VIRTIO_DEVICE(s), n)) {
>>          fprintf(stderr, "virtio-scsi: VRing setup failed\n");
>> @@ -95,7 +96,8 @@ void virtio_scsi_vring_push_notify(VirtIOSCSIReq *req)
>>  {
>>      vring_push(&req->vring->vring, &req->elem,
>>                 req->qsgl.size + req->resp_iov.size);
>> -    event_notifier_set(&req->vring->guest_notifier);
>> +    req->dev->pending_guest_notify |= 1 << (req->vring->qid - 2);
>> +    qemu_bh_schedule(req->dev->guest_notify_bh);
>>  }
>>
>>  static void virtio_scsi_iothread_handle_ctrl(EventNotifier *notifier)
>> diff --git a/hw/scsi/virtio-scsi.c b/hw/scsi/virtio-scsi.c
>> index 6e34a2c..98411ef 100644
>> --- a/hw/scsi/virtio-scsi.c
>> +++ b/hw/scsi/virtio-scsi.c
>> @@ -86,6 +86,21 @@ static void virtio_scsi_complete_req(VirtIOSCSIReq *req)
>>      virtio_scsi_free_req(req);
>>  }
>>
>> +static void notify_guest_bh(void *opaque)
>> +{
>> +    VirtIOSCSI *s = opaque;
>> +    unsigned int qid;
>> +    uint64_t pending = s->pending_guest_notify;
>> +
>> +    s->pending_guest_notify = 0;
>> +
>> +    while ((qid = ffsl(pending))) {
>> +        qid--;
>> +        event_notifier_set(&s->cmd_vrings[qid]->guest_notifier);
>
> Don't we need to handle ctrlq and eventq as well?

Most of times no frequent activity in ctrl and event vq, so it isn't
necessary to make them involved.

>
>> +        pending &= ~(1 << qid);
>> +    }
>> +}
>> +
>>  static void virtio_scsi_bad_req(void)
>>  {
>>      error_report("wrong size for virtio-scsi headers");
>> @@ -824,7 +839,12 @@ void virtio_scsi_common_realize(DeviceState *dev, Error 
>> **errp,
>>      }
>>
>>      if (s->conf.iothread) {
>> -        virtio_scsi_set_iothread(VIRTIO_SCSI(s), s->conf.iothread);
>> +        VirtIOSCSI *vis = VIRTIO_SCSI(s);
>> +
>> +        QEMU_BUILD_BUG_ON(VIRTIO_PCI_QUEUE_MAX > 64);
>> +        virtio_scsi_set_iothread(vis, s->conf.iothread);
>> +        vis->pending_guest_notify = 0;
>> +        vis->guest_notify_bh = aio_bh_new(vis->ctx, notify_guest_bh, vis);
>>      }
>>  }
>>
>> @@ -901,7 +921,11 @@ void virtio_scsi_common_unrealize(DeviceState *dev, 
>> Error **errp)
>>  {
>>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>> +    VirtIOSCSI *s = VIRTIO_SCSI(vs);
>>
>> +    if (vs->conf.iothread) {
>> +        qemu_bh_delete(s->guest_notify_bh);
>> +    }
>>      g_free(vs->cmd_vqs);
>>      virtio_cleanup(vdev);
>>  }
>> diff --git a/include/hw/virtio/virtio-scsi.h 
>> b/include/hw/virtio/virtio-scsi.h
>> index 9e1a49c..5e6c57e 100644
>> --- a/include/hw/virtio/virtio-scsi.h
>> +++ b/include/hw/virtio/virtio-scsi.h
>> @@ -163,6 +163,7 @@ typedef struct {
>>      Vring vring;
>>      EventNotifier host_notifier;
>>      EventNotifier guest_notifier;
>> +    uint32_t qid;
>
> Could this "bool notify_pending"? In this case pending_guest_notify in
> VirtIOSCSI is not necessary. I guess looking into no more than 64
> VirtIOSCSIVring elements in guest_notify_bh is not _that_ expensive so it's 
> not
> worth the complexity.

We need to know which queue the pending notifier is sent to in BH
handler, and 64 is the limit for queue number, not element number.

Actually it is a bit expensive, and 30K/sec write syscall is wasted in
the unnecessary & repeated notifying.

And the similar approach is applied in virtio-blk dataplane too, :-)


Thanks,
Ming Lei