From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43) id 1NSFgy-000765-Cb for qemu-devel@nongnu.org; Tue, 05 Jan 2010 15:07:48 -0500 Received: from exim by lists.gnu.org with spam-scanned (Exim 4.43) id 1NSFgt-00073d-Kc for qemu-devel@nongnu.org; Tue, 05 Jan 2010 15:07:47 -0500 Received: from [199.232.76.173] (port=42763 helo=monty-python.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1NSFgt-00073Y-9W for qemu-devel@nongnu.org; Tue, 05 Jan 2010 15:07:43 -0500 Received: from mx20.gnu.org ([199.232.41.8]:36940) by monty-python.gnu.org with esmtps (TLS-1.0:RSA_AES_256_CBC_SHA1:32) (Exim 4.60) (envelope-from ) id 1NSFgs-0001bA-Sv for qemu-devel@nongnu.org; Tue, 05 Jan 2010 15:07:43 -0500 Received: from mail-yw0-f176.google.com ([209.85.211.176]) by mx20.gnu.org with esmtp (Exim 4.60) (envelope-from ) id 1NRsuQ-00042j-Ve for qemu-devel@nongnu.org; Mon, 04 Jan 2010 14:48:11 -0500 Received: by ywh6 with SMTP id 6so15649876ywh.4 for ; Mon, 04 Jan 2010 11:47:09 -0800 (PST) Message-ID: <4B4253C2.6080508@codemonkey.ws> Date: Mon, 04 Jan 2010 14:46:58 -0600 From: Anthony Liguori MIME-Version: 1.0 Subject: Re: [Qemu-devel] [PATCH 2/8] virtio-console: qdev conversion, new virtio-serial-bus References: <1261597948-24293-1-git-send-email-amit.shah@redhat.com> <1261597948-24293-2-git-send-email-amit.shah@redhat.com> <1261597948-24293-3-git-send-email-amit.shah@redhat.com> <4B32A3D6.2010509@codemonkey.ws> <20091224052532.GB25261@amit-x200.redhat.com> In-Reply-To: <20091224052532.GB25261@amit-x200.redhat.com> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: agraf@suse.de, qemu-devel@nongnu.org, armbru@redhat.com, kraxel@redhat.com 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