[Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

Both the recent I/O loop and threadlet series have me concerned that we're
digging ourselves deeper into the NIH hole.  I think it's time we look at
something radical to let us borrow more code from existing projects instead of
reinventing everything through trial and error.

This series introduces a hard dependency on glib.  The initial use is portable
threads but I see this as just the beginning.  Glib/Gobject offer many nice
things including:

 - portable threads
 - rich data structure support
 - INI parser
 - JSON parser
 - generic type system
 - object oriented infrastructure
 - IO library
 - module system
 - introspection to enable support for dynamic language bindings

I see this series as the first step, followed by converting the I/O loop to
a GMainLoop instance.  Once we're there, we can start making deeper use of
GObjects including converting QDev to a GObject hierarchy.

I've spent the past few months working on C++ integration for QEMU.  I'm more
convinced than ever that we desperately in need of structured object oriented
mechanisms to be successful but am pretty strongly convinced that incremental
additional of C++ is not going to be successful.

On the other hand, while GObjects are uglier and require a lot of template code,
there's more than enough structure that I think it can guide us into a much
better object model implementation.

There is some ugliness.   GLib does not abstract signals because they're very
non-portable but QEMU makes extensive use of signaling.  I don't think it's
a major issue but some of the ugliness in this series is due to that fact.

This series is only lightly tested but also mostly mechanical.  I'm pretty
confused by the way tcg_halt_cond and friends works so I'm fairly sure I broke
that (for non-threaded TCG).

[Qemu-devel] [PATCH 1/7] io-thread: make sure to initialize qemu_work_cond and qemu_cpu_cond

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/cpus.c b/cpus.c
index 0309189..9cf7e6e 100644
--- a/cpus.c
+++ b/cpus.c
@@ -412,8 +412,10 @@ int qemu_init_main_loop(void)
     if (ret)
         return ret;
 
-    qemu_cond_init(&qemu_pause_cond);
+    qemu_cond_init(&qemu_cpu_cond);
     qemu_cond_init(&qemu_system_cond);
+    qemu_cond_init(&qemu_pause_cond);
+    qemu_cond_init(&qemu_work_cond);
     qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 1/7] io-thread: make sure to initialize qemu_work_cond and qemu_cpu_cond

[Jan Kiszka]

On 2011-01-24 22:00, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 0309189..9cf7e6e 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -412,8 +412,10 @@ int qemu_init_main_loop(void)
>      if (ret)
>          return ret;
>  
> -    qemu_cond_init(&qemu_pause_cond);
> +    qemu_cond_init(&qemu_cpu_cond);
>      qemu_cond_init(&qemu_system_cond);
> +    qemu_cond_init(&qemu_pause_cond);
> +    qemu_cond_init(&qemu_work_cond);
>      qemu_mutex_init(&qemu_fair_mutex);
>      qemu_mutex_init(&qemu_global_mutex);
>      qemu_mutex_lock(&qemu_global_mutex);

Please merge this independently of the glib patches. It's apparently
harmless only by chance so far.

Thanks,
Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 1/7] io-thread: make sure to initialize qemu_work_cond and qemu_cpu_cond

[Anthony Liguori]

On 02/08/2011 02:53 AM, Jan Kiszka wrote:
> On 2011-01-24 22:00, Anthony Liguori wrote:
>    
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/cpus.c b/cpus.c
>> index 0309189..9cf7e6e 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -412,8 +412,10 @@ int qemu_init_main_loop(void)
>>       if (ret)
>>           return ret;
>>
>> -    qemu_cond_init(&qemu_pause_cond);
>> +    qemu_cond_init(&qemu_cpu_cond);
>>       qemu_cond_init(&qemu_system_cond);
>> +    qemu_cond_init(&qemu_pause_cond);
>> +    qemu_cond_init(&qemu_work_cond);
>>       qemu_mutex_init(&qemu_fair_mutex);
>>       qemu_mutex_init(&qemu_global_mutex);
>>       qemu_mutex_lock(&qemu_global_mutex);
>>      
> Please merge this independently of the glib patches. It's apparently
> harmless only by chance so far.
>    

Plan is to merge the first and second patch of the series right after 
0.14.0 is released.

Regards,

Anthony Liguori


> Thanks,
> Jan
>
>    

[Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

Leave the disable options for now to help with testing but these will be removed
once we're confident in the thread implementations.

Disabled code bit rots.  These have been in tree long enough that we need to
either commit to making them work or just remove them entirely.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/configure b/configure
index d68f862..d5ac074 100755
--- a/configure
+++ b/configure
@@ -124,7 +124,7 @@ vnc_tls=""
 vnc_sasl=""
 vnc_jpeg=""
 vnc_png=""
-vnc_thread="no"
+vnc_thread="yes"
 xen=""
 linux_aio=""
 attr=""
@@ -161,7 +161,7 @@ darwin_user="no"
 bsd_user="no"
 guest_base=""
 uname_release=""
-io_thread="no"
+io_thread="yes"
 mixemu="no"
 kerneldir=""
 aix="no"
@@ -699,6 +699,8 @@ for opt do
   ;;
   --enable-io-thread) io_thread="yes"
   ;;
+  --disable-io-thread) io_thread="no"
+  ;;
   --disable-blobs) blobs="no"
   ;;
   --kerneldir=*) kerneldir="$optarg"
@@ -901,6 +903,7 @@ echo "  --enable-linux-aio       enable Linux AIO support"
 echo "  --disable-attr           disables attr and xattr support"
 echo "  --enable-attr            enable attr and xattr support"
 echo "  --enable-io-thread       enable IO thread"
+echo "  --disable-io-thread      disable IO thread"
 echo "  --disable-blobs          disable installing provided firmware blobs"
 echo "  --kerneldir=PATH         look for kernel includes in PATH"
 echo "  --enable-docs            enable documentation build"
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> Leave the disable options for now to help with testing but these will be removed
> once we're confident in the thread implementations.
>
> Disabled code bit rots.  These have been in tree long enough that we need to
> either commit to making them work or just remove them entirely.
>    

I/O thread disables icount apparently.

I'm not really sure why.  Marcelo, do you know the reason 
qemu_calculate_timeout returns a fixed value in the I/O thread 
regardless of icount?

Regards,

Anthony Liguori

> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>
> diff --git a/configure b/configure
> index d68f862..d5ac074 100755
> --- a/configure
> +++ b/configure
> @@ -124,7 +124,7 @@ vnc_tls=""
>   vnc_sasl=""
>   vnc_jpeg=""
>   vnc_png=""
> -vnc_thread="no"
> +vnc_thread="yes"
>   xen=""
>   linux_aio=""
>   attr=""
> @@ -161,7 +161,7 @@ darwin_user="no"
>   bsd_user="no"
>   guest_base=""
>   uname_release=""
> -io_thread="no"
> +io_thread="yes"
>   mixemu="no"
>   kerneldir=""
>   aix="no"
> @@ -699,6 +699,8 @@ for opt do
>     ;;
>     --enable-io-thread) io_thread="yes"
>     ;;
> +  --disable-io-thread) io_thread="no"
> +  ;;
>     --disable-blobs) blobs="no"
>     ;;
>     --kerneldir=*) kerneldir="$optarg"
> @@ -901,6 +903,7 @@ echo "  --enable-linux-aio       enable Linux AIO support"
>   echo "  --disable-attr           disables attr and xattr support"
>   echo "  --enable-attr            enable attr and xattr support"
>   echo "  --enable-io-thread       enable IO thread"
> +echo "  --disable-io-thread      disable IO thread"
>   echo "  --disable-blobs          disable installing provided firmware blobs"
>   echo "  --kerneldir=PATH         look for kernel includes in PATH"
>   echo "  --enable-docs            enable documentation build"
>    

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Edgar E. Iglesias]

On Mon, Jan 24, 2011 at 04:28:48PM -0600, Anthony Liguori wrote:
> On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> > Leave the disable options for now to help with testing but these will be removed
> > once we're confident in the thread implementations.
> >
> > Disabled code bit rots.  These have been in tree long enough that we need to
> > either commit to making them work or just remove them entirely.
> >    
> 
> I/O thread disables icount apparently.
> 
> I'm not really sure why.  Marcelo, do you know the reason 
> qemu_calculate_timeout returns a fixed value in the I/O thread 
> regardless of icount?

Hi,

The following commit hopefully fixed that issue.

commit 225d02cd1a34d5d87e8acefbf8e244a5d12f5f8c
Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
Date:   Sun Jan 23 04:44:51 2011 +0100

    Avoid deadlock whith iothread and icount
    
    When using the iothread together with icount, make sure the
    qemu_icount counter makes forward progress when the vcpu is
    idle to avoid deadlocks.
    
    Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>

See http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg01602.html
for more info.

One more thing I didn't mention on the email-thread or on IRC is
that last time I checked, qemu with io-thread was performing
significantly slower than non io-thread builds. That was with
TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.

Also, although -icount & iothread no longer deadlocks, icount
still sometimes performs incredibly slow with the io-thread (compared
to non-io-thread qemu). In particular when not using -icount auto but
a fixed ticks per insn values. Sometimes it's so slow I thought it
actually deadlocked, but no it was crawling :) I haven't had time
to look at it any closer but I hope to do soon.

These issues should be fixable though, so I'm not arguing against
enabling it per default. Just mentioning what I've seen FYI..

Cheers

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Marcelo Tosatti]

On Tue, Jan 25, 2011 at 10:17:41AM +0100, Edgar E. Iglesias wrote:
> On Mon, Jan 24, 2011 at 04:28:48PM -0600, Anthony Liguori wrote:
> > On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> > > Leave the disable options for now to help with testing but these will be removed
> > > once we're confident in the thread implementations.
> > >
> > > Disabled code bit rots.  These have been in tree long enough that we need to
> > > either commit to making them work or just remove them entirely.
> > >    
> > 
> > I/O thread disables icount apparently.
> > 
> > I'm not really sure why.  Marcelo, do you know the reason 
> > qemu_calculate_timeout returns a fixed value in the I/O thread 
> > regardless of icount?
> 
> Hi,
> 
> The following commit hopefully fixed that issue.
> 
> commit 225d02cd1a34d5d87e8acefbf8e244a5d12f5f8c
> Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> Date:   Sun Jan 23 04:44:51 2011 +0100
> 
>     Avoid deadlock whith iothread and icount
>     
>     When using the iothread together with icount, make sure the
>     qemu_icount counter makes forward progress when the vcpu is
>     idle to avoid deadlocks.
>     
>     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> 
> See http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg01602.html
> for more info.
> 
> One more thing I didn't mention on the email-thread or on IRC is
> that last time I checked, qemu with io-thread was performing
> significantly slower than non io-thread builds. That was with
> TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.
> 
> Also, although -icount & iothread no longer deadlocks, icount
> still sometimes performs incredibly slow with the io-thread (compared
> to non-io-thread qemu). In particular when not using -icount auto but
> a fixed ticks per insn values. Sometimes it's so slow I thought it
> actually deadlocked, but no it was crawling :) I haven't had time
> to look at it any closer but I hope to do soon.
> 
> These issues should be fixable though, so I'm not arguing against
> enabling it per default. Just mentioning what I've seen FYI..

Right, remember seeing 20% added overhead for network copy with TCG on
the initial iothread merge. One can argue its due to the added overhead
of locking/signalling between vcpu and iothread, where neither can run
in parallel (while in kvm mode they can). But its just handwaving until
detailed information is gathered on specific cases...

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Marcelo Tosatti]

On Tue, Jan 25, 2011 at 11:34:53AM -0200, Marcelo Tosatti wrote:
> On Tue, Jan 25, 2011 at 10:17:41AM +0100, Edgar E. Iglesias wrote:
> > On Mon, Jan 24, 2011 at 04:28:48PM -0600, Anthony Liguori wrote:
> > > On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> > > > Leave the disable options for now to help with testing but these will be removed
> > > > once we're confident in the thread implementations.
> > > >
> > > > Disabled code bit rots.  These have been in tree long enough that we need to
> > > > either commit to making them work or just remove them entirely.
> > > >    
> > > 
> > > I/O thread disables icount apparently.
> > > 
> > > I'm not really sure why.  Marcelo, do you know the reason 
> > > qemu_calculate_timeout returns a fixed value in the I/O thread 
> > > regardless of icount?
> > 
> > Hi,
> > 
> > The following commit hopefully fixed that issue.
> > 
> > commit 225d02cd1a34d5d87e8acefbf8e244a5d12f5f8c
> > Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > Date:   Sun Jan 23 04:44:51 2011 +0100
> > 
> >     Avoid deadlock whith iothread and icount
> >     
> >     When using the iothread together with icount, make sure the
> >     qemu_icount counter makes forward progress when the vcpu is
> >     idle to avoid deadlocks.
> >     
> >     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > 
> > See http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg01602.html
> > for more info.
> > 
> > One more thing I didn't mention on the email-thread or on IRC is
> > that last time I checked, qemu with io-thread was performing
> > significantly slower than non io-thread builds. That was with
> > TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.

Can you recall what was the test ?

> > Also, although -icount & iothread no longer deadlocks, icount
> > still sometimes performs incredibly slow with the io-thread (compared
> > to non-io-thread qemu). In particular when not using -icount auto but
> > a fixed ticks per insn values. Sometimes it's so slow I thought it
> > actually deadlocked, but no it was crawling :) I haven't had time
> > to look at it any closer but I hope to do soon.
> > 
> > These issues should be fixable though, so I'm not arguing against
> > enabling it per default. Just mentioning what I've seen FYI..
> 
> Right, remember seeing 20% added overhead for network copy with TCG on
> the initial iothread merge.

This is not the case anymore, network transfer speed is comparable.
Probably due to SIG_IPI delivery being reliable, which was fixed later.

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Marcelo Tosatti]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Mon, Feb 07, 2011 at 08:12:55AM -0200, Marcelo Tosatti wrote:
> > > One more thing I didn't mention on the email-thread or on IRC is
> > > that last time I checked, qemu with io-thread was performing
> > > significantly slower than non io-thread builds. That was with
> > > TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.
> 
> Can you recall what was the test ?
> 
> > > Also, although -icount & iothread no longer deadlocks, icount
> > > still sometimes performs incredibly slow with the io-thread (compared
> > > to non-io-thread qemu). In particular when not using -icount auto but
> > > a fixed ticks per insn values. Sometimes it's so slow I thought it
> > > actually deadlocked, but no it was crawling :) I haven't had time
> > > to look at it any closer but I hope to do soon.

Edgar, please give the attached patch a try with fixed icount value. The
calculation for next event makes no sense for iothread timeout, only for
vcpu context.

> > > 
> > > These issues should be fixable though, so I'm not arguing against
> > > enabling it per default. Just mentioning what I've seen FYI..
> > 
> > Right, remember seeing 20% added overhead for network copy with TCG on
> > the initial iothread merge.
> 
> This is not the case anymore, network transfer speed is comparable.
> Probably due to SIG_IPI delivery being reliable, which was fixed later.

Is there any other issue that prevents turning CONFIG_IOTHREAD on by
default?


[-- Attachment #2: qemu-fix-icount-timeout-iothread.patch --]
[-- Type: text/plain, Size: 725 bytes --]

diff --git a/cpus.c b/cpus.c
index 9c50a34..2280db1 100644
--- a/cpus.c
+++ b/cpus.c
@@ -748,7 +748,7 @@ static void qemu_tcg_wait_io_event(void)
     CPUState *env;
 
     while (!any_cpu_has_work())
-        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
diff --git a/vl.c b/vl.c
index 837be97..dbd81a1 100644
--- a/vl.c
+++ b/vl.c
@@ -1323,7 +1323,7 @@ void main_loop_wait(int nonblocking)
     if (nonblocking)
         timeout = 0;
     else {
-        timeout = qemu_calculate_timeout();
+        timeout = 1000;
         qemu_bh_update_timeout(&timeout);
     }
 

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/07/2011 05:03 PM, Marcelo Tosatti wrote:
> Is there any other issue that prevents turning CONFIG_IOTHREAD on by
> default?

I think Windows support.

Signal support is actually easy because we can "hack" the IPI as 
"suspend the VCPU thread+do work in the iothread context+resume the VCPU 
thread" (the IPI handler doesn't longjmp).

Threading primitives support is tricky but not hard (there is lots of 
code around, especially if you can make assumptions such as "always hold 
the mutex while signaling a cond. variable").

Paolo

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-07 17:23, Paolo Bonzini wrote:
> On 02/07/2011 05:03 PM, Marcelo Tosatti wrote:
>> Is there any other issue that prevents turning CONFIG_IOTHREAD on by
>> default?
> 
> I think Windows support.
> 
> Signal support is actually easy because we can "hack" the IPI as
> "suspend the VCPU thread+do work in the iothread context+resume the VCPU
> thread" (the IPI handler doesn't longjmp).
> 
> Threading primitives support is tricky but not hard (there is lots of
> code around, especially if you can make assumptions such as "always hold
> the mutex while signaling a cond. variable").

!CONFIG_IOTHREAD code is doomed to bitrot once we switch to default
iothread mode. So if Windows support is not converted to a threading
model with moderate differences to POSIX, it will likely bitrot a well.
Therefore, conversion should be started rather sooner than later (by
someone interested in that platform).

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/07/2011 11:10 AM, Jan Kiszka wrote:
> On 2011-02-07 17:23, Paolo Bonzini wrote:
>    
>> On 02/07/2011 05:03 PM, Marcelo Tosatti wrote:
>>      
>>> Is there any other issue that prevents turning CONFIG_IOTHREAD on by
>>> default?
>>>        
>> I think Windows support.
>>
>> Signal support is actually easy because we can "hack" the IPI as
>> "suspend the VCPU thread+do work in the iothread context+resume the VCPU
>> thread" (the IPI handler doesn't longjmp).
>>
>> Threading primitives support is tricky but not hard (there is lots of
>> code around, especially if you can make assumptions such as "always hold
>> the mutex while signaling a cond. variable").
>>      
> !CONFIG_IOTHREAD code is doomed to bitrot once we switch to default
> iothread mode. So if Windows support is not converted to a threading
> model with moderate differences to POSIX, it will likely bitrot a well.
> Therefore, conversion should be started rather sooner than later (by
> someone interested in that platform).
>    

As far as I'm concerned, Windows support is already deprecated as noone 
has stepped up to enhance it or support for a number of years now.  We 
shouldn't remove existing code that supports it or refuse to take 
reasonable patches but if enabling IO thread by default breaks it, so be it.

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

On Mon, Feb 07, 2011 at 03:02:03PM -0600, Anthony Liguori wrote:
> On 02/07/2011 11:10 AM, Jan Kiszka wrote:
>> On 2011-02-07 17:23, Paolo Bonzini wrote:
>>    
>>> On 02/07/2011 05:03 PM, Marcelo Tosatti wrote:
>>>      
>>>> Is there any other issue that prevents turning CONFIG_IOTHREAD on by
>>>> default?
>>>>        
>>> I think Windows support.
>>>
>>> Signal support is actually easy because we can "hack" the IPI as
>>> "suspend the VCPU thread+do work in the iothread context+resume the VCPU
>>> thread" (the IPI handler doesn't longjmp).
>>>
>>> Threading primitives support is tricky but not hard (there is lots of
>>> code around, especially if you can make assumptions such as "always hold
>>> the mutex while signaling a cond. variable").
>>>      
>> !CONFIG_IOTHREAD code is doomed to bitrot once we switch to default
>> iothread mode. So if Windows support is not converted to a threading
>> model with moderate differences to POSIX, it will likely bitrot a well.
>> Therefore, conversion should be started rather sooner than later (by
>> someone interested in that platform).
>>    
>
> As far as I'm concerned, Windows support is already deprecated as noone  
> has stepped up to enhance it or support for a number of years now.  We  
> shouldn't remove existing code that supports it or refuse to take  
> reasonable patches but if enabling IO thread by default breaks it, so be 
> it.

As far as I see, Blue Swirl and Stefan Weil are regularly committing
fixes for win32. Stefan Weil is also providing win32 binaries on his
website [1]. I wouldn't call that deprecated.

[1] http://qemu.weilnetz.de/

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/07/2011 03:45 PM, Aurelien Jarno wrote:
> On Mon, Feb 07, 2011 at 03:02:03PM -0600, Anthony Liguori wrote:
>    
>> As far as I'm concerned, Windows support is already deprecated as noone
>> has stepped up to enhance it or support for a number of years now.  We
>> shouldn't remove existing code that supports it or refuse to take
>> reasonable patches but if enabling IO thread by default breaks it, so be
>> it.
>>      
> As far as I see, Blue Swirl and Stefan Weil are regularly committing
> fixes for win32. Stefan Weil is also providing win32 binaries on his
> website [1]. I wouldn't call that deprecated.
>    

Occasional compile fixes is a long way from something that is regularly 
tested and well maintained.

Win32 still doesn't have a proper AIO implementation which is probably 
close to a 4 year old FIXME.

Regards,

Anthony Liguori

> [1] http://qemu.weilnetz.de/
>
>    

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

On Mon, Feb 07, 2011 at 08:09:52PM -0600, Anthony Liguori wrote:
> On 02/07/2011 03:45 PM, Aurelien Jarno wrote:
>> On Mon, Feb 07, 2011 at 03:02:03PM -0600, Anthony Liguori wrote:
>>    
>>> As far as I'm concerned, Windows support is already deprecated as noone
>>> has stepped up to enhance it or support for a number of years now.  We
>>> shouldn't remove existing code that supports it or refuse to take
>>> reasonable patches but if enabling IO thread by default breaks it, so be
>>> it.
>>>      
>> As far as I see, Blue Swirl and Stefan Weil are regularly committing
>> fixes for win32. Stefan Weil is also providing win32 binaries on his
>> website [1]. I wouldn't call that deprecated.
>>    
>
> Occasional compile fixes is a long way from something that is regularly  
> tested and well maintained.
>
> Win32 still doesn't have a proper AIO implementation which is probably  
> close to a 4 year old FIXME.
>

I forget to remember when we decided that AIO should be implemented on
any host OS. Any pointer?

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
> I forget to remember when we decided that AIO should be implemented on
> any host OS. Any pointer?

To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
Window targets, they also crash under SMP due to the Windows AP 
watchdog.  But then TCG and SMP do not go very well together anyway.

However, I think deprecating Win32 support would be a very bad idea.

Paolo

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 09:08, Paolo Bonzini wrote:
> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>> I forget to remember when we decided that AIO should be implemented on
>> any host OS. Any pointer?
> 
> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
> Window targets, they also crash under SMP due to the Windows AP 
> watchdog.  But then TCG and SMP do not go very well together anyway.
> 
> However, I think deprecating Win32 support would be a very bad idea.

It would be too early at this point.

But if Windows is once the only reason to keep tons of hardly tested
code paths around or to invest significant additional effort to change
logic or interfaces in this area, than I would prefer that step. I'm
hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
subtle differences are really a PITA and source of various breakages.

People interested in that platform should finally realize that its fate
is coupled to reducing the #ifdefs as well as the design differences we
see right now and even more in the future.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Jan Kiszka a écrit :
> On 2011-02-08 09:08, Paolo Bonzini wrote:
>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>> I forget to remember when we decided that AIO should be implemented on
>>> any host OS. Any pointer?
>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>> Window targets, they also crash under SMP due to the Windows AP 
>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>
>> However, I think deprecating Win32 support would be a very bad idea.
> 
> It would be too early at this point.
> 
> But if Windows is once the only reason to keep tons of hardly tested
> code paths around or to invest significant additional effort to change
> logic or interfaces in this area, than I would prefer that step. I'm
> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
> subtle differences are really a PITA and source of various breakages.
> 
> People interested in that platform should finally realize that its fate
> is coupled to reducing the #ifdefs as well as the design differences we
> see right now and even more in the future.
> 

The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
it's just that people who introduce IOTHREAD didn't care about Windows
support at all and added these #ifdef. Disabling Windows support because
of that is not fair.

We should probably get rid of KVM support in QEMU, so if someone has an
idea for a cool TCG feature that can't be supported in KVM, it's the
moment to submit it. We can add it with #ifdef, and in one year just ask
for KVM support removal.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 03:05 AM, Aurelien Jarno wrote:
> Jan Kiszka a écrit :
>    
>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>      
>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>        
>>>> I forget to remember when we decided that AIO should be implemented on
>>>> any host OS. Any pointer?
>>>>          
>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For
>>> Window targets, they also crash under SMP due to the Windows AP
>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>
>>> However, I think deprecating Win32 support would be a very bad idea.
>>>        
>> It would be too early at this point.
>>
>> But if Windows is once the only reason to keep tons of hardly tested
>> code paths around or to invest significant additional effort to change
>> logic or interfaces in this area, than I would prefer that step. I'm
>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>> subtle differences are really a PITA and source of various breakages.
>>
>> People interested in that platform should finally realize that its fate
>> is coupled to reducing the #ifdefs as well as the design differences we
>> see right now and even more in the future.
>>
>>      
> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>    

IOTHREAD is actually just as necessary for TCG as it is for KVM.  
Otherwise, you have a signal select race that cannot be avoided.

QEMU has never "supported" Windows.  It happens to compile on Windows, 
but historically the Windows build has been non-functional for long 
periods of time and is still missing basic features (like AIO).

Regards,

Anthony Liguori

> it's just that people who introduce IOTHREAD didn't care about Windows
> support at all and added these #ifdef. Disabling Windows support because
> of that is not fair.
>
> We should probably get rid of KVM support in QEMU, so if someone has an
> idea for a cool TCG feature that can't be supported in KVM, it's the
> moment to submit it. We can add it with #ifdef, and in one year just ask
> for KVM support removal.
>
>    

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 10:12 AM, Anthony Liguori wrote:
>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>
> QEMU has never "supported" Windows.

I think both assertions are false.

What's true is that the Win32 port has never evolved beyond "barely 
functional", at least by the standards with which QEMU is judged under 
Linux.

Paolo

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 10:05, Aurelien Jarno wrote:
> Jan Kiszka a écrit :
>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>> I forget to remember when we decided that AIO should be implemented on
>>>> any host OS. Any pointer?
>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>> Window targets, they also crash under SMP due to the Windows AP 
>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>
>>> However, I think deprecating Win32 support would be a very bad idea.
>>
>> It would be too early at this point.
>>
>> But if Windows is once the only reason to keep tons of hardly tested
>> code paths around or to invest significant additional effort to change
>> logic or interfaces in this area, than I would prefer that step. I'm
>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>> subtle differences are really a PITA and source of various breakages.
>>
>> People interested in that platform should finally realize that its fate
>> is coupled to reducing the #ifdefs as well as the design differences we
>> see right now and even more in the future.
>>
> 
> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
> it's just that people who introduce IOTHREAD didn't care about Windows
> support at all and added these #ifdef. Disabling Windows support because
> of that is not fair.

The TCG execution model won't scale long-term. It's already a main to
boot a quad or just dual core VM, even more when your host has at least
as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
future, and the iothread will just be one of 7, 17 or 257 threads.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Jan Kiszka a écrit :
> On 2011-02-08 10:05, Aurelien Jarno wrote:
>> Jan Kiszka a écrit :
>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>> any host OS. Any pointer?
>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>
>>>> However, I think deprecating Win32 support would be a very bad idea.
>>> It would be too early at this point.
>>>
>>> But if Windows is once the only reason to keep tons of hardly tested
>>> code paths around or to invest significant additional effort to change
>>> logic or interfaces in this area, than I would prefer that step. I'm
>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>> subtle differences are really a PITA and source of various breakages.
>>>
>>> People interested in that platform should finally realize that its fate
>>> is coupled to reducing the #ifdefs as well as the design differences we
>>> see right now and even more in the future.
>>>
>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>> it's just that people who introduce IOTHREAD didn't care about Windows
>> support at all and added these #ifdef. Disabling Windows support because
>> of that is not fair.
> 
> The TCG execution model won't scale long-term. It's already a main to
> boot a quad or just dual core VM, even more when your host has at least
> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
> future, and the iothread will just be one of 7, 17 or 257 threads.
> 

And what's the issue with that? People don't always look for performance
when using QEMU. They even often try to emulate old machines (and non
x86 ones), which anyway only have one CPU. This won't change in 5 years,
the only thing is that those machines will be 5 years older.

People have to keep in mind that QEMU doesn't mean only virtualization
and doesn't mean only x86.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 10:58, Aurelien Jarno wrote:
> Jan Kiszka a écrit :
>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>> Jan Kiszka a écrit :
>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>> any host OS. Any pointer?
>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>
>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>> It would be too early at this point.
>>>>
>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>> code paths around or to invest significant additional effort to change
>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>> subtle differences are really a PITA and source of various breakages.
>>>>
>>>> People interested in that platform should finally realize that its fate
>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>> see right now and even more in the future.
>>>>
>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>> support at all and added these #ifdef. Disabling Windows support because
>>> of that is not fair.
>>
>> The TCG execution model won't scale long-term. It's already a main to
>> boot a quad or just dual core VM, even more when your host has at least
>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>
> 
> And what's the issue with that? People don't always look for performance
> when using QEMU. They even often try to emulate old machines (and non
> x86 ones), which anyway only have one CPU. This won't change in 5 years,
> the only thing is that those machines will be 5 years older.
> 
> People have to keep in mind that QEMU doesn't mean only virtualization
> and doesn't mean only x86.

I'm not talking about virtualization here. I'm talking about usable
emulation of today's (!) embedded multi-core platforms. It matters a lot
if your test roundtrip for booting into a SMP guest and running some
apps is a few 10 seconds, a few minutes or even not practically working.
Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
just hope I'll never depend on this for work.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Jan Kiszka a écrit :
> On 2011-02-08 10:58, Aurelien Jarno wrote:
>> Jan Kiszka a écrit :
>>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>>> Jan Kiszka a écrit :
>>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>>> any host OS. Any pointer?
>>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>>
>>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>>> It would be too early at this point.
>>>>>
>>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>>> code paths around or to invest significant additional effort to change
>>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>>> subtle differences are really a PITA and source of various breakages.
>>>>>
>>>>> People interested in that platform should finally realize that its fate
>>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>>> see right now and even more in the future.
>>>>>
>>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>>> support at all and added these #ifdef. Disabling Windows support because
>>>> of that is not fair.
>>> The TCG execution model won't scale long-term. It's already a main to
>>> boot a quad or just dual core VM, even more when your host has at least
>>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>>
>> And what's the issue with that? People don't always look for performance
>> when using QEMU. They even often try to emulate old machines (and non
>> x86 ones), which anyway only have one CPU. This won't change in 5 years,
>> the only thing is that those machines will be 5 years older.
>>
>> People have to keep in mind that QEMU doesn't mean only virtualization
>> and doesn't mean only x86.
> 
> I'm not talking about virtualization here. I'm talking about usable
> emulation of today's (!) embedded multi-core platforms. It matters a lot
> if your test roundtrip for booting into a SMP guest and running some
> apps is a few 10 seconds, a few minutes or even not practically working.
> Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
> just hope I'll never depend on this for work.

Yes, it's slow. But is it a problem? You assume that people use QEMU
only for emulating SMP platforms. This is a wrong assumption. Beside the
x86 target, only sparc really supports SMP emulation.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Alexander Graf]

On 08.02.2011, at 11:06, Aurelien Jarno wrote:

> Jan Kiszka a écrit :
>> On 2011-02-08 10:58, Aurelien Jarno wrote:
>>> Jan Kiszka a écrit :
>>>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>>>> Jan Kiszka a écrit :
>>>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>>>> any host OS. Any pointer?
>>>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>>> 
>>>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>>>> It would be too early at this point.
>>>>>> 
>>>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>>>> code paths around or to invest significant additional effort to change
>>>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>>>> subtle differences are really a PITA and source of various breakages.
>>>>>> 
>>>>>> People interested in that platform should finally realize that its fate
>>>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>>>> see right now and even more in the future.
>>>>>> 
>>>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>>>> support at all and added these #ifdef. Disabling Windows support because
>>>>> of that is not fair.
>>>> The TCG execution model won't scale long-term. It's already a main to
>>>> boot a quad or just dual core VM, even more when your host has at least
>>>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>>>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>>> 
>>> And what's the issue with that? People don't always look for performance
>>> when using QEMU. They even often try to emulate old machines (and non
>>> x86 ones), which anyway only have one CPU. This won't change in 5 years,
>>> the only thing is that those machines will be 5 years older.
>>> 
>>> People have to keep in mind that QEMU doesn't mean only virtualization
>>> and doesn't mean only x86.
>> 
>> I'm not talking about virtualization here. I'm talking about usable
>> emulation of today's (!) embedded multi-core platforms. It matters a lot
>> if your test roundtrip for booting into a SMP guest and running some
>> apps is a few 10 seconds, a few minutes or even not practically working.
>> Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
>> just hope I'll never depend on this for work.
> 
> Yes, it's slow. But is it a problem? You assume that people use QEMU
> only for emulating SMP platforms. This is a wrong assumption. Beside the
> x86 target, only sparc really supports SMP emulation.

I guess his point here really is that soon SMP is commodity. Most new ARM cores move to SMP by default, MIPS is there already and even embedded PPC is multi-core for a while now. Sure, you can work around things by only emulating a single core at times, but it's not always good enough - especially if you're working on interrupt handling code.

Either way, the whole discussion is moot. We either do support Windows or we don't. Most of the developers don't even have windows machines, so it's very hard for them to do it - even less so do they have windows programming knowledge. So what we really need is for someone to implement the thread infrastructure and aio support on windows and then all is great (until the next big infrastructure feature of course).

If only the Android people wouldn't simply fork every project out there, but work upstream, we'd probably have quite a few folks happy to support windows from that crowd, as they depend on it heavily.


Alex

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Stefan Hajnoczi]

Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
citizens.  I think you'd like people to provide full support when they
introduce new features.

This is a good motivator to use glib and have a unified code path for
TCG/KVM and Linux/Windows.  Yes it will require some work and some
optimization, but at the end we'll have better host platform parity
and a simpler main loop for TCG/KVM to interact with.

Stefan

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Stefan Hajnoczi a écrit :
> Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
> citizens.  I think you'd like people to provide full support when they
> introduce new features.
> 

I think you really pointed the problem here. We should probably add a
feature that will make KVM second class citizen so that people can
understand what it means.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 11:27 AM, Aurelien Jarno wrote:
> Stefan Hajnoczi a écrit :
>> Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
>> citizens.  I think you'd like people to provide full support when they
>> introduce new features.
>
> I think you really pointed the problem here. We should probably add a
> feature that will make KVM second class citizen so that people can
> understand what it means.

I actually don't think introducing IOTHREAD made Windows a second class 
citizen, since it was left as a non-default choice for years.  People 
care about IOTHREAD now only because (after years) there is serious 
thought about making it the default.

I'm sure that if you add such a killer feature that is TCG-only, KVM 
people will try to support it in a shorter timeframe.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 11:27, Aurelien Jarno wrote:
> Stefan Hajnoczi a écrit :
>> Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
>> citizens.  I think you'd like people to provide full support when they
>> introduce new features.
>>
> 
> I think you really pointed the problem here. We should probably add a
> feature that will make KVM second class citizen so that people can
> understand what it means.

There are people out there who already thought loudly about forking or
rewriting those QEMU bits required for KVM support just to make "life
easier". I already disagreed on this, and I continue to do so as both
use cases nicely benefit from each other.

KVM is driving QEMU features today that would otherwise have taken years
to show up, if at all. On the other side, all those bits related to the
cross-arch platform emulation of non-x86 helps and will continue to help
KVM support on those archs as well (we already have it on PPC, we'll see
on ARM and likely more in the future).

So, please let's stop this useless finger pointing, on both sides. KVM
and QEMU is a symbiosis. Unfortunately, this is not (yet?) the case for
POSIX vs. Windows hosts.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 04:27 AM, Aurelien Jarno wrote:
> Stefan Hajnoczi a écrit :
>    
>> Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
>> citizens.  I think you'd like people to provide full support when they
>> introduce new features.
>>
>>      
> I think you really pointed the problem here. We should probably add a
> feature that will make KVM second class citizen so that people can
> understand what it means.
>    

Aurelien,

Have you actually run QEMU on Windows and tried to use it to do 
something useful?

As an exercise, walk through the various releases of QEMU and compare 
how well it works on Windows to any Unix platform.  Windows support in 
QEMU has always been a second class citizen.

If someone is willing to stand up and properly maintain it, I'm all for 
doing whatever we can to be supportive of that person but as of right 
now, that doesn't exist.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Anthony Liguori a écrit :
> On 02/08/2011 04:27 AM, Aurelien Jarno wrote:
>> Stefan Hajnoczi a écrit :
>>    
>>> Introducing IOTHREAD made !CONFIG_IOTHREAD platforms second class
>>> citizens.  I think you'd like people to provide full support when they
>>> introduce new features.
>>>
>>>      
>> I think you really pointed the problem here. We should probably add a
>> feature that will make KVM second class citizen so that people can
>> understand what it means.
>>    
> 
> Aurelien,
> 
> Have you actually run QEMU on Windows and tried to use it to do 
> something useful?
> 
> As an exercise, walk through the various releases of QEMU and compare 
> how well it works on Windows to any Unix platform.  Windows support in 
> QEMU has always been a second class citizen.

I never tried to get it working on windows, but I know some people using
it there. We should just don't ignore them. Maybe it's not perfect, but
it is enough for those people.

> If someone is willing to stand up and properly maintain it, I'm all for 
> doing whatever we can to be supportive of that person but as of right 
> now, that doesn't exist.

There are regular patches for windows support, Stefan Weil is producing
builds regularly. Maybe it doesn't have all the features, but people are
making sure it basically works.

Now you want to break that because the *new* feature you want to
introduce is not supported on windows. I insist on the fact it is a new
feature simply because it was *disabled* by default. So I don't buy the
argument about "that person don't exist".

Send a call for help on that subject, give the people some time to come
with a solution (let's put a deadline), and then if nobody appears we
can definitely consider Windows support as dead. But it should not be
done arbitrary.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Riku Voipio]

On Tue, Feb 08, 2011 at 12:05:31PM -0600, Anthony Liguori wrote:
> Aurelien,

> Have you actually run QEMU on Windows and tried to use it to do  
> something useful?

I'm not Aurelian, but we do use QEMU on win32 as part of Nokia Qt SDK.
While it is second class in many ways compared to Linux QEMU (or even
OS X), it is still quite useful for us.

Riku

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 11:06, Aurelien Jarno wrote:
> Jan Kiszka a écrit :
>> On 2011-02-08 10:58, Aurelien Jarno wrote:
>>> Jan Kiszka a écrit :
>>>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>>>> Jan Kiszka a écrit :
>>>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>>>> any host OS. Any pointer?
>>>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>>>
>>>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>>>> It would be too early at this point.
>>>>>>
>>>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>>>> code paths around or to invest significant additional effort to change
>>>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>>>> subtle differences are really a PITA and source of various breakages.
>>>>>>
>>>>>> People interested in that platform should finally realize that its fate
>>>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>>>> see right now and even more in the future.
>>>>>>
>>>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>>>> support at all and added these #ifdef. Disabling Windows support because
>>>>> of that is not fair.
>>>> The TCG execution model won't scale long-term. It's already a main to
>>>> boot a quad or just dual core VM, even more when your host has at least
>>>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>>>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>>>
>>> And what's the issue with that? People don't always look for performance
>>> when using QEMU. They even often try to emulate old machines (and non
>>> x86 ones), which anyway only have one CPU. This won't change in 5 years,
>>> the only thing is that those machines will be 5 years older.
>>>
>>> People have to keep in mind that QEMU doesn't mean only virtualization
>>> and doesn't mean only x86.
>>
>> I'm not talking about virtualization here. I'm talking about usable
>> emulation of today's (!) embedded multi-core platforms. It matters a lot
>> if your test roundtrip for booting into a SMP guest and running some
>> apps is a few 10 seconds, a few minutes or even not practically working.
>> Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
>> just hope I'll never depend on this for work.
> 
> Yes, it's slow. But is it a problem? You assume that people use QEMU
> only for emulating SMP platforms. This is a wrong assumption. Beside the
> x86 target, only sparc really supports SMP emulation.

That's too nearsighted. SMP will be commodity on practically _any_ arch
within the next years. And if QEMU doesn't keep up with it, feature and
performance-wise, it will loose market share.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Jan Kiszka a écrit :
> On 2011-02-08 11:06, Aurelien Jarno wrote:
>> Jan Kiszka a écrit :
>>> On 2011-02-08 10:58, Aurelien Jarno wrote:
>>>> Jan Kiszka a écrit :
>>>>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>>>>> Jan Kiszka a écrit :
>>>>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>>>>> any host OS. Any pointer?
>>>>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>>>>
>>>>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>>>>> It would be too early at this point.
>>>>>>>
>>>>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>>>>> code paths around or to invest significant additional effort to change
>>>>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>>>>> subtle differences are really a PITA and source of various breakages.
>>>>>>>
>>>>>>> People interested in that platform should finally realize that its fate
>>>>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>>>>> see right now and even more in the future.
>>>>>>>
>>>>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>>>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>>>>> support at all and added these #ifdef. Disabling Windows support because
>>>>>> of that is not fair.
>>>>> The TCG execution model won't scale long-term. It's already a main to
>>>>> boot a quad or just dual core VM, even more when your host has at least
>>>>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>>>>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>>>>
>>>> And what's the issue with that? People don't always look for performance
>>>> when using QEMU. They even often try to emulate old machines (and non
>>>> x86 ones), which anyway only have one CPU. This won't change in 5 years,
>>>> the only thing is that those machines will be 5 years older.
>>>>
>>>> People have to keep in mind that QEMU doesn't mean only virtualization
>>>> and doesn't mean only x86.
>>> I'm not talking about virtualization here. I'm talking about usable
>>> emulation of today's (!) embedded multi-core platforms. It matters a lot
>>> if your test roundtrip for booting into a SMP guest and running some
>>> apps is a few 10 seconds, a few minutes or even not practically working.
>>> Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
>>> just hope I'll never depend on this for work.
>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>> x86 target, only sparc really supports SMP emulation.
> 
> That's too nearsighted. SMP will be commodity on practically _any_ arch
> within the next years. And if QEMU doesn't keep up with it, feature and
> performance-wise, it will loose market share.
> 

Oh commercial arguments now. I am looking for something that answer my
needs, not about market share.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Jan Kiszka]

On 2011-02-08 11:26, Aurelien Jarno wrote:
> Jan Kiszka a écrit :
>> On 2011-02-08 11:06, Aurelien Jarno wrote:
>>> Jan Kiszka a écrit :
>>>> On 2011-02-08 10:58, Aurelien Jarno wrote:
>>>>> Jan Kiszka a écrit :
>>>>>> On 2011-02-08 10:05, Aurelien Jarno wrote:
>>>>>>> Jan Kiszka a écrit :
>>>>>>>> On 2011-02-08 09:08, Paolo Bonzini wrote:
>>>>>>>>> On 02/08/2011 08:26 AM, Aurelien Jarno wrote:
>>>>>>>>>> I forget to remember when we decided that AIO should be implemented on
>>>>>>>>>> any host OS. Any pointer?
>>>>>>>>> To be fair, I/O-heavy workloads are almost unusable without AIO.  For 
>>>>>>>>> Window targets, they also crash under SMP due to the Windows AP 
>>>>>>>>> watchdog.  But then TCG and SMP do not go very well together anyway.
>>>>>>>>>
>>>>>>>>> However, I think deprecating Win32 support would be a very bad idea.
>>>>>>>> It would be too early at this point.
>>>>>>>>
>>>>>>>> But if Windows is once the only reason to keep tons of hardly tested
>>>>>>>> code paths around or to invest significant additional effort to change
>>>>>>>> logic or interfaces in this area, than I would prefer that step. I'm
>>>>>>>> hacking on IOTHREAD vs. !IOTHREAD for some weeks now, and all those
>>>>>>>> subtle differences are really a PITA and source of various breakages.
>>>>>>>>
>>>>>>>> People interested in that platform should finally realize that its fate
>>>>>>>> is coupled to reducing the #ifdefs as well as the design differences we
>>>>>>>> see right now and even more in the future.
>>>>>>>>
>>>>>>> The guilty here is IOTHREAD. Windows support predates IOTHREAD concept,
>>>>>>> it's just that people who introduce IOTHREAD didn't care about Windows
>>>>>>> support at all and added these #ifdef. Disabling Windows support because
>>>>>>> of that is not fair.
>>>>>> The TCG execution model won't scale long-term. It's already a main to
>>>>>> boot a quad or just dual core VM, even more when your host has at least
>>>>>> as many real cores. I'm sure we'll see multi-threaded TCG CPUs in the
>>>>>> future, and the iothread will just be one of 7, 17 or 257 threads.
>>>>>>
>>>>> And what's the issue with that? People don't always look for performance
>>>>> when using QEMU. They even often try to emulate old machines (and non
>>>>> x86 ones), which anyway only have one CPU. This won't change in 5 years,
>>>>> the only thing is that those machines will be 5 years older.
>>>>>
>>>>> People have to keep in mind that QEMU doesn't mean only virtualization
>>>>> and doesn't mean only x86.
>>>> I'm not talking about virtualization here. I'm talking about usable
>>>> emulation of today's (!) embedded multi-core platforms. It matters a lot
>>>> if your test roundtrip for booting into a SMP guest and running some
>>>> apps is a few 10 seconds, a few minutes or even not practically working.
>>>> Ever tried to boot a 16 core VM in emulation mode? I did, for fun. I
>>>> just hope I'll never depend on this for work.
>>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>>> x86 target, only sparc really supports SMP emulation.
>>
>> That's too nearsighted. SMP will be commodity on practically _any_ arch
>> within the next years. And if QEMU doesn't keep up with it, feature and
>> performance-wise, it will loose market share.
>>
> 
> Oh commercial arguments now. I am looking for something that answer my
> needs, not about market share.
> 

"Market share" simply means user base, for commercial or for hobby,
academic, whatever use. QEMU has a nice position here ATM. Even
commercial competitors can help continuously comparing their solutions
with QEMU (I once enjoyed such a product presentation). However, time
does not stand still.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
> Yes, it's slow. But is it a problem? You assume that people use QEMU
> only for emulating SMP platforms. This is a wrong assumption. Beside the
> x86 target, only sparc really supports SMP emulation.
>    

It's *not* just about performance.

TCG requires a signal to break out of a tight chained TB loop.  If you 
have a guest in a tight loop waiting for something external (like 
polling on a in-memory flag), the device emulation will not get to run 
until a signal is fired.

Unless you set SIGIO on every file descriptor that selects polls on (and 
you can't because there are a number that just don't support SIGIO), 
then you have a race condition.

This can be fixed by running TCG in a separate thread than select() and 
sending a signal to the TCG VCPU when select() returns (effectively 
SIGIO in userspace).

This is exactly what the I/O thread does.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Tristan Gingold]

On Feb 8, 2011, at 6:58 PM, Anthony Liguori wrote:

> On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>> x86 target, only sparc really supports SMP emulation.
>>   
> 
> It's *not* just about performance.
> 
> TCG requires a signal to break out of a tight chained TB loop.  If you have a guest in a tight loop waiting for something external (like polling on a in-memory flag), the device emulation will not get to run until a signal is fired.
> 
> Unless you set SIGIO on every file descriptor that selects polls on (and you can't because there are a number that just don't support SIGIO), then you have a race condition.

A race condition ?  Looks like you are describing a dead-lock.

But the dead lock doesn't happen because of the timer which periodically exits from TCG.  Hence the performance issue.

> This can be fixed by running TCG in a separate thread than select() and sending a signal to the TCG VCPU when select() returns (effectively SIGIO in userspace).
> 
> This is exactly what the I/O thread does.


(Nobody was able to make it working on Windows - or nobody was interested in ?)

Tristan.

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

On Tue, Feb 08, 2011 at 12:07:02PM +0100, Tristan Gingold wrote:
> 
> On Feb 8, 2011, at 6:58 PM, Anthony Liguori wrote:
> 
> > On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
> >> Yes, it's slow. But is it a problem? You assume that people use QEMU
> >> only for emulating SMP platforms. This is a wrong assumption. Beside the
> >> x86 target, only sparc really supports SMP emulation.
> >>   
> > 
> > It's *not* just about performance.
> > 
> > TCG requires a signal to break out of a tight chained TB loop.  If you have a guest in a tight loop waiting for something external (like polling on a in-memory flag), the device emulation will not get to run until a signal is fired.
> > 
> > Unless you set SIGIO on every file descriptor that selects polls on (and you can't because there are a number that just don't support SIGIO), then you have a race condition.
> 
> A race condition ?  Looks like you are describing a dead-lock.
> 
> But the dead lock doesn't happen because of the timer which periodically exits from TCG.  Hence the performance issue.
> 
> > This can be fixed by running TCG in a separate thread than select() and sending a signal to the TCG VCPU when select() returns (effectively SIGIO in userspace).
> > 
> > This is exactly what the I/O thread does.
> 
> 
> (Nobody was able to make it working on Windows - or nobody was interested in ?)
> 
Given the I/O thread is disabled by default, my guess is that nobody 
really see an interest in looking at that.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 12:46 PM, Aurelien Jarno wrote:
> Given the I/O thread is disabled by default, my guess is that nobody
> really see an interest in looking at that.

I had started looking at it in my free time.  I stopped because the 
thread pool series were continuously changing the QemuThread APIs.  I 
can resume looking at it.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 05:46 AM, Aurelien Jarno wrote:
> On Tue, Feb 08, 2011 at 12:07:02PM +0100, Tristan Gingold wrote:
>    
>> On Feb 8, 2011, at 6:58 PM, Anthony Liguori wrote:
>>
>>      
>>> On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
>>>        
>>>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>>>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>>>> x86 target, only sparc really supports SMP emulation.
>>>>
>>>>          
>>> It's *not* just about performance.
>>>
>>> TCG requires a signal to break out of a tight chained TB loop.  If you have a guest in a tight loop waiting for something external (like polling on a in-memory flag), the device emulation will not get to run until a signal is fired.
>>>
>>> Unless you set SIGIO on every file descriptor that selects polls on (and you can't because there are a number that just don't support SIGIO), then you have a race condition.
>>>        
>> A race condition ?  Looks like you are describing a dead-lock.
>>
>> But the dead lock doesn't happen because of the timer which periodically exits from TCG.  Hence the performance issue.
>>      

With dynticks, you don't always have a periodic timer (unless the guest 
has a periodic timer enabled).  There's a good bit of early startup code 
that runs without a periodic timer enabled.

Now that said, we never truly sleep forever.  We'll set something like a 
5 second timeout.  But 5 seconds might as well be forever and this is 
certainly a giant hack.

Regards,

Anthony Liguori

>>> This can be fixed by running TCG in a separate thread than select() and sending a signal to the TCG VCPU when select() returns (effectively SIGIO in userspace).
>>>
>>> This is exactly what the I/O thread does.
>>>        
>>
>> (Nobody was able to make it working on Windows - or nobody was interested in ?)
>>
>>      
> Given the I/O thread is disabled by default, my guess is that nobody
> really see an interest in looking at that.
>
>    

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Anthony Liguori a écrit :
> On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>> x86 target, only sparc really supports SMP emulation.
>>    
> 
> It's *not* just about performance.
> 
> TCG requires a signal to break out of a tight chained TB loop.  If you 
> have a guest in a tight loop waiting for something external (like 
> polling on a in-memory flag), the device emulation will not get to run 
> until a signal is fired.
> 
> Unless you set SIGIO on every file descriptor that selects polls on (and 
> you can't because there are a number that just don't support SIGIO), 
> then you have a race condition.
> 

In practice you will get a signal when the next timer event expire. I
agree it's suboptimal, but it works, and has been like that for here.

Having that fixed through an I/O thread is actually quite nice, however
it should not be done ignoring all the *current* drawbacks of the
iothread mode. We know them (at least for some of them), so let's try to
solve them.

And now, I don't buy the argument "it's been there for years", it was
*disabled* by default.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
> however
> it should not be done ignoring all the*current*  drawbacks of the
> iothread mode. We know them (at least for some of them), so let's try to
> solve them.

Let's also enumerate them.

> And now, I don't buy the argument "it's been there for years", it was
> *disabled*  by default.

It was disabled by default only because it is most useful for KVM and 
people were using qemu-kvm's iothread.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Paolo Bonzini a écrit :
> On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
>> however
>> it should not be done ignoring all the*current*  drawbacks of the
>> iothread mode. We know them (at least for some of them), so let's try to
>> solve them.
> 
> Let's also enumerate them.
> 

>From what I know:
- performance regression in TCG mode
- windows support

I am going to look again at the first one tonight to provide some numbers.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Aurelien Jarno a écrit :
> Paolo Bonzini a écrit :
>> On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
>>> however
>>> it should not be done ignoring all the*current*  drawbacks of the
>>> iothread mode. We know them (at least for some of them), so let's try to
>>> solve them.
>> Let's also enumerate them.
>>
> 
> From what I know:
> - performance regression in TCG mode

I setup an x86_64 guest on an x86_64 host (Intel Xeon E5345). Nothing
was running except the standard daemons and the CPU governor was set to
"performance" on all CPU. I then compared the network performance using
netperf in default mode, through a tap interface and a virtio nic. I got
the following results (quite reproducible, std below 0.5):
- without IO thread: 107.36 MB/s
- with IO thread:     89.93 MB/s

I haven't redone the tests I have done two weeks ago on MIPS, ARM,
PowerPC and SH4 (using different emulated network cards: smc91c111,
rtl8139, e1000, virtio), but it was roughly the same slow down, except
on ARM where it was more important.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

On Tue, Feb 08, 2011 at 04:08:28PM +0100, Aurelien Jarno wrote:
> Aurelien Jarno a écrit :
> > Paolo Bonzini a écrit :
> >> On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
> >>> however
> >>> it should not be done ignoring all the*current*  drawbacks of the
> >>> iothread mode. We know them (at least for some of them), so let's try to
> >>> solve them.
> >> Let's also enumerate them.
> >>
> > 
> > From what I know:
> > - performance regression in TCG mode
> 
> I setup an x86_64 guest on an x86_64 host (Intel Xeon E5345). Nothing
> was running except the standard daemons and the CPU governor was set to
> "performance" on all CPU. I then compared the network performance using
> netperf in default mode, through a tap interface and a virtio nic. I got
> the following results (quite reproducible, std below 0.5):
> - without IO thread: 107.36 MB/s
> - with IO thread:     89.93 MB/s
> 

And the same test on the code from september 2009:
- without IO thread: 141.8 MB/s

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/09/2011 06:35 PM, Aurelien Jarno wrote:
> On Tue, Feb 08, 2011 at 04:08:28PM +0100, Aurelien Jarno wrote:
>    
>> Aurelien Jarno a écrit :
>>      
>>> Paolo Bonzini a écrit :
>>>        
>>>> On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
>>>>          
>>>>> however
>>>>> it should not be done ignoring all the*current*  drawbacks of the
>>>>> iothread mode. We know them (at least for some of them), so let's try to
>>>>> solve them.
>>>>>            
>>>> Let's also enumerate them.
>>>>
>>>>          
>>>  From what I know:
>>> - performance regression in TCG mode
>>>        
>> I setup an x86_64 guest on an x86_64 host (Intel Xeon E5345). Nothing
>> was running except the standard daemons and the CPU governor was set to
>> "performance" on all CPU. I then compared the network performance using
>> netperf in default mode, through a tap interface and a virtio nic. I got
>> the following results (quite reproducible, std below 0.5):
>> - without IO thread: 107.36 MB/s
>> - with IO thread:     89.93 MB/s
>>
>>      
> And the same test on the code from september 2009:
> - without IO thread: 141.8 MB/s
>    

virtio-net is super finicky regarding mitigation strategies and their 
relationship to the I/O thread.  Different benchmarks will behave 
differently.  virtio-blk is probably a better device to test as you'll 
get much more consistent results across different type of I/O patterns.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Marcelo Tosatti]

On Wed, Feb 09, 2011 at 09:07:52PM +0100, Anthony Liguori wrote:
> On 02/09/2011 06:35 PM, Aurelien Jarno wrote:
> >On Tue, Feb 08, 2011 at 04:08:28PM +0100, Aurelien Jarno wrote:
> >>Aurelien Jarno a écrit :
> >>>Paolo Bonzini a écrit :
> >>>>On 02/08/2011 12:15 PM, Aurelien Jarno wrote:
> >>>>>however
> >>>>>it should not be done ignoring all the*current*  drawbacks of the
> >>>>>iothread mode. We know them (at least for some of them), so let's try to
> >>>>>solve them.
> >>>>Let's also enumerate them.
> >>>>
> >>> From what I know:
> >>>- performance regression in TCG mode
> >>I setup an x86_64 guest on an x86_64 host (Intel Xeon E5345). Nothing
> >>was running except the standard daemons and the CPU governor was set to
> >>"performance" on all CPU. I then compared the network performance using
> >>netperf in default mode, through a tap interface and a virtio nic. I got
> >>the following results (quite reproducible, std below 0.5):
> >>- without IO thread: 107.36 MB/s
> >>- with IO thread:     89.93 MB/s
> >>
> >And the same test on the code from september 2009:
> >- without IO thread: 141.8 MB/s
> virtio-net is super finicky regarding mitigation strategies and
> their relationship to the I/O thread.  Different benchmarks will
> behave differently.  virtio-blk is probably a better device to test
> as you'll get much more consistent results across different type of
> I/O patterns.

netperf server on guest, RHEL5.4 guest (e1000), uq/master branch, TCG:

iothread: 236MB/s
no iothread: 215MB/s

Also noticed scp was slightly faster with iothread earlier this week,
don't remember numbers.

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 05:15 AM, Aurelien Jarno wrote:
> Anthony Liguori a écrit :
>    
>> On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
>>      
>>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>>> x86 target, only sparc really supports SMP emulation.
>>>
>>>        
>> It's *not* just about performance.
>>
>> TCG requires a signal to break out of a tight chained TB loop.  If you
>> have a guest in a tight loop waiting for something external (like
>> polling on a in-memory flag), the device emulation will not get to run
>> until a signal is fired.
>>
>> Unless you set SIGIO on every file descriptor that selects polls on (and
>> you can't because there are a number that just don't support SIGIO),
>> then you have a race condition.
>>
>>      
> In practice you will get a signal when the next timer event expire. I
> agree it's suboptimal, but it works, and has been like that for here.
>    

During early boot up before the periodic timer is enabled can cause 
quite a noticable issue here.

I think it's cris specifically that does polling I/O in the early 
startup before any periodic timer is enabled.

> Having that fixed through an I/O thread is actually quite nice, however
> it should not be done ignoring all the *current* drawbacks of the
> iothread mode. We know them (at least for some of them), so let's try to
> solve them.
>    

Yes, agree 100%.

> And now, I don't buy the argument "it's been there for years", it was
> *disabled* by default.
>    

Yeah, I think we need to enable it by default and commit to fixing all 
of the outstanding issues.

I think we've fixed all that we're aware of but we probably won't find 
the rest unless we enable it universally.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Anthony Liguori a écrit :
> On 02/08/2011 05:15 AM, Aurelien Jarno wrote:
>> Anthony Liguori a écrit :
>>    
>>> On 02/08/2011 04:06 AM, Aurelien Jarno wrote:
>>>      
>>>> Yes, it's slow. But is it a problem? You assume that people use QEMU
>>>> only for emulating SMP platforms. This is a wrong assumption. Beside the
>>>> x86 target, only sparc really supports SMP emulation.
>>>>
>>>>        
>>> It's *not* just about performance.
>>>
>>> TCG requires a signal to break out of a tight chained TB loop.  If you
>>> have a guest in a tight loop waiting for something external (like
>>> polling on a in-memory flag), the device emulation will not get to run
>>> until a signal is fired.
>>>
>>> Unless you set SIGIO on every file descriptor that selects polls on (and
>>> you can't because there are a number that just don't support SIGIO),
>>> then you have a race condition.
>>>
>>>      
>> In practice you will get a signal when the next timer event expire. I
>> agree it's suboptimal, but it works, and has been like that for here.
>>    
> 
> During early boot up before the periodic timer is enabled can cause 
> quite a noticable issue here.
> 
> I think it's cris specifically that does polling I/O in the early 
> startup before any periodic timer is enabled.
> 
>> Having that fixed through an I/O thread is actually quite nice, however
>> it should not be done ignoring all the *current* drawbacks of the
>> iothread mode. We know them (at least for some of them), so let's try to
>> solve them.
>>    
> 
> Yes, agree 100%.
> 
>> And now, I don't buy the argument "it's been there for years", it was
>> *disabled* by default.
>>    
> 
> Yeah, I think we need to enable it by default and commit to fixing all 
> of the outstanding issues.

So the strategy is let's break everything and wait for the maintainer to
fix that? This strategy doesn't work, we have seen for example that with
the SeaBIOS switch. While it brings nice features, it has broken the
isapc machine. And it's still not fixed...

Also this strategy doesn't scale, then the maintainers are spending
their time fixing bugs introduced because others didn't care. Resources
are not unlimited, especially for those doing that on their free time.

> I think we've fixed all that we're aware of but we probably won't find 
> the rest unless we enable it universally.

I agree that we are going to discover bugs, and it's normal. QEMU is
quite complex and it's not possible to test every combination. That said
we are already aware of some bugs, why not fix them, or at least try to
fix them? For example we haven't fixed the performance regression with
TCG (at least it wasn't the case two weeks ago).

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Anthony Liguori]

On 02/08/2011 07:30 AM, Aurelien Jarno wrote:
> So the strategy is let's break everything and wait for the maintainer to
> fix that? This strategy doesn't work, we have seen for example that with
> the SeaBIOS switch. While it brings nice features, it has broken the
> isapc machine. And it's still not fixed...
>    

The fundamental problem is that poorly thought out features have been 
committed in the past.  isapc is a good example of this.

You can't just remove a chipset but leave an ISA bus implementation and 
expect things to just keep working.  Even the early ISA-only systems had 
a chipset that firmware interfaced with.

> Also this strategy doesn't scale, then the maintainers are spending
> their time fixing bugs introduced because others didn't care. Resources
> are not unlimited, especially for those doing that on their free time.
>    

So are you suggesting that every half baked feature should hold up any 
other future developments?  I think the real problem is exactly the 
opposite of what you describe.  Why should we waste finite resources 
keeping something like Windows support limping along?

We need to do a better job of not adding features that there is no 
serious intention of every supporting in a meaningful way.  I think the 
recent discussion of w64 is a good example of this.  I can't imagine 
trying to support w64 in QEMU until someone actually makes w32 work in a 
reasonable way.

>> I think we've fixed all that we're aware of but we probably won't find
>> the rest unless we enable it universally.
>>      
> I agree that we are going to discover bugs, and it's normal. QEMU is
> quite complex and it's not possible to test every combination. That said
> we are already aware of some bugs, why not fix them, or at least try to
> fix them? For example we haven't fixed the performance regression with
> TCG (at least it wasn't the case two weeks ago).
>    

If there are known issues, yes, let's fix them before enabling it.

Regards,

Anthony Liguori

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

Anthony Liguori a écrit :
> On 02/08/2011 07:30 AM, Aurelien Jarno wrote:
>> So the strategy is let's break everything and wait for the maintainer to
>> fix that? This strategy doesn't work, we have seen for example that with
>> the SeaBIOS switch. While it brings nice features, it has broken the
>> isapc machine. And it's still not fixed...
>>    
> 
> The fundamental problem is that poorly thought out features have been 
> committed in the past.  isapc is a good example of this.
> 
> You can't just remove a chipset but leave an ISA bus implementation and 
> expect things to just keep working.  Even the early ISA-only systems had 
> a chipset that firmware interfaced with.
> 
>> Also this strategy doesn't scale, then the maintainers are spending
>> their time fixing bugs introduced because others didn't care. Resources
>> are not unlimited, especially for those doing that on their free time.
>>    
> 
> So are you suggesting that every half baked feature should hold up any 
> other future developments?  I think the real problem is exactly the 
> opposite of what you describe.  Why should we waste finite resources 
> keeping something like Windows support limping along?
> 
> We need to do a better job of not adding features that there is no 
> serious intention of every supporting in a meaningful way.  I think the 
> recent discussion of w64 is a good example of this.  I can't imagine 
> trying to support w64 in QEMU until someone actually makes w32 work in a 
> reasonable way.

Yes, we should at least leave people time to find a solution. If nobody
comes with a solution, let's consider it deprecated.

>>> I think we've fixed all that we're aware of but we probably won't find
>>> the rest unless we enable it universally.
>>>      
>> I agree that we are going to discover bugs, and it's normal. QEMU is
>> quite complex and it's not possible to test every combination. That said
>> we are already aware of some bugs, why not fix them, or at least try to
>> fix them? For example we haven't fixed the performance regression with
>> TCG (at least it wasn't the case two weeks ago).
>>    
> 
> If there are known issues, yes, let's fix them before enabling it.
> 

So please look at this TCG performance regression instead of talking
about enabling this just after the release. I don't consider TCG a half
baked feature, for people who forgot about that it's the original QEMU mode.

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Blue Swirl]

On Tue, Feb 8, 2011 at 5:09 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> Anthony Liguori a écrit :
>> On 02/08/2011 07:30 AM, Aurelien Jarno wrote:
>>> So the strategy is let's break everything and wait for the maintainer to
>>> fix that? This strategy doesn't work, we have seen for example that with
>>> the SeaBIOS switch. While it brings nice features, it has broken the
>>> isapc machine. And it's still not fixed...
>>>
>>
>> The fundamental problem is that poorly thought out features have been
>> committed in the past.  isapc is a good example of this.
>>
>> You can't just remove a chipset but leave an ISA bus implementation and
>> expect things to just keep working.  Even the early ISA-only systems had
>> a chipset that firmware interfaced with.
>>
>>> Also this strategy doesn't scale, then the maintainers are spending
>>> their time fixing bugs introduced because others didn't care. Resources
>>> are not unlimited, especially for those doing that on their free time.
>>>
>>
>> So are you suggesting that every half baked feature should hold up any
>> other future developments?  I think the real problem is exactly the
>> opposite of what you describe.  Why should we waste finite resources
>> keeping something like Windows support limping along?
>>
>> We need to do a better job of not adding features that there is no
>> serious intention of every supporting in a meaningful way.  I think the
>> recent discussion of w64 is a good example of this.  I can't imagine
>> trying to support w64 in QEMU until someone actually makes w32 work in a
>> reasonable way.
>
> Yes, we should at least leave people time to find a solution. If nobody
> comes with a solution, let's consider it deprecated.

I think win32 situation is somewhat similar (but not nearly as bad as)
to kqemu's. It was useful for some users, but there were no
maintenance and when it got in the way, it was removed because nobody
could fix it.

But I'd prefer a solution where somebody steps up as Windows
maintainer. I'm also doing regular mingw32 builds but otherwise not
much.

Re: [Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default

[Stefan Weil]

Am 09.02.2011 18:13, schrieb Blue Swirl:
> I think win32 situation is somewhat similar (but not nearly as bad as)
> to kqemu's. It was useful for some users, but there were no
> maintenance and when it got in the way, it was removed because nobody
> could fix it.
>
> But I'd prefer a solution where somebody steps up as Windows
> maintainer. I'm also doing regular mingw32 builds but otherwise not
> much.
>

VNC threads can be compiled for W32, too.
A short test of the resulting executable was successful, no problems.

The patch is available here:
http://repo.or.cz/w/qemu/ar7.git/commitdiff/aabf11dc0a938b84d76d7c147cbf0445d7bee297

I decided to create a new directory structure hosts/w32, so files can
be moved from the root to hosts/posix, hosts/w32, or hosts/xxx.
Include chains reduce code modifications and conditional compilations.
And people who don't want to see w32 support can remove it easily :-)

Supporting I/O threads for W32 will be possible, too.

I don't think that W32 support is a big problem. It never was.
Some of the real problems were already named in the previous mails.

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/09/2011 11:16 PM, Stefan Weil wrote:
>
> I decided to create a new directory structure hosts/w32, so files can
> be moved from the root to hosts/posix, hosts/w32, or hosts/xxx.
> Include chains reduce code modifications and conditional compilations.
> And people who don't want to see w32 support can remove it easily :-)
>
> Supporting I/O threads for W32 will be possible, too.

I have patches for Win32 iothread, I'm just posting the series split 
into multiple pieces.

Paolo

Re: [Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/09/2011 11:16 PM, Stefan Weil wrote:
> The patch is available here:
> http://repo.or.cz/w/qemu/ar7.git/commitdiff/aabf11dc0a938b84d76d7c147cbf0445d7bee297

> diff --git a/hosts/w32/include/signal.h b/hosts/w32/include/signal.h
> new file mode 100644
> index 0000000..e45f03c
> --- /dev/null
> +++ b/hosts/w32/include/signal.h
> @@ -0,0 +1,20 @@
> +/*
> + * QEMU w32 support
> + *
> + * Copyright (C) 2011 Stefan Weil
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef WIN32_SIGNAL_H
> +#define WIN32_SIGNAL_H
> +
> +#include_next <signal.h>
> +#include <sys/types.h>    /* sigset_t */
> +
> +int pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset);
> +int sigfillset(sigset_t *set);
> +
> +#endif /* WIN32_SIGNAL_H */
> diff --git a/hosts/w32/include/time.h b/hosts/w32/include/time.h
> new file mode 100644
> index 0000000..0b997d3
> --- /dev/null
> +++ b/hosts/w32/include/time.h
> @@ -0,0 +1,31 @@
> +/*
> + * QEMU w32 support
> + *
> + * Copyright (C) 2011 Stefan Weil
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#if !defined(W32_TIME_H)
> +#define W32_TIME_H
> +
> +#include_next <time.h>
> +
> +#ifndef HAVE_STRUCT_TIMESPEC
> +#define HAVE_STRUCT_TIMESPEC 1
> +struct timespec {
> +        long tv_sec;
> +        long tv_nsec;
> +};
> +#endif /* HAVE_STRUCT_TIMESPEC */
> +
> +typedef enum {
> +  CLOCK_REALTIME = 0
> +} clockid_t;
> +
> +int clock_getres (clockid_t clock_id, struct timespec *res);
> +int clock_gettime(clockid_t clock_id, struct timespec *pTimespec);
> +
> +#endif /* W32_TIME_H */
> diff --git a/os-win32.c b/os-win32.c
> index b214e6a..7778366 100644
> --- a/os-win32.c
> +++ b/os-win32.c
> @@ -36,6 +36,45 @@
>  /***********************************************************/
>  /* Functions missing in mingw */
>
> +#if defined(CONFIG_THREAD)
> +
> +int clock_gettime(clockid_t clock_id, struct timespec *pTimespec)
> +{
> +  int result = 0;
> +  if (clock_id == CLOCK_REALTIME && pTimespec != 0) {
> +    DWORD t = GetTickCount();
> +    const unsigned cps = 1000;
> +    struct timespec ts;
> +    ts.tv_sec  = t / cps;
> +    ts.tv_nsec = (t % cps) * (1000000000UL / cps);
> +    *pTimespec = ts;
> +  } else {
> +    errno = EINVAL;
> +    result = -1;
> +  }
> +  return result;
> +}

Why is this needed?  The only user of clock_gettime in the POSIX case is 
using CLOCK_MONOTONIC, and actually has a Win32 version already.

> +int pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset)
> +{
> +    /* Dummy, do nothing. */
> +    return EINVAL;
> +}
> +
> +int sigfillset(sigset_t *set)
> +{
> +    int result = 0;
> +    if (set) {
> +        *(set) = (sigset_t)(-1);
> +    } else {
> +        errno = EINVAL;
> +        result = -1;
> +    }
> +    return result;
> +}

Instead of these, it's better to provide a Win32 implementation of 
mutexes and condvars.  I'll submit it next week hopefully.

Paolo

Re: [Qemu-devel] [PATCH 2/7] Enable I/O thread and VNC threads by default

[Stefan Weil]

Am 10.02.2011 10:54, schrieb Paolo Bonzini:
> On 02/09/2011 11:16 PM, Stefan Weil wrote:
>> The patch is available here:
>> http://repo.or.cz/w/qemu/ar7.git/commitdiff/aabf11dc0a938b84d76d7c147cbf0445d7bee297 
>>
[snip]
>> diff --git a/os-win32.c b/os-win32.c
>> index b214e6a..7778366 100644
>> --- a/os-win32.c
>> +++ b/os-win32.c
>> @@ -36,6 +36,45 @@
>>  /***********************************************************/
>>  /* Functions missing in mingw */
>>
>> +#if defined(CONFIG_THREAD)
>> +
>> +int clock_gettime(clockid_t clock_id, struct timespec *pTimespec)
>> +{
>> +  int result = 0;
>> +  if (clock_id == CLOCK_REALTIME && pTimespec != 0) {
>> +    DWORD t = GetTickCount();
>> +    const unsigned cps = 1000;
>> +    struct timespec ts;
>> +    ts.tv_sec  = t / cps;
>> +    ts.tv_nsec = (t % cps) * (1000000000UL / cps);
>> +    *pTimespec = ts;
>> +  } else {
>> +    errno = EINVAL;
>> +    result = -1;
>> +  }
>> +  return result;
>> +}
>
> Why is this needed?  The only user of clock_gettime in the POSIX case 
> is using CLOCK_MONOTONIC, and actually has a Win32 version already.


qemu-thread.c uses clock_gettime(CLOCK_REALTIME, ...)


>
>> +int pthread_sigmask(int how, const sigset_t *set, sigset_t *oldset)
>> +{
>> +    /* Dummy, do nothing. */
>> +    return EINVAL;
>> +}
>> +
>> +int sigfillset(sigset_t *set)
>> +{
>> +    int result = 0;
>> +    if (set) {
>> +        *(set) = (sigset_t)(-1);
>> +    } else {
>> +        errno = EINVAL;
>> +        result = -1;
>> +    }
>> +    return result;
>> +}
>
> Instead of these, it's better to provide a Win32 implementation of 
> mutexes and condvars.  I'll submit it next week hopefully.
>
> Paolo


That's good news. My patch was only a quick hack to make threaded VNC work.

Thanks,
Stefan

[Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Paolo Bonzini]

On 02/08/2011 10:58 AM, Aurelien Jarno wrote:
> And what's the issue with that? People don't always look for performance
> when using QEMU. They even often try to emulate old machines (and non
> x86 ones), which anyway only have one CPU. This won't change in 5 years,
> the only thing is that those machines will be 5 years older.
>
> People have to keep in mind that QEMU doesn't mean only virtualization
> and doesn't mean only x86.

AFAIU nobody is proposing to rip linux-user or TCG, just to improve its 
implementation.

You just as well have to understand that AIO means fewer Windows blue 
screens of death and not only better performance.

Paolo

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Edgar E. Iglesias]

On Mon, Feb 07, 2011 at 02:03:50PM -0200, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 08:12:55AM -0200, Marcelo Tosatti wrote:
> > > > One more thing I didn't mention on the email-thread or on IRC is
> > > > that last time I checked, qemu with io-thread was performing
> > > > significantly slower than non io-thread builds. That was with
> > > > TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.
> > 
> > Can you recall what was the test ?
> > 
> > > > Also, although -icount & iothread no longer deadlocks, icount
> > > > still sometimes performs incredibly slow with the io-thread (compared
> > > > to non-io-thread qemu). In particular when not using -icount auto but
> > > > a fixed ticks per insn values. Sometimes it's so slow I thought it
> > > > actually deadlocked, but no it was crawling :) I haven't had time
> > > > to look at it any closer but I hope to do soon.
> 
> Edgar, please give the attached patch a try with fixed icount value. The
> calculation for next event makes no sense for iothread timeout, only for
> vcpu context.


Thanks Marcelo, this patch fixes the problems I was seeing here.

Cheers


> diff --git a/cpus.c b/cpus.c
> index 9c50a34..2280db1 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -748,7 +748,7 @@ static void qemu_tcg_wait_io_event(void)
>      CPUState *env;
>  
>      while (!any_cpu_has_work())
> -        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, qemu_calculate_timeout());
>  
>      qemu_mutex_unlock(&qemu_global_mutex);
>  
> diff --git a/vl.c b/vl.c
> index 837be97..dbd81a1 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -1323,7 +1323,7 @@ void main_loop_wait(int nonblocking)
>      if (nonblocking)
>          timeout = 0;
>      else {
> -        timeout = qemu_calculate_timeout();
> +        timeout = 1000;
>          qemu_bh_update_timeout(&timeout);
>      }
>  

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Aurelien Jarno]

On Mon, Feb 07, 2011 at 02:03:50PM -0200, Marcelo Tosatti wrote:
> On Mon, Feb 07, 2011 at 08:12:55AM -0200, Marcelo Tosatti wrote:
> > > > One more thing I didn't mention on the email-thread or on IRC is
> > > > that last time I checked, qemu with io-thread was performing
> > > > significantly slower than non io-thread builds. That was with
> > > > TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.
> > 
> > Can you recall what was the test ?
> > 

It's also something I've seen using network transfer in guest. IIRC the
biggest slowdown was using the smc91c111 card under qemu-system-arm
where it was about 20% slower. Other cards on other architectures (I
remember testing powerpc, mips and sh4) are more in the 5 to 10 % area.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Scott Wood]

On Mon, 7 Feb 2011 14:03:50 -0200
Marcelo Tosatti <mtosatti@redhat.com> wrote:

> Is there any other issue that prevents turning CONFIG_IOTHREAD on by
> default?
> 

This patch is needed for ppce500_mpc8544ds and ppc440_bamboo to work with
I/O thread enabled:

http://patchwork.ozlabs.org/patch/66743/

-Scott

Re: [Qemu-devel] Re: [PATCH 2/7] Enable I/O thread and VNC threads by default

[Edgar E. Iglesias]

On Mon, Feb 07, 2011 at 08:12:55AM -0200, Marcelo Tosatti wrote:
> On Tue, Jan 25, 2011 at 11:34:53AM -0200, Marcelo Tosatti wrote:
> > On Tue, Jan 25, 2011 at 10:17:41AM +0100, Edgar E. Iglesias wrote:
> > > On Mon, Jan 24, 2011 at 04:28:48PM -0600, Anthony Liguori wrote:
> > > > On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> > > > > Leave the disable options for now to help with testing but these will be removed
> > > > > once we're confident in the thread implementations.
> > > > >
> > > > > Disabled code bit rots.  These have been in tree long enough that we need to
> > > > > either commit to making them work or just remove them entirely.
> > > > >    
> > > > 
> > > > I/O thread disables icount apparently.
> > > > 
> > > > I'm not really sure why.  Marcelo, do you know the reason 
> > > > qemu_calculate_timeout returns a fixed value in the I/O thread 
> > > > regardless of icount?
> > > 
> > > Hi,
> > > 
> > > The following commit hopefully fixed that issue.
> > > 
> > > commit 225d02cd1a34d5d87e8acefbf8e244a5d12f5f8c
> > > Author: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > Date:   Sun Jan 23 04:44:51 2011 +0100
> > > 
> > >     Avoid deadlock whith iothread and icount
> > >     
> > >     When using the iothread together with icount, make sure the
> > >     qemu_icount counter makes forward progress when the vcpu is
> > >     idle to avoid deadlocks.
> > >     
> > >     Signed-off-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>
> > > 
> > > See http://lists.gnu.org/archive/html/qemu-devel/2011-01/msg01602.html
> > > for more info.
> > > 
> > > One more thing I didn't mention on the email-thread or on IRC is
> > > that last time I checked, qemu with io-thread was performing
> > > significantly slower than non io-thread builds. That was with
> > > TCG emulation (not kvm). Somewhere between 5 - 10% slower, IIRC.
> 
> Can you recall what was the test ?

Hi, didn't see this until now..

IIRC, the test was booting a CRIS linux guest.

Cheers

[Qemu-devel] [PATCH 0/2] vnc: the lost parts

[Corentin Chary]

Hi Anthony,
If you want to enable vnc threaded server by default, you should
really merge these two lost patchs :).
Thanks,

Corentin Chary (1):
  vnc: qemu can die if the client is disconnected while updating screen

Yoshiaki Tamura (1):
  vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

 ui/vnc-jobs-async.c |    4 ++++
 vl.c                |    2 ++
 2 files changed, 6 insertions(+), 0 deletions(-)

-- 
1.7.3.4

[Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

[Corentin Chary]

From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>

Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
deleting.  This may cause a crash when a read handler calls
qemu_set_fd_handler2() to delete handlers, but a write handler is
still invoked from main_loop_wait().  Because main_loop_wait() checks
handlers before calling, setting NULL upon deleting will protect
handlers being called if already deleted.

One example is the new threaded vnc server.  When an error occurs in
the context of a read handler, it'll releases resources and deletes
handlers.  However, because the write handler still exists, it'll be
called, and then crashes because of lack of resources.  This patch
fixes it.

Signed-off-by: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
Reviewed-by: Corentin Chary <corentincj@iksaif.net>
---
 vl.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/vl.c b/vl.c
index 14255c4..7a26bea 100644
--- a/vl.c
+++ b/vl.c
@@ -1037,6 +1037,8 @@ int qemu_set_fd_handler2(int fd,
         QLIST_FOREACH(ioh, &io_handlers, next) {
             if (ioh->fd == fd) {
                 ioh->deleted = 1;
+                ioh->fd_read = NULL;
+                ioh->fd_write = NULL;
                 break;
             }
         }
-- 
1.7.3.4

Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

[Stefan Hajnoczi]

On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>
> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
> deleting.  This may cause a crash when a read handler calls
> qemu_set_fd_handler2() to delete handlers, but a write handler is
> still invoked from main_loop_wait().  Because main_loop_wait() checks
> handlers before calling, setting NULL upon deleting will protect
> handlers being called if already deleted.
>
> One example is the new threaded vnc server.  When an error occurs in
> the context of a read handler, it'll releases resources and deletes
> handlers.  However, because the write handler still exists, it'll be
> called, and then crashes because of lack of resources.  This patch
> fixes it.

Does this case still happen with qemu.git/master?  In November I sent
a patch to check for deleted handlers:

commit 0290b57bdfec83ca78b6d119ea9847bb17943328
Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
Date:   Wed Nov 3 14:29:44 2010 +0000

    Delete IOHandlers after potentially running them

    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
    handler that deletes its IOHandler is exposed to .fd_write() being
    called on the deleted IOHandler.

    This patch fixes deletion so that .fd_read() and .fd_write() are never
    called on an IOHandler that is marked for deletion.

    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

So I don't think Yoshi's patch is necessary anymore?

Stefan

Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

[Corentin Chary]

On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
> <corentin.chary@gmail.com> wrote:
>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>
>> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
>> deleting.  This may cause a crash when a read handler calls
>> qemu_set_fd_handler2() to delete handlers, but a write handler is
>> still invoked from main_loop_wait().  Because main_loop_wait() checks
>> handlers before calling, setting NULL upon deleting will protect
>> handlers being called if already deleted.
>>
>> One example is the new threaded vnc server.  When an error occurs in
>> the context of a read handler, it'll releases resources and deletes
>> handlers.  However, because the write handler still exists, it'll be
>> called, and then crashes because of lack of resources.  This patch
>> fixes it.
>
> Does this case still happen with qemu.git/master?  In November I sent
> a patch to check for deleted handlers:
>
> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
> Date:   Wed Nov 3 14:29:44 2010 +0000
>
>    Delete IOHandlers after potentially running them
>
>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>    handler that deletes its IOHandler is exposed to .fd_write() being
>    called on the deleted IOHandler.
>
>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>    called on an IOHandler that is marked for deletion.
>
>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>
> So I don't think Yoshi's patch is necessary anymore?

Ho I didn't see that one.
It's probably not necessary, but it stills make sense to apply this
patch since there is
absolutly no reasons to keep the old value in fd_read and fd_write when
the user explicitly asked to set them to NULL.



-- 
Corentin Chary
http://xf.iksaif.net

Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

[Stefan Hajnoczi]

On Tue, Jan 25, 2011 at 10:13 AM, Corentin Chary
<corentin.chary@gmail.com> wrote:
> On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
>> <corentin.chary@gmail.com> wrote:
>>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>>
>>> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
>>> deleting.  This may cause a crash when a read handler calls
>>> qemu_set_fd_handler2() to delete handlers, but a write handler is
>>> still invoked from main_loop_wait().  Because main_loop_wait() checks
>>> handlers before calling, setting NULL upon deleting will protect
>>> handlers being called if already deleted.
>>>
>>> One example is the new threaded vnc server.  When an error occurs in
>>> the context of a read handler, it'll releases resources and deletes
>>> handlers.  However, because the write handler still exists, it'll be
>>> called, and then crashes because of lack of resources.  This patch
>>> fixes it.
>>
>> Does this case still happen with qemu.git/master?  In November I sent
>> a patch to check for deleted handlers:
>>
>> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
>> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>> Date:   Wed Nov 3 14:29:44 2010 +0000
>>
>>    Delete IOHandlers after potentially running them
>>
>>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>    handler that deletes its IOHandler is exposed to .fd_write() being
>>    called on the deleted IOHandler.
>>
>>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>>    called on an IOHandler that is marked for deletion.
>>
>>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>
>> So I don't think Yoshi's patch is necessary anymore?
>
> Ho I didn't see that one.
> It's probably not necessary, but it stills make sense to apply this
> patch since there is
> absolutly no reasons to keep the old value in fd_read and fd_write when
> the user explicitly asked to set them to NULL.

That's true, I don't see a good reason why we shouldn't clear them.
The only minor advantage to keeping them is that it helps when
debugging QEMU - you can identify the fd handler by its
fd_read/fd_write function pointers easily.

Stefan

Re: [Qemu-devel] [PATCH 1/2] vl.c: set NULL upon deleting handlers in qemu_set_fd_handler2()

[Yoshiaki Tamura]

2011/1/25 Stefan Hajnoczi <stefanha@gmail.com>:
> On Tue, Jan 25, 2011 at 10:13 AM, Corentin Chary
> <corentin.chary@gmail.com> wrote:
>> On Tue, Jan 25, 2011 at 10:03 AM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
>>> On Tue, Jan 25, 2011 at 8:33 AM, Corentin Chary
>>> <corentin.chary@gmail.com> wrote:
>>>> From: Yoshiaki Tamura <tamura.yoshiaki@lab.ntt.co.jp>
>>>>
>>>> Currently qemu_set_fd_handler2() is only setting ioh->deleted upon
>>>> deleting.  This may cause a crash when a read handler calls
>>>> qemu_set_fd_handler2() to delete handlers, but a write handler is
>>>> still invoked from main_loop_wait().  Because main_loop_wait() checks
>>>> handlers before calling, setting NULL upon deleting will protect
>>>> handlers being called if already deleted.
>>>>
>>>> One example is the new threaded vnc server.  When an error occurs in
>>>> the context of a read handler, it'll releases resources and deletes
>>>> handlers.  However, because the write handler still exists, it'll be
>>>> called, and then crashes because of lack of resources.  This patch
>>>> fixes it.
>>>
>>> Does this case still happen with qemu.git/master?  In November I sent
>>> a patch to check for deleted handlers:
>>>
>>> commit 0290b57bdfec83ca78b6d119ea9847bb17943328
>>> Author: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>> Date:   Wed Nov 3 14:29:44 2010 +0000
>>>
>>>    Delete IOHandlers after potentially running them
>>>
>>>    Since commit 4bed9837309e58d208183f81d8344996744292cf an .fd_read()
>>>    handler that deletes its IOHandler is exposed to .fd_write() being
>>>    called on the deleted IOHandler.
>>>
>>>    This patch fixes deletion so that .fd_read() and .fd_write() are never
>>>    called on an IOHandler that is marked for deletion.
>>>
>>>    Signed-off-by: Stefan Hajnoczi <stefanha@linux.vnet.ibm.com>
>>>    Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
>>>
>>> So I don't think Yoshi's patch is necessary anymore?
>>
>> Ho I didn't see that one.
>> It's probably not necessary, but it stills make sense to apply this
>> patch since there is
>> absolutly no reasons to keep the old value in fd_read and fd_write when
>> the user explicitly asked to set them to NULL.
>
> That's true, I don't see a good reason why we shouldn't clear them.
> The only minor advantage to keeping them is that it helps when
> debugging QEMU - you can identify the fd handler by its
> fd_read/fd_write function pointers easily.

Well, since I posted the patch in August, I won't get surprised
even the bug might be fixed due to other patches :)  Regardless of
that, I don't see the patch is doing something incorrect either.
If some codes were relying on unset values, that should be wrong.

Yoshi

>
> Stefan
>
>

[Qemu-devel] [PATCH 2/2] vnc: qemu can die if the client is disconnected while updating screen

[Corentin Chary]

From: Corentin Chary <corentincj@iksaif.net>

agraf reported that qemu_mutex_destroy(vs->output_mutex) was failing
in vnc_disconnect_finish() when the vnc client was disconnected while
updating the screen.

It's because vnc_worker_thread_loop() tries to unlock the mutex while
not locked.

Signed-off-by: Corentin Chary <corentincj@iksaif.net>
---
 ui/vnc-jobs-async.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 6e9cf08..0b5d750 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -227,6 +227,10 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
         if (job->vs->csock == -1) {
             vnc_unlock_display(job->vs->vd);
+            /* output mutex must be locked before going to
+             * disconnected:
+             */
+            vnc_lock_output(job->vs);
             goto disconnected;
         }
 
-- 
1.7.3.4

[Qemu-devel] [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it

[Anthony Liguori]

GLib is an extremely common library that has a portable thread implementation
along with tons of other goodies.

GLib and GObject have a fantastic amount of infrastructure we can leverage in
QEMU including an object oriented programming infrastructure.

Short term, it has a very nice thread pool implementation that we could leverage
in something like virtio-9p.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/Makefile b/Makefile
index 6d601ee..bf24d1b 100644
--- a/Makefile
+++ b/Makefile
@@ -104,6 +104,8 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
 
 QEMU_CFLAGS+=$(CURL_CFLAGS)
 
+QEMU_CFLAGS+=$(GLIB_CFLAGS)
+
 ui/cocoa.o: ui/cocoa.m
 
 ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
diff --git a/Makefile.objs b/Makefile.objs
index c3e52c5..283e62a 100644
--- a/Makefile.objs
+++ b/Makefile.objs
@@ -316,3 +316,5 @@ vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
 
 vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
 
+vl.o: QEMU_CFLAGS+=$(GLIB_CFLAGS)
+
diff --git a/Makefile.target b/Makefile.target
index e15b1c4..2c1c90e 100644
--- a/Makefile.target
+++ b/Makefile.target
@@ -204,6 +204,7 @@ QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
 QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
 QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
 QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
+QEMU_CFLAGS += $(GLIB_CFLAGS)
 
 # xen backend driver support
 obj-$(CONFIG_XEN) += xen_machine_pv.o xen_domainbuild.o
diff --git a/configure b/configure
index d5ac074..820fde9 100755
--- a/configure
+++ b/configure
@@ -1658,6 +1658,17 @@ EOF
 fi
 
 ##########################################
+# glib support probe
+if $pkg_config --modversion gthread-2.0 > /dev/null 2>&1 ; then
+    glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
+    glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
+    libs_softmmu="$glib_libs $libs_softmmu"
+else
+    echo "glib-2.0 required to compile QEMU"
+    exit 1
+fi
+
+##########################################
 # kvm probe
 if test "$kvm" != "no" ; then
     cat > $TMPC <<EOF
@@ -2677,6 +2688,7 @@ if test "$bluez" = "yes" ; then
   echo "CONFIG_BLUEZ=y" >> $config_host_mak
   echo "BLUEZ_CFLAGS=$bluez_cflags" >> $config_host_mak
 fi
+echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
 if test "$xen" = "yes" ; then
   echo "CONFIG_XEN=y" >> $config_host_mak
 fi
diff --git a/qemu-thread.c b/qemu-thread.c
index fbc78fe..2c521ab 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -10,183 +10,142 @@
  * See the COPYING file in the top-level directory.
  *
  */
-#include <stdlib.h>
-#include <stdio.h>
-#include <errno.h>
-#include <time.h>
-#include <signal.h>
-#include <stdint.h>
-#include <string.h>
-#include "qemu-thread.h"
 
-static void error_exit(int err, const char *msg)
-{
-    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
-    exit(1);
-}
+#include "qemu-common.h"
+#include "qemu-thread.h"
 
 void qemu_mutex_init(QemuMutex *mutex)
 {
-    int err;
-
-    err = pthread_mutex_init(&mutex->lock, NULL);
-    if (err)
-        error_exit(err, __func__);
+    g_static_mutex_init(&mutex->lock);
 }
 
 void qemu_mutex_destroy(QemuMutex *mutex)
 {
-    int err;
-
-    err = pthread_mutex_destroy(&mutex->lock);
-    if (err)
-        error_exit(err, __func__);
+    g_static_mutex_free(&mutex->lock);
 }
 
 void qemu_mutex_lock(QemuMutex *mutex)
 {
-    int err;
-
-    err = pthread_mutex_lock(&mutex->lock);
-    if (err)
-        error_exit(err, __func__);
+    g_static_mutex_lock(&mutex->lock);
 }
 
 int qemu_mutex_trylock(QemuMutex *mutex)
 {
-    return pthread_mutex_trylock(&mutex->lock);
-}
-
-static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
-{
-    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
-    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
-    if (ts->tv_nsec >= 1000000000) {
-        ts->tv_nsec -= 1000000000;
-        ts->tv_sec++;
-    }
-}
-
-int qemu_mutex_timedlock(QemuMutex *mutex, uint64_t msecs)
-{
-    int err;
-    struct timespec ts;
-
-    clock_gettime(CLOCK_REALTIME, &ts);
-    timespec_add_ms(&ts, msecs);
-
-    err = pthread_mutex_timedlock(&mutex->lock, &ts);
-    if (err && err != ETIMEDOUT)
-        error_exit(err, __func__);
-    return err;
+    return g_static_mutex_trylock(&mutex->lock);
 }
 
 void qemu_mutex_unlock(QemuMutex *mutex)
 {
-    int err;
-
-    err = pthread_mutex_unlock(&mutex->lock);
-    if (err)
-        error_exit(err, __func__);
+    g_static_mutex_unlock(&mutex->lock);
 }
 
 void qemu_cond_init(QemuCond *cond)
 {
-    int err;
-
-    err = pthread_cond_init(&cond->cond, NULL);
-    if (err)
-        error_exit(err, __func__);
+    cond->cond = g_cond_new();
 }
 
 void qemu_cond_destroy(QemuCond *cond)
 {
-    int err;
-
-    err = pthread_cond_destroy(&cond->cond);
-    if (err)
-        error_exit(err, __func__);
+    g_cond_free(cond->cond);
 }
 
 void qemu_cond_signal(QemuCond *cond)
 {
-    int err;
-
-    err = pthread_cond_signal(&cond->cond);
-    if (err)
-        error_exit(err, __func__);
+    g_cond_signal(cond->cond);
 }
 
 void qemu_cond_broadcast(QemuCond *cond)
 {
-    int err;
-
-    err = pthread_cond_broadcast(&cond->cond);
-    if (err)
-        error_exit(err, __func__);
+    g_cond_broadcast(cond->cond);
 }
 
 void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 {
-    int err;
-
-    err = pthread_cond_wait(&cond->cond, &mutex->lock);
-    if (err)
-        error_exit(err, __func__);
+    g_cond_wait(cond->cond, g_static_mutex_get_mutex(&mutex->lock));
 }
 
 int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs)
 {
-    struct timespec ts;
-    int err;
+    GTimeVal abs_time;
+
+    assert(cond->cond != NULL);
 
-    clock_gettime(CLOCK_REALTIME, &ts);
-    timespec_add_ms(&ts, msecs);
+    g_get_current_time(&abs_time);
+    g_time_val_add(&abs_time, msecs * 1000); /* MSEC to USEC */
 
-    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
-    if (err && err != ETIMEDOUT)
-        error_exit(err, __func__);
-    return err;
+    return g_cond_timed_wait(cond->cond,
+                             g_static_mutex_get_mutex(&mutex->lock), &abs_time);
+}
+
+struct trampoline_data
+{
+    QemuThread *thread;
+    void *(*startfn)(void *);
+    void *opaque;
+    QemuMutex lock;
+};
+
+static gpointer thread_trampoline(gpointer data)
+{
+    struct trampoline_data *td = data;
+    gpointer retval;
+
+    td->thread->tid = pthread_self();
+    qemu_mutex_unlock(&td->lock);
+
+    retval = td->startfn(td->opaque);
+    qemu_free(td);
+
+    return retval;
 }
 
 void qemu_thread_create(QemuThread *thread,
-                       void *(*start_routine)(void*),
-                       void *arg)
+                        void *(*start_routine)(void*),
+                        void *arg)
 {
-    int err;
+    struct trampoline_data *td = qemu_malloc(sizeof(*td));
+    sigset_t set, old;
 
-    /* Leave signal handling to the iothread.  */
-    sigset_t set, oldset;
+    td->startfn = start_routine;
+    td->opaque = arg;
+    td->thread = thread;
+    qemu_mutex_init(&td->lock);
+
+    /* on behalf of the new thread */
+    qemu_mutex_lock(&td->lock);
 
     sigfillset(&set);
-    pthread_sigmask(SIG_SETMASK, &set, &oldset);
-    err = pthread_create(&thread->thread, NULL, start_routine, arg);
-    if (err)
-        error_exit(err, __func__);
+    pthread_sigmask(SIG_SETMASK, &set, &old);
+    thread->thread = g_thread_create(thread_trampoline, td, TRUE, NULL);
+    pthread_sigmask(SIG_SETMASK, &old, NULL);
+
+    /* we're transfering ownership of this lock to the thread so we no
+     * longer hold it here */
 
-    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
+    qemu_mutex_lock(&td->lock);
+    /* validate tid */
+    qemu_mutex_unlock(&td->lock);
+
+    qemu_mutex_destroy(&td->lock);
 }
 
 void qemu_thread_signal(QemuThread *thread, int sig)
 {
-    int err;
-
-    err = pthread_kill(thread->thread, sig);
-    if (err)
-        error_exit(err, __func__);
+    pthread_kill(thread->tid, sig);
 }
 
 void qemu_thread_self(QemuThread *thread)
 {
-    thread->thread = pthread_self();
+    thread->thread = g_thread_self();
+    thread->tid = pthread_self();
 }
 
 int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
 {
-   return pthread_equal(thread1->thread, thread2->thread);
+    return (thread1->thread == thread2->thread);
 }
 
 void qemu_thread_exit(void *retval)
 {
-    pthread_exit(retval);
+    g_thread_exit(retval);
 }
diff --git a/qemu-thread.h b/qemu-thread.h
index 19bb30c..dc22a60 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -1,18 +1,19 @@
 #ifndef __QEMU_THREAD_H
 #define __QEMU_THREAD_H 1
-#include "semaphore.h"
-#include "pthread.h"
+#include <glib.h>
+#include <pthread.h>
 
 struct QemuMutex {
-    pthread_mutex_t lock;
+    GStaticMutex lock;
 };
 
 struct QemuCond {
-    pthread_cond_t cond;
+    GCond *cond;
 };
 
 struct QemuThread {
-    pthread_t thread;
+    GThread *thread;
+    pthread_t tid;
 };
 
 typedef struct QemuMutex QemuMutex;
diff --git a/vl.c b/vl.c
index 0292184..bbe0931 100644
--- a/vl.c
+++ b/vl.c
@@ -165,6 +165,8 @@ int main(int argc, char **argv)
 
 #include "ui/qemu-spice.h"
 
+#include <glib.h>
+
 //#define DEBUG_NET
 //#define DEBUG_SLIRP
 
@@ -1918,6 +1920,8 @@ int main(int argc, char **argv, char **envp)
     atexit(qemu_run_exit_notifiers);
     error_set_progname(argv[0]);
 
+    g_thread_init(NULL);
+
     init_clocks();
 
     qemu_cache_utils_init(envp);
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it

[Aurelien Jarno]

On Mon, Jan 24, 2011 at 03:00:41PM -0600, Anthony Liguori wrote:
> GLib is an extremely common library that has a portable thread implementation
> along with tons of other goodies.
> 
> GLib and GObject have a fantastic amount of infrastructure we can leverage in
> QEMU including an object oriented programming infrastructure.
> 
> Short term, it has a very nice thread pool implementation that we could leverage
> in something like virtio-9p.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/Makefile b/Makefile
> index 6d601ee..bf24d1b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -104,6 +104,8 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
>  
>  QEMU_CFLAGS+=$(CURL_CFLAGS)
>  
> +QEMU_CFLAGS+=$(GLIB_CFLAGS)
> +
>  ui/cocoa.o: ui/cocoa.m
>  
>  ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
> diff --git a/Makefile.objs b/Makefile.objs
> index c3e52c5..283e62a 100644
> --- a/Makefile.objs
> +++ b/Makefile.objs
> @@ -316,3 +316,5 @@ vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>  
>  vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
>  
> +vl.o: QEMU_CFLAGS+=$(GLIB_CFLAGS)
> +
> diff --git a/Makefile.target b/Makefile.target
> index e15b1c4..2c1c90e 100644
> --- a/Makefile.target
> +++ b/Makefile.target
> @@ -204,6 +204,7 @@ QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
>  QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
>  QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
>  QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
> +QEMU_CFLAGS += $(GLIB_CFLAGS)
>  
>  # xen backend driver support
>  obj-$(CONFIG_XEN) += xen_machine_pv.o xen_domainbuild.o
> diff --git a/configure b/configure
> index d5ac074..820fde9 100755
> --- a/configure
> +++ b/configure
> @@ -1658,6 +1658,17 @@ EOF
>  fi
>  
>  ##########################################
> +# glib support probe
> +if $pkg_config --modversion gthread-2.0 > /dev/null 2>&1 ; then
> +    glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
> +    glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
> +    libs_softmmu="$glib_libs $libs_softmmu"
> +else
> +    echo "glib-2.0 required to compile QEMU"
> +    exit 1
> +fi
> +
> +##########################################
>  # kvm probe
>  if test "$kvm" != "no" ; then
>      cat > $TMPC <<EOF
> @@ -2677,6 +2688,7 @@ if test "$bluez" = "yes" ; then
>    echo "CONFIG_BLUEZ=y" >> $config_host_mak
>    echo "BLUEZ_CFLAGS=$bluez_cflags" >> $config_host_mak
>  fi
> +echo "GLIB_CFLAGS=$glib_cflags" >> $config_host_mak
>  if test "$xen" = "yes" ; then
>    echo "CONFIG_XEN=y" >> $config_host_mak
>  fi
> diff --git a/qemu-thread.c b/qemu-thread.c
> index fbc78fe..2c521ab 100644
> --- a/qemu-thread.c
> +++ b/qemu-thread.c
> @@ -10,183 +10,142 @@
>   * See the COPYING file in the top-level directory.
>   *
>   */
> -#include <stdlib.h>
> -#include <stdio.h>
> -#include <errno.h>
> -#include <time.h>
> -#include <signal.h>
> -#include <stdint.h>
> -#include <string.h>

Why removing <signal.h>?

> -#include "qemu-thread.h"
>  
> -static void error_exit(int err, const char *msg)
> -{
> -    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
> -    exit(1);
> -}
> +#include "qemu-common.h"
> +#include "qemu-thread.h"
>  
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
> -    int err;
> -
> -    err = pthread_mutex_init(&mutex->lock, NULL);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_static_mutex_init(&mutex->lock);
>  }
>  
>  void qemu_mutex_destroy(QemuMutex *mutex)
>  {
> -    int err;
> -
> -    err = pthread_mutex_destroy(&mutex->lock);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_static_mutex_free(&mutex->lock);
>  }
>  
>  void qemu_mutex_lock(QemuMutex *mutex)
>  {
> -    int err;
> -
> -    err = pthread_mutex_lock(&mutex->lock);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_static_mutex_lock(&mutex->lock);
>  }
>  
>  int qemu_mutex_trylock(QemuMutex *mutex)
>  {
> -    return pthread_mutex_trylock(&mutex->lock);
> -}
> -
> -static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
> -{
> -    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
> -    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
> -    if (ts->tv_nsec >= 1000000000) {
> -        ts->tv_nsec -= 1000000000;
> -        ts->tv_sec++;
> -    }
> -}
> -
> -int qemu_mutex_timedlock(QemuMutex *mutex, uint64_t msecs)
> -{
> -    int err;
> -    struct timespec ts;
> -
> -    clock_gettime(CLOCK_REALTIME, &ts);
> -    timespec_add_ms(&ts, msecs);
> -
> -    err = pthread_mutex_timedlock(&mutex->lock, &ts);
> -    if (err && err != ETIMEDOUT)
> -        error_exit(err, __func__);
> -    return err;
> +    return g_static_mutex_trylock(&mutex->lock);
>  }
>  
>  void qemu_mutex_unlock(QemuMutex *mutex)
>  {
> -    int err;
> -
> -    err = pthread_mutex_unlock(&mutex->lock);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_static_mutex_unlock(&mutex->lock);
>  }
>  
>  void qemu_cond_init(QemuCond *cond)
>  {
> -    int err;
> -
> -    err = pthread_cond_init(&cond->cond, NULL);
> -    if (err)
> -        error_exit(err, __func__);
> +    cond->cond = g_cond_new();
>  }
>  
>  void qemu_cond_destroy(QemuCond *cond)
>  {
> -    int err;
> -
> -    err = pthread_cond_destroy(&cond->cond);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_cond_free(cond->cond);
>  }
>  
>  void qemu_cond_signal(QemuCond *cond)
>  {
> -    int err;
> -
> -    err = pthread_cond_signal(&cond->cond);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_cond_signal(cond->cond);
>  }
>  
>  void qemu_cond_broadcast(QemuCond *cond)
>  {
> -    int err;
> -
> -    err = pthread_cond_broadcast(&cond->cond);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_cond_broadcast(cond->cond);
>  }
>  
>  void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>  {
> -    int err;
> -
> -    err = pthread_cond_wait(&cond->cond, &mutex->lock);
> -    if (err)
> -        error_exit(err, __func__);
> +    g_cond_wait(cond->cond, g_static_mutex_get_mutex(&mutex->lock));
>  }
>  
>  int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs)
>  {
> -    struct timespec ts;
> -    int err;
> +    GTimeVal abs_time;
> +
> +    assert(cond->cond != NULL);
>  
> -    clock_gettime(CLOCK_REALTIME, &ts);
> -    timespec_add_ms(&ts, msecs);
> +    g_get_current_time(&abs_time);
> +    g_time_val_add(&abs_time, msecs * 1000); /* MSEC to USEC */
>  
> -    err = pthread_cond_timedwait(&cond->cond, &mutex->lock, &ts);
> -    if (err && err != ETIMEDOUT)
> -        error_exit(err, __func__);
> -    return err;
> +    return g_cond_timed_wait(cond->cond,
> +                             g_static_mutex_get_mutex(&mutex->lock), &abs_time);
> +}
> +
> +struct trampoline_data
> +{
> +    QemuThread *thread;
> +    void *(*startfn)(void *);
> +    void *opaque;
> +    QemuMutex lock;
> +};
> +
> +static gpointer thread_trampoline(gpointer data)
> +{
> +    struct trampoline_data *td = data;
> +    gpointer retval;
> +
> +    td->thread->tid = pthread_self();
> +    qemu_mutex_unlock(&td->lock);
> +
> +    retval = td->startfn(td->opaque);
> +    qemu_free(td);
> +
> +    return retval;
>  }
>  
>  void qemu_thread_create(QemuThread *thread,
> -                       void *(*start_routine)(void*),
> -                       void *arg)
> +                        void *(*start_routine)(void*),
> +                        void *arg)
>  {
> -    int err;
> +    struct trampoline_data *td = qemu_malloc(sizeof(*td));
> +    sigset_t set, old;
>  
> -    /* Leave signal handling to the iothread.  */
> -    sigset_t set, oldset;
> +    td->startfn = start_routine;
> +    td->opaque = arg;
> +    td->thread = thread;
> +    qemu_mutex_init(&td->lock);
> +
> +    /* on behalf of the new thread */
> +    qemu_mutex_lock(&td->lock);
>  
>      sigfillset(&set);

If it is still in use here? This simply doesn't build:

qemu-thread.c: In function ‘q_thread_create_nosignal’:
qemu-thread.c:34: error: implicit declaration of function ‘sigfillset’
qemu-thread.c:34: error: nested extern declaration of ‘sigfillset’
qemu-thread.c:35: error: implicit declaration of function ‘pthread_sigmask’
qemu-thread.c:35: error: nested extern declaration of ‘pthread_sigmask’
qemu-thread.c:35: error: ‘SIG_SETMASK’ undeclared (first use in this function)
qemu-thread.c:35: error: (Each undeclared identifier is reported only once
qemu-thread.c:35: error: for each function it appears in.)
qemu-thread.c: In function ‘qemu_sthread_create’:
qemu-thread.c:80: error: ‘SIG_SETMASK’ undeclared (first use in this function)
qemu-thread.c: In function ‘qemu_sthread_signal’:
qemu-thread.c:96: error: implicit declaration of function ‘pthread_kill’
qemu-thread.c:96: error: nested extern declaration of ‘pthread_kill’


> -    pthread_sigmask(SIG_SETMASK, &set, &oldset);
> -    err = pthread_create(&thread->thread, NULL, start_routine, arg);
> -    if (err)
> -        error_exit(err, __func__);
> +    pthread_sigmask(SIG_SETMASK, &set, &old);
> +    thread->thread = g_thread_create(thread_trampoline, td, TRUE, NULL);
> +    pthread_sigmask(SIG_SETMASK, &old, NULL);
> +
> +    /* we're transfering ownership of this lock to the thread so we no
> +     * longer hold it here */
>  
> -    pthread_sigmask(SIG_SETMASK, &oldset, NULL);
> +    qemu_mutex_lock(&td->lock);
> +    /* validate tid */
> +    qemu_mutex_unlock(&td->lock);
> +
> +    qemu_mutex_destroy(&td->lock);
>  }
>  
>  void qemu_thread_signal(QemuThread *thread, int sig)
>  {
> -    int err;
> -
> -    err = pthread_kill(thread->thread, sig);
> -    if (err)
> -        error_exit(err, __func__);
> +    pthread_kill(thread->tid, sig);
>  }
>  
>  void qemu_thread_self(QemuThread *thread)
>  {
> -    thread->thread = pthread_self();
> +    thread->thread = g_thread_self();
> +    thread->tid = pthread_self();
>  }
>  
>  int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
>  {
> -   return pthread_equal(thread1->thread, thread2->thread);
> +    return (thread1->thread == thread2->thread);
>  }
>  
>  void qemu_thread_exit(void *retval)
>  {
> -    pthread_exit(retval);
> +    g_thread_exit(retval);
>  }
> diff --git a/qemu-thread.h b/qemu-thread.h
> index 19bb30c..dc22a60 100644
> --- a/qemu-thread.h
> +++ b/qemu-thread.h
> @@ -1,18 +1,19 @@
>  #ifndef __QEMU_THREAD_H
>  #define __QEMU_THREAD_H 1
> -#include "semaphore.h"
> -#include "pthread.h"
> +#include <glib.h>
> +#include <pthread.h>
>  
>  struct QemuMutex {
> -    pthread_mutex_t lock;
> +    GStaticMutex lock;
>  };
>  
>  struct QemuCond {
> -    pthread_cond_t cond;
> +    GCond *cond;
>  };
>  
>  struct QemuThread {
> -    pthread_t thread;
> +    GThread *thread;
> +    pthread_t tid;
>  };
>  
>  typedef struct QemuMutex QemuMutex;
> diff --git a/vl.c b/vl.c
> index 0292184..bbe0931 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -165,6 +165,8 @@ int main(int argc, char **argv)
>  
>  #include "ui/qemu-spice.h"
>  
> +#include <glib.h>
> +
>  //#define DEBUG_NET
>  //#define DEBUG_SLIRP
>  
> @@ -1918,6 +1920,8 @@ int main(int argc, char **argv, char **envp)
>      atexit(qemu_run_exit_notifiers);
>      error_set_progname(argv[0]);
>  
> +    g_thread_init(NULL);
> +
>      init_clocks();
>  
>      qemu_cache_utils_init(envp);
> -- 
> 1.7.0.4
> 
> 
> 

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it

[Anthony Liguori]

On 01/25/2011 08:24 AM, Aurelien Jarno wrote:
> On Mon, Jan 24, 2011 at 03:00:41PM -0600, Anthony Liguori wrote:
>    
>> GLib is an extremely common library that has a portable thread implementation
>> along with tons of other goodies.
>>
>> GLib and GObject have a fantastic amount of infrastructure we can leverage in
>> QEMU including an object oriented programming infrastructure.
>>
>> Short term, it has a very nice thread pool implementation that we could leverage
>> in something like virtio-9p.
>>
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/Makefile b/Makefile
>> index 6d601ee..bf24d1b 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -104,6 +104,8 @@ audio/audio.o audio/fmodaudio.o: QEMU_CFLAGS += $(FMOD_CFLAGS)
>>
>>   QEMU_CFLAGS+=$(CURL_CFLAGS)
>>
>> +QEMU_CFLAGS+=$(GLIB_CFLAGS)
>> +
>>   ui/cocoa.o: ui/cocoa.m
>>
>>   ui/sdl.o audio/sdlaudio.o ui/sdl_zoom.o baum.o: QEMU_CFLAGS += $(SDL_CFLAGS)
>> diff --git a/Makefile.objs b/Makefile.objs
>> index c3e52c5..283e62a 100644
>> --- a/Makefile.objs
>> +++ b/Makefile.objs
>> @@ -316,3 +316,5 @@ vl.o: QEMU_CFLAGS+=$(GPROF_CFLAGS)
>>
>>   vl.o: QEMU_CFLAGS+=$(SDL_CFLAGS)
>>
>> +vl.o: QEMU_CFLAGS+=$(GLIB_CFLAGS)
>> +
>> diff --git a/Makefile.target b/Makefile.target
>> index e15b1c4..2c1c90e 100644
>> --- a/Makefile.target
>> +++ b/Makefile.target
>> @@ -204,6 +204,7 @@ QEMU_CFLAGS += $(VNC_TLS_CFLAGS)
>>   QEMU_CFLAGS += $(VNC_SASL_CFLAGS)
>>   QEMU_CFLAGS += $(VNC_JPEG_CFLAGS)
>>   QEMU_CFLAGS += $(VNC_PNG_CFLAGS)
>> +QEMU_CFLAGS += $(GLIB_CFLAGS)
>>
>>   # xen backend driver support
>>   obj-$(CONFIG_XEN) += xen_machine_pv.o xen_domainbuild.o
>> diff --git a/configure b/configure
>> index d5ac074..820fde9 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1658,6 +1658,17 @@ EOF
>>   fi
>>
>>   ##########################################
>> +# glib support probe
>> +if $pkg_config --modversion gthread-2.0>  /dev/null 2>&1 ; then
>> +    glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
>> +    glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
>> +    libs_softmmu="$glib_libs $libs_softmmu"
>> +else
>> +    echo "glib-2.0 required to compile QEMU"
>> +    exit 1
>> +fi
>> +
>> +##########################################
>>   # kvm probe
>>   if test "$kvm" != "no" ; then
>>       cat>  $TMPC<<EOF
>> @@ -2677,6 +2688,7 @@ if test "$bluez" = "yes" ; then
>>     echo "CONFIG_BLUEZ=y">>  $config_host_mak
>>     echo "BLUEZ_CFLAGS=$bluez_cflags">>  $config_host_mak
>>   fi
>> +echo "GLIB_CFLAGS=$glib_cflags">>  $config_host_mak
>>   if test "$xen" = "yes" ; then
>>     echo "CONFIG_XEN=y">>  $config_host_mak
>>   fi
>> diff --git a/qemu-thread.c b/qemu-thread.c
>> index fbc78fe..2c521ab 100644
>> --- a/qemu-thread.c
>> +++ b/qemu-thread.c
>> @@ -10,183 +10,142 @@
>>    * See the COPYING file in the top-level directory.
>>    *
>>    */
>> -#include<stdlib.h>
>> -#include<stdio.h>
>> -#include<errno.h>
>> -#include<time.h>
>> -#include<signal.h>
>> -#include<stdint.h>
>> -#include<string.h>
>>      
> Why removing<signal.h>?
>    

It looked like unnecessary includes crept in so I remove them all.  It 
still built for me but apparently not for you.  I'll update accordingly.

Maybe we should put signal.h in qemu-common.h?

Regards,

Anthony Liguori

>> -#include "qemu-thread.h"
>>
>> -static void error_exit(int err, const char *msg)
>> -{
>> -    fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
>> -    exit(1);
>> -}
>> +#include "qemu-common.h"
>> +#include "qemu-thread.h"
>>
>>   void qemu_mutex_init(QemuMutex *mutex)
>>   {
>> -    int err;
>> -
>> -    err = pthread_mutex_init(&mutex->lock, NULL);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_static_mutex_init(&mutex->lock);
>>   }
>>
>>   void qemu_mutex_destroy(QemuMutex *mutex)
>>   {
>> -    int err;
>> -
>> -    err = pthread_mutex_destroy(&mutex->lock);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_static_mutex_free(&mutex->lock);
>>   }
>>
>>   void qemu_mutex_lock(QemuMutex *mutex)
>>   {
>> -    int err;
>> -
>> -    err = pthread_mutex_lock(&mutex->lock);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_static_mutex_lock(&mutex->lock);
>>   }
>>
>>   int qemu_mutex_trylock(QemuMutex *mutex)
>>   {
>> -    return pthread_mutex_trylock(&mutex->lock);
>> -}
>> -
>> -static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
>> -{
>> -    ts->tv_sec = ts->tv_sec + (long)(msecs / 1000);
>> -    ts->tv_nsec = (ts->tv_nsec + ((long)msecs % 1000) * 1000000);
>> -    if (ts->tv_nsec>= 1000000000) {
>> -        ts->tv_nsec -= 1000000000;
>> -        ts->tv_sec++;
>> -    }
>> -}
>> -
>> -int qemu_mutex_timedlock(QemuMutex *mutex, uint64_t msecs)
>> -{
>> -    int err;
>> -    struct timespec ts;
>> -
>> -    clock_gettime(CLOCK_REALTIME,&ts);
>> -    timespec_add_ms(&ts, msecs);
>> -
>> -    err = pthread_mutex_timedlock(&mutex->lock,&ts);
>> -    if (err&&  err != ETIMEDOUT)
>> -        error_exit(err, __func__);
>> -    return err;
>> +    return g_static_mutex_trylock(&mutex->lock);
>>   }
>>
>>   void qemu_mutex_unlock(QemuMutex *mutex)
>>   {
>> -    int err;
>> -
>> -    err = pthread_mutex_unlock(&mutex->lock);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_static_mutex_unlock(&mutex->lock);
>>   }
>>
>>   void qemu_cond_init(QemuCond *cond)
>>   {
>> -    int err;
>> -
>> -    err = pthread_cond_init(&cond->cond, NULL);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    cond->cond = g_cond_new();
>>   }
>>
>>   void qemu_cond_destroy(QemuCond *cond)
>>   {
>> -    int err;
>> -
>> -    err = pthread_cond_destroy(&cond->cond);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_cond_free(cond->cond);
>>   }
>>
>>   void qemu_cond_signal(QemuCond *cond)
>>   {
>> -    int err;
>> -
>> -    err = pthread_cond_signal(&cond->cond);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_cond_signal(cond->cond);
>>   }
>>
>>   void qemu_cond_broadcast(QemuCond *cond)
>>   {
>> -    int err;
>> -
>> -    err = pthread_cond_broadcast(&cond->cond);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_cond_broadcast(cond->cond);
>>   }
>>
>>   void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>>   {
>> -    int err;
>> -
>> -    err = pthread_cond_wait(&cond->cond,&mutex->lock);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    g_cond_wait(cond->cond, g_static_mutex_get_mutex(&mutex->lock));
>>   }
>>
>>   int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs)
>>   {
>> -    struct timespec ts;
>> -    int err;
>> +    GTimeVal abs_time;
>> +
>> +    assert(cond->cond != NULL);
>>
>> -    clock_gettime(CLOCK_REALTIME,&ts);
>> -    timespec_add_ms(&ts, msecs);
>> +    g_get_current_time(&abs_time);
>> +    g_time_val_add(&abs_time, msecs * 1000); /* MSEC to USEC */
>>
>> -    err = pthread_cond_timedwait(&cond->cond,&mutex->lock,&ts);
>> -    if (err&&  err != ETIMEDOUT)
>> -        error_exit(err, __func__);
>> -    return err;
>> +    return g_cond_timed_wait(cond->cond,
>> +                             g_static_mutex_get_mutex(&mutex->lock),&abs_time);
>> +}
>> +
>> +struct trampoline_data
>> +{
>> +    QemuThread *thread;
>> +    void *(*startfn)(void *);
>> +    void *opaque;
>> +    QemuMutex lock;
>> +};
>> +
>> +static gpointer thread_trampoline(gpointer data)
>> +{
>> +    struct trampoline_data *td = data;
>> +    gpointer retval;
>> +
>> +    td->thread->tid = pthread_self();
>> +    qemu_mutex_unlock(&td->lock);
>> +
>> +    retval = td->startfn(td->opaque);
>> +    qemu_free(td);
>> +
>> +    return retval;
>>   }
>>
>>   void qemu_thread_create(QemuThread *thread,
>> -                       void *(*start_routine)(void*),
>> -                       void *arg)
>> +                        void *(*start_routine)(void*),
>> +                        void *arg)
>>   {
>> -    int err;
>> +    struct trampoline_data *td = qemu_malloc(sizeof(*td));
>> +    sigset_t set, old;
>>
>> -    /* Leave signal handling to the iothread.  */
>> -    sigset_t set, oldset;
>> +    td->startfn = start_routine;
>> +    td->opaque = arg;
>> +    td->thread = thread;
>> +    qemu_mutex_init(&td->lock);
>> +
>> +    /* on behalf of the new thread */
>> +    qemu_mutex_lock(&td->lock);
>>
>>       sigfillset(&set);
>>      
> If it is still in use here? This simply doesn't build:
>
> qemu-thread.c: In function ‘q_thread_create_nosignal’:
> qemu-thread.c:34: error: implicit declaration of function ‘sigfillset’
> qemu-thread.c:34: error: nested extern declaration of ‘sigfillset’
> qemu-thread.c:35: error: implicit declaration of function ‘pthread_sigmask’
> qemu-thread.c:35: error: nested extern declaration of ‘pthread_sigmask’
> qemu-thread.c:35: error: ‘SIG_SETMASK’ undeclared (first use in this function)
> qemu-thread.c:35: error: (Each undeclared identifier is reported only once
> qemu-thread.c:35: error: for each function it appears in.)
> qemu-thread.c: In function ‘qemu_sthread_create’:
> qemu-thread.c:80: error: ‘SIG_SETMASK’ undeclared (first use in this function)
> qemu-thread.c: In function ‘qemu_sthread_signal’:
> qemu-thread.c:96: error: implicit declaration of function ‘pthread_kill’
> qemu-thread.c:96: error: nested extern declaration of ‘pthread_kill’
>
>
>    
>> -    pthread_sigmask(SIG_SETMASK,&set,&oldset);
>> -    err = pthread_create(&thread->thread, NULL, start_routine, arg);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    pthread_sigmask(SIG_SETMASK,&set,&old);
>> +    thread->thread = g_thread_create(thread_trampoline, td, TRUE, NULL);
>> +    pthread_sigmask(SIG_SETMASK,&old, NULL);
>> +
>> +    /* we're transfering ownership of this lock to the thread so we no
>> +     * longer hold it here */
>>
>> -    pthread_sigmask(SIG_SETMASK,&oldset, NULL);
>> +    qemu_mutex_lock(&td->lock);
>> +    /* validate tid */
>> +    qemu_mutex_unlock(&td->lock);
>> +
>> +    qemu_mutex_destroy(&td->lock);
>>   }
>>
>>   void qemu_thread_signal(QemuThread *thread, int sig)
>>   {
>> -    int err;
>> -
>> -    err = pthread_kill(thread->thread, sig);
>> -    if (err)
>> -        error_exit(err, __func__);
>> +    pthread_kill(thread->tid, sig);
>>   }
>>
>>   void qemu_thread_self(QemuThread *thread)
>>   {
>> -    thread->thread = pthread_self();
>> +    thread->thread = g_thread_self();
>> +    thread->tid = pthread_self();
>>   }
>>
>>   int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
>>   {
>> -   return pthread_equal(thread1->thread, thread2->thread);
>> +    return (thread1->thread == thread2->thread);
>>   }
>>
>>   void qemu_thread_exit(void *retval)
>>   {
>> -    pthread_exit(retval);
>> +    g_thread_exit(retval);
>>   }
>> diff --git a/qemu-thread.h b/qemu-thread.h
>> index 19bb30c..dc22a60 100644
>> --- a/qemu-thread.h
>> +++ b/qemu-thread.h
>> @@ -1,18 +1,19 @@
>>   #ifndef __QEMU_THREAD_H
>>   #define __QEMU_THREAD_H 1
>> -#include "semaphore.h"
>> -#include "pthread.h"
>> +#include<glib.h>
>> +#include<pthread.h>
>>
>>   struct QemuMutex {
>> -    pthread_mutex_t lock;
>> +    GStaticMutex lock;
>>   };
>>
>>   struct QemuCond {
>> -    pthread_cond_t cond;
>> +    GCond *cond;
>>   };
>>
>>   struct QemuThread {
>> -    pthread_t thread;
>> +    GThread *thread;
>> +    pthread_t tid;
>>   };
>>
>>   typedef struct QemuMutex QemuMutex;
>> diff --git a/vl.c b/vl.c
>> index 0292184..bbe0931 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -165,6 +165,8 @@ int main(int argc, char **argv)
>>
>>   #include "ui/qemu-spice.h"
>>
>> +#include<glib.h>
>> +
>>   //#define DEBUG_NET
>>   //#define DEBUG_SLIRP
>>
>> @@ -1918,6 +1920,8 @@ int main(int argc, char **argv, char **envp)
>>       atexit(qemu_run_exit_notifiers);
>>       error_set_progname(argv[0]);
>>
>> +    g_thread_init(NULL);
>> +
>>       init_clocks();
>>
>>       qemu_cache_utils_init(envp);
>> -- 
>> 1.7.0.4
>>
>>
>>
>>      
>    

[Qemu-devel] Re: [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it

[Paolo Bonzini]

On 01/24/2011 10:00 PM, Anthony Liguori wrote:
>   int qemu_mutex_trylock(QemuMutex *mutex)
>   {
> -    return pthread_mutex_trylock(&mutex->lock);
> +    return g_static_mutex_trylock(&mutex->lock);

This is missing a !

Paolo

[Qemu-devel] Re: [PATCH 3/7] Add support for glib based threading and convert qemu thread to use it

[Anthony Liguori]

On 02/02/2011 11:32 AM, Paolo Bonzini wrote:
> On 01/24/2011 10:00 PM, Anthony Liguori wrote:
>>   int qemu_mutex_trylock(QemuMutex *mutex)
>>   {
>> -    return pthread_mutex_trylock(&mutex->lock);
>> +    return g_static_mutex_trylock(&mutex->lock);
>
> This is missing a !

Good catch.

Regards,

Anthony Liguori

>
> Paolo

[Qemu-devel] [PATCH 4/7] Get rid of QemuMutex and teach its callers about GStaticMutex

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/cpus.c b/cpus.c
index 9cf7e6e..0f8e33b 100644
--- a/cpus.c
+++ b/cpus.c
@@ -321,8 +321,8 @@ void vm_stop(int reason)
 
 #include "qemu-thread.h"
 
-QemuMutex qemu_global_mutex;
-static QemuMutex qemu_fair_mutex;
+GStaticMutex qemu_global_mutex;
+static GStaticMutex qemu_fair_mutex;
 
 static QemuThread io_thread;
 
@@ -416,9 +416,9 @@ int qemu_init_main_loop(void)
     qemu_cond_init(&qemu_system_cond);
     qemu_cond_init(&qemu_pause_cond);
     qemu_cond_init(&qemu_work_cond);
-    qemu_mutex_init(&qemu_fair_mutex);
-    qemu_mutex_init(&qemu_global_mutex);
-    qemu_mutex_lock(&qemu_global_mutex);
+    g_static_mutex_init(&qemu_fair_mutex);
+    g_static_mutex_init(&qemu_global_mutex);
+    g_static_mutex_lock(&qemu_global_mutex);
 
     qemu_thread_self(&io_thread);
 
@@ -454,7 +454,8 @@ void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
     while (!wi.done) {
         CPUState *self_env = cpu_single_env;
 
-        qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex);
+        qemu_cond_wait(&qemu_work_cond,
+                       g_static_mutex_get_mutex(&qemu_global_mutex));
         cpu_single_env = self_env;
     }
 }
@@ -490,19 +491,20 @@ static void qemu_tcg_wait_io_event(void)
     CPUState *env;
 
     while (!any_cpu_has_work())
-        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(tcg_halt_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 1000);
 
-    qemu_mutex_unlock(&qemu_global_mutex);
+    g_static_mutex_unlock(&qemu_global_mutex);
 
     /*
      * Users of qemu_global_mutex can be starved, having no chance
      * to acquire it since this path will get to it first.
      * So use another lock to provide fairness.
      */
-    qemu_mutex_lock(&qemu_fair_mutex);
-    qemu_mutex_unlock(&qemu_fair_mutex);
+    g_static_mutex_lock(&qemu_fair_mutex);
+    g_static_mutex_unlock(&qemu_fair_mutex);
 
-    qemu_mutex_lock(&qemu_global_mutex);
+    g_static_mutex_lock(&qemu_global_mutex);
 
     for (env = first_cpu; env != NULL; env = env->next_cpu) {
         qemu_wait_io_event_common(env);
@@ -551,12 +553,12 @@ static void qemu_kvm_eat_signal(CPUState *env, int timeout)
     sigaddset(&waitset, SIGBUS);
 
     do {
-        qemu_mutex_unlock(&qemu_global_mutex);
+        g_static_mutex_unlock(&qemu_global_mutex);
 
         r = sigtimedwait(&waitset, &siginfo, &ts);
         e = errno;
 
-        qemu_mutex_lock(&qemu_global_mutex);
+        g_static_mutex_lock(&qemu_global_mutex);
 
         if (r == -1 && !(e == EAGAIN || e == EINTR)) {
             fprintf(stderr, "sigtimedwait: %s\n", strerror(e));
@@ -585,7 +587,8 @@ static void qemu_kvm_eat_signal(CPUState *env, int timeout)
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
     while (!cpu_has_work(env))
-        qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(env->halt_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 1000);
 
     qemu_kvm_eat_signal(env, 0);
     qemu_wait_io_event_common(env);
@@ -597,7 +600,7 @@ static void *kvm_cpu_thread_fn(void *arg)
 {
     CPUState *env = arg;
 
-    qemu_mutex_lock(&qemu_global_mutex);
+    g_static_mutex_lock(&qemu_global_mutex);
     qemu_thread_self(env->thread);
     if (kvm_enabled())
         kvm_init_vcpu(env);
@@ -610,7 +613,8 @@ static void *kvm_cpu_thread_fn(void *arg)
 
     /* and wait for machine initialization */
     while (!qemu_system_ready)
-        qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+        qemu_cond_timedwait(&qemu_system_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
 
     while (1) {
         if (cpu_can_run(env))
@@ -629,14 +633,15 @@ static void *tcg_cpu_thread_fn(void *arg)
     qemu_thread_self(env->thread);
 
     /* signal CPU creation */
-    qemu_mutex_lock(&qemu_global_mutex);
+    g_static_mutex_lock(&qemu_global_mutex);
     for (env = first_cpu; env != NULL; env = env->next_cpu)
         env->created = 1;
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* and wait for machine initialization */
     while (!qemu_system_ready)
-        qemu_cond_timedwait(&qemu_system_cond, &qemu_global_mutex, 100);
+        qemu_cond_timedwait(&qemu_system_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
 
     while (1) {
         cpu_exec_all();
@@ -737,22 +742,22 @@ static sigset_t block_io_signals(void)
 void qemu_mutex_lock_iothread(void)
 {
     if (kvm_enabled()) {
-        qemu_mutex_lock(&qemu_fair_mutex);
-        qemu_mutex_lock(&qemu_global_mutex);
-        qemu_mutex_unlock(&qemu_fair_mutex);
+        g_static_mutex_lock(&qemu_fair_mutex);
+        g_static_mutex_lock(&qemu_global_mutex);
+        g_static_mutex_unlock(&qemu_fair_mutex);
     } else {
-        qemu_mutex_lock(&qemu_fair_mutex);
-        if (qemu_mutex_trylock(&qemu_global_mutex)) {
+        g_static_mutex_lock(&qemu_fair_mutex);
+        if (g_static_mutex_trylock(&qemu_global_mutex)) {
             qemu_thread_signal(tcg_cpu_thread, SIG_IPI);
-            qemu_mutex_lock(&qemu_global_mutex);
+            g_static_mutex_lock(&qemu_global_mutex);
         }
-        qemu_mutex_unlock(&qemu_fair_mutex);
+        g_static_mutex_unlock(&qemu_fair_mutex);
     }
 }
 
 void qemu_mutex_unlock_iothread(void)
 {
-    qemu_mutex_unlock(&qemu_global_mutex);
+    g_static_mutex_unlock(&qemu_global_mutex);
 }
 
 static int all_vcpus_paused(void)
@@ -779,7 +784,8 @@ void pause_all_vcpus(void)
     }
 
     while (!all_vcpus_paused()) {
-        qemu_cond_timedwait(&qemu_pause_cond, &qemu_global_mutex, 100);
+        qemu_cond_timedwait(&qemu_pause_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
         penv = first_cpu;
         while (penv) {
             qemu_cpu_kick(penv);
@@ -810,7 +816,9 @@ static void tcg_init_vcpu(void *_env)
         qemu_cond_init(env->halt_cond);
         qemu_thread_create(env->thread, tcg_cpu_thread_fn, env);
         while (env->created == 0)
-            qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+            qemu_cond_timedwait(&qemu_cpu_cond,
+                                g_static_mutex_get_mutex(&qemu_global_mutex),
+                                100);
         tcg_cpu_thread = env->thread;
         tcg_halt_cond = env->halt_cond;
     } else {
@@ -826,7 +834,8 @@ static void kvm_start_vcpu(CPUState *env)
     qemu_cond_init(env->halt_cond);
     qemu_thread_create(env->thread, kvm_cpu_thread_fn, env);
     while (env->created == 0)
-        qemu_cond_timedwait(&qemu_cpu_cond, &qemu_global_mutex, 100);
+        qemu_cond_timedwait(&qemu_cpu_cond,
+                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
 }
 
 void qemu_init_vcpu(void *_env)
diff --git a/qemu-thread.c b/qemu-thread.c
index 2c521ab..df17eb4 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -14,31 +14,6 @@
 #include "qemu-common.h"
 #include "qemu-thread.h"
 
-void qemu_mutex_init(QemuMutex *mutex)
-{
-    g_static_mutex_init(&mutex->lock);
-}
-
-void qemu_mutex_destroy(QemuMutex *mutex)
-{
-    g_static_mutex_free(&mutex->lock);
-}
-
-void qemu_mutex_lock(QemuMutex *mutex)
-{
-    g_static_mutex_lock(&mutex->lock);
-}
-
-int qemu_mutex_trylock(QemuMutex *mutex)
-{
-    return g_static_mutex_trylock(&mutex->lock);
-}
-
-void qemu_mutex_unlock(QemuMutex *mutex)
-{
-    g_static_mutex_unlock(&mutex->lock);
-}
-
 void qemu_cond_init(QemuCond *cond)
 {
     cond->cond = g_cond_new();
@@ -59,12 +34,12 @@ void qemu_cond_broadcast(QemuCond *cond)
     g_cond_broadcast(cond->cond);
 }
 
-void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
+void qemu_cond_wait(QemuCond *cond, GMutex *mutex)
 {
-    g_cond_wait(cond->cond, g_static_mutex_get_mutex(&mutex->lock));
+    g_cond_wait(cond->cond, mutex);
 }
 
-int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs)
+int qemu_cond_timedwait(QemuCond *cond, GMutex *mutex, uint64_t msecs)
 {
     GTimeVal abs_time;
 
@@ -73,8 +48,7 @@ int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs)
     g_get_current_time(&abs_time);
     g_time_val_add(&abs_time, msecs * 1000); /* MSEC to USEC */
 
-    return g_cond_timed_wait(cond->cond,
-                             g_static_mutex_get_mutex(&mutex->lock), &abs_time);
+    return g_cond_timed_wait(cond->cond, mutex, &abs_time);
 }
 
 struct trampoline_data
@@ -82,7 +56,7 @@ struct trampoline_data
     QemuThread *thread;
     void *(*startfn)(void *);
     void *opaque;
-    QemuMutex lock;
+    GStaticMutex lock;
 };
 
 static gpointer thread_trampoline(gpointer data)
@@ -91,7 +65,7 @@ static gpointer thread_trampoline(gpointer data)
     gpointer retval;
 
     td->thread->tid = pthread_self();
-    qemu_mutex_unlock(&td->lock);
+    g_static_mutex_unlock(&td->lock);
 
     retval = td->startfn(td->opaque);
     qemu_free(td);
@@ -109,10 +83,10 @@ void qemu_thread_create(QemuThread *thread,
     td->startfn = start_routine;
     td->opaque = arg;
     td->thread = thread;
-    qemu_mutex_init(&td->lock);
+    g_static_mutex_init(&td->lock);
 
     /* on behalf of the new thread */
-    qemu_mutex_lock(&td->lock);
+    g_static_mutex_lock(&td->lock);
 
     sigfillset(&set);
     pthread_sigmask(SIG_SETMASK, &set, &old);
@@ -122,11 +96,11 @@ void qemu_thread_create(QemuThread *thread,
     /* we're transfering ownership of this lock to the thread so we no
      * longer hold it here */
 
-    qemu_mutex_lock(&td->lock);
+    g_static_mutex_lock(&td->lock);
     /* validate tid */
-    qemu_mutex_unlock(&td->lock);
+    g_static_mutex_unlock(&td->lock);
 
-    qemu_mutex_destroy(&td->lock);
+    g_static_mutex_free(&td->lock);
 }
 
 void qemu_thread_signal(QemuThread *thread, int sig)
diff --git a/qemu-thread.h b/qemu-thread.h
index dc22a60..dec6848 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -3,10 +3,6 @@
 #include <glib.h>
 #include <pthread.h>
 
-struct QemuMutex {
-    GStaticMutex lock;
-};
-
 struct QemuCond {
     GCond *cond;
 };
@@ -16,23 +12,15 @@ struct QemuThread {
     pthread_t tid;
 };
 
-typedef struct QemuMutex QemuMutex;
 typedef struct QemuCond QemuCond;
 typedef struct QemuThread QemuThread;
 
-void qemu_mutex_init(QemuMutex *mutex);
-void qemu_mutex_destroy(QemuMutex *mutex);
-void qemu_mutex_lock(QemuMutex *mutex);
-int qemu_mutex_trylock(QemuMutex *mutex);
-int qemu_mutex_timedlock(QemuMutex *mutex, uint64_t msecs);
-void qemu_mutex_unlock(QemuMutex *mutex);
-
 void qemu_cond_init(QemuCond *cond);
 void qemu_cond_destroy(QemuCond *cond);
 void qemu_cond_signal(QemuCond *cond);
 void qemu_cond_broadcast(QemuCond *cond);
-void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex);
-int qemu_cond_timedwait(QemuCond *cond, QemuMutex *mutex, uint64_t msecs);
+void qemu_cond_wait(QemuCond *cond, GMutex *mutex);
+int qemu_cond_timedwait(QemuCond *cond, GMutex *mutex, uint64_t msecs);
 
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 6e9cf08..48f567e 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -50,7 +50,7 @@
 
 struct VncJobQueue {
     QemuCond cond;
-    QemuMutex mutex;
+    GStaticMutex mutex;
     QemuThread thread;
     Buffer buffer;
     bool exit;
@@ -67,12 +67,12 @@ static VncJobQueue *queue;
 
 static void vnc_lock_queue(VncJobQueue *queue)
 {
-    qemu_mutex_lock(&queue->mutex);
+    g_static_mutex_lock(&queue->mutex);
 }
 
 static void vnc_unlock_queue(VncJobQueue *queue)
 {
-    qemu_mutex_unlock(&queue->mutex);
+    g_static_mutex_unlock(&queue->mutex);
 }
 
 VncJob *vnc_job_new(VncState *vs)
@@ -152,7 +152,7 @@ void vnc_jobs_join(VncState *vs)
 {
     vnc_lock_queue(queue);
     while (vnc_has_job_locked(vs)) {
-        qemu_cond_wait(&queue->cond, &queue->mutex);
+        qemu_cond_wait(&queue->cond, g_static_mutex_get_mutex(&queue->mutex));
     }
     vnc_unlock_queue(queue);
 }
@@ -195,7 +195,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
     vnc_lock_queue(queue);
     while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
-        qemu_cond_wait(&queue->cond, &queue->mutex);
+        qemu_cond_wait(&queue->cond, g_static_mutex_get_mutex(&queue->mutex));
     }
     /* Here job can only be NULL if queue->exit is true */
     job = QTAILQ_FIRST(&queue->jobs);
@@ -275,7 +275,7 @@ static VncJobQueue *vnc_queue_init(void)
     VncJobQueue *queue = qemu_mallocz(sizeof(VncJobQueue));
 
     qemu_cond_init(&queue->cond);
-    qemu_mutex_init(&queue->mutex);
+    g_static_mutex_init(&queue->mutex);
     QTAILQ_INIT(&queue->jobs);
     return queue;
 }
@@ -283,7 +283,7 @@ static VncJobQueue *vnc_queue_init(void)
 static void vnc_queue_clear(VncJobQueue *q)
 {
     qemu_cond_destroy(&queue->cond);
-    qemu_mutex_destroy(&queue->mutex);
+    g_static_mutex_free(&queue->mutex);
     buffer_free(&queue->buffer);
     qemu_free(q);
     queue = NULL; /* Unset global queue */
diff --git a/ui/vnc-jobs.h b/ui/vnc-jobs.h
index b8dab81..f4cc262 100644
--- a/ui/vnc-jobs.h
+++ b/ui/vnc-jobs.h
@@ -50,7 +50,7 @@ void vnc_stop_worker_thread(void);
 static inline int vnc_trylock_display(VncDisplay *vd)
 {
 #ifdef CONFIG_VNC_THREAD
-    return qemu_mutex_trylock(&vd->mutex);
+    return g_static_mutex_trylock(&vd->mutex);
 #else
     return 0;
 #endif
@@ -59,28 +59,28 @@ static inline int vnc_trylock_display(VncDisplay *vd)
 static inline void vnc_lock_display(VncDisplay *vd)
 {
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_lock(&vd->mutex);
+    g_static_mutex_lock(&vd->mutex);
 #endif
 }
 
 static inline void vnc_unlock_display(VncDisplay *vd)
 {
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_unlock(&vd->mutex);
+    g_static_mutex_unlock(&vd->mutex);
 #endif
 }
 
 static inline void vnc_lock_output(VncState *vs)
 {
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_lock(&vs->output_mutex);
+    g_static_mutex_lock(&vs->output_mutex);
 #endif
 }
 
 static inline void vnc_unlock_output(VncState *vs)
 {
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_unlock(&vs->output_mutex);
+    g_static_mutex_unlock(&vs->output_mutex);
 #endif
 }
 
diff --git a/ui/vnc.c b/ui/vnc.c
index 495d6d6..4efd684 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1046,7 +1046,7 @@ static void vnc_disconnect_finish(VncState *vs)
     vnc_unlock_output(vs);
 
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_destroy(&vs->output_mutex);
+    g_static_mutex_free(&vs->output_mutex);
 #endif
     qemu_free(vs);
 }
@@ -2386,7 +2386,7 @@ static void vnc_connect(VncDisplay *vd, int csock)
     vs->as.endianness = 0;
 
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_init(&vs->output_mutex);
+    g_static_mutex_init(&vs->output_mutex);
 #endif
 
     QTAILQ_INSERT_HEAD(&vd->clients, vs, next);
@@ -2448,7 +2448,7 @@ void vnc_display_init(DisplayState *ds)
         exit(1);
 
 #ifdef CONFIG_VNC_THREAD
-    qemu_mutex_init(&vs->mutex);
+    g_static_mutex_init(&vs->mutex);
     vnc_start_worker_thread();
 #endif
 
diff --git a/ui/vnc.h b/ui/vnc.h
index 4f895be..5c6a676 100644
--- a/ui/vnc.h
+++ b/ui/vnc.h
@@ -108,7 +108,7 @@ struct VncDisplay
     kbd_layout_t *kbd_layout;
     int lock_key_sync;
 #ifdef CONFIG_VNC_THREAD
-    QemuMutex mutex;
+    GStaticMutex mutex;
 #endif
 
     QEMUCursor *cursor;
@@ -244,7 +244,7 @@ struct VncState
 #ifndef CONFIG_VNC_THREAD
     VncJob job;
 #else
-    QemuMutex output_mutex;
+    GStaticMutex output_mutex;
 #endif
 
     /* Encoding specific, if you add something here, don't forget to
-- 
1.7.0.4

[Qemu-devel] Re: [PATCH 4/7] Get rid of QemuMutex and teach its callers about GStaticMutex

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2008 bytes --]

On 2011-01-24 22:00, Anthony Liguori wrote:
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/cpus.c b/cpus.c
> index 9cf7e6e..0f8e33b 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -321,8 +321,8 @@ void vm_stop(int reason)
>  
>  #include "qemu-thread.h"
>  
> -QemuMutex qemu_global_mutex;
> -static QemuMutex qemu_fair_mutex;
> +GStaticMutex qemu_global_mutex;
> +static GStaticMutex qemu_fair_mutex;
>  
>  static QemuThread io_thread;
>  
> @@ -416,9 +416,9 @@ int qemu_init_main_loop(void)
>      qemu_cond_init(&qemu_system_cond);
>      qemu_cond_init(&qemu_pause_cond);
>      qemu_cond_init(&qemu_work_cond);
> -    qemu_mutex_init(&qemu_fair_mutex);
> -    qemu_mutex_init(&qemu_global_mutex);
> -    qemu_mutex_lock(&qemu_global_mutex);
> +    g_static_mutex_init(&qemu_fair_mutex);
> +    g_static_mutex_init(&qemu_global_mutex);
> +    g_static_mutex_lock(&qemu_global_mutex);
>  

Just replacing our own abstraction with glib's looks like a step in the
wrong direction. From a first glance at that library and its semantics
it has at least two major drawbacks:

 - Error handling of things like g_mutex_lock or g_cond_wait is, well,
   very "simplistic". Once we start to use more sophisticated locking,
   more bugs will occur here, and we will need more support than glib is
   able to provide (or can you control error handling elsewhere?).

 - GMutex is not powerful enough for optional things like PI mutexes -
   which is required once we want to schedule parts of qemu with RT
   priorities (I did it, it works surprisingly well).

The same concerns apply to other abstractions glib provides for
threading and synchronization. One may work around them, but that will
break abstractions again.

Glib seems to fit standard use case quite comfortably but fails in more
advanced scenarios qemu is already useable for (just lacking a few
additional lines of code).

In short: we need full POSIX where available.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

Re: [Qemu-devel] Re: [PATCH 4/7] Get rid of QemuMutex and teach its callers about GStaticMutex

[Anthony Liguori]

On 01/24/2011 04:24 PM, Jan Kiszka wrote:
> On 2011-01-24 22:00, Anthony Liguori wrote:
>    
>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>
>> diff --git a/cpus.c b/cpus.c
>> index 9cf7e6e..0f8e33b 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -321,8 +321,8 @@ void vm_stop(int reason)
>>
>>   #include "qemu-thread.h"
>>
>> -QemuMutex qemu_global_mutex;
>> -static QemuMutex qemu_fair_mutex;
>> +GStaticMutex qemu_global_mutex;
>> +static GStaticMutex qemu_fair_mutex;
>>
>>   static QemuThread io_thread;
>>
>> @@ -416,9 +416,9 @@ int qemu_init_main_loop(void)
>>       qemu_cond_init(&qemu_system_cond);
>>       qemu_cond_init(&qemu_pause_cond);
>>       qemu_cond_init(&qemu_work_cond);
>> -    qemu_mutex_init(&qemu_fair_mutex);
>> -    qemu_mutex_init(&qemu_global_mutex);
>> -    qemu_mutex_lock(&qemu_global_mutex);
>> +    g_static_mutex_init(&qemu_fair_mutex);
>> +    g_static_mutex_init(&qemu_global_mutex);
>> +    g_static_mutex_lock(&qemu_global_mutex);
>>
>>      
> Just replacing our own abstraction with glib's looks like a step in the
> wrong direction. From a first glance at that library and its semantics
> it has at least two major drawbacks:
>
>   - Error handling of things like g_mutex_lock or g_cond_wait is, well,
>     very "simplistic". Once we start to use more sophisticated locking,
>     more bugs will occur here, and we will need more support than glib is
>     able to provide (or can you control error handling elsewhere?).
>
>   - GMutex is not powerful enough for optional things like PI mutexes -
>     which is required once we want to schedule parts of qemu with RT
>     priorities (I did it, it works surprisingly well).
>    

One of the nice design characteristics of glib/gobject/gtk is that it 
cohabitates well with other APIs.

Nothing stops you from using pthread mutex directly if you really need 
to.  It makes you less portable, but sometimes it's a price that has to 
be paid for functionality.

> The same concerns apply to other abstractions glib provides for
> threading and synchronization. One may work around them, but that will
> break abstractions again.
>
> Glib seems to fit standard use case quite comfortably but fails in more
> advanced scenarios qemu is already useable for (just lacking a few
> additional lines of code).
>
> In short: we need full POSIX where available.
>    

If the problem we have is that we have such advanced use of threading 
and locking in QEMU such that the glib API is not enough and we find 
ourselves constantly calling into the pthread's API directly, then 
that's a wonderful problem to have.

Regards,

Anthony Liguori

> Jan
>
>    

Re: [Qemu-devel] Re: [PATCH 4/7] Get rid of QemuMutex and teach its callers about GStaticMutex

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 2414 bytes --]

On 2011-01-25 01:02, Anthony Liguori wrote:
> On 01/24/2011 04:24 PM, Jan Kiszka wrote:
>> On 2011-01-24 22:00, Anthony Liguori wrote:
>>   
>>> Signed-off-by: Anthony Liguori<aliguori@us.ibm.com>
>>>
>>> diff --git a/cpus.c b/cpus.c
>>> index 9cf7e6e..0f8e33b 100644
>>> --- a/cpus.c
>>> +++ b/cpus.c
>>> @@ -321,8 +321,8 @@ void vm_stop(int reason)
>>>
>>>   #include "qemu-thread.h"
>>>
>>> -QemuMutex qemu_global_mutex;
>>> -static QemuMutex qemu_fair_mutex;
>>> +GStaticMutex qemu_global_mutex;
>>> +static GStaticMutex qemu_fair_mutex;
>>>
>>>   static QemuThread io_thread;
>>>
>>> @@ -416,9 +416,9 @@ int qemu_init_main_loop(void)
>>>       qemu_cond_init(&qemu_system_cond);
>>>       qemu_cond_init(&qemu_pause_cond);
>>>       qemu_cond_init(&qemu_work_cond);
>>> -    qemu_mutex_init(&qemu_fair_mutex);
>>> -    qemu_mutex_init(&qemu_global_mutex);
>>> -    qemu_mutex_lock(&qemu_global_mutex);
>>> +    g_static_mutex_init(&qemu_fair_mutex);
>>> +    g_static_mutex_init(&qemu_global_mutex);
>>> +    g_static_mutex_lock(&qemu_global_mutex);
>>>
>>>      
>> Just replacing our own abstraction with glib's looks like a step in the
>> wrong direction. From a first glance at that library and its semantics
>> it has at least two major drawbacks:
>>
>>   - Error handling of things like g_mutex_lock or g_cond_wait is, well,
>>     very "simplistic". Once we start to use more sophisticated locking,
>>     more bugs will occur here, and we will need more support than glib is
>>     able to provide (or can you control error handling elsewhere?).
>>
>>   - GMutex is not powerful enough for optional things like PI mutexes -
>>     which is required once we want to schedule parts of qemu with RT
>>     priorities (I did it, it works surprisingly well).
>>    
> 
> One of the nice design characteristics of glib/gobject/gtk is that it
> cohabitates well with other APIs.
> 
> Nothing stops you from using pthread mutex directly if you really need
> to.  It makes you less portable, but sometimes it's a price that has to
> be paid for functionality.

I'm not talking about adding new PI mutexes, I'm talking about
effectively reverting this patch as I need to initializes the existing
ones with PI enabled in the attributes. Or replacing g_mutex_* with
wrappers again to add real error handling. Really, glib is too primitive
here.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 259 bytes --]

[Qemu-devel] [PATCH 5/7] threads: get rid of QemuCond and teach callers about GCond

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/cpu-defs.h b/cpu-defs.h
index 8d4bf86..9343824 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -204,7 +204,7 @@ typedef struct CPUWatchpoint {
     uint32_t stop;   /* Stop request */                                 \
     uint32_t stopped; /* Artificially stopped */                        \
     struct QemuThread *thread;                                          \
-    struct QemuCond *halt_cond;                                         \
+    struct _GCond *halt_cond;                                           \
     struct qemu_work_item *queued_work_first, *queued_work_last;        \
     const char *cpu_model_str;                                          \
     struct KVMState *kvm_state;                                         \
diff --git a/cpus.c b/cpus.c
index 0f8e33b..bc7363f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -327,15 +327,15 @@ static GStaticMutex qemu_fair_mutex;
 static QemuThread io_thread;
 
 static QemuThread *tcg_cpu_thread;
-static QemuCond *tcg_halt_cond;
+static GCond *tcg_halt_cond;
 
 static int qemu_system_ready;
 /* cpu creation */
-static QemuCond qemu_cpu_cond;
+static GCond *qemu_cpu_cond;
 /* system init */
-static QemuCond qemu_system_cond;
-static QemuCond qemu_pause_cond;
-static QemuCond qemu_work_cond;
+static GCond *qemu_system_cond;
+static GCond *qemu_pause_cond;
+static GCond *qemu_work_cond;
 
 static void tcg_init_ipi(void);
 static void kvm_init_ipi(CPUState *env);
@@ -412,10 +412,10 @@ int qemu_init_main_loop(void)
     if (ret)
         return ret;
 
-    qemu_cond_init(&qemu_cpu_cond);
-    qemu_cond_init(&qemu_system_cond);
-    qemu_cond_init(&qemu_pause_cond);
-    qemu_cond_init(&qemu_work_cond);
+    qemu_cpu_cond = g_cond_new();
+    qemu_system_cond = g_cond_new();
+    qemu_pause_cond = g_cond_new();
+    qemu_work_cond = g_cond_new();
     g_static_mutex_init(&qemu_fair_mutex);
     g_static_mutex_init(&qemu_global_mutex);
     g_static_mutex_lock(&qemu_global_mutex);
@@ -428,7 +428,7 @@ int qemu_init_main_loop(void)
 void qemu_main_loop_start(void)
 {
     qemu_system_ready = 1;
-    qemu_cond_broadcast(&qemu_system_cond);
+    g_cond_broadcast(qemu_system_cond);
 }
 
 void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
@@ -454,8 +454,8 @@ void run_on_cpu(CPUState *env, void (*func)(void *data), void *data)
     while (!wi.done) {
         CPUState *self_env = cpu_single_env;
 
-        qemu_cond_wait(&qemu_work_cond,
-                       g_static_mutex_get_mutex(&qemu_global_mutex));
+        g_cond_wait(qemu_work_cond,
+                    g_static_mutex_get_mutex(&qemu_global_mutex));
         cpu_single_env = self_env;
     }
 }
@@ -473,7 +473,7 @@ static void flush_queued_work(CPUState *env)
         wi->done = true;
     }
     env->queued_work_last = NULL;
-    qemu_cond_broadcast(&qemu_work_cond);
+    g_cond_broadcast(qemu_work_cond);
 }
 
 static void qemu_wait_io_event_common(CPUState *env)
@@ -481,7 +481,7 @@ static void qemu_wait_io_event_common(CPUState *env)
     if (env->stop) {
         env->stop = 0;
         env->stopped = 1;
-        qemu_cond_signal(&qemu_pause_cond);
+        g_cond_signal(qemu_pause_cond);
     }
     flush_queued_work(env);
 }
@@ -490,9 +490,13 @@ static void qemu_tcg_wait_io_event(void)
 {
     CPUState *env;
 
-    while (!any_cpu_has_work())
-        qemu_cond_timedwait(tcg_halt_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 1000);
+    while (!any_cpu_has_work()) {
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 1000000);
+        g_cond_timed_wait(tcg_halt_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+    }
 
     g_static_mutex_unlock(&qemu_global_mutex);
 
@@ -586,9 +590,13 @@ static void qemu_kvm_eat_signal(CPUState *env, int timeout)
 
 static void qemu_kvm_wait_io_event(CPUState *env)
 {
-    while (!cpu_has_work(env))
-        qemu_cond_timedwait(env->halt_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 1000);
+    while (!cpu_has_work(env)) {
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 1000000);
+        g_cond_timed_wait(env->halt_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+    }
 
     qemu_kvm_eat_signal(env, 0);
     qemu_wait_io_event_common(env);
@@ -609,12 +617,16 @@ static void *kvm_cpu_thread_fn(void *arg)
 
     /* signal CPU creation */
     env->created = 1;
-    qemu_cond_signal(&qemu_cpu_cond);
+    g_cond_signal(qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
-        qemu_cond_timedwait(&qemu_system_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
+    while (!qemu_system_ready) {
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 100000);
+        g_cond_timed_wait(qemu_system_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+    }
 
     while (1) {
         if (cpu_can_run(env))
@@ -636,12 +648,16 @@ static void *tcg_cpu_thread_fn(void *arg)
     g_static_mutex_lock(&qemu_global_mutex);
     for (env = first_cpu; env != NULL; env = env->next_cpu)
         env->created = 1;
-    qemu_cond_signal(&qemu_cpu_cond);
+    g_cond_signal(qemu_cpu_cond);
 
     /* and wait for machine initialization */
-    while (!qemu_system_ready)
-        qemu_cond_timedwait(&qemu_system_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
+    while (!qemu_system_ready) {
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 100000);
+        g_cond_timed_wait(qemu_system_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+    }
 
     while (1) {
         cpu_exec_all();
@@ -654,7 +670,7 @@ static void *tcg_cpu_thread_fn(void *arg)
 void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
-    qemu_cond_broadcast(env->halt_cond);
+    g_cond_broadcast(env->halt_cond);
     qemu_thread_signal(env->thread, SIG_IPI);
 }
 
@@ -784,8 +800,11 @@ void pause_all_vcpus(void)
     }
 
     while (!all_vcpus_paused()) {
-        qemu_cond_timedwait(&qemu_pause_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 10000);
+        g_cond_timed_wait(qemu_pause_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
         penv = first_cpu;
         while (penv) {
             qemu_cpu_kick(penv);
@@ -812,13 +831,15 @@ static void tcg_init_vcpu(void *_env)
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
         env->thread = qemu_mallocz(sizeof(QemuThread));
-        env->halt_cond = qemu_mallocz(sizeof(QemuCond));
-        qemu_cond_init(env->halt_cond);
+        env->halt_cond = g_cond_new();
         qemu_thread_create(env->thread, tcg_cpu_thread_fn, env);
-        while (env->created == 0)
-            qemu_cond_timedwait(&qemu_cpu_cond,
-                                g_static_mutex_get_mutex(&qemu_global_mutex),
-                                100);
+        while (env->created == 0) {
+            GTimeVal t;
+            g_get_current_time(&t);
+            g_time_val_add(&t, 10000);
+            g_cond_timed_wait(qemu_cpu_cond,
+                              g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+        }
         tcg_cpu_thread = env->thread;
         tcg_halt_cond = env->halt_cond;
     } else {
@@ -830,12 +851,15 @@ static void tcg_init_vcpu(void *_env)
 static void kvm_start_vcpu(CPUState *env)
 {
     env->thread = qemu_mallocz(sizeof(QemuThread));
-    env->halt_cond = qemu_mallocz(sizeof(QemuCond));
-    qemu_cond_init(env->halt_cond);
+    env->halt_cond = g_cond_new();
     qemu_thread_create(env->thread, kvm_cpu_thread_fn, env);
-    while (env->created == 0)
-        qemu_cond_timedwait(&qemu_cpu_cond,
-                            g_static_mutex_get_mutex(&qemu_global_mutex), 100);
+    while (env->created == 0) {
+        GTimeVal t;
+        g_get_current_time(&t);
+        g_time_val_add(&t, 100000);
+        g_cond_timed_wait(qemu_cpu_cond,
+                          g_static_mutex_get_mutex(&qemu_global_mutex), &t);
+    }
 }
 
 void qemu_init_vcpu(void *_env)
diff --git a/qemu-thread.c b/qemu-thread.c
index df17eb4..748da5e 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -14,43 +14,6 @@
 #include "qemu-common.h"
 #include "qemu-thread.h"
 
-void qemu_cond_init(QemuCond *cond)
-{
-    cond->cond = g_cond_new();
-}
-
-void qemu_cond_destroy(QemuCond *cond)
-{
-    g_cond_free(cond->cond);
-}
-
-void qemu_cond_signal(QemuCond *cond)
-{
-    g_cond_signal(cond->cond);
-}
-
-void qemu_cond_broadcast(QemuCond *cond)
-{
-    g_cond_broadcast(cond->cond);
-}
-
-void qemu_cond_wait(QemuCond *cond, GMutex *mutex)
-{
-    g_cond_wait(cond->cond, mutex);
-}
-
-int qemu_cond_timedwait(QemuCond *cond, GMutex *mutex, uint64_t msecs)
-{
-    GTimeVal abs_time;
-
-    assert(cond->cond != NULL);
-
-    g_get_current_time(&abs_time);
-    g_time_val_add(&abs_time, msecs * 1000); /* MSEC to USEC */
-
-    return g_cond_timed_wait(cond->cond, mutex, &abs_time);
-}
-
 struct trampoline_data
 {
     QemuThread *thread;
diff --git a/qemu-thread.h b/qemu-thread.h
index dec6848..2c99c94 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -3,25 +3,13 @@
 #include <glib.h>
 #include <pthread.h>
 
-struct QemuCond {
-    GCond *cond;
-};
-
 struct QemuThread {
     GThread *thread;
     pthread_t tid;
 };
 
-typedef struct QemuCond QemuCond;
 typedef struct QemuThread QemuThread;
 
-void qemu_cond_init(QemuCond *cond);
-void qemu_cond_destroy(QemuCond *cond);
-void qemu_cond_signal(QemuCond *cond);
-void qemu_cond_broadcast(QemuCond *cond);
-void qemu_cond_wait(QemuCond *cond, GMutex *mutex);
-int qemu_cond_timedwait(QemuCond *cond, GMutex *mutex, uint64_t msecs);
-
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg);
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 48f567e..0c2b1a0 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -49,7 +49,7 @@
 */
 
 struct VncJobQueue {
-    QemuCond cond;
+    GCond *cond;
     GStaticMutex mutex;
     QemuThread thread;
     Buffer buffer;
@@ -108,7 +108,7 @@ void vnc_job_push(VncJob *job)
         qemu_free(job);
     } else {
         QTAILQ_INSERT_TAIL(&queue->jobs, job, next);
-        qemu_cond_broadcast(&queue->cond);
+        g_cond_broadcast(queue->cond);
     }
     vnc_unlock_queue(queue);
 }
@@ -152,7 +152,7 @@ void vnc_jobs_join(VncState *vs)
 {
     vnc_lock_queue(queue);
     while (vnc_has_job_locked(vs)) {
-        qemu_cond_wait(&queue->cond, g_static_mutex_get_mutex(&queue->mutex));
+        g_cond_wait(queue->cond, g_static_mutex_get_mutex(&queue->mutex));
     }
     vnc_unlock_queue(queue);
 }
@@ -195,7 +195,7 @@ static int vnc_worker_thread_loop(VncJobQueue *queue)
 
     vnc_lock_queue(queue);
     while (QTAILQ_EMPTY(&queue->jobs) && !queue->exit) {
-        qemu_cond_wait(&queue->cond, g_static_mutex_get_mutex(&queue->mutex));
+        g_cond_wait(queue->cond, g_static_mutex_get_mutex(&queue->mutex));
     }
     /* Here job can only be NULL if queue->exit is true */
     job = QTAILQ_FIRST(&queue->jobs);
@@ -265,7 +265,7 @@ disconnected:
     vnc_lock_queue(queue);
     QTAILQ_REMOVE(&queue->jobs, job, next);
     vnc_unlock_queue(queue);
-    qemu_cond_broadcast(&queue->cond);
+    g_cond_broadcast(queue->cond);
     qemu_free(job);
     return 0;
 }
@@ -274,7 +274,7 @@ static VncJobQueue *vnc_queue_init(void)
 {
     VncJobQueue *queue = qemu_mallocz(sizeof(VncJobQueue));
 
-    qemu_cond_init(&queue->cond);
+    queue->cond = g_cond_new();
     g_static_mutex_init(&queue->mutex);
     QTAILQ_INIT(&queue->jobs);
     return queue;
@@ -282,7 +282,7 @@ static VncJobQueue *vnc_queue_init(void)
 
 static void vnc_queue_clear(VncJobQueue *q)
 {
-    qemu_cond_destroy(&queue->cond);
+    g_cond_free(queue->cond);
     g_static_mutex_free(&queue->mutex);
     buffer_free(&queue->buffer);
     qemu_free(q);
@@ -327,5 +327,5 @@ void vnc_stop_worker_thread(void)
     queue->exit = true;
     vnc_unlock_queue(queue);
     vnc_jobs_clear(NULL);
-    qemu_cond_broadcast(&queue->cond);
+    g_cond_broadcast(queue->cond);
 }
-- 
1.7.0.4

[Qemu-devel] [PATCH 6/7] Teach vnc server to use GThread directly

[Anthony Liguori]

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/qemu-thread.c b/qemu-thread.c
index 748da5e..6d0c51e 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -14,6 +14,31 @@
 #include "qemu-common.h"
 #include "qemu-thread.h"
 
+#ifdef _WIN32
+GThread *q_thread_create_nosignal(GThreadFunc func,
+                                  gpointer data,
+                                  gboolean joinable,
+                                  GError **error)
+{
+    return g_thread_create(func, data, joinable, NULL);
+}
+#else
+GThread *q_thread_create_nosignal(GThreadFunc func,
+                                  gpointer data,
+                                  gboolean joinable,
+                                  GError **error)
+{
+    GThread *tid;
+    sigset_t set, old;
+
+    sigfillset(&set);
+    pthread_sigmask(SIG_SETMASK, &set, &old);
+    tid = g_thread_create(func, data, joinable, error);
+    pthread_sigmask(SIG_SETMASK, &old, NULL);
+    return tid;
+}
+#endif
+
 struct trampoline_data
 {
     QemuThread *thread;
diff --git a/qemu-thread.h b/qemu-thread.h
index 2c99c94..ec7fabd 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -10,6 +10,11 @@ struct QemuThread {
 
 typedef struct QemuThread QemuThread;
 
+GThread *q_thread_create_nosignal(GThreadFunc func,
+                                  gpointer data,
+                                  gboolean joinable,
+                                  GError **error);
+
 void qemu_thread_create(QemuThread *thread,
                        void *(*start_routine)(void*),
                        void *arg);
diff --git a/ui/vnc-jobs-async.c b/ui/vnc-jobs-async.c
index 0c2b1a0..2df245d 100644
--- a/ui/vnc-jobs-async.c
+++ b/ui/vnc-jobs-async.c
@@ -51,7 +51,7 @@
 struct VncJobQueue {
     GCond *cond;
     GStaticMutex mutex;
-    QemuThread thread;
+    GThread *thread;
     Buffer buffer;
     bool exit;
     QTAILQ_HEAD(, VncJob) jobs;
@@ -289,11 +289,11 @@ static void vnc_queue_clear(VncJobQueue *q)
     queue = NULL; /* Unset global queue */
 }
 
-static void *vnc_worker_thread(void *arg)
+static gpointer vnc_worker_thread(gpointer arg)
 {
     VncJobQueue *queue = arg;
 
-    qemu_thread_self(&queue->thread);
+    queue->thread = g_thread_self();
 
     while (!vnc_worker_thread_loop(queue)) ;
     vnc_queue_clear(queue);
@@ -308,7 +308,7 @@ void vnc_start_worker_thread(void)
         return ;
 
     q = vnc_queue_init();
-    qemu_thread_create(&q->thread, vnc_worker_thread, q);
+    q->thread = q_thread_create_nosignal(vnc_worker_thread, q, FALSE, NULL);
     queue = q; /* Set global queue */
 }
 
-- 
1.7.0.4

Re: [Qemu-devel] [PATCH 6/7] Teach vnc server to use GThread directly

[Stefan Hajnoczi]

On Mon, Jan 24, 2011 at 9:00 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> +#ifdef _WIN32
> +GThread *q_thread_create_nosignal(GThreadFunc func,
> +                                  gpointer data,
> +                                  gboolean joinable,
> +                                  GError **error)

Let's not introduce a new naming convention, qemu_* is the norm.

Stefan

[Qemu-devel] [PATCH 7/7] Rename QemuThread to QemuSThread to indicate that it is not a generic thread

[Anthony Liguori]

This is an abstraction to support signalable threads.  Signaling is inheritedly
non-portable so this type of thread should only be used when absolutely
necessary.  I think the current users are the only users that should ever need
to exist so strongly discourage future use.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/cpu-defs.h b/cpu-defs.h
index 9343824..ed2a209 100644
--- a/cpu-defs.h
+++ b/cpu-defs.h
@@ -203,7 +203,7 @@ typedef struct CPUWatchpoint {
     uint32_t created;                                                   \
     uint32_t stop;   /* Stop request */                                 \
     uint32_t stopped; /* Artificially stopped */                        \
-    struct QemuThread *thread;                                          \
+    struct QemuSThread *thread;                                          \
     struct _GCond *halt_cond;                                           \
     struct qemu_work_item *queued_work_first, *queued_work_last;        \
     const char *cpu_model_str;                                          \
diff --git a/cpus.c b/cpus.c
index bc7363f..eebfe11 100644
--- a/cpus.c
+++ b/cpus.c
@@ -324,9 +324,9 @@ void vm_stop(int reason)
 GStaticMutex qemu_global_mutex;
 static GStaticMutex qemu_fair_mutex;
 
-static QemuThread io_thread;
+static QemuSThread io_thread;
 
-static QemuThread *tcg_cpu_thread;
+static QemuSThread *tcg_cpu_thread;
 static GCond *tcg_halt_cond;
 
 static int qemu_system_ready;
@@ -420,7 +420,7 @@ int qemu_init_main_loop(void)
     g_static_mutex_init(&qemu_global_mutex);
     g_static_mutex_lock(&qemu_global_mutex);
 
-    qemu_thread_self(&io_thread);
+    qemu_sthread_self(&io_thread);
 
     return 0;
 }
@@ -609,7 +609,7 @@ static void *kvm_cpu_thread_fn(void *arg)
     CPUState *env = arg;
 
     g_static_mutex_lock(&qemu_global_mutex);
-    qemu_thread_self(env->thread);
+    qemu_sthread_self(env->thread);
     if (kvm_enabled())
         kvm_init_vcpu(env);
 
@@ -642,7 +642,7 @@ static void *tcg_cpu_thread_fn(void *arg)
     CPUState *env = arg;
 
     tcg_init_ipi();
-    qemu_thread_self(env->thread);
+    qemu_sthread_self(env->thread);
 
     /* signal CPU creation */
     g_static_mutex_lock(&qemu_global_mutex);
@@ -671,17 +671,17 @@ void qemu_cpu_kick(void *_env)
 {
     CPUState *env = _env;
     g_cond_broadcast(env->halt_cond);
-    qemu_thread_signal(env->thread, SIG_IPI);
+    qemu_sthread_signal(env->thread, SIG_IPI);
 }
 
 int qemu_cpu_self(void *_env)
 {
     CPUState *env = _env;
-    QemuThread this;
+    QemuSThread this;
 
-    qemu_thread_self(&this);
+    qemu_sthread_self(&this);
 
-    return qemu_thread_equal(&this, env->thread);
+    return qemu_sthread_equal(&this, env->thread);
 }
 
 static void cpu_signal(int sig)
@@ -764,7 +764,7 @@ void qemu_mutex_lock_iothread(void)
     } else {
         g_static_mutex_lock(&qemu_fair_mutex);
         if (g_static_mutex_trylock(&qemu_global_mutex)) {
-            qemu_thread_signal(tcg_cpu_thread, SIG_IPI);
+            qemu_sthread_signal(tcg_cpu_thread, SIG_IPI);
             g_static_mutex_lock(&qemu_global_mutex);
         }
         g_static_mutex_unlock(&qemu_fair_mutex);
@@ -830,9 +830,9 @@ static void tcg_init_vcpu(void *_env)
     CPUState *env = _env;
     /* share a single thread for all cpus with TCG */
     if (!tcg_cpu_thread) {
-        env->thread = qemu_mallocz(sizeof(QemuThread));
+        env->thread = qemu_mallocz(sizeof(QemuSThread));
         env->halt_cond = g_cond_new();
-        qemu_thread_create(env->thread, tcg_cpu_thread_fn, env);
+        qemu_sthread_create(env->thread, tcg_cpu_thread_fn, env);
         while (env->created == 0) {
             GTimeVal t;
             g_get_current_time(&t);
@@ -850,9 +850,9 @@ static void tcg_init_vcpu(void *_env)
 
 static void kvm_start_vcpu(CPUState *env)
 {
-    env->thread = qemu_mallocz(sizeof(QemuThread));
+    env->thread = qemu_mallocz(sizeof(QemuSThread));
     env->halt_cond = g_cond_new();
-    qemu_thread_create(env->thread, kvm_cpu_thread_fn, env);
+    qemu_sthread_create(env->thread, kvm_cpu_thread_fn, env);
     while (env->created == 0) {
         GTimeVal t;
         g_get_current_time(&t);
@@ -887,10 +887,10 @@ static void qemu_system_vmstop_request(int reason)
 
 void vm_stop(int reason)
 {
-    QemuThread me;
-    qemu_thread_self(&me);
+    QemuSThread me;
+    qemu_sthread_self(&me);
 
-    if (!qemu_thread_equal(&me, &io_thread)) {
+    if (!qemu_sthread_equal(&me, &io_thread)) {
         qemu_system_vmstop_request(reason);
         /*
          * FIXME: should not return to device code in case
diff --git a/qemu-thread.c b/qemu-thread.c
index 6d0c51e..7ba4950 100644
--- a/qemu-thread.c
+++ b/qemu-thread.c
@@ -41,7 +41,7 @@ GThread *q_thread_create_nosignal(GThreadFunc func,
 
 struct trampoline_data
 {
-    QemuThread *thread;
+    QemuSThread *thread;
     void *(*startfn)(void *);
     void *opaque;
     GStaticMutex lock;
@@ -61,7 +61,7 @@ static gpointer thread_trampoline(gpointer data)
     return retval;
 }
 
-void qemu_thread_create(QemuThread *thread,
+void qemu_sthread_create(QemuSThread *thread,
                         void *(*start_routine)(void*),
                         void *arg)
 {
@@ -91,23 +91,23 @@ void qemu_thread_create(QemuThread *thread,
     g_static_mutex_free(&td->lock);
 }
 
-void qemu_thread_signal(QemuThread *thread, int sig)
+void qemu_sthread_signal(QemuSThread *thread, int sig)
 {
     pthread_kill(thread->tid, sig);
 }
 
-void qemu_thread_self(QemuThread *thread)
+void qemu_sthread_self(QemuSThread *thread)
 {
     thread->thread = g_thread_self();
     thread->tid = pthread_self();
 }
 
-int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2)
+int qemu_sthread_equal(QemuSThread *thread1, QemuSThread *thread2)
 {
     return (thread1->thread == thread2->thread);
 }
 
-void qemu_thread_exit(void *retval)
+void qemu_sthread_exit(void *retval)
 {
     g_thread_exit(retval);
 }
diff --git a/qemu-thread.h b/qemu-thread.h
index ec7fabd..a98a955 100644
--- a/qemu-thread.h
+++ b/qemu-thread.h
@@ -3,24 +3,38 @@
 #include <glib.h>
 #include <pthread.h>
 
-struct QemuThread {
-    GThread *thread;
-    pthread_t tid;
-};
-
-typedef struct QemuThread QemuThread;
-
+/**
+ * Light wrapper that sets signal mask appropriately for a non-I/O thread
+ */
 GThread *q_thread_create_nosignal(GThreadFunc func,
                                   gpointer data,
                                   gboolean joinable,
                                   GError **error);
 
-void qemu_thread_create(QemuThread *thread,
-                       void *(*start_routine)(void*),
-                       void *arg);
-void qemu_thread_signal(QemuThread *thread, int sig);
-void qemu_thread_self(QemuThread *thread);
-int qemu_thread_equal(QemuThread *thread1, QemuThread *thread2);
-void qemu_thread_exit(void *retval);
+/**
+ * Signal Threads
+ *
+ * Signal threads are non-portable types of threads that can be signaled
+ * directly.  This is an interface that should disappear but requires that an
+ * appropriate abstraction be made.  As of today, both TCG and KVM only support
+ * being interrupted via a signal so for platforms that don't support this,
+ * some other provisions must be made.
+ *
+ * Please do not use this interface in new code.  Just use GThreads directly.
+ */
+struct QemuSThread {
+    GThread *thread;
+    pthread_t tid;
+};
+
+typedef struct QemuSThread QemuSThread;
+
+void qemu_sthread_create(QemuSThread *thread,
+                         void *(*start_routine)(void*),
+                         void *arg);
+void qemu_sthread_signal(QemuSThread *thread, int sig);
+void qemu_sthread_self(QemuSThread *thread);
+int qemu_sthread_equal(QemuSThread *thread1, QemuSThread *thread2);
+void qemu_sthread_exit(void *retval);
 
 #endif
-- 
1.7.0.4

[Qemu-devel] Re: [RFC 0/7] Introduce hard dependency on glib

[Paolo Bonzini]

On 01/24/2011 10:00 PM, Anthony Liguori wrote:
> Both the recent I/O loop and threadlet series have me concerned that we're
> digging ourselves deeper into the NIH hole.  I think it's time we look at
> something radical to let us borrow more code from existing projects instead of
> reinventing everything through trial and error.
>
> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
>
>   - portable threads
>   - IO library
>   - rich data structure support

These are where GLib shines.

>   - INI parser

Not compatible with our config files anyway.

>   - JSON parser

I think our JSON parser is much better than JsonGlib (which isn't anyway 
a part of GLib proper).

>   - generic type system
>   - object oriented infrastructure
>   - module system
>   - introspection to enable support for dynamic language bindings

See below.

> I see this series as the first step, followed by converting the I/O loop to
> a GMainLoop instance.  Once we're there, we can start making deeper use of
> GObjects including converting QDev to a GObject hierarchy.

I am not sure how these other bits will look like and maybe it won't 
make sense at all, but it doesn't matter.  The bits you posted are 
already a step in the right direction IMHO, and the same goes for 
converting the I/O loop to GMainLoop if you can pull that :) and using 
glib's thread pool.

(Regarding GMainLoop, my main worry is that we'd have to reinvent a lot 
of GSources to integrate properly with slirp, with QEMU's multiple 
clocks, and so on.  And GSources are black magic...).

I'd also add to the benefits a good multi-threaded memory allocator.

A small qualm I have is that glib (and GNOME in general) is one of the 
hardest pieces of free software to contribute to in my experience.  I 
have patches which had multiple acks and are still languishing after a 
year or so.  Still not an excuse for NIH.

Paolo

Re: [Qemu-devel] Re: [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

On 01/24/2011 03:28 PM, Paolo Bonzini wrote:
> On 01/24/2011 10:00 PM, Anthony Liguori wrote:
>> Both the recent I/O loop and threadlet series have me concerned that 
>> we're
>> digging ourselves deeper into the NIH hole.  I think it's time we 
>> look at
>> something radical to let us borrow more code from existing projects 
>> instead of
>> reinventing everything through trial and error.
>>
>> This series introduces a hard dependency on glib.  The initial use is 
>> portable
>> threads but I see this as just the beginning.  Glib/Gobject offer 
>> many nice
>> things including:
>>
>>   - portable threads
>>   - IO library
>>   - rich data structure support
>
> These are where GLib shines.
>
>>   - INI parser
>
> Not compatible with our config files anyway.
>
>>   - JSON parser
>
> I think our JSON parser is much better than JsonGlib (which isn't 
> anyway a part of GLib proper).

Not sure how much either of these matter, but we should at least drop 
QObject and convert our JSON parser to use GValues such that we can 
treat the JSON parser as a stand alone component.

>>   - generic type system
>>   - object oriented infrastructure
>>   - module system
>>   - introspection to enable support for dynamic language bindings
>
> See below.
>
>> I see this series as the first step, followed by converting the I/O 
>> loop to
>> a GMainLoop instance.  Once we're there, we can start making deeper 
>> use of
>> GObjects including converting QDev to a GObject hierarchy.
>
> I am not sure how these other bits will look like and maybe it won't 
> make sense at all, but it doesn't matter.  The bits you posted are 
> already a step in the right direction IMHO, and the same goes for 
> converting the I/O loop to GMainLoop if you can pull that :) and using 
> glib's thread pool.
>
> (Regarding GMainLoop, my main worry is that we'd have to reinvent a 
> lot of GSources to integrate properly with slirp, with QEMU's multiple 
> clocks, and so on.  And GSources are black magic...).

I've been thinking about this myself.  I think slirp is probably best 
handled as a GSource as much as I don't want to do it.  I don't see 
another option.

We can't use the g_timeout_source directly because the interval is 
milliseconds.  Since our clock mechanisms all use signals to generate 
clock events, I think we can tie it all to a pipe() and just use a 
GIOChannel to generate events.  Obviously, implementing a GSource is the 
best long term approach but I think there's a reasonable short term one.

Regards,

Anthony Liguori

> I'd also add to the benefits a good multi-threaded memory allocator.
>
> A small qualm I have is that glib (and GNOME in general) is one of the 
> hardest pieces of free software to contribute to in my experience.  I 
> have patches which had multiple acks and are still languishing after a 
> year or so.  Still not an excuse for NIH.
>
> Paolo
>

[Qemu-devel] Re: [RFC 0/7] Introduce hard dependency on glib

[Paolo Bonzini]

On 01/24/2011 11:01 PM, Anthony Liguori wrote:
>>> - JSON parser
>>
>> I think our JSON parser is much better than JsonGlib (which isn't
>> anyway a part of GLib proper).
>
> Not sure how much either of these matter, but we should at least drop
> QObject and convert our JSON parser to use GValues such that we can
> treat the JSON parser as a stand alone component.

Not too nice actually, as GHashTable and GArrays are not GValues.

> I've been thinking about this myself. I think slirp is probably best
> handled as a GSource as much as I don't want to do it. I don't see
> another option.

Yes.  Track the previous bitmap of fd's and add/remove pollfd's when 
they appear into (resp. disappear from) the bitmap.

Note that the glib main loop can be quadratic in the number of file 
descriptors BTW.

> We can't use the g_timeout_source directly because the interval is
> milliseconds.

Our deadlines are rounded to milliseconds anyway.  However, with an 
iothread you really do not need signals anymore.  The dynticks code maps 
pretty well to a GSource.

> Obviously, implementing a GSource is the
> best long term approach but I think there's a reasonable short term one.

Or yet another unfinished transition.

Paolo "let's do it in Vala then"

Re: [Qemu-devel] Re: [RFC 0/7] Introduce hard dependency on glib

[Daniel P. Berrange]

On Tue, Jan 25, 2011 at 11:41:23AM +0100, Paolo Bonzini wrote:
> On 01/24/2011 11:01 PM, Anthony Liguori wrote:
> >>>- JSON parser
> >>
> >>I think our JSON parser is much better than JsonGlib (which isn't
> >>anyway a part of GLib proper).
> >
> >Not sure how much either of these matter, but we should at least drop
> >QObject and convert our JSON parser to use GValues such that we can
> >treat the JSON parser as a stand alone component.
> 
> Not too nice actually, as GHashTable and GArrays are not GValues.

It might be worth considering (the fairly newly introduced)
GVariant, rather than GValue. The GVariant code was designed
and is used for (de)serializing DBus messages in GLib. As
such I'd imagine it would be fairly well suited to handling
serialization of other RPC messages like our own JSON.

Regards,
Daniel

Re: [Qemu-devel] Re: [RFC 0/7] Introduce hard dependency on glib

[Paolo Bonzini]

On 01/25/2011 12:14 PM, Daniel P. Berrange wrote:
> It might be worth considering (the fairly newly introduced)
> GVariant, rather than GValue. The GVariant code was designed
> and is used for (de)serializing DBus messages in GLib. As
> such I'd imagine it would be fairly well suited to handling
> serialization of other RPC messages like our own JSON.

Good hint, thanks.

Paolo

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

[-- Attachment #1: Type: text/plain, Size: 2238 bytes --]

On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> Both the recent I/O loop and threadlet series have me concerned that we're
> digging ourselves deeper into the NIH hole.  I think it's time we look at
> something radical to let us borrow more code from existing projects instead of
> reinventing everything through trial and error.
>
> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
>
>   - portable threads
>   - rich data structure support
>   - INI parser
>   - JSON parser
>   - generic type system
>   - object oriented infrastructure
>   - IO library
>   - module system
>   - introspection to enable support for dynamic language bindings
>
> I see this series as the first step, followed by converting the I/O loop to
> a GMainLoop instance.  Once we're there, we can start making deeper use of
> GObjects including converting QDev to a GObject hierarchy.
>
> I've spent the past few months working on C++ integration for QEMU.  I'm more
> convinced than ever that we desperately in need of structured object oriented
> mechanisms to be successful but am pretty strongly convinced that incremental
> additional of C++ is not going to be successful.
>
> On the other hand, while GObjects are uglier and require a lot of template code,
> there's more than enough structure that I think it can guide us into a much
> better object model implementation.
>
> There is some ugliness.   GLib does not abstract signals because they're very
> non-portable but QEMU makes extensive use of signaling.  I don't think it's
> a major issue but some of the ugliness in this series is due to that fact.
>
> This series is only lightly tested but also mostly mechanical.  I'm pretty
> confused by the way tcg_halt_cond and friends works so I'm fairly sure I broke
> that (for non-threaded TCG).
>    

Just to share where this is going, attached patch removes the posix-aio 
thread pool and replaces it with a GThreadPool.

Need to do a lot of functional and performance testing before making a 
change like this so I'll keep this in a separate series, but thought it 
might be interesting.

Regards,

Anthony Liguori


[-- Attachment #2: 0001-posix-aio-convert-to-glib-based-thread-pool.patch --]
[-- Type: text/x-patch, Size: 19551 bytes --]

>From 5fdc51b2aac307c0219e1489b80bc18e9a3db0d1 Mon Sep 17 00:00:00 2001
From: Anthony Liguori <aliguori@us.ibm.com>
Date: Mon, 24 Jan 2011 18:19:08 -0600
Subject: [PATCH 8/7] posix-aio: convert to glib based thread pool

This removes the custom pthread based thread pool in favor of a GThreadPool.
I believe this patch implements all of the necessary functionality but it needs
quite a lot more testing and performance analysis.

One thing I'm sure will break--we used to deliver a signal on every I/O
completion.  This just slows down the I/O path.  The reason we did this was
because at the time, I believe Cris depended on that signal to break out of
QEMU because it did I/O without a periodic timer installed.

At this point in time, I think any architecture that requires signals needs to
do so with a periodic timer or some other mechanism.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

diff --git a/configure b/configure
index 820fde9..1af6b44 100755
--- a/configure
+++ b/configure
@@ -1663,6 +1663,7 @@ if $pkg_config --modversion gthread-2.0 > /dev/null 2>&1 ; then
     glib_cflags=`$pkg_config --cflags gthread-2.0 2>/dev/null`
     glib_libs=`$pkg_config --libs gthread-2.0 2>/dev/null`
     libs_softmmu="$glib_libs $libs_softmmu"
+    libs_tools="$glib_libs $libs_tools"
 else
     echo "glib-2.0 required to compile QEMU"
     exit 1
diff --git a/posix-aio-compat.c b/posix-aio-compat.c
index fa5494d..4d65396 100644
--- a/posix-aio-compat.c
+++ b/posix-aio-compat.c
@@ -12,27 +12,24 @@
  */
 
 #include <sys/ioctl.h>
-#include <sys/types.h>
-#include <pthread.h>
-#include <unistd.h>
-#include <errno.h>
-#include <time.h>
-#include <signal.h>
-#include <string.h>
-#include <stdlib.h>
-#include <stdio.h>
-
-#include "qemu-queue.h"
+
 #include "osdep.h"
 #include "sysemu.h"
 #include "qemu-common.h"
 #include "trace.h"
 #include "block_int.h"
+#include "qemu-thread.h"
 
 #include "block/raw-posix-aio.h"
 
+typedef enum AioState {
+    INACTIVE,
+    CANCELLED,
+    ACTIVE,
+    COMPLETED
+} AioState;
 
-struct qemu_paiocb {
+typedef struct AioAiocb {
     BlockDriverAIOCB common;
     int aio_fildes;
     union {
@@ -40,34 +37,29 @@ struct qemu_paiocb {
         void *aio_ioctl_buf;
     };
     int aio_niov;
-    size_t aio_nbytes;
-#define aio_ioctl_cmd   aio_nbytes /* for QEMU_AIO_IOCTL */
-    int ev_signo;
+    union {
+        size_t aio_nbytes;
+        long aio_ioctl_cmd;
+    };
     off_t aio_offset;
-
-    QTAILQ_ENTRY(qemu_paiocb) node;
     int aio_type;
     ssize_t ret;
-    int active;
-    struct qemu_paiocb *next;
-
     int async_context_id;
-};
 
-typedef struct PosixAioState {
+    /* This state can only be set/get when the aio pool lock is held */
+    AioState state;
+} AioAiocb;
+
+typedef struct AioPool {
+    GThreadPool *pool;
     int rfd, wfd;
-    struct qemu_paiocb *first_aio;
-} PosixAioState;
+    GList *requests;
 
+    /* If this turns out to be contended, push to a per-request lock */
+    GMutex *lock;
+} AioPool;
 
-static pthread_mutex_t lock = PTHREAD_MUTEX_INITIALIZER;
-static pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
-static pthread_t thread_id;
-static pthread_attr_t attr;
-static int max_threads = 64;
-static int cur_threads = 0;
-static int idle_threads = 0;
-static QTAILQ_HEAD(, qemu_paiocb) request_list;
+static AioPool aio_pool;
 
 #ifdef CONFIG_PREADV
 static int preadv_present = 1;
@@ -75,51 +67,7 @@ static int preadv_present = 1;
 static int preadv_present = 0;
 #endif
 
-static void die2(int err, const char *what)
-{
-    fprintf(stderr, "%s failed: %s\n", what, strerror(err));
-    abort();
-}
-
-static void die(const char *what)
-{
-    die2(errno, what);
-}
-
-static void mutex_lock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_lock(mutex);
-    if (ret) die2(ret, "pthread_mutex_lock");
-}
-
-static void mutex_unlock(pthread_mutex_t *mutex)
-{
-    int ret = pthread_mutex_unlock(mutex);
-    if (ret) die2(ret, "pthread_mutex_unlock");
-}
-
-static int cond_timedwait(pthread_cond_t *cond, pthread_mutex_t *mutex,
-                           struct timespec *ts)
-{
-    int ret = pthread_cond_timedwait(cond, mutex, ts);
-    if (ret && ret != ETIMEDOUT) die2(ret, "pthread_cond_timedwait");
-    return ret;
-}
-
-static void cond_signal(pthread_cond_t *cond)
-{
-    int ret = pthread_cond_signal(cond);
-    if (ret) die2(ret, "pthread_cond_signal");
-}
-
-static void thread_create(pthread_t *thread, pthread_attr_t *attr,
-                          void *(*start_routine)(void*), void *arg)
-{
-    int ret = pthread_create(thread, attr, start_routine, arg);
-    if (ret) die2(ret, "pthread_create");
-}
-
-static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
+static ssize_t handle_aiocb_ioctl(AioAiocb *aiocb)
 {
     int ret;
 
@@ -138,7 +86,7 @@ static ssize_t handle_aiocb_ioctl(struct qemu_paiocb *aiocb)
     return aiocb->aio_nbytes;
 }
 
-static ssize_t handle_aiocb_flush(struct qemu_paiocb *aiocb)
+static ssize_t handle_aiocb_flush(AioAiocb *aiocb)
 {
     int ret;
 
@@ -178,7 +126,7 @@ qemu_pwritev(int fd, const struct iovec *iov, int nr_iov, off_t offset)
 
 #endif
 
-static ssize_t handle_aiocb_rw_vector(struct qemu_paiocb *aiocb)
+static ssize_t handle_aiocb_rw_vector(AioAiocb *aiocb)
 {
     size_t offset = 0;
     ssize_t len;
@@ -201,7 +149,7 @@ static ssize_t handle_aiocb_rw_vector(struct qemu_paiocb *aiocb)
     return len;
 }
 
-static ssize_t handle_aiocb_rw_linear(struct qemu_paiocb *aiocb, char *buf)
+static ssize_t handle_aiocb_rw_linear(AioAiocb *aiocb, char *buf)
 {
     ssize_t offset = 0;
     ssize_t len;
@@ -232,7 +180,7 @@ static ssize_t handle_aiocb_rw_linear(struct qemu_paiocb *aiocb, char *buf)
     return offset;
 }
 
-static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
+static ssize_t handle_aiocb_rw(AioAiocb *aiocb)
 {
     ssize_t nbytes;
     char *buf;
@@ -302,278 +250,94 @@ static ssize_t handle_aiocb_rw(struct qemu_paiocb *aiocb)
     return nbytes;
 }
 
-static void *aio_thread(void *unused)
+static void aio_routine(gpointer data, gpointer user_data)
 {
-    pid_t pid;
-
-    pid = getpid();
-
-    while (1) {
-        struct qemu_paiocb *aiocb;
-        ssize_t ret = 0;
-        qemu_timeval tv;
-        struct timespec ts;
-
-        qemu_gettimeofday(&tv);
-        ts.tv_sec = tv.tv_sec + 10;
-        ts.tv_nsec = 0;
-
-        mutex_lock(&lock);
-
-        while (QTAILQ_EMPTY(&request_list) &&
-               !(ret == ETIMEDOUT)) {
-            ret = cond_timedwait(&cond, &lock, &ts);
-        }
-
-        if (QTAILQ_EMPTY(&request_list))
-            break;
-
-        aiocb = QTAILQ_FIRST(&request_list);
-        QTAILQ_REMOVE(&request_list, aiocb, node);
-        aiocb->active = 1;
-        idle_threads--;
-        mutex_unlock(&lock);
-
-        switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
-        case QEMU_AIO_READ:
-        case QEMU_AIO_WRITE:
-            ret = handle_aiocb_rw(aiocb);
-            break;
-        case QEMU_AIO_FLUSH:
-            ret = handle_aiocb_flush(aiocb);
-            break;
-        case QEMU_AIO_IOCTL:
-            ret = handle_aiocb_ioctl(aiocb);
-            break;
-        default:
-            fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
-            ret = -EINVAL;
-            break;
-        }
-
-        mutex_lock(&lock);
-        aiocb->ret = ret;
-        idle_threads++;
-        mutex_unlock(&lock);
+    AioPool *s = user_data;
+    AioAiocb *aiocb = data;
+    ssize_t ret = 0;
+    char ch = 0;
+    AioState state;
+    ssize_t len;
 
-        if (kill(pid, aiocb->ev_signo)) die("kill failed");
+    g_mutex_lock(s->lock);
+    if (aiocb->state != CANCELLED) {
+        aiocb->state = ACTIVE;
     }
-
-    idle_threads--;
-    cur_threads--;
-    mutex_unlock(&lock);
-
-    return NULL;
-}
-
-static void spawn_thread(void)
-{
-    sigset_t set, oldset;
-
-    cur_threads++;
-    idle_threads++;
-
-    /* block all signals */
-    if (sigfillset(&set)) die("sigfillset");
-    if (sigprocmask(SIG_SETMASK, &set, &oldset)) die("sigprocmask");
-
-    thread_create(&thread_id, &attr, aio_thread, NULL);
-
-    if (sigprocmask(SIG_SETMASK, &oldset, NULL)) die("sigprocmask restore");
-}
-
-static void qemu_paio_submit(struct qemu_paiocb *aiocb)
-{
-    aiocb->ret = -EINPROGRESS;
-    aiocb->active = 0;
-    mutex_lock(&lock);
-    if (idle_threads == 0 && cur_threads < max_threads)
-        spawn_thread();
-    QTAILQ_INSERT_TAIL(&request_list, aiocb, node);
-    mutex_unlock(&lock);
-    cond_signal(&cond);
-}
-
-static ssize_t qemu_paio_return(struct qemu_paiocb *aiocb)
-{
-    ssize_t ret;
-
-    mutex_lock(&lock);
-    ret = aiocb->ret;
-    mutex_unlock(&lock);
-
-    return ret;
-}
-
-static int qemu_paio_error(struct qemu_paiocb *aiocb)
-{
-    ssize_t ret = qemu_paio_return(aiocb);
-
-    if (ret < 0)
-        ret = -ret;
-    else
-        ret = 0;
-
-    return ret;
-}
-
-static int posix_aio_process_queue(void *opaque)
-{
-    PosixAioState *s = opaque;
-    struct qemu_paiocb *acb, **pacb;
-    int ret;
-    int result = 0;
-    int async_context_id = get_async_context_id();
-
-    for(;;) {
-        pacb = &s->first_aio;
-        for(;;) {
-            acb = *pacb;
-            if (!acb)
-                return result;
-
-            /* we're only interested in requests in the right context */
-            if (acb->async_context_id != async_context_id) {
-                pacb = &acb->next;
-                continue;
-            }
-
-            ret = qemu_paio_error(acb);
-            if (ret == ECANCELED) {
-                /* remove the request */
-                *pacb = acb->next;
-                qemu_aio_release(acb);
-                result = 1;
-            } else if (ret != EINPROGRESS) {
-                /* end of aio */
-                if (ret == 0) {
-                    ret = qemu_paio_return(acb);
-                    if (ret == acb->aio_nbytes)
-                        ret = 0;
-                    else
-                        ret = -EINVAL;
-                } else {
-                    ret = -ret;
-                }
-                /* remove the request */
-                *pacb = acb->next;
-                /* call the callback */
-                acb->common.cb(acb->common.opaque, ret);
-                qemu_aio_release(acb);
-                result = 1;
-                break;
-            } else {
-                pacb = &acb->next;
-            }
-        }
+    state = aiocb->state;
+    g_mutex_unlock(s->lock);
+        
+    if (state == CANCELLED) {
+        return;
     }
 
-    return result;
-}
-
-static void posix_aio_read(void *opaque)
-{
-    PosixAioState *s = opaque;
-    ssize_t len;
-
-    /* read all bytes from signal pipe */
-    for (;;) {
-        char bytes[16];
-
-        len = read(s->rfd, bytes, sizeof(bytes));
-        if (len == -1 && errno == EINTR)
-            continue; /* try again */
-        if (len == sizeof(bytes))
-            continue; /* more to read */
+    switch (aiocb->aio_type & QEMU_AIO_TYPE_MASK) {
+    case QEMU_AIO_READ:
+    case QEMU_AIO_WRITE:
+        ret = handle_aiocb_rw(aiocb);
+        break;
+    case QEMU_AIO_FLUSH:
+        ret = handle_aiocb_flush(aiocb);
+        break;
+    case QEMU_AIO_IOCTL:
+        ret = handle_aiocb_ioctl(aiocb);
+        break;
+    default:
+        fprintf(stderr, "invalid aio request (0x%x)\n", aiocb->aio_type);
+        ret = -EINVAL;
         break;
     }
 
-    posix_aio_process_queue(s);
-}
-
-static int posix_aio_flush(void *opaque)
-{
-    PosixAioState *s = opaque;
-    return !!s->first_aio;
-}
-
-static PosixAioState *posix_aio_state;
+    aiocb->ret = ret;
+    g_mutex_lock(s->lock);
+    aiocb->state = COMPLETED;
+    g_mutex_unlock(s->lock);
 
-static void aio_signal_handler(int signum)
-{
-    if (posix_aio_state) {
-        char byte = 0;
-        ssize_t ret;
-
-        ret = write(posix_aio_state->wfd, &byte, sizeof(byte));
-        if (ret < 0 && errno != EAGAIN)
-            die("write()");
-    }
+    do {
+        len = write(s->wfd, &ch, sizeof(ch));
+    } while (len == -1 && errno == EINTR);
 
-    qemu_service_io();
+    return;
 }
 
-static void paio_remove(struct qemu_paiocb *acb)
+
+static void qemu_paio_submit(AioAiocb *aiocb)
 {
-    struct qemu_paiocb **pacb;
-
-    /* remove the callback from the queue */
-    pacb = &posix_aio_state->first_aio;
-    for(;;) {
-        if (*pacb == NULL) {
-            fprintf(stderr, "paio_remove: aio request not found!\n");
-            break;
-        } else if (*pacb == acb) {
-            *pacb = acb->next;
-            qemu_aio_release(acb);
-            break;
-        }
-        pacb = &(*pacb)->next;
-    }
+    AioPool *s = &aio_pool;
+    aiocb->state = INACTIVE;
+    aiocb->async_context_id = get_async_context_id();
+    s->requests = g_list_append(s->requests, aiocb);
+    g_thread_pool_push(s->pool, aiocb, NULL);
 }
 
-static void paio_cancel(BlockDriverAIOCB *blockacb)
+static void qemu_paio_cancel(BlockDriverAIOCB *acb)
 {
-    struct qemu_paiocb *acb = (struct qemu_paiocb *)blockacb;
-    int active = 0;
-
-    mutex_lock(&lock);
-    if (!acb->active) {
-        QTAILQ_REMOVE(&request_list, acb, node);
-        acb->ret = -ECANCELED;
-    } else if (acb->ret == -EINPROGRESS) {
-        active = 1;
-    }
-    mutex_unlock(&lock);
+    AioAiocb *aiocb = container_of(acb, AioAiocb, common);
+    AioPool *s = &aio_pool;
 
-    if (active) {
-        /* fail safe: if the aio could not be canceled, we wait for
-           it */
-        while (qemu_paio_error(acb) == EINPROGRESS)
-            ;
+    g_mutex_lock(s->lock);
+    if (aiocb->state == INACTIVE) {
+        aiocb->state = CANCELLED;
     }
-
-    paio_remove(acb);
+    g_mutex_unlock(s->lock);
+    
 }
 
 static AIOPool raw_aio_pool = {
-    .aiocb_size         = sizeof(struct qemu_paiocb),
-    .cancel             = paio_cancel,
+    .aiocb_size         = sizeof(AioAiocb),
+    .cancel             = qemu_paio_cancel,
 };
 
 BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
         int64_t sector_num, QEMUIOVector *qiov, int nb_sectors,
         BlockDriverCompletionFunc *cb, void *opaque, int type)
 {
-    struct qemu_paiocb *acb;
+    AioAiocb *acb;
 
     acb = qemu_aio_get(&raw_aio_pool, bs, cb, opaque);
     if (!acb)
         return NULL;
     acb->aio_type = type;
     acb->aio_fildes = fd;
-    acb->ev_signo = SIGUSR2;
-    acb->async_context_id = get_async_context_id();
 
     if (qiov) {
         acb->aio_iov = qiov->iov;
@@ -582,9 +346,6 @@ BlockDriverAIOCB *paio_submit(BlockDriverState *bs, int fd,
     acb->aio_nbytes = nb_sectors * 512;
     acb->aio_offset = sector_num * 512;
 
-    acb->next = posix_aio_state->first_aio;
-    posix_aio_state->first_aio = acb;
-
     trace_paio_submit(acb, opaque, sector_num, nb_sectors, type);
     qemu_paio_submit(acb);
     return &acb->common;
@@ -594,68 +355,114 @@ BlockDriverAIOCB *paio_ioctl(BlockDriverState *bs, int fd,
         unsigned long int req, void *buf,
         BlockDriverCompletionFunc *cb, void *opaque)
 {
-    struct qemu_paiocb *acb;
+    AioAiocb *acb;
 
     acb = qemu_aio_get(&raw_aio_pool, bs, cb, opaque);
     if (!acb)
         return NULL;
     acb->aio_type = QEMU_AIO_IOCTL;
     acb->aio_fildes = fd;
-    acb->ev_signo = SIGUSR2;
-    acb->async_context_id = get_async_context_id();
     acb->aio_offset = 0;
     acb->aio_ioctl_buf = buf;
     acb->aio_ioctl_cmd = req;
 
-    acb->next = posix_aio_state->first_aio;
-    posix_aio_state->first_aio = acb;
-
     qemu_paio_submit(acb);
     return &acb->common;
 }
 
-int paio_init(void)
+static int paio_process_queue(void *opaque)
 {
-    struct sigaction act;
-    PosixAioState *s;
-    int fds[2];
-    int ret;
+    AioPool *s = opaque;
+    GList *i, *next_i;
+    GList *completed_requests = NULL;
+    int async_context_id = get_async_context_id();
+    int did_work = 0;
 
-    if (posix_aio_state)
-        return 0;
+    /* Search the list to build a list of completed requests, we do
+     * this as it's own pass so that we minimize the time we're holding
+     * the shared lock.
+     */
+    g_mutex_lock(s->lock);
+    for (i = s->requests; i != NULL; i = next_i) {
+        AioAiocb *aiocb = i->data;
+        next_i = g_list_next(i);
+
+        /* don't complete a request that isn't part of this async context */
+        if (aiocb->async_context_id != async_context_id) {
+            continue;
+        }
 
-    s = qemu_malloc(sizeof(PosixAioState));
+        if (aiocb->state == CANCELLED || aiocb->state == COMPLETED) {
+            s->requests = g_list_remove_link(s->requests, i);
+            completed_requests = g_list_concat(completed_requests, i);
+        }
+    }
+    g_mutex_unlock(s->lock);
+
+    /* Dispatch any completed requests */
+    for (i = completed_requests; i != NULL; i = g_list_next(i)) {
+        AioAiocb *aiocb = i->data;
+        if (aiocb->state == COMPLETED) {
+            if (aiocb->ret == aiocb->aio_nbytes) {
+                aiocb->ret = 0;
+            }
+            aiocb->common.cb(aiocb->common.opaque, aiocb->ret);
+            did_work = 1;
+        }
+        qemu_aio_release(aiocb);
+    }
 
-    sigfillset(&act.sa_mask);
-    act.sa_flags = 0; /* do not restart syscalls to interrupt select() */
-    act.sa_handler = aio_signal_handler;
-    sigaction(SIGUSR2, &act, NULL);
+    g_list_free(completed_requests);
+
+    return did_work;
+}
 
-    s->first_aio = NULL;
-    if (qemu_pipe(fds) == -1) {
-        fprintf(stderr, "failed to create pipe\n");
-        return -1;
+static int paio_io_flush(void *opaque)
+{
+    AioPool *s = opaque;
+    if (s->requests == NULL) {
+        return 0;
     }
+    return 1;
+}
 
-    s->rfd = fds[0];
-    s->wfd = fds[1];
+static void paio_complete(void *opaque)
+{
+    AioPool *s = opaque;
+    char buffer[1024];
+    ssize_t len;
 
-    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
-    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
+    /* Drain event queue */
+    do {
+        len = read(s->rfd, buffer, sizeof(buffer));
+    } while (len == -1 && errno == EINTR);
+
+    if (len == -1 && errno == EAGAIN) {
+        return;
+    }
+
+    paio_process_queue(s);
+}
 
-    qemu_aio_set_fd_handler(s->rfd, posix_aio_read, NULL, posix_aio_flush,
-        posix_aio_process_queue, s);
+int paio_init(void)
+{
+    AioPool *s = &aio_pool;
+    int fds[2];
 
-    ret = pthread_attr_init(&attr);
-    if (ret)
-        die2(ret, "pthread_attr_init");
+    if (pipe(fds) == -1) {
+        return -errno;
+    }
 
-    ret = pthread_attr_setdetachstate(&attr, PTHREAD_CREATE_DETACHED);
-    if (ret)
-        die2(ret, "pthread_attr_setdetachstate");
+    s->pool = g_thread_pool_new(aio_routine, s, 64, FALSE, NULL);
+    s->rfd = fds[0];
+    s->wfd = fds[1];
+    s->requests = NULL;
+    s->lock = g_mutex_new();
 
-    QTAILQ_INIT(&request_list);
+    fcntl(s->wfd, F_SETFL, O_NONBLOCK);
+    fcntl(s->rfd, F_SETFL, O_NONBLOCK);
+    qemu_aio_set_fd_handler(s->rfd, paio_complete, NULL,
+                            paio_io_flush, paio_process_queue, s);
 
-    posix_aio_state = s;
     return 0;
 }
diff --git a/qemu-tool.c b/qemu-tool.c
index 392e1c9..ca9c711 100644
--- a/qemu-tool.c
+++ b/qemu-tool.c
@@ -111,3 +111,11 @@ int qemu_set_fd_handler2(int fd,
 {
     return 0;
 }
+
+int qemu_set_fd_handler(int fd,
+                        IOHandler *fd_read,
+                        IOHandler *fd_write,
+                        void *opaque)
+{
+    return 0;
+}
-- 
1.7.0.4

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Edgar E. Iglesias]

On Mon, Jan 24, 2011 at 06:24:13PM -0600, Anthony Liguori wrote:
> On 01/24/2011 03:00 PM, Anthony Liguori wrote:
> > Both the recent I/O loop and threadlet series have me concerned that we're
> > digging ourselves deeper into the NIH hole.  I think it's time we look at
> > something radical to let us borrow more code from existing projects instead of
> > reinventing everything through trial and error.
> >
> > This series introduces a hard dependency on glib.  The initial use is portable
> > threads but I see this as just the beginning.  Glib/Gobject offer many nice
> > things including:
> >
> >   - portable threads
> >   - rich data structure support
> >   - INI parser
> >   - JSON parser
> >   - generic type system
> >   - object oriented infrastructure
> >   - IO library
> >   - module system
> >   - introspection to enable support for dynamic language bindings
> >
> > I see this series as the first step, followed by converting the I/O loop to
> > a GMainLoop instance.  Once we're there, we can start making deeper use of
> > GObjects including converting QDev to a GObject hierarchy.
> >
> > I've spent the past few months working on C++ integration for QEMU.  I'm more
> > convinced than ever that we desperately in need of structured object oriented
> > mechanisms to be successful but am pretty strongly convinced that incremental
> > additional of C++ is not going to be successful.
> >
> > On the other hand, while GObjects are uglier and require a lot of template code,
> > there's more than enough structure that I think it can guide us into a much
> > better object model implementation.
> >
> > There is some ugliness.   GLib does not abstract signals because they're very
> > non-portable but QEMU makes extensive use of signaling.  I don't think it's
> > a major issue but some of the ugliness in this series is due to that fact.
> >
> > This series is only lightly tested but also mostly mechanical.  I'm pretty
> > confused by the way tcg_halt_cond and friends works so I'm fairly sure I broke
> > that (for non-threaded TCG).
> >    
> 
> Just to share where this is going, attached patch removes the posix-aio 
> thread pool and replaces it with a GThreadPool.
> 
> Need to do a lot of functional and performance testing before making a 
> change like this so I'll keep this in a separate series, but thought it 
> might be interesting.
> 
> Regards,
> 
> Anthony Liguori
> 

> From 5fdc51b2aac307c0219e1489b80bc18e9a3db0d1 Mon Sep 17 00:00:00 2001
> From: Anthony Liguori <aliguori@us.ibm.com>
> Date: Mon, 24 Jan 2011 18:19:08 -0600
> Subject: [PATCH 8/7] posix-aio: convert to glib based thread pool
> 
> This removes the custom pthread based thread pool in favor of a GThreadPool.
> I believe this patch implements all of the necessary functionality but it needs
> quite a lot more testing and performance analysis.
> 
> One thing I'm sure will break--we used to deliver a signal on every I/O
> completion.  This just slows down the I/O path.  The reason we did this was
> because at the time, I believe Cris depended on that signal to break out of
> QEMU because it did I/O without a periodic timer installed.

Hi Anthony,

I have no memory of any such issues. Anyway, if you've got a tree a can
clone, I'll be happy to give it a go and let you know if CRIS works ok.
There's also a bootable CRIS linux guest image on the wiki's download
page if you wan't to try yourself.

Cheers

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Stefan Hajnoczi]

On Mon, Jan 24, 2011 at 9:00 PM, Anthony Liguori <aliguori@us.ibm.com> wrote:
> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
>
>  - portable threads
>  - rich data structure support
>  - INI parser
>  - JSON parser
>  - generic type system
>  - object oriented infrastructure
>  - IO library
>  - module system
>  - introspection to enable support for dynamic language bindings
>
> I see this series as the first step, followed by converting the I/O loop to
> a GMainLoop instance.  Once we're there, we can start making deeper use of
> GObjects including converting QDev to a GObject hierarchy.
>
> I've spent the past few months working on C++ integration for QEMU.  I'm more
> convinced than ever that we desperately in need of structured object oriented
> mechanisms to be successful but am pretty strongly convinced that incremental
> additional of C++ is not going to be successful.

I like this direction much more than rewriting everything in C++.  We
can replace some of the homebrew stuff with widely used glib
implementations.  We get new functionality like hash tables and
balanced trees for free.

Even just moving to a solid event loop and I/O handlers is a win.

Stefan

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Gerd Hoffmann]

On 01/24/11 22:00, Anthony Liguori wrote:
> Both the recent I/O loop and threadlet series have me concerned that we're
> digging ourselves deeper into the NIH hole.  I think it's time we look at
> something radical to let us borrow more code from existing projects instead of
> reinventing everything through trial and error.

Somehow this idea crossed my mind as well while thinking about the io 
handler issue.  Why limit yourself to steal ideas from glib if we can 
just use the library instead?

> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
>
>   - portable threads
>   - rich data structure support
>   - INI parser
>   - JSON parser
>   - generic type system
>   - object oriented infrastructure
>   - IO library
>   - module system
>   - introspection to enable support for dynamic language bindings

Even if we only offload some portability issues to glib and use the 
gmainloop we'll have a net win I suspect.  Using gmainloop will make it 
alot easier to integrate third party libs which quite often offer glib 
integration.  We could easily use avahi to announce our vnc server via 
mdns/zeroconf/bonjour for example.  Also I'd be tempted to just rewrite 
pulseaudio support using the glib support in pulse.

Turn all qemu internals into gobjects is certainly non-trivial, 
especially managing the transition phase.  But nevertheless it probably 
is worth the effort long-term as glib has all sorts of language bindings.

<dream>
So we could do all the high-level stuff such as config parsing in a 
high-level language some day.
</dream>

> I've spent the past few months working on C++ integration for QEMU.  I'm more
> convinced than ever that we desperately in need of structured object oriented
> mechanisms to be successful but am pretty strongly convinced that incremental
> additional of C++ is not going to be successful.

Agree.  I doubt switching to C++ will fly.  But using glib has pretty 
good chances to be a big success long-term.

I think we should get 0.14 out of the door before starting to work on 
that though ;)

cheers,
   Gerd

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Daniel P. Berrange]

On Tue, Jan 25, 2011 at 12:51:42PM +0100, Gerd Hoffmann wrote:
> On 01/24/11 22:00, Anthony Liguori wrote:
> >Both the recent I/O loop and threadlet series have me concerned that we're
> >digging ourselves deeper into the NIH hole.  I think it's time we look at
> >something radical to let us borrow more code from existing projects instead of
> >reinventing everything through trial and error.
> 
> Somehow this idea crossed my mind as well while thinking about the
> io handler issue.  Why limit yourself to steal ideas from glib if we
> can just use the library instead?
>
> >This series introduces a hard dependency on glib.  The initial use is portable
> >threads but I see this as just the beginning.  Glib/Gobject offer many nice
> >things including:
> >
> >  - portable threads
> >  - rich data structure support
> >  - INI parser
> >  - JSON parser
> >  - generic type system
> >  - object oriented infrastructure
> >  - IO library
> >  - module system
> >  - introspection to enable support for dynamic language bindings
> 
> Even if we only offload some portability issues to glib and use the
> gmainloop we'll have a net win I suspect.  Using gmainloop will make
> it alot easier to integrate third party libs which quite often offer
> glib integration.  We could easily use avahi to announce our vnc
> server via mdns/zeroconf/bonjour for example.  Also I'd be tempted
> to just rewrite pulseaudio support using the glib support in pulse.
> 
> Turn all qemu internals into gobjects is certainly non-trivial,
> especially managing the transition phase.  But nevertheless it
> probably is worth the effort long-term as glib has all sorts of
> language bindings.

If your internals are all properly designed & encapsulated
GObjects and you enable GObject Introspection, you'll get
direct access from non-C languages more or less for free
these days. You won't have to actually write language bindings
for any of your objects - worst case there is a completely
automatic code generator, but for most dynamic languages not
even that is required anymore in GObject.

Of course getting QEMU internals to be properly encapsulated
objects is a non-trivial task, but it can continue to be done
incrementally of course, and using a more structured object
system would help to enforce rigourous design practice in this
respect.

> <dream>
> So we could do all the high-level stuff such as config parsing in a
> high-level language some day.
> </dream>

Or if really crazy, you could write new devices in Javascript :-)
Could be useful for rapid prototyping of new ideas at least.

Regards,
Daniel

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Stefano Stabellini]

On Tue, 25 Jan 2011, Gerd Hoffmann wrote:
> > I've spent the past few months working on C++ integration for QEMU.  I'm more
> > convinced than ever that we desperately in need of structured object oriented
> > mechanisms to be successful but am pretty strongly convinced that incremental
> > additional of C++ is not going to be successful.
> 
> Agree.  I doubt switching to C++ will fly.  But using glib has pretty 
> good chances to be a big success long-term.
> 

Why is everyone so pessimistic on switching to C++?

Anthony: considering that you have direct experience on trying to do
this, why are you convinced it is not going to work?

I am asking because I have always found the glib GObject stuff a little
ugly compared to well written C++ code (of course you can write ugly
code in any language if you want to).

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

On 01/25/2011 08:48 AM, Stefano Stabellini wrote:
> On Tue, 25 Jan 2011, Gerd Hoffmann wrote:
>    
>>> I've spent the past few months working on C++ integration for QEMU.  I'm more
>>> convinced than ever that we desperately in need of structured object oriented
>>> mechanisms to be successful but am pretty strongly convinced that incremental
>>> additional of C++ is not going to be successful.
>>>        
>> Agree.  I doubt switching to C++ will fly.  But using glib has pretty
>> good chances to be a big success long-term.
>>
>>      
> Why is everyone so pessimistic on switching to C++?
>
> Anthony: considering that you have direct experience on trying to do
> this, why are you convinced it is not going to work?
>    

I'm not ruling out C++ altogether but at this stage, I can't see an 
incremental transition to C++ working all that well.

I tried to isolate the device models and have a well defined interface 
between the C and C++ code but the trouble is that if you want the C++ 
side of things to be Good C++ code, you end up having to replace large 
chunks of QEMU code.

It's possible that I just took the wrong approach.

My end goal is not C++, it's to improve the device model.  I haven't 
tried doing it with GObject yet but before we even get there, there's a 
lot of good we can do with glib.

Regards,

Anthony Liguori

> I am asking because I have always found the glib GObject stuff a little
> ugly compared to well written C++ code (of course you can write ugly
> code in any language if you want to).
>    

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Stefano Stabellini]

On Tue, 25 Jan 2011, Anthony Liguori wrote:
> My end goal is not C++, it's to improve the device model.  I haven't 
> tried doing it with GObject yet but before we even get there, there's a 
> lot of good we can do with glib.

I certainly agree with this.

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Aurelien Jarno]

On Mon, Jan 24, 2011 at 03:00:38PM -0600, Anthony Liguori wrote:
> Both the recent I/O loop and threadlet series have me concerned that we're
> digging ourselves deeper into the NIH hole.  I think it's time we look at
> something radical to let us borrow more code from existing projects instead of
> reinventing everything through trial and error.
> 
> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
> 
>  - portable threads
>  - rich data structure support
>  - INI parser
>  - JSON parser
>  - generic type system
>  - object oriented infrastructure
>  - IO library
>  - module system
>  - introspection to enable support for dynamic language bindings
> 
> I see this series as the first step, followed by converting the I/O loop to
> a GMainLoop instance.  Once we're there, we can start making deeper use of
> GObjects including converting QDev to a GObject hierarchy.
> 
> I've spent the past few months working on C++ integration for QEMU.  I'm more
> convinced than ever that we desperately in need of structured object oriented
> mechanisms to be successful but am pretty strongly convinced that incremental
> additional of C++ is not going to be successful.
> 
> On the other hand, while GObjects are uglier and require a lot of template code,
> there's more than enough structure that I think it can guide us into a much
> better object model implementation.
> 
> There is some ugliness.   GLib does not abstract signals because they're very
> non-portable but QEMU makes extensive use of signaling.  I don't think it's
> a major issue but some of the ugliness in this series is due to that fact.
> 
> This series is only lightly tested but also mostly mechanical.  I'm pretty
> confused by the way tcg_halt_cond and friends works so I'm fairly sure I broke
> that (for non-threaded TCG).

This patch series indeed doesn't work at all on TCG. The CPU emulation
never starts and the processed is only killable with -9. Surprisingly it
works with io_thread disabled, while without this patch series, QEMU both
work with and without io_thread (maybe with some overhead, but at least
it works).

That's a pitty given the main loop is critical for TCG, and I am really
fearing regressions or slowdown that we should detect before going to
far in this direction.

Note that contrary to KVM, it's easy to test TCG as you don't need any
extension on your CPU. The same images can be used.

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

On 01/25/2011 08:23 AM, Aurelien Jarno wrote:
> On Mon, Jan 24, 2011 at 03:00:38PM -0600, Anthony Liguori wrote:
>    
>> Both the recent I/O loop and threadlet series have me concerned that we're
>> digging ourselves deeper into the NIH hole.  I think it's time we look at
>> something radical to let us borrow more code from existing projects instead of
>> reinventing everything through trial and error.
>>
>> This series introduces a hard dependency on glib.  The initial use is portable
>> threads but I see this as just the beginning.  Glib/Gobject offer many nice
>> things including:
>>
>>   - portable threads
>>   - rich data structure support
>>   - INI parser
>>   - JSON parser
>>   - generic type system
>>   - object oriented infrastructure
>>   - IO library
>>   - module system
>>   - introspection to enable support for dynamic language bindings
>>
>> I see this series as the first step, followed by converting the I/O loop to
>> a GMainLoop instance.  Once we're there, we can start making deeper use of
>> GObjects including converting QDev to a GObject hierarchy.
>>
>> I've spent the past few months working on C++ integration for QEMU.  I'm more
>> convinced than ever that we desperately in need of structured object oriented
>> mechanisms to be successful but am pretty strongly convinced that incremental
>> additional of C++ is not going to be successful.
>>
>> On the other hand, while GObjects are uglier and require a lot of template code,
>> there's more than enough structure that I think it can guide us into a much
>> better object model implementation.
>>
>> There is some ugliness.   GLib does not abstract signals because they're very
>> non-portable but QEMU makes extensive use of signaling.  I don't think it's
>> a major issue but some of the ugliness in this series is due to that fact.
>>
>> This series is only lightly tested but also mostly mechanical.  I'm pretty
>> confused by the way tcg_halt_cond and friends works so I'm fairly sure I broke
>> that (for non-threaded TCG).
>>      
> This patch series indeed doesn't work at all on TCG. The CPU emulation
> never starts and the processed is only killable with -9. Surprisingly it
> works with io_thread disabled, while without this patch series, QEMU both
> work with and without io_thread (maybe with some overhead, but at least
> it works).
>
> That's a pitty given the main loop is critical for TCG, and I am really
> fearing regressions or slowdown that we should detect before going to
> far in this direction.
>
> Note that contrary to KVM, it's easy to test TCG as you don't need any
> extension on your CPU. The same images can be used.
>    

Yeah, it's fixable and I will fix it.  I'm still trying to understand 
the I/O thread interactions with TCG.

Regards,

Anthony Liguori

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

On 01/25/2011 10:47 PM, Daniel Veillard wrote:
> On Mon, Jan 24, 2011 at 03:00:38PM -0600, Anthony Liguori wrote:
>    
>> Both the recent I/O loop and threadlet series have me concerned that we're
>> digging ourselves deeper into the NIH hole.  I think it's time we look at
>> something radical to let us borrow more code from existing projects instead of
>> reinventing everything through trial and error.
>>
>> This series introduces a hard dependency on glib.  The initial use is portable
>> threads but I see this as just the beginning.  Glib/Gobject offer many nice
>> things including:
>>
>>   - portable threads
>>   - rich data structure support
>>   - INI parser
>>   - JSON parser
>>   - generic type system
>>   - object oriented infrastructure
>>   - IO library
>>   - module system
>>   - introspection to enable support for dynamic language bindings
>>
>> I see this series as the first step, followed by converting the I/O loop to
>> a GMainLoop instance.  Once we're there, we can start making deeper use of
>> GObjects including converting QDev to a GObject hierarchy.
>>      
>    Hum, one of the reason I tried to avoid glib dependancies on my own
> libraries code is the behaviour on memory allocation error, unless that
> changed (unlikely) or my recollection is wrong (more likely) glib does
> a direct exit() on memory allocation errors. This might be fine for QEmu
> as a standalone program but may turn a disaster if standalone libraries
> are made out of it and expected for reuse say by libvirt(d).
>    

We have independently done the same thing in QEMU and as such would face 
the same "problem" either way.

Quite a debate could be had on the merits of this.  For QEMU, we made 
this decision for practical purposes.  We did not handle malloc failures 
consistently so adopting a consistent behavior eliminated the 
possibility of NULL pointer dereferences which are exploitable.

Regards,

Anthony Liguori

>    I just want to raise that point as it's a rather fundamental decision
> of the glib/Gnome stack, which was a fine decision to take in the context
> of writing GUI framework, but is really a poor one for implementing low level
> or server infrastructure,
>
> Daniel
>
>    

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Stefan Hajnoczi]

On Wed, Jan 26, 2011 at 3:53 PM, Anthony Liguori
<aliguori@linux.vnet.ibm.com> wrote:
> On 01/25/2011 10:47 PM, Daniel Veillard wrote:
>>
>> On Mon, Jan 24, 2011 at 03:00:38PM -0600, Anthony Liguori wrote:
>>
>>>
>>> Both the recent I/O loop and threadlet series have me concerned that
>>> we're
>>> digging ourselves deeper into the NIH hole.  I think it's time we look at
>>> something radical to let us borrow more code from existing projects
>>> instead of
>>> reinventing everything through trial and error.
>>>
>>> This series introduces a hard dependency on glib.  The initial use is
>>> portable
>>> threads but I see this as just the beginning.  Glib/Gobject offer many
>>> nice
>>> things including:
>>>
>>>  - portable threads
>>>  - rich data structure support
>>>  - INI parser
>>>  - JSON parser
>>>  - generic type system
>>>  - object oriented infrastructure
>>>  - IO library
>>>  - module system
>>>  - introspection to enable support for dynamic language bindings
>>>
>>> I see this series as the first step, followed by converting the I/O loop
>>> to
>>> a GMainLoop instance.  Once we're there, we can start making deeper use
>>> of
>>> GObjects including converting QDev to a GObject hierarchy.
>>>
>>
>>   Hum, one of the reason I tried to avoid glib dependancies on my own
>> libraries code is the behaviour on memory allocation error, unless that
>> changed (unlikely) or my recollection is wrong (more likely) glib does
>> a direct exit() on memory allocation errors. This might be fine for QEmu
>> as a standalone program but may turn a disaster if standalone libraries
>> are made out of it and expected for reuse say by libvirt(d).
>>
>
> We have independently done the same thing in QEMU and as such would face the
> same "problem" either way.
>
> Quite a debate could be had on the merits of this.  For QEMU, we made this
> decision for practical purposes.  We did not handle malloc failures
> consistently so adopting a consistent behavior eliminated the possibility of
> NULL pointer dereferences which are exploitable.

Both g_malloc() and g_try_malloc() are available where g_try_malloc()
returns NULL on failure.  g_malloc() matches qemu_malloc() exit
behavior today but in the future it would be possible to use
g_try_malloc() for proper out-of-memory handling.

But I think you're pointing out that glib uses g_malloc() internally.
That is a design flaw and basically makes g_try_malloc() useless.

Stefan

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Anthony Liguori]

On 01/26/2011 03:23 PM, Stefan Hajnoczi wrote:
>
> Both g_malloc() and g_try_malloc() are available where g_try_malloc()
> returns NULL on failure.  g_malloc() matches qemu_malloc() exit
> behavior today but in the future it would be possible to use
> g_try_malloc() for proper out-of-memory handling.
>
> But I think you're pointing out that glib uses g_malloc() internally.
> That is a design flaw and basically makes g_try_malloc() useless.
>    

No.  glib cannot be used for applications that want to handle malloc() 
robustly.

But I've never met an application that does so it doesn't really bother me.

Regards,

Anthony LIguori

> Stefan
>    

Re: [Qemu-devel] [RFC 0/7] Introduce hard dependency on glib

[Johannes Stezenbach]

Hi,

just a few thoughts from a qemu novice...

On Mon, Jan 24, 2011 at 03:00:38PM -0600, Anthony Liguori wrote:
> This series introduces a hard dependency on glib.  The initial use is portable
> threads but I see this as just the beginning.  Glib/Gobject offer many nice
> things including:
> 
>  - portable threads
>  - rich data structure support
>  - INI parser
>  - JSON parser
>  - generic type system
>  - object oriented infrastructure
>  - IO library
>  - module system
>  - introspection to enable support for dynamic language bindings
> 
> I see this series as the first step, followed by converting the I/O loop to
> a GMainLoop instance.  Once we're there, we can start making deeper use of
> GObjects including converting QDev to a GObject hierarchy.

My personal experience with the GMainLoop is that it adds
a lot of processing overhead compared to hand-written code.
However, this is from a few years back and was on an embedded
system, and I don't have numbers to back up my claim...
Looking at the GMainLoop code I'd say it's not written
for high performance, but on the plus side it handles cases
like recursive invocation and adding/removing event sources
to another thread's GMainLoop.

Anyway, in case you expect high performance you might
want to benchmark it against something like libevent.

> I've spent the past few months working on C++ integration for QEMU.  I'm more
> convinced than ever that we desperately in need of structured object oriented
> mechanisms to be successful but am pretty strongly convinced that incremental
> additional of C++ is not going to be successful.
> 
> On the other hand, while GObjects are uglier and require a lot of template code,
> there's more than enough structure that I think it can guide us into a much
> better object model implementation.
> 
> There is some ugliness.   GLib does not abstract signals because they're very
> non-portable but QEMU makes extensive use of signaling.  I don't think it's
> a major issue but some of the ugliness in this series is due to that fact.

I found working with GObjects by writing C code manually
extremely error prone and boring.  However, I hear Vala
is the new way if you're determined to use GObject:
http://live.gnome.org/Vala
(Note that I haven't used Vala myself, and this is not a recommendation
for Vala; what I want to say is: if Vala doesn't work for you
you'll have a lot of fun with GObject.)

Regarding dynamic language bindings:  One side effect of that is that
all arguments and return values from g_signal_emit() signal handlers
are marshaled via an elaborate scheme just in case there might be
a script language binding in the mix.  Again, this is clearly not
written for high performance.  I'm not sure this is relevant for
qemu, but you might want to take a look at Glib sources and do some
benchmarks before using to avoid unpleasant surprises.


Johannes