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

[Fam Zheng]

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?

> +        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.

>  } VirtIOSCSIVring;
>  
>  typedef struct VirtIOSCSICommon {
> @@ -198,6 +199,9 @@ typedef struct VirtIOSCSI {
>      bool dataplane_fenced;
>      Error *blocker;
>      Notifier migration_state_notifier;
> +
> +    QEMUBH *guest_notify_bh;
> +    uint64_t pending_guest_notify;

Fam

>  } VirtIOSCSI;
>  
>  typedef struct VirtIOSCSIReq {
> -- 
> 1.7.9.5
>