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

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

[Ming Lei]

Hi,

The 1st patch fixes an allocation problem.

The 2nd one supresses writing eventfd a lot(~30K/sec in my test).

V1:
	- use g_new() in 1/2
	- avoid to dereference VIRTIO_SCSI() directly 2/2
	- add build check on queue depth

Thanks,
Ming Lei

[Qemu-devel] [PATCH v1 1/2] virtio-scsi: dataplane: fix allocation for 'cmd_vrings'

[Ming Lei]

The size of each element should be sizeof(VirtIOSCSIVring *).

Signed-off-by: Ming Lei <ming.lei@canonical.com>
---
 hw/scsi/virtio-scsi-dataplane.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 855439e..df17229 100644
--- a/hw/scsi/virtio-scsi-dataplane.c
+++ b/hw/scsi/virtio-scsi-dataplane.c
@@ -239,7 +239,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
     if (!s->event_vring) {
         goto fail_vrings;
     }
-    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * vs->conf.num_queues);
+    s->cmd_vrings = g_new(VirtIOSCSIVring *, vs->conf.num_queues);
     for (i = 0; i < vs->conf.num_queues; i++) {
         s->cmd_vrings[i] =
             virtio_scsi_vring_init(s, vs->cmd_vqs[i],
-- 
1.7.9.5

Re: [Qemu-devel] [PATCH v1 1/2] virtio-scsi: dataplane: fix allocation for 'cmd_vrings'

[Fam Zheng]

On Tue, 11/11 09:17, Ming Lei wrote:
> The size of each element should be sizeof(VirtIOSCSIVring *).
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 855439e..df17229 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -239,7 +239,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
>      if (!s->event_vring) {
>          goto fail_vrings;
>      }
> -    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * vs->conf.num_queues);
> +    s->cmd_vrings = g_new(VirtIOSCSIVring *, vs->conf.num_queues);
>      for (i = 0; i < vs->conf.num_queues; i++) {
>          s->cmd_vrings[i] =
>              virtio_scsi_vring_init(s, vs->cmd_vqs[i],
> -- 
> 1.7.9.5
> 

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

Re: [Qemu-devel] [PATCH v1 1/2] virtio-scsi: dataplane: fix allocation for 'cmd_vrings'

[Paolo Bonzini]

On 11/11/2014 02:17, Ming Lei wrote:
> The size of each element should be sizeof(VirtIOSCSIVring *).
> 
> Signed-off-by: Ming Lei <ming.lei@canonical.com>
> ---
>  hw/scsi/virtio-scsi-dataplane.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 855439e..df17229 100644
> --- a/hw/scsi/virtio-scsi-dataplane.c
> +++ b/hw/scsi/virtio-scsi-dataplane.c
> @@ -239,7 +239,7 @@ void virtio_scsi_dataplane_start(VirtIOSCSI *s)
>      if (!s->event_vring) {
>          goto fail_vrings;
>      }
> -    s->cmd_vrings = g_malloc0(sizeof(VirtIOSCSIVring) * vs->conf.num_queues);
> +    s->cmd_vrings = g_new(VirtIOSCSIVring *, vs->conf.num_queues);
>      for (i = 0; i < vs->conf.num_queues; i++) {
>          s->cmd_vrings[i] =
>              virtio_scsi_vring_init(s, vs->cmd_vqs[i],
> 

Applied, thanks.

Paolo

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

[Ming Lei]

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 <ming.lei@canonical.com>
---
 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);
+        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;
 } 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;
 } VirtIOSCSI;
 
 typedef struct VirtIOSCSIReq {
-- 
1.7.9.5

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 <ming.lei@canonical.com>
> ---
>  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
> 

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 <famz@redhat.com> 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 <ming.lei@canonical.com>
>> ---
>>  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

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

[Fam Zheng]

On Tue, 11/11 10:29, Ming Lei wrote:
> On Tue, Nov 11, 2014 at 9:52 AM, Fam Zheng <famz@redhat.com> 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 <ming.lei@canonical.com>
> >> ---
> >>  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.

I mean virtio_scsi_vring_push_notify is not only used to notify cmd, it should
also work for ctrl and event, because its caller virtio_scsi_complete_req is
also called in virtio_scsi_handle_ctrl_req and virtio_scsi_push_event.

> 
> >
> >> +        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, :-)
> 

I mean something like:

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;
+    bool notify_pending;

@@ -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->vring->pending_notify = true;
+    qemu_bh_schedule(req->dev->guest_notify_bh);
 }

And in notify_guest_bh you go through all the vrings and call
event_notifier_set on those with vring->pending_notify == true.

Fam

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

[Ming Lei]

On Tue, Nov 11, 2014 at 10:52 AM, Fam Zheng <famz@redhat.com> wrote:
> On Tue, 11/11 10:29, Ming Lei wrote:
>> On Tue, Nov 11, 2014 at 9:52 AM, Fam Zheng <famz@redhat.com> 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 <ming.lei@canonical.com>
>> >> ---
>> >>  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.
>
> I mean virtio_scsi_vring_push_notify is not only used to notify cmd, it should
> also work for ctrl and event, because its caller virtio_scsi_complete_req is
> also called in virtio_scsi_handle_ctrl_req and virtio_scsi_push_event.
>
>>
>> >
>> >> +        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, :-)
>>
>
> I mean something like:
>
> 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;
> +    bool notify_pending;
>
> @@ -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->vring->pending_notify = true;
> +    qemu_bh_schedule(req->dev->guest_notify_bh);
>  }
>
> And in notify_guest_bh you go through all the vrings and call
> event_notifier_set on those with vring->pending_notify == true.

You are right, and I missed ctrl and event vq, and it should be
simpler to introduce vring->pending_notify.

Will do that in v2, thanks.

Thanks,

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

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

On Tue, Nov 11, 2014 at 09:17:10AM +0800, Ming Lei wrote:
> +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);
> +        pending &= ~(1 << qid);
> +    }
> +}

Looks like we're not honoring virtio's usual interrupt mitigation
mechanism (e.g. EVENT_IDX) for virtio-scsi.  Why is
vring_should_notify() not used?

> +
>  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);

Have you checked state transitions like PCI or virtio reset?  They need
to cancel the BH and clear ->pending_guest_notify.

There is also a transition from dataplane to non-dataplane mode for live
migration.

Please make sure no interrupts are dropped or stale data is kept across
these transitions.

[-- Attachment #2: Type: application/pgp-signature, Size: 473 bytes --]

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

[Ming Lei]

On Wed, Nov 12, 2014 at 1:28 AM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> On Tue, Nov 11, 2014 at 09:17:10AM +0800, Ming Lei wrote:
>> +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);
>> +        pending &= ~(1 << qid);
>> +    }
>> +}
>
> Looks like we're not honoring virtio's usual interrupt mitigation
> mechanism (e.g. EVENT_IDX) for virtio-scsi.  Why is
> vring_should_notify() not used?

Yes, that should be used, but in virtio-blk, BH still can suppress
about ~13K/sec write(), and for virtio-scsi, it should be more since
there may be more queues.

>
>> +
>>  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);
>
> Have you checked state transitions like PCI or virtio reset?  They need
> to cancel the BH and clear ->pending_guest_notify.
>
> There is also a transition from dataplane to non-dataplane mode for live
> migration.
>
> Please make sure no interrupts are dropped or stale data is kept across
> these transitions.

Good catch, and I will make sure that in next version, and virtio-blk
dataplane need to consider that too.

Thanks,