diff mbox

[RFC,v3,13/19] tcg: rename tcg_current_cpu to tcg_current_rr_cpu

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

Commit Message

Alex Bennée June 3, 2016, 8:40 p.m. UTC 
..and make the definition local to cpus. In preparation for MTTCG the
concept of a global tcg_current_cpu will no longer make sense. However
we still need to keep track of it in the single-threaded case to be able
to exit quickly when required.

qemu_cpu_kick_no_halt() moves and becomes qemu_cpu_kick_rr_cpu() to
emphasise its use-case. qemu_cpu_kick now kicks the relevant cpu as
well as qemu_kick_rr_cpu() which will become a no-op in MTTCG.

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

---
 cpu-exec-common.c       |  1 -
 cpu-exec.c              |  3 ---
 cpus.c                  | 41 ++++++++++++++++++++++++-----------------
 include/exec/exec-all.h |  2 --
 4 files changed, 24 insertions(+), 23 deletions(-)

-- 
2.7.4

Comments

Alex Bennée June 7, 2016, 12:59 p.m. UTC | #1 
Alex Bennée <alex.bennee@linaro.org> writes:

> ..and make the definition local to cpus. In preparation for MTTCG the

> concept of a global tcg_current_cpu will no longer make sense. However

> we still need to keep track of it in the single-threaded case to be able

> to exit quickly when required.

>

> qemu_cpu_kick_no_halt() moves and becomes qemu_cpu_kick_rr_cpu() to

> emphasise its use-case. qemu_cpu_kick now kicks the relevant cpu as

> well as qemu_kick_rr_cpu() which will become a no-op in MTTCG.


I detected a failure case that a -smp 2 guest doesn't cleanly shutdown
in RR mode. My command line:

running command: ['/home/alex/lsrc/qemu/qemu.git/arm-softmmu/qemu-system-arm', '-machine', 'type=virt', '-display', 'none', '-smp', '1', '-m', '4096', '-cpu', 'cortex-a15', '-serial', 'telnet:127.0.0.1:4444', '-monitor', 'stdio', '-netdev', 'user,id=unet,hostfwd=tcp::2222-:22', '-device', 'virtio-net-device,netdev=unet', '-drive', 'file=/home/alex/lsrc/qemu/images/jessie-arm32.qcow2,id=myblock,index=0,if=none', '-device', 'virtio-blk-device,drive=myblock', '-append', 'console=ttyAMA0 systemd.unit=benchmark.service root=/dev/vda1', '-kernel', '/home/alex/lsrc/qemu/images/aarch32-current-linux-kernel-only.img', '-smp', '2'] (notty=False, with timeout)
command timed out!
run 1: ret=-1 (FALSE), time=20.000228 (0/1)
1 fails

(N.B. the duplicate -smp is just an artefact of my shell script glue.
The -smp 2 is the final smp that gets acted on. The benchmark.service is
just a simple systemd target that shuts the image down again.)

>

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

> ---

>  cpu-exec-common.c       |  1 -

>  cpu-exec.c              |  3 ---

>  cpus.c                  | 41 ++++++++++++++++++++++++-----------------

>  include/exec/exec-all.h |  2 --

>  4 files changed, 24 insertions(+), 23 deletions(-)

>

> diff --git a/cpu-exec-common.c b/cpu-exec-common.c

> index 132cd03..f42d24a 100644

> --- a/cpu-exec-common.c

> +++ b/cpu-exec-common.c

> @@ -24,7 +24,6 @@

>  #include "exec/memory-internal.h"

>

>  bool exit_request;

> -CPUState *tcg_current_cpu;

>

>  /* exit the current TB from a signal handler. The host registers are

>     restored in a state compatible with the CPU emulator

> diff --git a/cpu-exec.c b/cpu-exec.c

> index ae81e8e..68e804b 100644

> --- a/cpu-exec.c

> +++ b/cpu-exec.c

> @@ -597,7 +597,6 @@ int cpu_exec(CPUState *cpu)

>          return EXCP_HALTED;

>      }

>

> -    atomic_mb_set(&tcg_current_cpu, cpu);

>      rcu_read_lock();

>

>      if (unlikely(atomic_mb_read(&exit_request))) {

> @@ -658,7 +657,5 @@ int cpu_exec(CPUState *cpu)

>      /* fail safe : never use current_cpu outside cpu_exec() */

>      current_cpu = NULL;

>

> -    /* Does not need atomic_mb_set because a spurious wakeup is okay.  */

> -    atomic_set(&tcg_current_cpu, NULL);

>      return ret;

>  }

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

> index 12e04c9..a139ad3 100644

> --- a/cpus.c

> +++ b/cpus.c

> @@ -942,6 +942,7 @@ void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)

>  {

>      struct qemu_work_item wi;

>

> +    /* Always true when using tcg RR scheduling from a vCPU context */

>      if (qemu_cpu_is_self(cpu)) {

>          func(data);

>          return;

> @@ -1216,15 +1217,29 @@ static int tcg_cpu_exec(CPUState *cpu)

>   * This is done explicitly rather than relying on side-effects

>   * elsewhere.

>   */

> -static void qemu_cpu_kick_no_halt(void);

>  #define TCG_KICK_FREQ (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + \

>                         NANOSECONDS_PER_SECOND / 10)

>

> +/* only used in single-thread tcg mode */

> +static CPUState *tcg_current_rr_cpu;

> +

> +/* Kick the currently round-robin scheduled vCPU */

> +static void qemu_cpu_kick_rr_cpu(void)

> +{

> +    CPUState *cpu;

> +    do {

> +        cpu = atomic_mb_read(&tcg_current_rr_cpu);

> +        if (cpu) {

> +            cpu_exit(cpu);

> +        }

> +    } while (cpu != atomic_mb_read(&tcg_current_rr_cpu));

> +}

> +

>  static void kick_tcg_thread(void *opaque)

>  {

>      QEMUTimer *self = *(QEMUTimer **) opaque;

>      timer_mod(self, TCG_KICK_FREQ);

> -    qemu_cpu_kick_no_halt();

> +    qemu_cpu_kick_rr_cpu();

>  }

>

>  static void *qemu_tcg_cpu_thread_fn(void *arg)

> @@ -1275,6 +1290,7 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)

>          }

>

>          for (; cpu != NULL && !exit_request; cpu = CPU_NEXT(cpu)) {

> +            atomic_mb_set(&tcg_current_rr_cpu, cpu);

>

>              qemu_clock_enable(QEMU_CLOCK_VIRTUAL,

>                                (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);

> @@ -1290,6 +1306,8 @@ static void *qemu_tcg_cpu_thread_fn(void *arg)

>              }

>

>          } /* for cpu.. */

> +        /* Does not need atomic_mb_set because a spurious wakeup is okay.  */

> +        atomic_set(&tcg_current_rr_cpu, NULL);

>

>          /* Pairs with smp_wmb in qemu_cpu_kick.  */

>          atomic_mb_set(&exit_request, 0);

> @@ -1326,24 +1344,13 @@ static void qemu_cpu_kick_thread(CPUState *cpu)

>  #endif

>  }

>

> -static void qemu_cpu_kick_no_halt(void)

> -{

> -    CPUState *cpu;

> -    /* Ensure whatever caused the exit has reached the CPU threads before

> -     * writing exit_request.

> -     */

> -    atomic_mb_set(&exit_request, 1);

> -    cpu = atomic_mb_read(&tcg_current_cpu);

> -    if (cpu) {

> -        cpu_exit(cpu);

> -    }

> -}

> -

>  void qemu_cpu_kick(CPUState *cpu)

>  {

>      qemu_cond_broadcast(cpu->halt_cond);

>      if (tcg_enabled()) {

> -        qemu_cpu_kick_no_halt();

> +        cpu_exit(cpu);

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

> +        qemu_cpu_kick_rr_cpu();

>      } else {

>          qemu_cpu_kick_thread(cpu);

>      }

> @@ -1384,7 +1391,7 @@ void qemu_mutex_lock_iothread(void)

>          atomic_dec(&iothread_requesting_mutex);

>      } else {

>          if (qemu_mutex_trylock(&qemu_global_mutex)) {

> -            qemu_cpu_kick_no_halt();

> +            qemu_cpu_kick_rr_cpu();

>              qemu_mutex_lock(&qemu_global_mutex);

>          }

>          atomic_dec(&iothread_requesting_mutex);

> diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h

> index e30f02b..31f4c38 100644

> --- a/include/exec/exec-all.h

> +++ b/include/exec/exec-all.h

> @@ -405,8 +405,6 @@ bool memory_region_is_unassigned(MemoryRegion *mr);

>  /* vl.c */

>  extern int singlestep;

>

> -/* cpu-exec.c, accessed with atomic_mb_read/atomic_mb_set */

> -extern CPUState *tcg_current_cpu;

>  extern bool exit_request;

>

>  #endif



--
Alex Bennée
diff mbox

Patch

diff --git a/cpu-exec-common.c b/cpu-exec-common.c
index 132cd03..f42d24a 100644
--- a/cpu-exec-common.c
+++ b/cpu-exec-common.c
@@ -24,7 +24,6 @@ 
 #include "exec/memory-internal.h"
 
 bool exit_request;
-CPUState *tcg_current_cpu;
 
 /* exit the current TB from a signal handler. The host registers are
    restored in a state compatible with the CPU emulator
diff --git a/cpu-exec.c b/cpu-exec.c
index ae81e8e..68e804b 100644
--- a/cpu-exec.c
+++ b/cpu-exec.c
@@ -597,7 +597,6 @@  int cpu_exec(CPUState *cpu)
         return EXCP_HALTED;
     }
 
-    atomic_mb_set(&tcg_current_cpu, cpu);
     rcu_read_lock();
 
     if (unlikely(atomic_mb_read(&exit_request))) {
@@ -658,7 +657,5 @@  int cpu_exec(CPUState *cpu)
     /* fail safe : never use current_cpu outside cpu_exec() */
     current_cpu = NULL;
 
-    /* Does not need atomic_mb_set because a spurious wakeup is okay.  */
-    atomic_set(&tcg_current_cpu, NULL);
     return ret;
 }
diff --git a/cpus.c b/cpus.c
index 12e04c9..a139ad3 100644
--- a/cpus.c
+++ b/cpus.c
@@ -942,6 +942,7 @@  void run_on_cpu(CPUState *cpu, void (*func)(void *data), void *data)
 {
     struct qemu_work_item wi;
 
+    /* Always true when using tcg RR scheduling from a vCPU context */
     if (qemu_cpu_is_self(cpu)) {
         func(data);
         return;
@@ -1216,15 +1217,29 @@  static int tcg_cpu_exec(CPUState *cpu)
  * This is done explicitly rather than relying on side-effects
  * elsewhere.
  */
-static void qemu_cpu_kick_no_halt(void);
 #define TCG_KICK_FREQ (qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + \
                        NANOSECONDS_PER_SECOND / 10)
 
+/* only used in single-thread tcg mode */
+static CPUState *tcg_current_rr_cpu;
+
+/* Kick the currently round-robin scheduled vCPU */
+static void qemu_cpu_kick_rr_cpu(void)
+{
+    CPUState *cpu;
+    do {
+        cpu = atomic_mb_read(&tcg_current_rr_cpu);
+        if (cpu) {
+            cpu_exit(cpu);
+        }
+    } while (cpu != atomic_mb_read(&tcg_current_rr_cpu));
+}
+
 static void kick_tcg_thread(void *opaque)
 {
     QEMUTimer *self = *(QEMUTimer **) opaque;
     timer_mod(self, TCG_KICK_FREQ);
-    qemu_cpu_kick_no_halt();
+    qemu_cpu_kick_rr_cpu();
 }
 
 static void *qemu_tcg_cpu_thread_fn(void *arg)
@@ -1275,6 +1290,7 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
         }
 
         for (; cpu != NULL && !exit_request; cpu = CPU_NEXT(cpu)) {
+            atomic_mb_set(&tcg_current_rr_cpu, cpu);
 
             qemu_clock_enable(QEMU_CLOCK_VIRTUAL,
                               (cpu->singlestep_enabled & SSTEP_NOTIMER) == 0);
@@ -1290,6 +1306,8 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
             }
 
         } /* for cpu.. */
+        /* Does not need atomic_mb_set because a spurious wakeup is okay.  */
+        atomic_set(&tcg_current_rr_cpu, NULL);
 
         /* Pairs with smp_wmb in qemu_cpu_kick.  */
         atomic_mb_set(&exit_request, 0);
@@ -1326,24 +1344,13 @@  static void qemu_cpu_kick_thread(CPUState *cpu)
 #endif
 }
 
-static void qemu_cpu_kick_no_halt(void)
-{
-    CPUState *cpu;
-    /* Ensure whatever caused the exit has reached the CPU threads before
-     * writing exit_request.
-     */
-    atomic_mb_set(&exit_request, 1);
-    cpu = atomic_mb_read(&tcg_current_cpu);
-    if (cpu) {
-        cpu_exit(cpu);
-    }
-}
-
 void qemu_cpu_kick(CPUState *cpu)
 {
     qemu_cond_broadcast(cpu->halt_cond);
     if (tcg_enabled()) {
-        qemu_cpu_kick_no_halt();
+        cpu_exit(cpu);
+        /* Also ensure current RR cpu is kicked */
+        qemu_cpu_kick_rr_cpu();
     } else {
         qemu_cpu_kick_thread(cpu);
     }
@@ -1384,7 +1391,7 @@  void qemu_mutex_lock_iothread(void)
         atomic_dec(&iothread_requesting_mutex);
     } else {
         if (qemu_mutex_trylock(&qemu_global_mutex)) {
-            qemu_cpu_kick_no_halt();
+            qemu_cpu_kick_rr_cpu();
             qemu_mutex_lock(&qemu_global_mutex);
         }
         atomic_dec(&iothread_requesting_mutex);
diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index e30f02b..31f4c38 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -405,8 +405,6 @@  bool memory_region_is_unassigned(MemoryRegion *mr);
 /* vl.c */
 extern int singlestep;
 
-/* cpu-exec.c, accessed with atomic_mb_read/atomic_mb_set */
-extern CPUState *tcg_current_cpu;
 extern bool exit_request;
 
 #endif