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 <address@hidden>
> > ---
> >   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 <address@hidden>