From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=45806 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1P8XQT-0003g3-5q for qemu-devel@nongnu.org; Wed, 20 Oct 2010 08:05:50 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1P8XQR-0002zS-PC for qemu-devel@nongnu.org; Wed, 20 Oct 2010 08:05:48 -0400 Received: from mail-gy0-f173.google.com ([209.85.160.173]:43171) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1P8XQR-0002zO-Kr for qemu-devel@nongnu.org; Wed, 20 Oct 2010 08:05:47 -0400 Received: by gyb11 with SMTP id 11so2089252gyb.4 for ; Wed, 20 Oct 2010 05:05:47 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <20101020115744.GA10594@amit-laptop.redhat.com> References: <20101019173946.16514.62027.stgit@localhost6.localdomain6> <20101020115744.GA10594@amit-laptop.redhat.com> Date: Wed, 20 Oct 2010 13:05:46 +0100 Message-ID: Subject: Re: [Qemu-devel] v6: [PATCH 0/3]: Threadlets: A generic task offloading framework From: Stefan Hajnoczi Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Amit Shah Cc: Arun R Bharadwaj , qemu-devel@nongnu.org On Wed, Oct 20, 2010 at 12:57 PM, Amit Shah wrote: > On (Tue) Oct 19 2010 [23:12:20], Arun R Bharadwaj wrote: >> Hi, >> >> This is the v6 of the patch-series to have a generic asynchronous task >> offloading framework (called threadlets) within qemu. >> >> Request to consider pulling this series as discussed during the >> Qemu-devel call. > > I tried this out with virtio-serial (patch below). =A0Have a couple of > things to note: > > - Guests get a SIGUSR2 on startup sometimes. =A0This doesn't happen with > =A0qemu.git, so looks like it's introduced by this patchset. > > - After running some tests, I get an abort. =A0I still have to look at > =A0what's causing it, but doesn't look like it's related to virtio-serial > =A0code. > > Program received signal SIGABRT, Aborted. > 0x0000003dc76329a5 in raise () from /lib64/libc.so.6 > Missing separate debuginfos, use: debuginfo-install > SDL-1.2.14-8.fc13.x86_64 glibc-2.12.1-2.x86_64 > libX11-1.3.1-3.fc13.x86_64 libXau-1.0.5-1.fc12.x86_64 > libpng-1.2.44-1.fc13.x86_64 libxcb-1.5-1.fc13.x86_64 > ncurses-libs-5.7-7.20100130.fc13.x86_64 zlib-1.2.3-23.fc12.x86_64 > (gdb) bt > #0 =A00x0000003dc76329a5 in raise () from /lib64/libc.so.6 > #1 =A00x0000003dc7634185 in abort () from /lib64/libc.so.6 > #2 =A00x00000000004bf829 in qemu_get_ram_ptr (addr=3D) > at /home/amit/src/qemu/exec.c:2936 > #3 =A00x00000000004bf9a7 in lduw_phys (addr=3D) at > /home/amit/src/qemu/exec.c:3836 > #4 =A00x0000000000557c90 in vring_avail_idx (vq=3D0x17b9320, idx=3D1333) = at > /home/amit/src/qemu/hw/virtio.c:133 > #5 =A0virtqueue_num_heads (vq=3D0x17b9320, idx=3D1333) at > /home/amit/src/qemu/hw/virtio.c:252 > #6 =A00x0000000000557e5e in virtqueue_avail_bytes (vq=3D0x17b9320, > in_bytes=3D4096, out_bytes=3D0) at /home/amit/src/qemu/hw/virtio.c:311 > > - I'm using a threadlet to queue up several work items which are to be > =A0processed in a fifo order. =A0There's no cancel function for a threadl= et > =A0that either processes all work and then quits the thread or just > =A0cancels all pending work and quits. > > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0Amit > > > diff --git a/hw/virtio-serial-bus.c b/hw/virtio-serial-bus.c > index 74ba5ec..caaafbe 100644 > --- a/hw/virtio-serial-bus.c > +++ b/hw/virtio-serial-bus.c > @@ -51,6 +51,14 @@ struct VirtIOSerial { > =A0 =A0 struct virtio_console_config config; > =A0}; > > +typedef struct VirtIOSerialWork { > + =A0 =A0ThreadletWork work; > + =A0 =A0VirtIOSerialPort *port; > + =A0 =A0VirtQueue *vq; > + =A0 =A0VirtIODevice *vdev; > + =A0 =A0int discard; > +} VirtIOSerialWork; > + > =A0static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t = id) > =A0{ > =A0 =A0 VirtIOSerialPort *port; > @@ -113,10 +121,20 @@ static size_t write_to_port(VirtIOSerialPort *port, > =A0 =A0 return offset; > =A0} > > -static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq, > - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 VirtIOD= evice *vdev, bool discard) > +static void async_flush_queued_data(ThreadletWork *work) > =A0{ > + =A0 =A0VirtIOSerialPort *port; > + =A0 =A0VirtIOSerialWork *vs_work; > + =A0 =A0VirtQueue *vq; > + =A0 =A0VirtIODevice *vdev; > =A0 =A0 VirtQueueElement elem; > + =A0 =A0int discard; > + > + =A0 =A0vs_work =3D DO_UPCAST(VirtIOSerialWork, work, work); > + =A0 =A0port =3D vs_work->port; > + =A0 =A0vq =3D vs_work->vq; > + =A0 =A0vdev =3D vs_work->vdev; > + =A0 =A0discard =3D vs_work->discard; > > =A0 =A0 assert(port || discard); > =A0 =A0 assert(virtio_queue_ready(vq)); You cannot access guest memory using QEMU RAM functions (or use the virtqueue_pop() function which uses them) from a thread without taking the QEMU global mutex. The abort stack trace is a result of accessing guest RAM from two threads simultaneously. In general it is not safe to use QEMU functions from a thread unless they are explicitly written to work outside the QEMU global mutex. Most functions assume the global mutex, which serializes I/O thread and vcpu changes to global state, is held. Stefan