diff mbox series

[RFC,v1,3/9] cpus: only take BQL for sleeping threads

Message ID 20170505103822.20641-4-alex.bennee@linaro.org
State New
Headers show
Series BQL and Replay Lock changes | expand

Commit Message

Alex Bennée May 5, 2017, 10:38 a.m. UTC 
Now the only real need to hold the BQL is for when we sleep on the
cpu->halt conditional. The lock is actually dropped while the thread
sleeps so the actual window for contention is pretty small. This also
means we can remove the special case hack for exclusive work and
simply declare that work no longer has an implicit BQL held. This
isn't a major problem async work is generally only changing things in
the context of its own vCPU. If it needs to work across vCPUs it
should be using the exclusive mechanism or possibly taking the lock
itself.

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

---
 cpus-common.c | 13 +++++--------
 cpus.c        | 10 ++++------
 2 files changed, 9 insertions(+), 14 deletions(-)

-- 
2.11.0

Comments

Paolo Bonzini May 5, 2017, 3:15 p.m. UTC | #1 
On 05/05/2017 12:38, Alex Bennée wrote:
>  

>      while (qemu_tcg_should_sleep(cpu)) {

> +        qemu_mutex_lock_iothread();

>          stop_tcg_kick_timer();

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

> +        qemu_mutex_unlock_iothread();

>      }


This is racy.  You need to recheck the condition under the lock, or to
switch from QemuCond to QemuEvent (then you still need to check the
condition twice, the second between qemu_event_reset and qemu_event_wait).

Paolo
Alex Bennée May 5, 2017, 3:28 p.m. UTC | #2 
Paolo Bonzini <pbonzini@redhat.com> writes:

> On 05/05/2017 12:38, Alex Bennée wrote:

>>

>>      while (qemu_tcg_should_sleep(cpu)) {

>> +        qemu_mutex_lock_iothread();

>>          stop_tcg_kick_timer();

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

>> +        qemu_mutex_unlock_iothread();

>>      }

>

> This is racy.  You need to recheck the condition under the lock, or to

> switch from QemuCond to QemuEvent (then you still need to check the

> condition twice, the second between qemu_event_reset and qemu_event_wait).



Doh of course, being a bit too eager there ;-)

--
Alex Bennée
diff mbox series

Patch

diff --git a/cpus-common.c b/cpus-common.c
index 59f751ecf9..64661c3193 100644
--- a/cpus-common.c
+++ b/cpus-common.c
@@ -310,6 +310,11 @@  void async_safe_run_on_cpu(CPUState *cpu, run_on_cpu_func func,
     queue_work_on_cpu(cpu, wi);
 }
 
+/* Work items run outside of the BQL. This is essential for avoiding a
+ * deadlock for exclusive work but also applies to non-exclusive work.
+ * If the work requires cross-vCPU changes then it should use the
+ * exclusive mechanism.
+ */
 void process_queued_cpu_work(CPUState *cpu)
 {
     struct qemu_work_item *wi;
@@ -327,17 +332,9 @@  void process_queued_cpu_work(CPUState *cpu)
         }
         qemu_mutex_unlock(&cpu->work_mutex);
         if (wi->exclusive) {
-            /* Running work items outside the BQL avoids the following deadlock:
-             * 1) start_exclusive() is called with the BQL taken while another
-             * CPU is running; 2) cpu_exec in the other CPU tries to takes the
-             * BQL, so it goes to sleep; start_exclusive() is sleeping too, so
-             * neither CPU can proceed.
-             */
-            qemu_mutex_unlock_iothread();
             start_exclusive();
             wi->func(cpu, wi->data);
             end_exclusive();
-            qemu_mutex_lock_iothread();
         } else {
             wi->func(cpu, wi->data);
         }
diff --git a/cpus.c b/cpus.c
index 89ae8cb30a..df279dd320 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1069,31 +1069,29 @@  static bool qemu_tcg_should_sleep(CPUState *cpu)
 
 static void qemu_tcg_wait_io_event(CPUState *cpu)
 {
-    qemu_mutex_lock_iothread();
 
     while (qemu_tcg_should_sleep(cpu)) {
+        qemu_mutex_lock_iothread();
         stop_tcg_kick_timer();
         qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_mutex_unlock_iothread();
     }
 
     start_tcg_kick_timer();
 
     qemu_wait_io_event_common(cpu);
-
-    qemu_mutex_unlock_iothread();
 }
 
 static void qemu_kvm_wait_io_event(CPUState *cpu)
 {
-    qemu_mutex_lock_iothread();
 
     while (cpu_thread_is_idle(cpu)) {
+        qemu_mutex_lock_iothread();
         qemu_cond_wait(cpu->halt_cond, &qemu_global_mutex);
+        qemu_mutex_unlock_iothread();
     }
 
     qemu_wait_io_event_common(cpu);
-
-    qemu_mutex_unlock_iothread();
 }
 
 static void *qemu_kvm_cpu_thread_fn(void *arg)