From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from [140.186.70.92] (port=51534 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1PrSPU-0005WD-Ac
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 04:50:34 -0500
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <jan.kiszka@siemens.com>) id 1PrSPJ-0005H3-BO
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 04:50:18 -0500
Received: from thoth.sbs.de ([192.35.17.2]:26270)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <jan.kiszka@siemens.com>) id 1PrSPJ-0005GU-2j
	for qemu-devel@nongnu.org; Mon, 21 Feb 2011 04:50:17 -0500
Message-ID: <4D623556.7050204@siemens.com>
Date: Mon, 21 Feb 2011 10:50:14 +0100
From: Jan Kiszka <jan.kiszka@siemens.com>
MIME-Version: 1.0
References: <1298277820-8817-1-git-send-email-pbonzini@redhat.com>
	<1298277820-8817-8-git-send-email-pbonzini@redhat.com>
In-Reply-To: <1298277820-8817-8-git-send-email-pbonzini@redhat.com>
Content-Type: text/plain; charset=ISO-8859-15
Content-Transfer-Encoding: 7bit
Subject: [Qemu-devel] Re: [PATCH 07/21] add assertions on the owner of a
	QemuMutex
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Paolo Bonzini <pbonzini@redhat.com>
Cc: qemu-devel@nongnu.org

On 2011-02-21 09:43, Paolo Bonzini wrote:
> These are already present in the Win32 implementation, add them to
> the pthread wrappers as well.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  qemu-thread-posix.c |   27 +++++++++++++++++++++++++--
>  qemu-thread-posix.h |    1 +
>  2 files changed, 26 insertions(+), 2 deletions(-)
> 
> diff --git a/qemu-thread-posix.c b/qemu-thread-posix.c
> index 28b3f80..2176f81 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,11 @@ void qemu_mutex_unlock(QemuMutex *mutex)
>  {
>      int err;
>  
> +    /* An EDEADLOCK would arrive after we reset the owner.  So this
> +       assert is for ease of debugging (it lets you see what is the
> +       actual owner.  */

Don't get this. Why do you want to avoid the proper error detection of
pthread?

> +    assert(pthread_equal(mutex->owner, pthread_self()));
> +    mutex->owner = pthread_null;
>      err = pthread_mutex_unlock(&mutex->lock);
>      if (err)
>          error_exit(err, __func__);
> @@ -130,7 +150,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 {

You said that you want to add owner tracking for assertion in
cond_signal/broadcast. That's OK. But the assertions in the mutex layer
are redundant for PTHREAD_MUTEX_ERRORCHECK - unless I'm missing
something now.

Jan

-- 
Siemens AG, Corporate Technology, CT T DE IT 1
Corporate Competence Center Embedded Linux