smp broken in tcg mode with --enable-io-thread?

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

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti <[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c +0 -0

 #if defined(TARGET_I386)
     if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

Quoted 9 lines · Marcelo Tosatti

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti <[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c int cpu_exec(CPUState *env1) · line 242

Quoted 8 lines

cpu-exec.c +1 -3

@@ -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...

Quoted 10 lines

cpu-exec.c +0 -0

 #if defined(TARGET_I386)
     if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

cpus.c void qemu_mutex_lock_iothread(void) · line 604

Quoted 7 lines

cpus.c +1 -0

@@ -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:

cpu-exec.c +1 -2

index 026980a..9f4191d 100644
@@ -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)

cpus.c +14 -8

index fcd0f09..2ce839d 100644
@@ -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

Quoted 42 lines · Jan Kiszka

Quoted 9 lines · Marcelo Tosatti

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti <[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c int cpu_exec(CPUState *env1) · line 242

Quoted 8 lines

cpu-exec.c +1 -3

@@ -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...

Quoted 10 lines

cpu-exec.c +0 -0

 #if defined(TARGET_I386)
     if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

cpus.c void qemu_mutex_lock_iothread(void) · line 604

Quoted 7 lines

cpus.c +1 -0

@@ -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.

Quoted 3 lines

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.

Quoted 41 lines · Jan Kiszka

Quoted 40 lines · Jan Kiszka

Quoted 9 lines · Marcelo Tosatti

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti <[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c int cpu_exec(CPUState *env1) · line 242

Quoted 8 lines

cpu-exec.c +1 -3

@@ -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...

Quoted 10 lines

cpu-exec.c +0 -0

 #if defined(TARGET_I386)
     if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

cpus.c void qemu_mutex_lock_iothread(void) · line 604

Quoted 7 lines

cpus.c +1 -0

@@ -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.

Quoted 3 lines

Quoted 3 lines

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:

cpu-exec.c +0 -1

index c776605..0149da5 100644
@@ -238,7 +238,6 @@ int cpu_exec(CPUState *env1)
 
     if (exit_request) {
         env->exit_request = 1;
-        exit_request = 0;
     }
 
 #if defined(TARGET_I386)

cpus.c +6 -1

index 826886c..14f7cfc 100644
@@ -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).

Quoted 45 lines · Marcelo Tosatti

Quoted 44 lines · Jan Kiszka

Quoted 43 lines · Jan Kiszka

Quoted 9 lines · Marcelo Tosatti

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti <[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c int cpu_exec(CPUState *env1) · line 242

Quoted 8 lines

cpu-exec.c +1 -3

@@ -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...

Quoted 10 lines

cpu-exec.c +0 -0

 #if defined(TARGET_I386)
     if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

cpus.c void qemu_mutex_lock_iothread(void) · line 604

Quoted 7 lines

cpus.c +1 -0

@@ -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.

Quoted 2 lines

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.

Quoted 57 lines · Jan Kiszka

Quoted 55 lines · Marcelo Tosatti

Quoted 46 lines · Jan Kiszka

Quoted 44 lines · Jan Kiszka

Quoted 9 lines · Marcelo Tosatti

Clear exit_request when iothread grabs the global lock.

Signed-off-by Marcelo Tosatti<[email protected]>

cpu-exec.c +0 -0

index 026980a..74cb973 100644

cpu-exec.c int cpu_exec(CPUState *env1) · line 241

Quoted 9 lines

cpu-exec.c +1 -3

@@ -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...

Quoted 11 lines

cpu-exec.c +0 -0

  #if defined(TARGET_I386)
      if (!kvm_enabled()) {

cpus.c +0 -0

index fcd0f09..ef1ab22 100644

cpus.c void qemu_mutex_lock_iothread(void) · line 604

Quoted 7 lines

cpus.c +1 -0

@@ -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.

Quoted 4 lines

Quoted 4 lines

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:

Quoted 4 lines

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

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).

Quoted 3 lines · Jan Kiszka

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:

cpu-exec.c +0 -0

index 026980a..bfc34e4 100644

Quoted 7 lines · Jan Kiszka

Quoted 3 lines · Jan Kiszka

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:

cpu-exec.c int cpu_exec(CPUState *env1) · line 602

Quoted 12 lines

cpu-exec.c +2 -2

index 026980a..bfc34e4 100644
@@ -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?