[Qemu-devel] [PATCH 0/2] coroutine: Assertions and debugging aids

[Qemu-devel] [PATCH 0/2] coroutine: Assertions and debugging aids

[Kevin Wolf]

A while ago we were debugging a hang where coroutines were waiting for a mutex
to be unlocked, but we couldn't find out who held the lock. This series adds
some information to Coroutine and CoMutex that both allows to add a few
assertions to check locking behaviour and can be used to find the culprit when
analysing a core dump.

Kevin Wolf (2):
  coroutine: Let CoMutex remember who holds it
  coroutine: Assert that no locks are held on termination

 include/qemu/coroutine.h     |  1 +
 include/qemu/coroutine_int.h |  1 +
 util/qemu-coroutine-lock.c   | 14 ++++++++++++++
 util/qemu-coroutine.c        |  1 +
 4 files changed, 17 insertions(+)

-- 
1.8.3.1

[Qemu-devel] [PATCH 1/2] coroutine: Let CoMutex remember who holds it

[Kevin Wolf]

In cases of deadlocks, knowing who holds a given CoMutex is really
helpful for debugging. Keeping the information around doesn't cost much
and allows us to add another assertion to keep the code correct, so
let's just add it.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/coroutine.h   | 1 +
 util/qemu-coroutine-lock.c | 3 +++
 2 files changed, 4 insertions(+)

diff --git a/include/qemu/coroutine.h b/include/qemu/coroutine.h
index ac8d4c9..29a2078 100644
--- a/include/qemu/coroutine.h
+++ b/include/qemu/coroutine.h
@@ -143,6 +143,7 @@ bool qemu_co_queue_empty(CoQueue *queue);
  */
 typedef struct CoMutex {
     bool locked;
+    Coroutine *holder;
     CoQueue queue;
 } CoMutex;
 
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index 22aa9ab..f30ee81 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -129,6 +129,7 @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex)
     }
 
     mutex->locked = true;
+    mutex->holder = self;
 
     trace_qemu_co_mutex_lock_return(mutex, self);
 }
@@ -140,9 +141,11 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
     trace_qemu_co_mutex_unlock_entry(mutex, self);
 
     assert(mutex->locked == true);
+    assert(mutex->holder == self);
     assert(qemu_in_coroutine());
 
     mutex->locked = false;
+    mutex->holder = NULL;
     qemu_co_queue_next(&mutex->queue);
 
     trace_qemu_co_mutex_unlock_return(mutex, self);
-- 
1.8.3.1

[Qemu-devel] [PATCH 2/2] coroutine: Assert that no locks are held on termination

[Kevin Wolf]

A coroutine that takes a lock must also release it again. If the
coroutine terminates without having released all its locks, it's buggy
and we'll probably run into a deadlock sooner or later. Make sure that
we don't get such cases.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 include/qemu/coroutine_int.h |  1 +
 util/qemu-coroutine-lock.c   | 11 +++++++++++
 util/qemu-coroutine.c        |  1 +
 3 files changed, 13 insertions(+)

diff --git a/include/qemu/coroutine_int.h b/include/qemu/coroutine_int.h
index 581a7f5..6df9d33 100644
--- a/include/qemu/coroutine_int.h
+++ b/include/qemu/coroutine_int.h
@@ -39,6 +39,7 @@ struct Coroutine {
     void *entry_arg;
     Coroutine *caller;
     QSLIST_ENTRY(Coroutine) pool_next;
+    size_t locks_held;
 
     /* Coroutines that should be woken up when we yield or terminate */
     QSIMPLEQ_HEAD(, Coroutine) co_queue_wakeup;
diff --git a/util/qemu-coroutine-lock.c b/util/qemu-coroutine-lock.c
index f30ee81..14cf9ce 100644
--- a/util/qemu-coroutine-lock.c
+++ b/util/qemu-coroutine-lock.c
@@ -130,6 +130,7 @@ void coroutine_fn qemu_co_mutex_lock(CoMutex *mutex)
 
     mutex->locked = true;
     mutex->holder = self;
+    self->locks_held++;
 
     trace_qemu_co_mutex_lock_return(mutex, self);
 }
@@ -146,6 +147,7 @@ void coroutine_fn qemu_co_mutex_unlock(CoMutex *mutex)
 
     mutex->locked = false;
     mutex->holder = NULL;
+    self->locks_held--;
     qemu_co_queue_next(&mutex->queue);
 
     trace_qemu_co_mutex_unlock_return(mutex, self);
@@ -159,14 +161,19 @@ void qemu_co_rwlock_init(CoRwlock *lock)
 
 void qemu_co_rwlock_rdlock(CoRwlock *lock)
 {
+    Coroutine *self = qemu_coroutine_self();
+
     while (lock->writer) {
         qemu_co_queue_wait(&lock->queue);
     }
     lock->reader++;
+    self->locks_held++;
 }
 
 void qemu_co_rwlock_unlock(CoRwlock *lock)
 {
+    Coroutine *self = qemu_coroutine_self();
+
     assert(qemu_in_coroutine());
     if (lock->writer) {
         lock->writer = false;
@@ -179,12 +186,16 @@ void qemu_co_rwlock_unlock(CoRwlock *lock)
             qemu_co_queue_next(&lock->queue);
         }
     }
+    self->locks_held--;
 }
 
 void qemu_co_rwlock_wrlock(CoRwlock *lock)
 {
+    Coroutine *self = qemu_coroutine_self();
+
     while (lock->writer || lock->reader) {
         qemu_co_queue_wait(&lock->queue);
     }
     lock->writer = true;
+    self->locks_held++;
 }
diff --git a/util/qemu-coroutine.c b/util/qemu-coroutine.c
index 89f21a9..3cbf225 100644
--- a/util/qemu-coroutine.c
+++ b/util/qemu-coroutine.c
@@ -122,6 +122,7 @@ void qemu_coroutine_enter(Coroutine *co)
     case COROUTINE_YIELD:
         return;
     case COROUTINE_TERMINATE:
+        assert(!co->locks_held);
         trace_qemu_coroutine_terminate(co);
         coroutine_delete(co);
         return;
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH 0/2] coroutine: Assertions and debugging aids

[Paolo Bonzini]

> A while ago we were debugging a hang where coroutines were waiting for a
> mutex
> to be unlocked, but we couldn't find out who held the lock. This series adds
> some information to Coroutine and CoMutex that both allows to add a few
> assertions to check locking behaviour and can be used to find the culprit
> when analysing a core dump.
> 
> Kevin Wolf (2):
>   coroutine: Let CoMutex remember who holds it
>   coroutine: Assert that no locks are held on termination
> 
>  include/qemu/coroutine.h     |  1 +
>  include/qemu/coroutine_int.h |  1 +
>  util/qemu-coroutine-lock.c   | 14 ++++++++++++++
>  util/qemu-coroutine.c        |  1 +
>  4 files changed, 17 insertions(+)

Thanks for finally getting round to this.

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

Re: [Qemu-devel] [Qemu-block] [PATCH 0/2] coroutine: Assertions and debugging aids

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 839 bytes --]

On Thu, Aug 11, 2016 at 06:22:20PM +0200, Kevin Wolf wrote:
> A while ago we were debugging a hang where coroutines were waiting for a mutex
> to be unlocked, but we couldn't find out who held the lock. This series adds
> some information to Coroutine and CoMutex that both allows to add a few
> assertions to check locking behaviour and can be used to find the culprit when
> analysing a core dump.
> 
> Kevin Wolf (2):
>   coroutine: Let CoMutex remember who holds it
>   coroutine: Assert that no locks are held on termination
> 
>  include/qemu/coroutine.h     |  1 +
>  include/qemu/coroutine_int.h |  1 +
>  util/qemu-coroutine-lock.c   | 14 ++++++++++++++
>  util/qemu-coroutine.c        |  1 +
>  4 files changed, 17 insertions(+)
> 
> -- 
> 1.8.3.1
> 
> 

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [Qemu-devel] [PATCH 0/2] coroutine: Assertions and debugging aids

[Kevin Wolf]

Am 11.08.2016 um 18:22 hat Kevin Wolf geschrieben:
> A while ago we were debugging a hang where coroutines were waiting for a mutex
> to be unlocked, but we couldn't find out who held the lock. This series adds
> some information to Coroutine and CoMutex that both allows to add a few
> assertions to check locking behaviour and can be used to find the culprit when
> analysing a core dump.

Applied to block-next.

Kevin