Re: [Qemu-devel] v6: [PATCH 0/3]: Threadlets: A generic task offloading framework

[Anthony Liguori <anthony@codemonkey.ws>]

On 10/20/2010 07:05 AM, Stefan Hajnoczi wrote:
> On Wed, Oct 20, 2010 at 12:57 PM, Amit Shah<amit.shah@redhat.com>  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).  Have a couple of
>> things to note:
>>
>> - Guests get a SIGUSR2 on startup sometimes.  This doesn't happen with
>>   qemu.git, so looks like it's introduced by this patchset.
>>
>> - After running some tests, I get an abort.  I still have to look at
>>   what's causing it, but doesn't look like it's related to virtio-serial
>>   code.
>>
>> 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  0x0000003dc76329a5 in raise () from /lib64/libc.so.6
>> #1  0x0000003dc7634185 in abort () from /lib64/libc.so.6
>> #2  0x00000000004bf829 in qemu_get_ram_ptr (addr=<value optimized out>)
>> at /home/amit/src/qemu/exec.c:2936
>> #3  0x00000000004bf9a7 in lduw_phys (addr=<value optimized out>) at
>> /home/amit/src/qemu/exec.c:3836
>> #4  0x0000000000557c90 in vring_avail_idx (vq=0x17b9320, idx=1333) at
>> /home/amit/src/qemu/hw/virtio.c:133
>> #5  virtqueue_num_heads (vq=0x17b9320, idx=1333) at
>> /home/amit/src/qemu/hw/virtio.c:252
>> #6  0x0000000000557e5e in virtqueue_avail_bytes (vq=0x17b9320,
>> in_bytes=4096, out_bytes=0) at /home/amit/src/qemu/hw/virtio.c:311
>>
>> - I'm using a threadlet to queue up several work items which are to be
>>   processed in a fifo order.  There's no cancel function for a threadlet
>>   that either processes all work and then quits the thread or just
>>   cancels all pending work and quits.
>>
>>                 Amit
>>
>>
>> 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 {
>>      struct virtio_console_config config;
>>   };
>>
>> +typedef struct VirtIOSerialWork {
>> +    ThreadletWork work;
>> +    VirtIOSerialPort *port;
>> +    VirtQueue *vq;
>> +    VirtIODevice *vdev;
>> +    int discard;
>> +} VirtIOSerialWork;
>> +
>>   static VirtIOSerialPort *find_port_by_id(VirtIOSerial *vser, uint32_t id)
>>   {
>>      VirtIOSerialPort *port;
>> @@ -113,10 +121,20 @@ static size_t write_to_port(VirtIOSerialPort *port,
>>      return offset;
>>   }
>>
>> -static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
>> -                                 VirtIODevice *vdev, bool discard)
>> +static void async_flush_queued_data(ThreadletWork *work)
>>   {
>> +    VirtIOSerialPort *port;
>> +    VirtIOSerialWork *vs_work;
>> +    VirtQueue *vq;
>> +    VirtIODevice *vdev;
>>      VirtQueueElement elem;
>> +    int discard;
>> +
>> +    vs_work = DO_UPCAST(VirtIOSerialWork, work, work);
>> +    port = vs_work->port;
>> +    vq = vs_work->vq;
>> +    vdev = vs_work->vdev;
>> +    discard = vs_work->discard;
>>
>>      assert(port || discard);
>>      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.
>    

Yes, threadlets are only meant to be used to make synchronous system 
calls asynchronous.  They are not meant to add parallelism to QEMU (yet).

Regards,

Anthony Liguori

> Stefan
>
>