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

2013-08-05 Thread Gal Hammer 
Signed-off-by: Gal Hammer 
---
 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.

2013-08-05 Thread Laszlo Ersek 
On 08/05/13 10:18, Gal Hammer wrote:
> Signed-off-by: Gal Hammer 
> ---
>  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

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

2013-08-05 Thread 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 
---
  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