diff mbox

[RFC,v3,17/19] tcg: enable thread-per-vCPU

Message ID 1464986428-6739-18-git-send-email-alex.bennee@linaro.org
State New
Headers show

Commit Message

Alex Bennée June 3, 2016, 8:40 p.m. UTC
There are a number of changes that occur at the same time here:

  - tb_lock is no longer a NOP for SoftMMU

  The tb_lock protects both translation and memory map structures. The
  debug assert is updated to reflect this.

  - introduce a single vCPU qemu_tcg_cpu_thread_fn

  One of these is spawned per vCPU with its own Thread and Condition
  variables. qemu_tcg_single_cpu_thread_fn is the new name for the old
  single threaded function.

  - the TLS current_cpu variable is now live for the lifetime of MTTCG
    vCPU threads. This is for future work where async jobs need to know
    the vCPU context they are operating in.

The user to switch on multi-thread behaviour and spawn a thread
per-vCPU. For a simple test like:

  ./arm/run ./arm/locking-test.flat -smp 4 -accel tcg,thread=multi

Will now use 4 vCPU threads and have an expected FAIL (instead of the
unexpected PASS) as the default mode of the test has no protection when
incrementing a shared variable.

However we still default to a single thread for all vCPUs as individual
front-end and back-ends need additional fixes to safely support:
  - atomic behaviour
  - tb invalidation
  - memory ordering

The function default_mttcg_enabled can be tweaked as support is added.

Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

[AJB: Some fixes, conditionally, commit rewording]
Signed-off-by: Alex Bennée <alex.bennee@linaro.org>


---
v1 (ajb):
  - fix merge conflicts
  - maintain single-thread approach
v2
  - re-base fixes (no longer has tb_find_fast lock tweak ahead)
  - remove bogus break condition on cpu->stop/stopped
  - only process exiting cpus exit_request
  - handle all cpus idle case (fixes shutdown issues)
  - sleep on EXCP_HALTED in mttcg mode (prevent crash on start-up)
  - move icount timer into helper
v3
  - update the commit message
  - rm kick_timer tweaks (move to earlier tcg_current_cpu tweaks)
  - ensure linux-user clears cpu->exit_request in loop
  - purging of global exit_request and tcg_current_cpu in earlier patches
  - fix checkpatch warnings
---
 cpu-exec.c        |   8 ----
 cpus.c            | 122 ++++++++++++++++++++++++++++++++++++++++--------------
 linux-user/main.c |   1 +
 translate-all.c   |  18 +++-----
 4 files changed, 98 insertions(+), 51 deletions(-)

-- 
2.7.4

Comments

Alex Bennée Aug. 10, 2016, 2:44 p.m. UTC | #1
Sergey Fedorov <serge.fdrv@gmail.com> writes:

> On 03/06/16 23:40, Alex Bennée wrote:

>> There are a number of changes that occur at the same time here:

>>

>>   - tb_lock is no longer a NOP for SoftMMU

>>

>>   The tb_lock protects both translation and memory map structures. The

>>   debug assert is updated to reflect this.

>

> This could be a separate patch.

>

> If we use tb_lock in system-mode to protect the structures protected by

> mmap_lock in user-mode then maybe we can merge those two locks because,

> as I remember, tb_lock in user-mode emulation is only held outside of

> mmap_lock for patching TB for direct jumps.


OK

>

>>

>>   - introduce a single vCPU qemu_tcg_cpu_thread_fn

>>

>>   One of these is spawned per vCPU with its own Thread and Condition

>>   variables. qemu_tcg_single_cpu_thread_fn is the new name for the old

>>   single threaded function.

>

> So we have 'tcg_current_rr_cpu' and 'qemu_cpu_kick_rr_cpu() at this

> moment, maybe name this function like qemu_tcg_rr_cpu_thread_fn()? ;)


OK

>

>>

>>   - the TLS current_cpu variable is now live for the lifetime of MTTCG

>>     vCPU threads. This is for future work where async jobs need to know

>>     the vCPU context they are operating in.

>

> This is important change because we set 'current_cpu' to NULL outside of

> cpu_exec() before, I wonder why.


It's hard to tell, it is not heavily defended. The number of places that
check current_cpu != NULL is fairly limited.

>

>>

>> The user to switch on multi-thread behaviour and spawn a thread

>> per-vCPU. For a simple test like:

>>

>>   ./arm/run ./arm/locking-test.flat -smp 4 -accel tcg,thread=multi

>

> It would be nice to mention that the simple test is from kvm_unit_tests.

>

>>

>> Will now use 4 vCPU threads and have an expected FAIL (instead of the

>> unexpected PASS) as the default mode of the test has no protection when

>> incrementing a shared variable.

>>

>> However we still default to a single thread for all vCPUs as individual

>> front-end and back-ends need additional fixes to safely support:

>>   - atomic behaviour

>>   - tb invalidation

>>   - memory ordering

>>

>> The function default_mttcg_enabled can be tweaked as support is added.

>>

>> Signed-off-by: KONRAD Frederic <fred.konrad@greensocs.com>

>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

>> [AJB: Some fixes, conditionally, commit rewording]

>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

>>

> (snip)

>> diff --git a/cpus.c b/cpus.c

>> index 35374fd..419caa2 100644

>> --- a/cpus.c

>> +++ b/cpus.c

> (snip)

>> @@ -1042,9 +1039,7 @@ static void qemu_tcg_wait_io_event(CPUState *cpu)

>>          qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);

>>      }

>>

>> -    CPU_FOREACH(cpu) {

>> -        qemu_wait_io_event_common(cpu);

>> -    }

>> +    qemu_wait_io_event_common(cpu);

>

> Is it okay for single-threaded CPU loop?

>

>>  }

>>

>>  static void qemu_kvm_wait_io_event(CPUState *cpu)

> (snip)

>> @@ -1331,6 +1324,69 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)

>>      return NULL;

>>  }

>>

>> +/* Multi-threaded TCG

>> + *

>> + * In the multi-threaded case each vCPU has its own thread. The TLS

>> + * variable current_cpu can be used deep in the code to find the

>> + * current CPUState for a given thread.

>> + */

>> +

>> +static void *qemu_tcg_cpu_thread_fn(void *arg)

>> +{

>> +    CPUState *cpu = arg;

>> +

>> +    rcu_register_thread();

>> +

>> +    qemu_mutex_lock_iothread();

>> +    qemu_thread_get_self(cpu->thread);

>> +

>> +    cpu->thread_id = qemu_get_thread_id();

>> +    cpu->created = true;

>> +    cpu->can_do_io = 1;

>> +    current_cpu = cpu;

>> +    qemu_cond_signal(&qemu_cpu_cond);

>> +

>> +    /* process any pending work */

>> +    atomic_mb_set(&cpu->exit_request, 1);

>> +

>> +    while (1) {

>> +        bool sleep = false;

>> +

>> +        if (cpu_can_run(cpu)) {

>> +            int r = tcg_cpu_exec(cpu);

>> +            switch (r) {

>> +            case EXCP_DEBUG:

>> +                cpu_handle_guest_debug(cpu);

>> +                break;

>> +            case EXCP_HALTED:

>> +                /* during start-up the vCPU is reset and the thread is

>> +                 * kicked several times. If we don't ensure we go back

>> +                 * to sleep in the halted state we won't cleanly

>> +                 * start-up when the vCPU is enabled.

>> +                 */

>> +                sleep = true;

>> +                break;

>> +            default:

>> +                /* Ignore everything else? */

>> +                break;

>> +            }

>> +        } else {

>> +            sleep = true;

>> +        }

>> +

>> +        handle_icount_deadline();

>> +

>> +        if (sleep) {

>> +            qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);

>> +        }

>> +

>> +        atomic_mb_set(&cpu->exit_request, 0);

>> +        qemu_tcg_wait_io_event(cpu);

>

> Do we really want to wait in qemu_tcg_wait_io_event() while

> "all_cpu_threads_idle()"?


I've cleaned up this logic.

>

>> +    }

>> +

>> +    return NULL;

>> +}

>> +

>>  static void qemu_cpu_kick_thread(CPUState *cpu)

>>  {

>>  #ifndef _WIN32

>> @@ -1355,7 +1411,7 @@ void qemu_cpu_kick(CPUState *cpu)

>>      qemu_cond_broadcast(cpu->halt_cond);

>>      if (tcg_enabled()) {

>>          cpu_exit(cpu);

>> -        /* Also ensure current RR cpu is kicked */

>> +        /* NOP unless doing single-thread RR */

>>          qemu_cpu_kick_rr_cpu();

>>      } else {

>>          qemu_cpu_kick_thread(cpu);

>> @@ -1422,13 +1478,6 @@ void pause_all_vcpus(void)

>>

>>      if (qemu_in_vcpu_thread()) {

>>          cpu_stop_current();

>> -        if (!kvm_enabled()) {

>> -            CPU_FOREACH(cpu) {

>> -                cpu->stop = false;

>> -                cpu->stopped = true;

>> -            }

>> -            return;

>> -        }

>

> I think this change is incompatible with single-threaded CPU loop as

> well.


Why, we already stop and kick the vCPU above so we will exit.

>

>>      }

>>

>>      while (!all_vcpus_paused()) {

>>

> (snip)

>

> Kind regards,

> Sergey



--
Alex Bennée
diff mbox

Patch

diff --git a/cpu-exec.c b/cpu-exec.c
index e1fb9ca..5ad3865 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -297,7 +297,6 @@  static TranslationBlock *tb_find_slow(CPUState *cpu,
         goto found;
     }
 
-#ifdef CONFIG_USER_ONLY
     /* mmap_lock is needed by tb_gen_code, and mmap_lock must be
      * taken outside tb_lock.  Since we're momentarily dropping
      * tb_lock, there's a chance that our desired tb has been
@@ -311,14 +310,11 @@  static TranslationBlock *tb_find_slow(CPUState *cpu,
         mmap_unlock();
         goto found;
     }
-#endif
 
     /* if no translated code available, then translate it now */
     tb = tb_gen_code(cpu, pc, cs_base, flags, 0);
 
-#ifdef CONFIG_USER_ONLY
     mmap_unlock();
-#endif
 
 found:
     /* we add the TB in the virtual pc hash table */
@@ -523,7 +519,6 @@  static inline void cpu_handle_interrupt(CPUState *cpu,
 
     }
     if (unlikely(cpu->exit_request || replay_has_interrupt())) {
-        cpu->exit_request = 0;
         cpu->exception_index = EXCP_INTERRUPT;
         cpu_loop_exit(cpu);
     }
@@ -661,8 +656,5 @@  int cpu_exec(CPUState *cpu)
     cc->cpu_exec_exit(cpu);
     rcu_read_unlock();
 
-    /* fail safe : never use current_cpu outside cpu_exec() */
-    current_cpu = NULL;
-
     return ret;
 }
diff --git a/cpus.c b/cpus.c
index 35374fd..419caa2 100644
--- a/cpus.c
+++ b/cpus.c
@@ -962,10 +962,7 @@  void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
 
     qemu_cpu_kick(cpu);
     while (!atomic_mb_read(&wi.done)) {
-        CPUState *self_cpu = current_cpu;
-
         qemu_cond_wait(&qemu_work_cond, &qemu_global_mutex);
-        current_cpu = self_cpu;
     }
 }
 
@@ -1027,13 +1024,13 @@  static void flush_queued_work(CPUState *cpu)
 
 static void qemu_wait_io_event_common(CPUState *cpu)
 {
+    atomic_mb_set(&cpu->thread_kicked, false);
     if (cpu->stop) {
         cpu->stop = false;
         cpu->stopped = true;
         qemu_cond_broadcast(&qemu_pause_cond);
     }
     flush_queued_work(cpu);
-    cpu->thread_kicked = false;
 }
 
 static void qemu_tcg_wait_io_event(CPUState *cpu)
@@ -1042,9 +1039,7 @@  static void qemu_tcg_wait_io_event(CPUState *cpu)
         qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
     }
 
-    CPU_FOREACH(cpu) {
-        qemu_wait_io_event_common(cpu);
-    }
+    qemu_wait_io_event_common(cpu);
 }
 
 static void qemu_kvm_wait_io_event(CPUState *cpu)
@@ -1111,6 +1106,7 @@  static void *qemu_dummy_cpu_thread_fn(void *arg)
     qemu_thread_get_self(cpu->thread);
     cpu->thread_id = qemu_get_thread_id();
     cpu->can_do_io = 1;
+    current_cpu = cpu;
 
     sigemptyset(&waitset);
     sigaddset(&waitset, SIG_IPI);
@@ -1119,9 +1115,7 @@  static void *qemu_dummy_cpu_thread_fn(void *arg)
     cpu->created = true;
     qemu_cond_signal(&qemu_cpu_cond);
 
-    current_cpu = cpu;
     while (1) {
-        current_cpu = NULL;
         qemu_mutex_unlock_iothread();
         do {
             int sig;
@@ -1132,7 +1126,6 @@  static void *qemu_dummy_cpu_thread_fn(void *arg)
             exit(1);
         }
         qemu_mutex_lock_iothread();
-        current_cpu = cpu;
         qemu_wait_io_event_common(cpu);
     }
 
@@ -1249,7 +1242,7 @@  static void kick_tcg_thread(void *opaque)
     qemu_cpu_kick_rr_cpu();
 }
 
-static void *qemu_tcg_cpu_thread_fn(void *arg)
+static void *qemu_tcg_single_cpu_thread_fn(void *arg)
 {
     CPUState *cpu = arg;
     QEMUTimer *kick_timer;
@@ -1331,6 +1324,69 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
     return NULL;
 }
 
+/* Multi-threaded TCG
+ *
+ * In the multi-threaded case each vCPU has its own thread. The TLS
+ * variable current_cpu can be used deep in the code to find the
+ * current CPUState for a given thread.
+ */
+
+static void *qemu_tcg_cpu_thread_fn(void *arg)
+{
+    CPUState *cpu = arg;
+
+    rcu_register_thread();
+
+    qemu_mutex_lock_iothread();
+    qemu_thread_get_self(cpu->thread);
+
+    cpu->thread_id = qemu_get_thread_id();
+    cpu->created = true;
+    cpu->can_do_io = 1;
+    current_cpu = cpu;
+    qemu_cond_signal(&qemu_cpu_cond);
+
+    /* process any pending work */
+    atomic_mb_set(&cpu->exit_request, 1);
+
+    while (1) {
+        bool sleep = false;
+
+        if (cpu_can_run(cpu)) {
+            int r = tcg_cpu_exec(cpu);
+            switch (r) {
+            case EXCP_DEBUG:
+                cpu_handle_guest_debug(cpu);
+                break;
+            case EXCP_HALTED:
+                /* during start-up the vCPU is reset and the thread is
+                 * kicked several times. If we don't ensure we go back
+                 * to sleep in the halted state we won't cleanly
+                 * start-up when the vCPU is enabled.
+                 */
+                sleep = true;
+                break;
+            default:
+                /* Ignore everything else? */
+                break;
+            }
+        } else {
+            sleep = true;
+        }
+
+        handle_icount_deadline();
+
+        if (sleep) {
+            qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        }
+
+        atomic_mb_set(&cpu->exit_request, 0);
+        qemu_tcg_wait_io_event(cpu);
+    }
+
+    return NULL;
+}
+
 static void qemu_cpu_kick_thread(CPUState *cpu)
 {
 #ifndef _WIN32
@@ -1355,7 +1411,7 @@  void qemu_cpu_kick(CPUState *cpu)
     qemu_cond_broadcast(cpu->halt_cond);
     if (tcg_enabled()) {
         cpu_exit(cpu);
-        /* Also ensure current RR cpu is kicked */
+        /* NOP unless doing single-thread RR */
         qemu_cpu_kick_rr_cpu();
     } else {
         qemu_cpu_kick_thread(cpu);
@@ -1422,13 +1478,6 @@  void pause_all_vcpus(void)
 
     if (qemu_in_vcpu_thread()) {
         cpu_stop_current();
-        if (!kvm_enabled()) {
-            CPU_FOREACH(cpu) {
-                cpu->stop = false;
-                cpu->stopped = true;
-            }
-            return;
-        }
     }
 
     while (!all_vcpus_paused()) {
@@ -1462,29 +1511,42 @@  void resume_all_vcpus(void)
 static void qemu_tcg_init_vcpu(CPUState *cpu)
 {
     char thread_name[VCPU_THREAD_NAME_SIZE];
-    static QemuCond *tcg_halt_cond;
-    static QemuThread *tcg_cpu_thread;
+    static QemuCond *single_tcg_halt_cond;
+    static QemuThread *single_tcg_cpu_thread;
 
-    /* share a single thread for all cpus with TCG */
-    if (!tcg_cpu_thread) {
+    if (qemu_tcg_mttcg_enabled() || !single_tcg_cpu_thread) {
         cpu->thread = g_malloc0(sizeof(QemuThread));
         cpu->halt_cond = g_malloc0(sizeof(QemuCond));
         qemu_cond_init(cpu->halt_cond);
-        tcg_halt_cond = cpu->halt_cond;
-        snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
+
+        if (qemu_tcg_mttcg_enabled()) {
+            /* create a thread per vCPU with TCG (MTTCG) */
+            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "CPU %d/TCG",
                  cpu->cpu_index);
-        qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
-                           cpu, QEMU_THREAD_JOINABLE);
+
+            qemu_thread_create(cpu->thread, thread_name, qemu_tcg_cpu_thread_fn,
+                               cpu, QEMU_THREAD_JOINABLE);
+
+        } else {
+            /* share a single thread for all cpus with TCG */
+            snprintf(thread_name, VCPU_THREAD_NAME_SIZE, "ALL CPUs/TCG");
+            qemu_thread_create(cpu->thread, thread_name,
+                               qemu_tcg_single_cpu_thread_fn,
+                               cpu, QEMU_THREAD_JOINABLE);
+
+            single_tcg_halt_cond = cpu->halt_cond;
+            single_tcg_cpu_thread = cpu->thread;
+        }
 #ifdef _WIN32
         cpu->hThread = qemu_thread_get_handle(cpu->thread);
 #endif
         while (!cpu->created) {
             qemu_cond_wait(&qemu_cpu_cond, &qemu_global_mutex);
         }
-        tcg_cpu_thread = cpu->thread;
     } else {
-        cpu->thread = tcg_cpu_thread;
-        cpu->halt_cond = tcg_halt_cond;
+        /* For non-MTTCG cases we share the thread */
+        cpu->thread = single_tcg_cpu_thread;
+        cpu->halt_cond = single_tcg_halt_cond;
     }
 }
 
diff --git a/linux-user/main.c b/linux-user/main.c
index b2bc6ab..522a1d7 100644
--- a/linux-user/main.c
+++ b/linux-user/main.c
@@ -207,6 +207,7 @@  static inline void cpu_exec_end(CPUState *cpu)
     }
     exclusive_idle();
     pthread_mutex_unlock(&exclusive_lock);
+    cpu->exit_request = false;
 }
 
 void cpu_list_lock(void)
diff --git a/translate-all.c b/translate-all.c
index 4bc5718..95e5284 100644
--- a/translate-all.c
+++ b/translate-all.c
@@ -83,7 +83,11 @@ 
 #endif
 
 #ifdef CONFIG_SOFTMMU
-#define assert_memory_lock() do { /* nothing */ } while (0)
+#define assert_memory_lock() do {           \
+        if (DEBUG_MEM_LOCKS) {              \
+            g_assert(have_tb_lock);         \
+        }                                   \
+    } while (0)
 #else
 #define assert_memory_lock() do {               \
         if (DEBUG_MEM_LOCKS) {                  \
@@ -147,36 +151,28 @@  static void *l1_map[V_L1_SIZE];
 TCGContext tcg_ctx;
 
 /* translation block context */
-#ifdef CONFIG_USER_ONLY
 __thread int have_tb_lock;
-#endif
 
 void tb_lock(void)
 {
-#ifdef CONFIG_USER_ONLY
     assert(!have_tb_lock);
     qemu_mutex_lock(&tcg_ctx.tb_ctx.tb_lock);
     have_tb_lock++;
-#endif
 }
 
 void tb_unlock(void)
 {
-#ifdef CONFIG_USER_ONLY
     assert(have_tb_lock);
     have_tb_lock--;
     qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
-#endif
 }
 
 void tb_lock_reset(void)
 {
-#ifdef CONFIG_USER_ONLY
     if (have_tb_lock) {
         qemu_mutex_unlock(&tcg_ctx.tb_ctx.tb_lock);
         have_tb_lock = 0;
     }
-#endif
 }
 
 #ifdef DEBUG_LOCKING
@@ -185,15 +181,11 @@  void tb_lock_reset(void)
 #define DEBUG_TB_LOCKS 0
 #endif
 
-#ifdef CONFIG_SOFTMMU
-#define assert_tb_lock() do { /* nothing */ } while (0)
-#else
 #define assert_tb_lock() do {               \
         if (DEBUG_TB_LOCKS) {               \
             g_assert(have_tb_lock);         \
         }                                   \
     } while (0)
-#endif
 
 
 static TranslationBlock *tb_find_pc(uintptr_t tc_ptr);