[Qemu-devel] smp broken in tcg mode with --enable-io-thread?

[Qemu-devel] smp broken in tcg mode with --enable-io-thread?

[Jan Kiszka]

Hi,

looks like SMP in emulation mode is broken once --enable-io-thread is
turned on. Linux SMP guests lock up early during boot, often the whole
QEMU process becomes uncontrollable after a while. That's at least the
case here with x86 targets. The problem disappears when using
-enable-kvm or building without --enable-io-thread.

It's not on my critical path (ie. no time to go into details), I just
stumbled over it and wanted to drop a note.

Jan

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

Re: [Qemu-devel] smp broken in tcg mode with --enable-io-thread?

[Anthony Liguori]

On 06/18/2010 11:37 AM, Jan Kiszka wrote:
> Hi,
>
> looks like SMP in emulation mode is broken once --enable-io-thread is
> turned on. Linux SMP guests lock up early during boot, often the whole
> QEMU process becomes uncontrollable after a while. That's at least the
> case here with x86 targets. The problem disappears when using
> -enable-kvm or building without --enable-io-thread.
>    

That's unfortunate.  I've been thinking we should enable io thread by 
default but this sort of regression would certainly prevent that.

Glauber/Marcelo, any guesses about what's happening?

Regards,

Anthony Liguori

> It's not on my critical path (ie. no time to go into details), I just
> stumbled over it and wanted to drop a note.
>
> Jan
>
>    

[Qemu-devel] [PATCH] fix smp with tcg mode and --enable-io-thread

[Marcelo Tosatti]

Clear exit_request when iothread grabs the global lock. 

Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>

diff --git a/cpu-exec.c b/cpu-exec.c
index 026980a..74cb973 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
     asm("");
     env = env1;
 
-    if (exit_request) {
+    if (exit_request)
         env->exit_request = 1;
-        exit_request = 0;
-    }
 
 #if defined(TARGET_I386)
     if (!kvm_enabled()) {
diff --git a/cpus.c b/cpus.c
index fcd0f09..ef1ab22 100644
--- a/cpus.c
+++ b/cpus.c
@@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
         }
         qemu_mutex_unlock(&qemu_fair_mutex);
     }
+   exit_request = 0;
 }
 
 void qemu_mutex_unlock_iothread(void)

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Marcelo Tosatti wrote:
> Clear exit_request when iothread grabs the global lock. 
> 
> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 026980a..74cb973 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
>      asm("");
>      env = env1;
>  
> -    if (exit_request) {
> +    if (exit_request)
>          env->exit_request = 1;
> -        exit_request = 0;
> -    }

Coding style...

>  
>  #if defined(TARGET_I386)
>      if (!kvm_enabled()) {
> diff --git a/cpus.c b/cpus.c
> index fcd0f09..ef1ab22 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
>          }
>          qemu_mutex_unlock(&qemu_fair_mutex);
>      }
> +   exit_request = 0;
>  }
>  
>  void qemu_mutex_unlock_iothread(void)
> 
> 

I looked into this a bit as well, and that's what I also have in my
queue.

But things are still widely broken: pause_all_vcpus and run_on_cpu as
there is no guarantee that all VCPUs regularly call into
qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.

The former issue is mostly fine with this patch:

diff --git a/cpu-exec.c b/cpu-exec.c
index 026980a..9f4191d 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -236,9 +236,8 @@ int cpu_exec(CPUState *env1)
     asm("");
     env = env1;
 
-    if (exit_request) {
+    if (unlikely(exit_request)) {
         env->exit_request = 1;
-        exit_request = 0;
     }
 
 #if defined(TARGET_I386)
diff --git a/cpus.c b/cpus.c
index fcd0f09..2ce839d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -39,7 +39,6 @@
 #define SIG_IPI SIGUSR1
 #endif
 
-static CPUState *cur_cpu;
 static CPUState *next_cpu;
 
 /***********************************************************/
@@ -331,6 +330,7 @@ int qemu_init_main_loop(void)
         return ret;
 
     qemu_cond_init(&qemu_pause_cond);
+    qemu_cond_init(&qemu_system_cond);
     qemu_mutex_init(&qemu_fair_mutex);
     qemu_mutex_init(&qemu_global_mutex);
     qemu_mutex_lock(&qemu_global_mutex);
@@ -401,10 +401,12 @@ static void qemu_wait_io_event_common(CPUState *env)
     flush_queued_work(env);
 }
 
-static void qemu_wait_io_event(CPUState *env)
+static void qemu_tcg_wait_io_event(void)
 {
+    CPUState *env;
+
     while (!tcg_has_work())
-        qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
+        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
 
     qemu_mutex_unlock(&qemu_global_mutex);
 
@@ -417,7 +419,10 @@ static void qemu_wait_io_event(CPUState *env)
     qemu_mutex_unlock(&qemu_fair_mutex);
 
     qemu_mutex_lock(&qemu_global_mutex);
-    qemu_wait_io_event_common(env);
+
+    for (env = first_cpu; env != NULL; env = env->next_cpu) {
+        qemu_wait_io_event_common(env);
+    }
 }
 
 static void qemu_kvm_eat_signal(CPUState *env, int timeout)
@@ -502,7 +507,7 @@ static void *tcg_cpu_thread_fn(void *arg)
 
     while (1) {
         tcg_cpu_exec();
-        qemu_wait_io_event(cur_cpu);
+        qemu_tcg_wait_io_event();
     }
 
     return NULL;
@@ -768,11 +773,11 @@ bool tcg_cpu_exec(void)
 
     if (next_cpu == NULL)
         next_cpu = first_cpu;
-    for (; next_cpu != NULL; next_cpu = next_cpu->next_cpu) {
-        CPUState *env = cur_cpu = next_cpu;
+    for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {
+        CPUState *env = next_cpu;
 
         qemu_clock_enable(vm_clock,
-                          (cur_cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);
+                          (env->singlestep_enabled & SSTEP_NOTIMER) == 0);
 
         if (qemu_alarm_pending())
             break;
@@ -787,6 +792,7 @@ bool tcg_cpu_exec(void)
             break;
         }
     }
+    exit_request = 0;
     return tcg_has_work();
 }
 

I haven't looked into the breakpoint bug yet as performance (likely
VCPU scheduling) in iothread mode is still suffering compared to classic
mode, specifically when you go up with the number of VCPUs (try -smp 8).
And there is some race that cause a lock up in qemu_mutex_lock_iothread
after a while (the cpu_unlink_tb seems to race with the linking - just a
guess so far).

Anyway, all this is getting terribly complex, hard to grasp, and maybe
therefore fragile as well. Specifically the SMP round-robin scheduling
looks like it requires a redesign for iothread mode (my feeling is it
only works by chance ATM). Part of the issue may not be new, but could
have been shadowed by the frequently interrupting I/O load when using a
single thread. The rest seems to lack a bit a user base...

Jan


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

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>> Clear exit_request when iothread grabs the global lock. 
>>
>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 026980a..74cb973 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
>>      asm("");
>>      env = env1;
>>  
>> -    if (exit_request) {
>> +    if (exit_request)
>>          env->exit_request = 1;
>> -        exit_request = 0;
>> -    }
> 
> Coding style...
> 
>>  
>>  #if defined(TARGET_I386)
>>      if (!kvm_enabled()) {
>> diff --git a/cpus.c b/cpus.c
>> index fcd0f09..ef1ab22 100644
>> --- a/cpus.c
>> +++ b/cpus.c
>> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
>>          }
>>          qemu_mutex_unlock(&qemu_fair_mutex);
>>      }
>> +   exit_request = 0;
>>  }
>>  
>>  void qemu_mutex_unlock_iothread(void)
>>
>>
> 
> I looked into this a bit as well, and that's what I also have in my
> queue.
> 
> But things are still widely broken: pause_all_vcpus and run_on_cpu as
> there is no guarantee that all VCPUs regularly call into
> qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.
> 
> The former issue is mostly fine with this patch:
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 026980a..9f4191d 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -236,9 +236,8 @@ int cpu_exec(CPUState *env1)
>      asm("");
>      env = env1;
>  
> -    if (exit_request) {
> +    if (unlikely(exit_request)) {
>          env->exit_request = 1;
> -        exit_request = 0;
>      }
>  
>  #if defined(TARGET_I386)
> diff --git a/cpus.c b/cpus.c
> index fcd0f09..2ce839d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -39,7 +39,6 @@
>  #define SIG_IPI SIGUSR1
>  #endif
>  
> -static CPUState *cur_cpu;
>  static CPUState *next_cpu;
>  
>  /***********************************************************/
> @@ -331,6 +330,7 @@ int qemu_init_main_loop(void)
>          return ret;
>  
>      qemu_cond_init(&qemu_pause_cond);
> +    qemu_cond_init(&qemu_system_cond);
>      qemu_mutex_init(&qemu_fair_mutex);
>      qemu_mutex_init(&qemu_global_mutex);
>      qemu_mutex_lock(&qemu_global_mutex);
> @@ -401,10 +401,12 @@ static void qemu_wait_io_event_common(CPUState *env)
>      flush_queued_work(env);
>  }
>  
> -static void qemu_wait_io_event(CPUState *env)
> +static void qemu_tcg_wait_io_event(void)
>  {
> +    CPUState *env;
> +
>      while (!tcg_has_work())
> -        qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
> +        qemu_cond_timedwait(tcg_halt_cond, &qemu_global_mutex, 1000);
>  
>      qemu_mutex_unlock(&qemu_global_mutex);
>  
> @@ -417,7 +419,10 @@ static void qemu_wait_io_event(CPUState *env)
>      qemu_mutex_unlock(&qemu_fair_mutex);
>  
>      qemu_mutex_lock(&qemu_global_mutex);
> -    qemu_wait_io_event_common(env);
> +
> +    for (env = first_cpu; env != NULL; env = env->next_cpu) {
> +        qemu_wait_io_event_common(env);
> +    }
>  }
>  
>  static void qemu_kvm_eat_signal(CPUState *env, int timeout)
> @@ -502,7 +507,7 @@ static void *tcg_cpu_thread_fn(void *arg)
>  
>      while (1) {
>          tcg_cpu_exec();
> -        qemu_wait_io_event(cur_cpu);
> +        qemu_tcg_wait_io_event();
>      }
>  
>      return NULL;
> @@ -768,11 +773,11 @@ bool tcg_cpu_exec(void)
>  
>      if (next_cpu == NULL)
>          next_cpu = first_cpu;
> -    for (; next_cpu != NULL; next_cpu = next_cpu->next_cpu) {
> -        CPUState *env = cur_cpu = next_cpu;
> +    for (; next_cpu != NULL && !exit_request; next_cpu = next_cpu->next_cpu) {
> +        CPUState *env = next_cpu;
>  
>          qemu_clock_enable(vm_clock,
> -                          (cur_cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);
> +                          (env->singlestep_enabled & SSTEP_NOTIMER) == 0);
>  
>          if (qemu_alarm_pending())
>              break;
> @@ -787,6 +792,7 @@ bool tcg_cpu_exec(void)
>              break;
>          }
>      }
> +    exit_request = 0;
>      return tcg_has_work();
>  }
>  
> 
> I haven't looked into the breakpoint bug yet as performance (likely
> VCPU scheduling) in iothread mode is still suffering compared to classic
> mode, specifically when you go up with the number of VCPUs (try -smp 8).

Hmm, retesting this, I cannot reproduce the performance regression
anymore. Likely I confused something or had instrumentations applied.

Locking up still works "reliably". :(

Jan

> And there is some race that cause a lock up in qemu_mutex_lock_iothread
> after a while (the cpu_unlink_tb seems to race with the linking - just a
> guess so far).
> 
> Anyway, all this is getting terribly complex, hard to grasp, and maybe
> therefore fragile as well. Specifically the SMP round-robin scheduling
> looks like it requires a redesign for iothread mode (my feeling is it
> only works by chance ATM). Part of the issue may not be new, but could
> have been shadowed by the frequently interrupting I/O load when using a
> single thread. The rest seems to lack a bit a user base...
> 
> Jan
> 



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

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Jan Kiszka wrote:
> And there is some race that cause a lock up in qemu_mutex_lock_iothread
> after a while (the cpu_unlink_tb seems to race with the linking - just a
> guess so far).

This seems to fix a long-standing race between cpu_exec and
signal-driven cpu_unlink_tb:

diff --git a/cpu-exec.c b/cpu-exec.c
index 026980a..bfc34e4 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -599,8 +598,9 @@ int cpu_exec(CPUState *env1)
                    TB, but before it is linked into a potentially
                    infinite loop and becomes env->current_tb. Avoid
                    starting execution if there is a pending interrupt. */
-                if (!unlikely (env->exit_request)) {
-                    env->current_tb = tb;
+                env->current_tb = tb;
+                asm("");
+                if (likely(!env->exit_request)) {
                     tc_ptr = tb->tc_ptr;
                 /* execute the generated code */
 #if defined(__sparc__) && !defined(CONFIG_SOLARIS)
@@ -609,7 +609,6 @@ int cpu_exec(CPUState *env1)
 #define env cpu_single_env
 #endif
                     next_tb = tcg_qemu_tb_exec(tc_ptr);
-                    env->current_tb = NULL;
                     if ((next_tb & 3) == 2) {
                         /* Instruction counter expired.  */
                         int insns_left;
@@ -638,6 +637,7 @@ int cpu_exec(CPUState *env1)
                         }
                     }
                 }
+                env->current_tb = NULL;
                 /* reset soft MMU for next block (it can currently
                    only be set by a memory fault) */
             } /* for(;;) */

Still testing, though.

Jan


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

Re: [Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Alexander Graf]

On 22.06.2010, at 00:13, Jan Kiszka wrote:

> Jan Kiszka wrote:
>> And there is some race that cause a lock up in qemu_mutex_lock_iothread
>> after a while (the cpu_unlink_tb seems to race with the linking - just a
>> guess so far).
> 
> This seems to fix a long-standing race between cpu_exec and
> signal-driven cpu_unlink_tb:
> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index 026980a..bfc34e4 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -599,8 +598,9 @@ int cpu_exec(CPUState *env1)
>                    TB, but before it is linked into a potentially
>                    infinite loop and becomes env->current_tb. Avoid
>                    starting execution if there is a pending interrupt. */
> -                if (!unlikely (env->exit_request)) {
> -                    env->current_tb = tb;
> +                env->current_tb = tb;
> +                asm("");

This is just barrier(), no?


Alex

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Marcelo Tosatti]

On Mon, Jun 21, 2010 at 10:58:32PM +0200, Jan Kiszka wrote:
> Jan Kiszka wrote:
> > Marcelo Tosatti wrote:
> >> Clear exit_request when iothread grabs the global lock. 
> >>
> >> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
> >>
> >> diff --git a/cpu-exec.c b/cpu-exec.c
> >> index 026980a..74cb973 100644
> >> --- a/cpu-exec.c
> >> +++ b/cpu-exec.c
> >> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
> >>      asm("");
> >>      env = env1;
> >>  
> >> -    if (exit_request) {
> >> +    if (exit_request)
> >>          env->exit_request = 1;
> >> -        exit_request = 0;
> >> -    }
> > 
> > Coding style...
> > 
> >>  
> >>  #if defined(TARGET_I386)
> >>      if (!kvm_enabled()) {
> >> diff --git a/cpus.c b/cpus.c
> >> index fcd0f09..ef1ab22 100644
> >> --- a/cpus.c
> >> +++ b/cpus.c
> >> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
> >>          }
> >>          qemu_mutex_unlock(&qemu_fair_mutex);
> >>      }
> >> +   exit_request = 0;
> >>  }
> >>  
> >>  void qemu_mutex_unlock_iothread(void)
> >>
> >>
> > 
> > I looked into this a bit as well, and that's what I also have in my
> > queue.
> > 
> > But things are still widely broken: pause_all_vcpus and run_on_cpu as
> > there is no guarantee that all VCPUs regularly call into
> > qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.

This fixes pause for me:


diff --git a/cpu-exec.c b/cpu-exec.c
index c776605..0149da5 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -238,7 +238,6 @@ int cpu_exec(CPUState *env1)
 
     if (exit_request) {
         env->exit_request = 1;
-        exit_request = 0;
     }
 
 #if defined(TARGET_I386)
diff --git a/cpus.c b/cpus.c
index 826886c..14f7cfc 100644
--- a/cpus.c
+++ b/cpus.c
@@ -403,6 +403,8 @@ static void qemu_wait_io_event_common(CPUState *env)
 
 static void qemu_wait_io_event(CPUState *env)
 {
+    CPUState *e;
+
     while (!tcg_has_work())
         qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
 
@@ -417,7 +419,9 @@ static void qemu_wait_io_event(CPUState *env)
     qemu_mutex_unlock(&qemu_fair_mutex);
 
     qemu_mutex_lock(&qemu_global_mutex);
-    qemu_wait_io_event_common(env);
+
+    for (e = first_cpu; e != NULL; e = e->next_cpu)
+        qemu_wait_io_event_common(e);
 }
 
 static void qemu_kvm_eat_signal(CPUState *env, int timeout)
@@ -614,6 +618,7 @@ void qemu_mutex_lock_iothread(void)
         }
         qemu_mutex_unlock(&qemu_fair_mutex);
     }
+   exit_request = 0;
 }
 
 void qemu_mutex_unlock_iothread(void)



Perhaps there is a similar problem with debugging (round robin 
in tcg_cpu_exec fails when there is a timer pending, and the 
iothread is not processing pending timers).

Re: [Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Alexander Graf wrote:
> On 22.06.2010, at 00:13, Jan Kiszka wrote:
> 
>> Jan Kiszka wrote:
>>> And there is some race that cause a lock up in qemu_mutex_lock_iothread
>>> after a while (the cpu_unlink_tb seems to race with the linking - just a
>>> guess so far).
>> This seems to fix a long-standing race between cpu_exec and
>> signal-driven cpu_unlink_tb:
>>
>> diff --git a/cpu-exec.c b/cpu-exec.c
>> index 026980a..bfc34e4 100644
>> --- a/cpu-exec.c
>> +++ b/cpu-exec.c
>> @@ -599,8 +598,9 @@ int cpu_exec(CPUState *env1)
>>                    TB, but before it is linked into a potentially
>>                    infinite loop and becomes env->current_tb. Avoid
>>                    starting execution if there is a pending interrupt. */
>> -                if (!unlikely (env->exit_request)) {
>> -                    env->current_tb = tb;
>> +                env->current_tb = tb;
>> +                asm("");
> 
> This is just barrier(), no?
> 

Yes, thoughtlessly copied from other places in cpu-exec.c. Guess it's
time to define this properly in qemu-barriers.h, also clobbering "memory".

Jan


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

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Marcelo Tosatti wrote:
> On Mon, Jun 21, 2010 at 10:58:32PM +0200, Jan Kiszka wrote:
>> Jan Kiszka wrote:
>>> Marcelo Tosatti wrote:
>>>> Clear exit_request when iothread grabs the global lock. 
>>>>
>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>
>>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>>> index 026980a..74cb973 100644
>>>> --- a/cpu-exec.c
>>>> +++ b/cpu-exec.c
>>>> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
>>>>      asm("");
>>>>      env = env1;
>>>>  
>>>> -    if (exit_request) {
>>>> +    if (exit_request)
>>>>          env->exit_request = 1;
>>>> -        exit_request = 0;
>>>> -    }
>>> Coding style...
>>>
>>>>  
>>>>  #if defined(TARGET_I386)
>>>>      if (!kvm_enabled()) {
>>>> diff --git a/cpus.c b/cpus.c
>>>> index fcd0f09..ef1ab22 100644
>>>> --- a/cpus.c
>>>> +++ b/cpus.c
>>>> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
>>>>          }
>>>>          qemu_mutex_unlock(&qemu_fair_mutex);
>>>>      }
>>>> +   exit_request = 0;
>>>>  }
>>>>  
>>>>  void qemu_mutex_unlock_iothread(void)
>>>>
>>>>
>>> I looked into this a bit as well, and that's what I also have in my
>>> queue.
>>>
>>> But things are still widely broken: pause_all_vcpus and run_on_cpu as
>>> there is no guarantee that all VCPUs regularly call into
>>> qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.
> 
> This fixes pause for me:
> 

Partially. It caused regressions on the SMP scheduling without the early
loop exit in my patch. I will break up my changes later today and post
them as series.

> 
> diff --git a/cpu-exec.c b/cpu-exec.c
> index c776605..0149da5 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -238,7 +238,6 @@ int cpu_exec(CPUState *env1)
>  
>      if (exit_request) {
>          env->exit_request = 1;
> -        exit_request = 0;
>      }
>  
>  #if defined(TARGET_I386)
> diff --git a/cpus.c b/cpus.c
> index 826886c..14f7cfc 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -403,6 +403,8 @@ static void qemu_wait_io_event_common(CPUState *env)
>  
>  static void qemu_wait_io_event(CPUState *env)
>  {
> +    CPUState *e;
> +
>      while (!tcg_has_work())
>          qemu_cond_timedwait(env->halt_cond, &qemu_global_mutex, 1000);
>  
> @@ -417,7 +419,9 @@ static void qemu_wait_io_event(CPUState *env)
>      qemu_mutex_unlock(&qemu_fair_mutex);
>  
>      qemu_mutex_lock(&qemu_global_mutex);
> -    qemu_wait_io_event_common(env);
> +
> +    for (e = first_cpu; e != NULL; e = e->next_cpu)
> +        qemu_wait_io_event_common(e);
>  }
>  
>  static void qemu_kvm_eat_signal(CPUState *env, int timeout)
> @@ -614,6 +618,7 @@ void qemu_mutex_lock_iothread(void)
>          }
>          qemu_mutex_unlock(&qemu_fair_mutex);
>      }
> +   exit_request = 0;
>  }
>  
>  void qemu_mutex_unlock_iothread(void)
> 
> 
> 
> Perhaps there is a similar problem with debugging (round robin 
> in tcg_cpu_exec fails when there is a timer pending, and the 
> iothread is not processing pending timers).
> 

Frankly, I still can't explain the round-robin logic. What complicates
the situation is that it currently has to work in both threading modes.
I really think we need a proper time-slicing model, maybe with early
yield if the guest runs on some pause instruction, ie. spins on a lock.

Jan


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

[Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Jan Kiszka]

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

Jan Kiszka wrote:
> Marcelo Tosatti wrote:
>> On Mon, Jun 21, 2010 at 10:58:32PM +0200, Jan Kiszka wrote:
>>> Jan Kiszka wrote:
>>>> Marcelo Tosatti wrote:
>>>>> Clear exit_request when iothread grabs the global lock. 
>>>>>
>>>>> Signed-off-by: Marcelo Tosatti <mtosatti@redhat.com>
>>>>>
>>>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>>>> index 026980a..74cb973 100644
>>>>> --- a/cpu-exec.c
>>>>> +++ b/cpu-exec.c
>>>>> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
>>>>>      asm("");
>>>>>      env = env1;
>>>>>  
>>>>> -    if (exit_request) {
>>>>> +    if (exit_request)
>>>>>          env->exit_request = 1;
>>>>> -        exit_request = 0;
>>>>> -    }
>>>> Coding style...
>>>>
>>>>>  
>>>>>  #if defined(TARGET_I386)
>>>>>      if (!kvm_enabled()) {
>>>>> diff --git a/cpus.c b/cpus.c
>>>>> index fcd0f09..ef1ab22 100644
>>>>> --- a/cpus.c
>>>>> +++ b/cpus.c
>>>>> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
>>>>>          }
>>>>>          qemu_mutex_unlock(&qemu_fair_mutex);
>>>>>      }
>>>>> +   exit_request = 0;
>>>>>  }
>>>>>  
>>>>>  void qemu_mutex_unlock_iothread(void)
>>>>>
>>>>>
>>>> I looked into this a bit as well, and that's what I also have in my
>>>> queue.
>>>>
>>>> But things are still widely broken: pause_all_vcpus and run_on_cpu as
>>>> there is no guarantee that all VCPUs regularly call into
>>>> qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.
>> This fixes pause for me:
>>
> 
> Partially. It caused regressions on the SMP scheduling without the early
> loop exit in my patch. I will break up my changes later today and post
> them as series.

After fixing the APIC/IOAPIC fallouts, the series is almost done.
Unfortunately, host&guest debugging is totally broken for
CONFIG_IOTHREAD (I also noticed that [1] is still not merged). I will
try to fix this first as it may require some more refactorings.

Jan

[1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/52718


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

Re: [Qemu-devel] Re: [PATCH] fix smp with tcg mode and --enable-io-thread

[Anthony Liguori]

On 06/23/2010 02:42 AM, Jan Kiszka wrote:
> Jan Kiszka wrote:
>    
>> Marcelo Tosatti wrote:
>>      
>>> On Mon, Jun 21, 2010 at 10:58:32PM +0200, Jan Kiszka wrote:
>>>        
>>>> Jan Kiszka wrote:
>>>>          
>>>>> Marcelo Tosatti wrote:
>>>>>            
>>>>>> Clear exit_request when iothread grabs the global lock.
>>>>>>
>>>>>> Signed-off-by: Marcelo Tosatti<mtosatti@redhat.com>
>>>>>>
>>>>>> diff --git a/cpu-exec.c b/cpu-exec.c
>>>>>> index 026980a..74cb973 100644
>>>>>> --- a/cpu-exec.c
>>>>>> +++ b/cpu-exec.c
>>>>>> @@ -236,10 +236,8 @@ int cpu_exec(CPUState *env1)
>>>>>>       asm("");
>>>>>>       env = env1;
>>>>>>
>>>>>> -    if (exit_request) {
>>>>>> +    if (exit_request)
>>>>>>           env->exit_request = 1;
>>>>>> -        exit_request = 0;
>>>>>> -    }
>>>>>>              
>>>>> Coding style...
>>>>>
>>>>>            
>>>>>>
>>>>>>   #if defined(TARGET_I386)
>>>>>>       if (!kvm_enabled()) {
>>>>>> diff --git a/cpus.c b/cpus.c
>>>>>> index fcd0f09..ef1ab22 100644
>>>>>> --- a/cpus.c
>>>>>> +++ b/cpus.c
>>>>>> @@ -598,6 +598,7 @@ void qemu_mutex_lock_iothread(void)
>>>>>>           }
>>>>>>           qemu_mutex_unlock(&qemu_fair_mutex);
>>>>>>       }
>>>>>> +   exit_request = 0;
>>>>>>   }
>>>>>>
>>>>>>   void qemu_mutex_unlock_iothread(void)
>>>>>>
>>>>>>
>>>>>>              
>>>>> I looked into this a bit as well, and that's what I also have in my
>>>>> queue.
>>>>>
>>>>> But things are still widely broken: pause_all_vcpus and run_on_cpu as
>>>>> there is no guarantee that all VCPUs regularly call into
>>>>> qemu_wait_io_event. Also breakpoints don't work, not only in SMP mode.
>>>>>            
>>> This fixes pause for me:
>>>
>>>        
>> Partially. It caused regressions on the SMP scheduling without the early
>> loop exit in my patch. I will break up my changes later today and post
>> them as series.
>>      
> After fixing the APIC/IOAPIC fallouts, the series is almost done.
> Unfortunately, host&guest debugging is totally broken for
> CONFIG_IOTHREAD (I also noticed that [1] is still not merged).

Did it not get applied to uq/master or has there just not been a merge 
request yet?

Regards,

Anthony LIguori

>   I will
> try to fix this first as it may require some more refactorings.
>
> Jan
>
> [1] http://thread.gmane.org/gmane.comp.emulators.kvm.devel/52718
>
>