Re: [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus

[Anthony Liguori]

On 12/23/2009 11:25 PM, Amit Shah wrote:
> On (Wed) Dec 23 2009 [17:12:22], Anthony Liguori wrote:


> > > +struct VirtIOSerial {
> > > +    VirtIODevice vdev;
> > > +
> > > +    VirtQueue *c_ivq, *c_ovq;
> > > +    /* Arrays of ivqs and ovqs: one per port */
> > > +    VirtQueue **ivqs, **ovqs;
> > > +
> > > +    VirtIOSerialBus *bus;
> > > +
> > > +    QTAILQ_HEAD(, VirtIOSerialPort) ports;
> > > +    struct virtio_console_config config;
> > > +
> > > +    /*
> > > +     * This lock serialises writes to the guest via the ivq
> > > +     */
> > > +    pthread_mutex_t ivq_lock;
> >
> > This indicates some thing is wrong.  You should never need a mutex in a
> > device.
>
> You mean in the no locking needed sense, or not using a mutex sense? If
> the latter, what's the recommended way to do the locking?

There's a big global lock so this level of locking in the device is 
unnecessary.  Since we don't know what our re-entrance points are going 
to be, it's also incorrect because it makes assumptions that may not be 
true in the future.

> > > +    int header_len;
> > > +
> > > +    vq = port->ivq;
> > > +    if (!virtio_queue_ready(vq)) {
> > > +        return 0;
> > > +    }
> > > +    if (!size) {
> > > +        return 0;
> > > +    }
> > > +    header.flags = 0;
> > > +    header_len = use_multiport(port->vser) ? sizeof(header) : 0;
> > > +
> > > +    pthread_mutex_lock(&port->vser->ivq_lock);
> > > +    while (offset<   size) {
> >
> > CodingStyle seems consistently off in a curious way.  Always add spaces
> > after arithmetic operators.
>
> Curious case of patches getting modified for whitespace somewhere? (Or
> the mail client doing it?)
>
> I see the patch is fine in the copy I got CC'ed on; I deleted the one I
> got for the qemu-devel list so I can't verify that...
>
> http://article.gmane.org/gmane.comp.emulators.qemu/60257
>
> shows it made it fine to the list, so guess it's your mail client doing
> something funny.

Yup.  It's a thunderbird bug.  I upgraded and now it looks right.  Sorry 
for the noise.

> > The pthread_mutex_lock() can't be right.  qemu already runs with a
> > single global lock.  Even if you had another thread, there's no easy way
> > you could safely hold this lock without grabbing the global qemu lock.
>
> I know my current locking scheme is inadequate; but what's the
> preference here? I can remove the locking for now but we'll definitely
> want qemu to have more fine-grained locking, so when that happens,
> revisit all the code to ensure they're safe?
> on on this structure.

Definitely remove it.  Untested code is broken code and it will break 
the build on Windows.

> I'll annotate and read/write using the le format.

Just use ldl_p and stl_p. (or ldw/stw as appropriate).

> > > +static void virtser_bus_dev_print(Monitor *mon, DeviceState *qdev, int indent)
> > > +{
> > > +    VirtIOSerialDevice *dev = DO_UPCAST(VirtIOSerialDevice, qdev, qdev);
> > > +    VirtIOSerialPort *port = DO_UPCAST(VirtIOSerialPort, dev,&dev->qdev);
> > > +
> > > +    monitor_printf(mon, "%*s dev-prop-int: id: %u\n",
> > > +                   indent, "", port->id);
> > > +    monitor_printf(mon, "%*s dev-prop-int: is_console: %d\n",
> > > +                   indent, "", port->is_console);
> > > +}
> >
> >
> > This doesn't look used to me.
>
> It's helpful for debugging purposes, mostly: 'info qtree' on the monitor
> will print this out and one can examine port state.

Unused static functions will cause the build to fail with -Werror.

>                 Amit