diff mbox series

[RFC,v4,12/23] cpus: push BQL lock to qemu_*_wait_io_event

Message ID 20180119084409.7100.23132.stgit@pasha-VirtualBox
State New
Headers show
Series None | expand

Commit Message

Pavel Dovgalyuk Jan. 19, 2018, 8:44 a.m. UTC 
From: Alex Bennée <alex.bennee@linaro.org>


We only really need to grab the lock for initial setup (so we don't
race with the thread-spawning thread). After that we can drop the lock
for the whole main loop and only grab it for waiting for IO events.

There is a slight wrinkle for the round-robin TCG thread as we also
expire timers which needs to be done under BQL as they are in the
main-loop.

This is stage one of reducing the lock impact as we can drop the
requirement of implicit BQL for async work and only grab the lock when
we need to sleep on the cpu->halt_cond.

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

Tested-by: Pavel Dovgalyuk <pavel.dovgaluk@ispras.ru>

---
 accel/kvm/kvm-all.c   |    4 ----
 cpus.c                |   22 +++++++++++++++-------
 dtc                   |    2 +-
 target/i386/hax-all.c |    2 --
 4 files changed, 16 insertions(+), 14 deletions(-)

Comments

Paolo Bonzini Jan. 19, 2018, 8:55 a.m. UTC | #1 
On 19/01/2018 09:44, Pavel Dovgalyuk wrote:
>  static void qemu_wait_io_event(CPUState *cpu)

>  {

> +    qemu_mutex_lock_iothread();

> +

>      while (cpu_thread_is_idle(cpu)) {

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

>      }


This function locks the iothread and never unlocks it.  Please test your
patches better.

> @@ -1190,6 +1194,8 @@ static void *qemu_kvm_cpu_thread_fn(void *arg)

>  

>      /* signal CPU creation */

>      cpu->created = true;

> +    qemu_mutex_unlock_iothread();

> +

>      qemu_cond_signal(&qemu_cpu_cond);

>  

>      do {


This function calls qemu_wait_io_event with BQL not taken.

> @@ -1232,10 +1238,10 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)

>  

>      /* signal CPU creation */

>      cpu->created = true;

> +    qemu_mutex_unlock_iothread();

>      qemu_cond_signal(&qemu_cpu_cond);

>  

>      while (1) {

> -        qemu_mutex_unlock_iothread();

>          do {

>              int sig;

>              r = sigwait(&waitset, &sig);

> @@ -1246,6 +1252,7 @@ static void *qemu_dummy_cpu_thread_fn(void *arg)

>          }

>          qemu_mutex_lock_iothread();

>          qemu_wait_io_event(cpu);

> +        qemu_mutex_unlock_iothread();


This function calls it with BQL taken.

Even without the mistakes, these changes are hard to follow.  If
possible, please separate the hax_vcpu_hax_exec and kvm_cpu_exec changes
to a separate patch.

Paolo
Pavel Dovgalyuk Jan. 19, 2018, 11:52 a.m. UTC | #2 
> From: Paolo Bonzini [mailto:pbonzini@redhat.com]

> On 19/01/2018 09:44, Pavel Dovgalyuk wrote:

> >  static void qemu_wait_io_event(CPUState *cpu)

> >  {

> > +    qemu_mutex_lock_iothread();

> > +

> >      while (cpu_thread_is_idle(cpu)) {

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

> >      }

> 

> This function locks the iothread and never unlocks it.  Please test your

> patches better.


Yes, there was kind of mess. Lock was inside the function and unlock outside,
that's why it worked for me.

Pavel Dovgalyuk
diff mbox series

Patch

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index f290f48..8d1d2c4 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -1857,9 +1857,7 @@  int kvm_cpu_exec(CPUState *cpu)
         return EXCP_HLT;
     }
 
-    qemu_mutex_unlock_iothread();
     cpu_exec_start(cpu);
-
     do {
         MemTxAttrs attrs;
 
@@ -1989,8 +1987,6 @@  int kvm_cpu_exec(CPUState *cpu)
     } while (ret == 0);
 
     cpu_exec_end(cpu);
-    qemu_mutex_lock_iothread();
-
     if (ret < 0) {
         cpu_dump_state(cpu, stderr, fprintf, CPU_DUMP_CODE);
         vm_stop(RUN_STATE_INTERNAL_ERROR);
diff --git a/cpus.c b/cpus.c
index 7b6ce74..ca86d9f 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1150,10 +1150,14 @@  static void qemu_tcg_rr_wait_io_event(CPUState *cpu)
     start_tcg_kick_timer();
 
     qemu_wait_io_event_common(cpu);
+
+    qemu_mutex_unlock_iothread();
 }
 
 static void qemu_wait_io_event(CPUState *cpu)
 {
+    qemu_mutex_lock_iothread();
+
     while (cpu_thread_is_idle(cpu)) {
         qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
     }
@@ -1190,6 +1194,8 @@  static void *qemu_kvm_cpu_thread_fn(void *arg)
 
     /* signal CPU creation */
     cpu->created = true;
+    qemu_mutex_unlock_iothread();
+
     qemu_cond_signal(&qemu_cpu_cond);
 
     do {
@@ -1232,10 +1238,10 @@  static void *qemu_dummy_cpu_thread_fn(void *arg)
 
     /* signal CPU creation */
     cpu->created = true;
+    qemu_mutex_unlock_iothread();
     qemu_cond_signal(&qemu_cpu_cond);
 
     while (1) {
-        qemu_mutex_unlock_iothread();
         do {
             int sig;
             r = sigwait(&waitset, &sig);
@@ -1246,6 +1252,7 @@  static void *qemu_dummy_cpu_thread_fn(void *arg)
         }
         qemu_mutex_lock_iothread();
         qemu_wait_io_event(cpu);
+        qemu_mutex_unlock_iothread();
     }
 
     return NULL;
@@ -1334,11 +1341,9 @@  static int tcg_cpu_exec(CPUState *cpu)
 #ifdef CONFIG_PROFILER
     ti = profile_getclock();
 #endif
-    qemu_mutex_unlock_iothread();
     cpu_exec_start(cpu);
     ret = cpu_exec(cpu);
     cpu_exec_end(cpu);
-    qemu_mutex_lock_iothread();
 #ifdef CONFIG_PROFILER
     tcg_time += profile_getclock() - ti;
 #endif
@@ -1398,6 +1403,7 @@  static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
             qemu_wait_io_event_common(cpu);
         }
     }
+    qemu_mutex_unlock_iothread();
 
     start_tcg_kick_timer();
 
@@ -1407,6 +1413,8 @@  static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
     cpu->exit_request = 1;
 
     while (1) {
+        qemu_mutex_lock_iothread();
+
         /* Account partial waits to QEMU_CLOCK_VIRTUAL.  */
         qemu_account_warp_timer();
 
@@ -1415,6 +1423,8 @@  static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
          */
         handle_icount_deadline();
 
+        qemu_mutex_unlock_iothread();
+
         if (!cpu) {
             cpu = first_cpu;
         }
@@ -1440,9 +1450,7 @@  static void *qemu_tcg_rr_cpu_thread_fn(void *arg)
                     cpu_handle_guest_debug(cpu);
                     break;
                 } else if (r == EXCP_ATOMIC) {
-                    qemu_mutex_unlock_iothread();
                     cpu_exec_step_atomic(cpu);
-                    qemu_mutex_lock_iothread();
                     break;
                 }
             } else if (cpu->stop) {
@@ -1483,6 +1491,7 @@  static void *qemu_hax_cpu_thread_fn(void *arg)
     current_cpu = cpu;
 
     hax_init_vcpu(cpu);
+    qemu_mutex_unlock_iothread();
     qemu_cond_signal(&qemu_cpu_cond);
 
     while (1) {
@@ -1569,6 +1578,7 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
     cpu->created = true;
     cpu->can_do_io = 1;
     current_cpu = cpu;
+    qemu_mutex_unlock_iothread();
     qemu_cond_signal(&qemu_cpu_cond);
 
     /* process any pending work */
@@ -1593,9 +1603,7 @@  static void *qemu_tcg_cpu_thread_fn(void *arg)
                 g_assert(cpu->halted);
                 break;
             case EXCP_ATOMIC:
-                qemu_mutex_unlock_iothread();
                 cpu_exec_step_atomic(cpu);
-                qemu_mutex_lock_iothread();
             default:
                 /* Ignore everything else? */
                 break;
diff --git a/dtc b/dtc
index e543880..558cd81 160000
--- a/dtc
+++ b/dtc
@@ -1 +1 @@ 
-Subproject commit e54388015af1fb4bf04d0bca99caba1074d9cc42
+Subproject commit 558cd81bdd432769b59bff01240c44f82cfb1a9d
diff --git a/target/i386/hax-all.c b/target/i386/hax-all.c
index 934ec4a..54b1fc7 100644
--- a/target/i386/hax-all.c
+++ b/target/i386/hax-all.c
@@ -513,11 +513,9 @@  static int hax_vcpu_hax_exec(CPUArchState *env)
 
         hax_vcpu_interrupt(env);
 
-        qemu_mutex_unlock_iothread();
         cpu_exec_start(cpu);
         hax_ret = hax_vcpu_run(vcpu);
         cpu_exec_end(cpu);
-        qemu_mutex_lock_iothread();
 
         /* Simply continue the vcpu_run if system call interrupted */
         if (hax_ret == -EINTR || hax_ret == -EAGAIN) {