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

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

[Gal Hammer]

The redundant notification caused the Windows' driver to duplicate the
pending write request's buffer. The driver was fixed, but I think this
change is still required.

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 v2] virtio-serial: Do not notify virtqueue if no element was pushed back.

[Laszlo Ersek]

On 08/11/13 15:25, Gal Hammer wrote:
> The redundant notification caused the Windows' driver to duplicate the
> pending write request's buffer. The driver was fixed, but I think this
> change is still required.
> 
> 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)
> 

Differs from v1 only in a more precise subject / more details in the
commit msg.

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

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

[Amit Shah]

Hi,

On (Sun) 11 Aug 2013 [16:25:25], Gal Hammer wrote:
> The redundant notification caused the Windows' driver to duplicate the
> pending write request's buffer. The driver was fixed, but I think this
> change is still required.
> 
> 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);
> +    }
>  }

Can you describe when you hit this bug in the commit message?

I can imagine something like this happening when a fast guest is
sending lots of data, while the host is slow to catch up, e.g. the
host chardev is a file on disk, and that takes a while to flush the
data from the guest, filling the chardev buffer, and causing the flow
control to kick in.  While in that state, if the guest writes more
data to the virtqueue, a few iterations would occur where the host
will not pop any item from the queue, and the guest is kicked
unnecessarily.

Is that what you're seeing?  Your reply to v1 of the patch doesn't
quite say that clearly.

Patch looks good, thanks.


		Amit