| Message ID | 1375690705-17266-1-git-send-email-ghammer@redhat.com |
|---|---|
| State | New |
| Headers | show |
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>
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> >
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)
Signed-off-by: Gal Hammer <ghammer@redhat.com> --- hw/char/virtio-serial-bus.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)