[Qemu-devel] [PATCH] virtio: Do not notify virtqueue if no element was pushed back.

[Qemu-devel] [PATCH] virtio: Do not notify virtqueue if no element was pushed back.

[Gal Hammer]

Signed-off-by: Gal Hammer <ghammer@redhat.com>
---
 hw/char/virtio-serial-bus.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
index da417c7..0d38b4b 100644
--- a/hw/char/virtio-serial-bus.c
+++ b/hw/char/virtio-serial-bus.c
@@ -105,6 +105,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
                                  VirtIODevice *vdev)
 {
     VirtIOSerialPortClass *vsc;
+    bool elem_pushed = false;
 
     assert(port);
     assert(virtio_queue_ready(vq));
@@ -145,9 +146,12 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
             break;
         }
         virtqueue_push(vq, &port->elem, 0);
+        elem_pushed = true;
         port->elem.out_num = 0;
     }
-    virtio_notify(vdev, vq);
+    if (elem_pushed) {
+        virtio_notify(vdev, vq);
+    }
 }
 
 static void flush_queued_data(VirtIOSerialPort *port)
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] virtio: Do not notify virtqueue if no element was pushed back.

[Laszlo Ersek]

On 08/05/13 10:18, Gal Hammer wrote:
> Signed-off-by: Gal Hammer <ghammer@redhat.com>
> ---
>  hw/char/virtio-serial-bus.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
> index da417c7..0d38b4b 100644
> --- a/hw/char/virtio-serial-bus.c
> +++ b/hw/char/virtio-serial-bus.c
> @@ -105,6 +105,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>                                   VirtIODevice *vdev)
>  {
>      VirtIOSerialPortClass *vsc;
> +    bool elem_pushed = false;
>  
>      assert(port);
>      assert(virtio_queue_ready(vq));
> @@ -145,9 +146,12 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>              break;
>          }
>          virtqueue_push(vq, &port->elem, 0);
> +        elem_pushed = true;
>          port->elem.out_num = 0;
>      }
> -    virtio_notify(vdev, vq);
> +    if (elem_pushed) {
> +        virtio_notify(vdev, vq);
> +    }
>  }
>  
>  static void flush_queued_data(VirtIOSerialPort *port)
> 

I could be missing something, but it looks good to me.

BTW the subject should say: "virtio-serial: ...", not generic "virtio: ...".

How did you catch this? Is this a performance problem, or did it expose
a guest driver bug? (The guest driver should be prepared for spurious
wakeups.)

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

Re: [Qemu-devel] [PATCH] virtio: Do not notify virtqueue if no element was pushed back.

[Gal Hammer]

On 05/08/2013 14:49, Laszlo Ersek wrote:
> On 08/05/13 10:18, Gal Hammer wrote:
>> Signed-off-by: Gal Hammer <ghammer@redhat.com>
>> ---
>>   hw/char/virtio-serial-bus.c | 6 +++++-
>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/char/virtio-serial-bus.c b/hw/char/virtio-serial-bus.c
>> index da417c7..0d38b4b 100644
>> --- a/hw/char/virtio-serial-bus.c
>> +++ b/hw/char/virtio-serial-bus.c
>> @@ -105,6 +105,7 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>>                                    VirtIODevice *vdev)
>>   {
>>       VirtIOSerialPortClass *vsc;
>> +    bool elem_pushed = false;
>>
>>       assert(port);
>>       assert(virtio_queue_ready(vq));
>> @@ -145,9 +146,12 @@ static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>>               break;
>>           }
>>           virtqueue_push(vq, &port->elem, 0);
>> +        elem_pushed = true;
>>           port->elem.out_num = 0;
>>       }
>> -    virtio_notify(vdev, vq);
>> +    if (elem_pushed) {
>> +        virtio_notify(vdev, vq);
>> +    }
>>   }
>>
>>   static void flush_queued_data(VirtIOSerialPort *port)
>>
>
> I could be missing something, but it looks good to me.

Thanks.

> BTW the subject should say: "virtio-serial: ...", not generic "virtio: ...".

OK. I can change that.

> How did you catch this? Is this a performance problem, or did it expose
> a guest driver bug? (The guest driver should be prepared for spurious
> wakeups.)

I had a bug in the Windows' driver that caused a buffer duplication when 
the char device didn't read the whole message. It was found when working 
with SPICE client over WAN (i.e. slow connection) and trying to 
copy&paste a large image.

The driver was fixed. However I still think that this is needed although 
I'm not sure if there is a major performance penalty because of this 
redundant notification.

     Gal.

> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
>