Patchwork [v2,upstream,07/22] add assertions on the owner of a QemuMutex

login
register
mail settings
Submitter Paolo Bonzini
Date Feb. 26, 2011, 3:40 p.m.
Message ID <1298734819-1960-8-git-send-email-pbonzini@redhat.com>
Download mbox | patch
Permalink /patch/84658/
State New
Headers show

Comments

Paolo Bonzini - Feb. 26, 2011, 3:40 p.m.
These are already present in the Win32 implementation, add them to
the pthread wrappers as well.  Use PTHREAD_MUTEX_ERRORCHECK for mutex
operations, and track the owner separately for cond_signal/broadcast.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 qemu-thread-posix.c |   23 +++++++++++++++++++++--
 qemu-thread-posix.h |    1 +
 2 files changed, 22 insertions(+), 2 deletions(-)
Jan Kiszka - Feb. 27, 2011, 9:33 a.m.
On 2011-02-26 16:40, Paolo Bonzini wrote:
> These are already present in the Win32 implementation, add them to
> the pthread wrappers as well.  Use PTHREAD_MUTEX_ERRORCHECK for mutex
> operations, and track the owner separately for cond_signal/broadcast.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-thread-posix.c |   23 +++++++++++++++++++++--
>  qemu-thread-posix.h |    1 +
>  2 files changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
> index e307773..a4c6e25 100644
> --- a/qemu-thread-posix.c
> +++ b/qemu-thread-posix.c
> @@ -16,9 +16,12 @@
>  #include <time.h>
>  #include <signal.h>
>  #include <stdint.h>
> +#include <assert.h>
>  #include <string.h>
>  #include "qemu-thread.h"
>  
> +static pthread_t pthread_null;
> +
>  static void error_exit(int err, const char *msg)
>  {
>      fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
> @@ -28,8 +31,13 @@ static void error_exit(int err, const char *msg)
>  void qemu_mutex_init(QemuMutex *mutex)
>  {
>      int err;
> +    pthread_mutexattr_t mutexattr;
>  
> -    err = pthread_mutex_init(&mutex->lock, NULL);
> +    mutex->owner = pthread_null;
> +    pthread_mutexattr_init(&mutexattr);
> +    pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK);
> +    err = pthread_mutex_init(&mutex->lock, &mutexattr);
> +    pthread_mutexattr_destroy(&mutexattr);
>      if (err)
>          error_exit(err, __func__);
>  }
> @@ -48,13 +56,20 @@ void qemu_mutex_lock(QemuMutex *mutex)
>      int err;
>  
>      err = pthread_mutex_lock(&mutex->lock);
> +    mutex->owner = pthread_self();
>      if (err)
>          error_exit(err, __func__);
>  }
>  
>  int qemu_mutex_trylock(QemuMutex *mutex)
>  {
> -    return pthread_mutex_trylock(&mutex->lock);
> +    int err;
> +    err = pthread_mutex_trylock(&mutex->lock);
> +    if (err == 0) {
> +        mutex->owner = pthread_self();
> +    }
> +
> +    return !!err;
>  }
>  
>  static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
> @@ -85,6 +100,7 @@ void qemu_mutex_unlock(QemuMutex *mutex)
>  {
>      int err;
>  
> +    mutex->owner = pthread_null;
>      err = pthread_mutex_unlock(&mutex->lock);
>      if (err)
>          error_exit(err, __func__);
> @@ -130,7 +146,10 @@ void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
>  {
>      int err;
>  
> +    assert(pthread_equal(mutex->owner, pthread_self()));
> +    mutex->owner = pthread_null;
>      err = pthread_cond_wait(&cond->cond, &mutex->lock);

Though POSIX is not 100% explicit on this, every sane pthread_cond_wait
implementation will apply the same error checking as on
pthread_mutex_unlock when the given mutex is of
PTHREAD_MUTEX_ERRORCHECK. So, this assert is actually redundant as well.

Now that we are left without any assertions, I start wondering about one
of the original missions: enforce qemu_cond_signal/broadcast to be
called under a mutex. What about extending those services with a mutex
argument and applying the assert there? Could become a static-inline
wrapper so that the argument is optimized away if assert() is inactive.

Jan
Paolo Bonzini - Feb. 27, 2011, 3:06 p.m.
On 02/27/2011 10:33 AM, Jan Kiszka wrote:
> Now that we are left without any assertions, I start wondering about one
> of the original missions: enforce qemu_cond_signal/broadcast to be
> called under a mutex. What about extending those services with a mutex
> argument and applying the assert there?

That is one of the patches in my queue that I haven't submitted yet. :)

Paolo

Patch

diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
index e307773..a4c6e25 100644
--- a/qemu-thread-posix.c
+++ b/qemu-thread-posix.c
@@ -16,9 +16,12 @@ 
 #include <time.h>
 #include <signal.h>
 #include <stdint.h>
+#include <assert.h>
 #include <string.h>
 #include "qemu-thread.h"
 
+static pthread_t pthread_null;
+
 static void error_exit(int err, const char *msg)
 {
     fprintf(stderr, "qemu: %s: %s\n", msg, strerror(err));
@@ -28,8 +31,13 @@  static void error_exit(int err, const char *msg)
 void qemu_mutex_init(QemuMutex *mutex)
 {
     int err;
+    pthread_mutexattr_t mutexattr;
 
-    err = pthread_mutex_init(&mutex->lock, NULL);
+    mutex->owner = pthread_null;
+    pthread_mutexattr_init(&mutexattr);
+    pthread_mutexattr_settype(&mutexattr, PTHREAD_MUTEX_ERRORCHECK);
+    err = pthread_mutex_init(&mutex->lock, &mutexattr);
+    pthread_mutexattr_destroy(&mutexattr);
     if (err)
         error_exit(err, __func__);
 }
@@ -48,13 +56,20 @@  void qemu_mutex_lock(QemuMutex *mutex)
     int err;
 
     err = pthread_mutex_lock(&mutex->lock);
+    mutex->owner = pthread_self();
     if (err)
         error_exit(err, __func__);
 }
 
 int qemu_mutex_trylock(QemuMutex *mutex)
 {
-    return pthread_mutex_trylock(&mutex->lock);
+    int err;
+    err = pthread_mutex_trylock(&mutex->lock);
+    if (err == 0) {
+        mutex->owner = pthread_self();
+    }
+
+    return !!err;
 }
 
 static void timespec_add_ms(struct timespec *ts, uint64_t msecs)
@@ -85,6 +100,7 @@  void qemu_mutex_unlock(QemuMutex *mutex)
 {
     int err;
 
+    mutex->owner = pthread_null;
     err = pthread_mutex_unlock(&mutex->lock);
     if (err)
         error_exit(err, __func__);
@@ -130,7 +146,10 @@  void qemu_cond_wait(QemuCond *cond, QemuMutex *mutex)
 {
     int err;
 
+    assert(pthread_equal(mutex->owner, pthread_self()));
+    mutex->owner = pthread_null;
     err = pthread_cond_wait(&cond->cond, &mutex->lock);
+    mutex->owner = pthread_self();
     if (err)
         error_exit(err, __func__);
 }
diff --git a/qemu-thread-posix.h b/qemu-thread-posix.h
index 7af371c..11978db 100644
--- a/qemu-thread-posix.h
+++ b/qemu-thread-posix.h
@@ -4,6 +4,7 @@ 
 
 struct QemuMutex {
     pthread_mutex_t lock;
+    pthread_t owner;
 };
 
 struct QemuCond {