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

[Anthony Liguori <anthony@codemonkey.ws>]

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