diff mbox

v6: [PATCH 0/3]: Threadlets: A generic task offloading framework

Message ID 20101020115744.GA10594@amit-laptop.redhat.com
State New
Headers show

Commit Message

Amit Shah Oct. 20, 2010, 11:57 a.m. UTC 
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

Comments

Stefan Hajnoczi Oct. 20, 2010, 12:05 p.m. UTC | #1 
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.

Stefan
Anthony Liguori Oct. 20, 2010, 1:18 p.m. UTC | #2 
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
>
>
Amit Shah Oct. 22, 2010, 9:59 a.m. UTC | #3 
On (Wed) Oct 20 2010 [08:18:51], Anthony Liguori wrote:
> 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).

Yes -- I realised that.  (But I don't see why the virtio rings get
modified elsewhere other than the only function I push/pop from above.)

Anyway, just one question as I've still not read the code: does a
running work item in a threadlet block migration?  Do the remaining work
items in a threadlet get migrated fine?

		Amit
Stefan Hajnoczi Oct. 23, 2010, 12:05 p.m. UTC | #4 
On Fri, Oct 22, 2010 at 10:59 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Wed) Oct 20 2010 [08:18:51], Anthony Liguori wrote:
>> 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).
>
> Yes -- I realised that.  (But I don't see why the virtio rings get
> modified elsewhere other than the only function I push/pop from above.)

The particular race condition triggered by virtqueue_pop() is because
guest RAM access functions try to optimize frequently accessed memory
to keep lookup time to a minimum.  The current RAM block is put onto
the beginning of the lookup list so the next access to it will be
faster.  When two threads are looking up guest memory simultaneously
one may be modifying the list while the other traverses it...

> Anyway, just one question as I've still not read the code: does a
> running work item in a threadlet block migration?  Do the remaining work
> items in a threadlet get migrated fine?

There's no migration support in the threadlets infrastructure itself.

Threadlets users need to think about how to safely migrate, either
through cancellation or waiting for all pending work to complete.  For
example, migration will quiesce aio requests using qemu_aio_flush() so
there is no outstanding work to be migrated.

Stefan
Amit Shah Oct. 27, 2010, 7:57 a.m. UTC | #5 
On (Sat) Oct 23 2010 [13:05:48], Stefan Hajnoczi wrote:
> >> >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).
> >
> > Yes -- I realised that.  (But I don't see why the virtio rings get
> > modified elsewhere other than the only function I push/pop from above.)
> 
> The particular race condition triggered by virtqueue_pop() is because
> guest RAM access functions try to optimize frequently accessed memory
> to keep lookup time to a minimum.  The current RAM block is put onto
> the beginning of the lookup list so the next access to it will be
> faster.  When two threads are looking up guest memory simultaneously
> one may be modifying the list while the other traverses it...

Right, thanks.

> > Anyway, just one question as I've still not read the code: does a
> > running work item in a threadlet block migration?  Do the remaining work
> > items in a threadlet get migrated fine?
> 
> There's no migration support in the threadlets infrastructure itself.
> 
> Threadlets users need to think about how to safely migrate, either
> through cancellation or waiting for all pending work to complete.  For
> example, migration will quiesce aio requests using qemu_aio_flush() so
> there is no outstanding work to be migrated.

The problem then is that the API doesn't have functions to either stall
all pending work in a threadlet or cancel all work or even block the
caller till all work is finished.  I think the API relies on the work
signalling some sort of completion, which means callers (or threadlet
users) have to keep track of all the work scheduled, which might be
uneecessary.

		Amit
Stefan Hajnoczi Oct. 27, 2010, 8:37 a.m. UTC | #6 
On Wed, Oct 27, 2010 at 8:57 AM, Amit Shah <amit.shah@redhat.com> wrote:
> On (Sat) Oct 23 2010 [13:05:48], Stefan Hajnoczi wrote:
>> >> >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).
>> >
>> > Yes -- I realised that.  (But I don't see why the virtio rings get
>> > modified elsewhere other than the only function I push/pop from above.)
>>
>> The particular race condition triggered by virtqueue_pop() is because
>> guest RAM access functions try to optimize frequently accessed memory
>> to keep lookup time to a minimum.  The current RAM block is put onto
>> the beginning of the lookup list so the next access to it will be
>> faster.  When two threads are looking up guest memory simultaneously
>> one may be modifying the list while the other traverses it...
>
> Right, thanks.
>
>> > Anyway, just one question as I've still not read the code: does a
>> > running work item in a threadlet block migration?  Do the remaining work
>> > items in a threadlet get migrated fine?
>>
>> There's no migration support in the threadlets infrastructure itself.
>>
>> Threadlets users need to think about how to safely migrate, either
>> through cancellation or waiting for all pending work to complete.  For
>> example, migration will quiesce aio requests using qemu_aio_flush() so
>> there is no outstanding work to be migrated.
>
> The problem then is that the API doesn't have functions to either stall
> all pending work in a threadlet or cancel all work or even block the
> caller till all work is finished.  I think the API relies on the work
> signalling some sort of completion, which means callers (or threadlet
> users) have to keep track of all the work scheduled, which might be
> uneecessary.

I agree that placing responsibility on the caller is not ideal in
simple cases like waiting for all work to complete.  On the other hand
it allows custom behavior to be implemented since the caller has full
control.

I'd wait to see how callers actually use threadlets and deal with
migration.  Patterns may emerge and can be put into common code.

Stefan
diff mbox

Patch

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));
@@ -136,6 +154,24 @@  static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
         virtqueue_push(vq, &elem, 0);
     }
     virtio_notify(vdev, vq);
+
+    qemu_free(vs_work);
+}
+
+static void do_flush_queued_data(VirtIOSerialPort *port, VirtQueue *vq,
+                                 VirtIODevice *vdev, bool discard)
+{
+    VirtIOSerialWork *vs_work;
+
+    /* TODO: can just do the needful if discard is true */
+
+    vs_work = qemu_malloc(sizeof(*vs_work));
+    vs_work->work.func = async_flush_queued_data;
+    vs_work->discard = discard;
+    vs_work->vdev = vdev;
+    vs_work->vq = vq;
+    vs_work->port = port;
+    submit_threadletwork_to_queue(&port->tqueue, &vs_work->work);
 }
 
 static void flush_queued_data(VirtIOSerialPort *port, bool discard)
@@ -699,6 +735,12 @@  static int virtser_port_qdev_init(DeviceState *qdev, DeviceInfo *base)
     port->ivq = port->vser->ivqs[port->id];
     port->ovq = port->vser->ovqs[port->id];
 
+    /*
+     * Just one thread to process all the work -- we don't want guest
+     * buffers to be processed out-of-order.
+     */
+    threadlet_queue_init(&port->tqueue, 1, 1);
+
     add_port(port->vser, port->id);
 
     /* Send an update to the guest about this new port added */
@@ -717,6 +759,8 @@  static int virtser_port_qdev_exit(DeviceState *qdev)
 
     QTAILQ_REMOVE(&vser->ports, port, next);
 
+    /* TODO: Cancel threadlet */
+
     if (port->info->exit)
         port->info->exit(dev);
 
diff --git a/hw/virtio-serial.h b/hw/virtio-serial.h
index ff08c40..15e0982 100644
--- a/hw/virtio-serial.h
+++ b/hw/virtio-serial.h
@@ -15,6 +15,7 @@ 
 #ifndef _QEMU_VIRTIO_SERIAL_H
 #define _QEMU_VIRTIO_SERIAL_H
 
+#include "qemu-threadlets.h"
 #include "qdev.h"
 #include "virtio.h"
 
@@ -88,6 +89,13 @@  struct VirtIOSerialPort {
     VirtQueue *ivq, *ovq;
 
     /*
+     * Threadlet queue for processing work for this port -- ensures we
+     * don't block the guest that writes out data and also other
+     * ports.
+     */
+    ThreadletQueue tqueue;
+
+    /*
      * This name is sent to the guest and exported via sysfs.
      * The guest could create symlinks based on this information.
      * The name is in the reverse fqdn format, like org.qemu.console.0