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

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

[Ming Lei]

The 1st patch fixes an allocation problem.

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

Thanks,
Ming Lei

[Qemu-devel] [PATCH 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..8a7cd9f 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_malloc0(sizeof(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 1/2] virtio-scsi-dataplane: fix allocation for 'cmd_vrings'

[Markus Armbruster]

Ming Lei <ming.lei@canonical.com> writes:

> 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..8a7cd9f 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_malloc0(sizeof(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],

Please use something like

    s->cmd_vrings = g_new0(VirtIOSCSIVring *, vs->conf.num_queues);

This one crept in since I cleaned up g_malloc() use globally:

commit 02c4f26b1517d9e403ec10d6f6ca3c0276d19e43
Author: Markus Armbruster <armbru@redhat.com>
Date:   Tue Aug 19 10:31:09 2014 +0200

    block: Use g_new() & friends to avoid multiplying sizes
    
    g_new(T, n) is safer than g_malloc(sizeof(*v) * n) for two reasons.
    One, it catches multiplication overflowing size_t.  Two, it returns
    T * rather than void *, which lets the compiler catch more type
    errors.
    
    Perhaps a conversion to g_malloc_n() would be neater in places, but
    that's merely four years old, and we can't use such newfangled stuff.
    
    This commit only touches allocations with size arguments of the form
    sizeof(T), plus two that use 4 instead of sizeof(uint32_t).  We can
    make the others safe by converting to g_malloc_n() when it becomes
    available to us in a couple of years.

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

[Ming Lei]

On Mon, Nov 10, 2014 at 4:24 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Ming Lei <ming.lei@canonical.com> writes:
>
>> 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..8a7cd9f 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_malloc0(sizeof(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],
>
> Please use something like
>
>     s->cmd_vrings = g_new0(VirtIOSCSIVring *, vs->conf.num_queues);
>
> This one crept in since I cleaned up g_malloc() use globally:

Your idea is good, but this one is a fix patch, and I
think the g_new() conversion should be done in another
patch since the two changes are different logically.

Thanks,

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

[Kevin Wolf]

Am 10.11.2014 um 10:14 hat Ming Lei geschrieben:
> On Mon, Nov 10, 2014 at 4:24 PM, Markus Armbruster <armbru@redhat.com> wrote:
> > Ming Lei <ming.lei@canonical.com> writes:
> >
> >> 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..8a7cd9f 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_malloc0(sizeof(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],
> >
> > Please use something like
> >
> >     s->cmd_vrings = g_new0(VirtIOSCSIVring *, vs->conf.num_queues);
> >
> > This one crept in since I cleaned up g_malloc() use globally:
> 
> Your idea is good, but this one is a fix patch, and I
> think the g_new() conversion should be done in another
> patch since the two changes are different logically.

It's not really unrelated: g_new() would have caught the incorrect type
and made it a compiler error. So changing to g_new() in a patch fixing
such a bug is actually a logical conclusion and makes it more obvious
that your patch is correct.

Kevin

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

[Ming Lei]

On Mon, Nov 10, 2014 at 5:21 PM, Kevin Wolf <kwolf@redhat.com> wrote:
> Am 10.11.2014 um 10:14 hat Ming Lei geschrieben:
>> On Mon, Nov 10, 2014 at 4:24 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> > Ming Lei <ming.lei@canonical.com> writes:
>> >
>> >> 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..8a7cd9f 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_malloc0(sizeof(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],
>> >
>> > Please use something like
>> >
>> >     s->cmd_vrings = g_new0(VirtIOSCSIVring *, vs->conf.num_queues);
>> >
>> > This one crept in since I cleaned up g_malloc() use globally:
>>
>> Your idea is good, but this one is a fix patch, and I
>> think the g_new() conversion should be done in another
>> patch since the two changes are different logically.
>
> It's not really unrelated: g_new() would have caught the incorrect type
> and made it a compiler error. So changing to g_new() in a patch fixing
> such a bug is actually a logical conclusion and makes it more obvious
> that your patch is correct.

Fair enough, will post v1 with g_new() conversion.

Thanks,

[Qemu-devel] [PATCH 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           |   22 ++++++++++++++++++++++
 include/hw/virtio/virtio-scsi.h |    4 ++++
 3 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
index 8a7cd9f..1fb83de 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..1b9c35c 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");
@@ -825,6 +840,10 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
 
     if (s->conf.iothread) {
         virtio_scsi_set_iothread(VIRTIO_SCSI(s), s->conf.iothread);
+        VIRTIO_SCSI(s)->pending_guest_notify = 0;
+        VIRTIO_SCSI(s)->guest_notify_bh = aio_bh_new(VIRTIO_SCSI(s)->ctx,
+                                                     notify_guest_bh,
+                                                     VIRTIO_SCSI(s));
     }
 }
 
@@ -902,6 +921,9 @@ void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp)
     VirtIODevice *vdev = VIRTIO_DEVICE(dev);
     VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
 
+    if (vs->conf.iothread) {
+        qemu_bh_delete(VIRTIO_SCSI(vs)->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 2/2] virtio-scsi-dataplane: notify guest as batch

[Paolo Bonzini]

On 09/11/2014 16:50, 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           |   22 ++++++++++++++++++++++
>  include/hw/virtio/virtio-scsi.h |    4 ++++
>  3 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/scsi/virtio-scsi-dataplane.c b/hw/scsi/virtio-scsi-dataplane.c
> index 8a7cd9f..1fb83de 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..1b9c35c 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");
> @@ -825,6 +840,10 @@ void virtio_scsi_common_realize(DeviceState *dev, Error **errp,
>  
>      if (s->conf.iothread) {
>          virtio_scsi_set_iothread(VIRTIO_SCSI(s), s->conf.iothread);
> +        VIRTIO_SCSI(s)->pending_guest_notify = 0;
> +        VIRTIO_SCSI(s)->guest_notify_bh = aio_bh_new(VIRTIO_SCSI(s)->ctx,
> +                                                     notify_guest_bh,
> +                                                     VIRTIO_SCSI(s));

Please use a separete variable to host VIRTIO_SCSI(s).

This is usually the standard if the result of the cast is dereferenced.

>      }
>  }
>  
> @@ -902,6 +921,9 @@ void virtio_scsi_common_unrealize(DeviceState *dev, Error **errp)
>      VirtIODevice *vdev = VIRTIO_DEVICE(dev);
>      VirtIOSCSICommon *vs = VIRTIO_SCSI_COMMON(dev);
>  
> +    if (vs->conf.iothread) {
> +        qemu_bh_delete(VIRTIO_SCSI(vs)->guest_notify_bh);

Here too.

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

Please add a

BUILD_BUG_ON(VIRTIO_PCI_QUEUE_MAX <= 64);

too.

>  } VirtIOSCSI;
>  
>  typedef struct VirtIOSCSIReq {
> 

Thanks,

Paolo