[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 gham...@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.

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

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 gham...@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 
copypaste 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 ler...@redhat.com