[Qemu-devel] [PATCH v4 0/4] qemu-thread: support --enable-debug-mutex

[Qemu-devel] [PATCH v4 0/4] qemu-thread: support --enable-debug-mutex

[Peter Xu]

v4:
- cherrypick Emilio's patch to add "-m" parameter to atomic_add-bench
  tool.
- use inline functions [Emilio]

Micro benchmark result on "taskset -c 0 atomic_add-bench -m":

|--------+---------------------+------------------------|
|        | with --enable-debug | without --enable-debug |
|--------+---------------------+------------------------|
| Before | 20 Mops/s           | 34 Mops/s              |
| After  | 18 Mops/s           | 34 Mops/s              |
|--------+---------------------+------------------------|

v3:
- note down owner for every locking operations [Emilio]
- let Windows use it too [Emilio]
- added two more patches: patch 1 & 3.

Patch 1 generalize some common hooks for qemu-thread implementation
for both POSIX and Windows.

Patch 2 introduces the new debugging facility.

Patch 3 turns on mutex debugging automatically for "--enable-debug". I
suppose most developers are with that so we naturally benefit from it.

Please review.  Thanks.

Emilio G. Cota (1):
  tests/atomic_add-bench: add -m option to use mutexes

Peter Xu (3):
  qemu-thread: introduce qemu-thread-common.h
  QemuMutex: support --enable-debug-mutex
  configure: enable debug-mutex if debug enabled

 configure                   | 11 +++++++++++
 include/qemu/thread-posix.h |  4 ++++
 include/qemu/thread-win32.h |  4 ++++
 util/qemu-thread-common.h   | 46 +++++++++++++++++++++++++++++++++++++++++++++
 tests/atomic_add-bench.c    | 19 +++++++++++++++++--
 util/qemu-thread-posix.c    | 17 +++++++----------
 util/qemu-thread-win32.c    | 15 +++++++--------
 7 files changed, 96 insertions(+), 20 deletions(-)
 create mode 100644 util/qemu-thread-common.h

-- 
2.14.3

[Qemu-devel] [PATCH v4 1/4] tests/atomic_add-bench: add -m option to use mutexes

[Peter Xu]

From: "Emilio G. Cota" <cota@braap.org>

This allows us to use atomic-add-bench as a microbenchmark
for evaluating qemu_mutex_lock's performance.

Signed-off-by: Emilio G. Cota <cota@braap.org>
[cherry picked from https://github.com/cota/qemu/commit/f04f34df]
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 tests/atomic_add-bench.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/tests/atomic_add-bench.c b/tests/atomic_add-bench.c
index caa1e8e689..f96d448f77 100644
--- a/tests/atomic_add-bench.c
+++ b/tests/atomic_add-bench.c
@@ -8,6 +8,7 @@ struct thread_info {
 } QEMU_ALIGNED(64);
 
 struct count {
+    QemuMutex lock;
     unsigned long val;
 } QEMU_ALIGNED(64);
 
@@ -18,11 +19,13 @@ static unsigned int n_ready_threads;
 static struct count *counts;
 static unsigned int duration = 1;
 static unsigned int range = 1024;
+static bool use_mutex;
 static bool test_start;
 static bool test_stop;
 
 static const char commands_string[] =
     " -n = number of threads\n"
+    " -m = use mutexes instead of atomic increments\n"
     " -d = duration in seconds\n"
     " -r = range (will be rounded up to pow2)";
 
@@ -59,7 +62,13 @@ static void *thread_func(void *arg)
 
         info->r = xorshift64star(info->r);
         index = info->r & (range - 1);
-        atomic_inc(&counts[index].val);
+        if (use_mutex) {
+            qemu_mutex_lock(&counts[index].lock);
+            counts[index].val += 1;
+            qemu_mutex_unlock(&counts[index].lock);
+        } else {
+            atomic_inc(&counts[index].val);
+        }
     }
     return NULL;
 }
@@ -91,6 +100,9 @@ static void create_threads(void)
     th_info = g_new(struct thread_info, n_threads);
     counts = qemu_memalign(64, sizeof(*counts) * range);
     memset(counts, 0, sizeof(*counts) * range);
+    for (i = 0; i < range; i++) {
+        qemu_mutex_init(&counts[i].lock);
+    }
 
     for (i = 0; i < n_threads; i++) {
         struct thread_info *info = &th_info[i];
@@ -131,7 +143,7 @@ static void parse_args(int argc, char *argv[])
     int c;
 
     for (;;) {
-        c = getopt(argc, argv, "hd:n:r:");
+        c = getopt(argc, argv, "hd:n:mr:");
         if (c < 0) {
             break;
         }
@@ -145,6 +157,9 @@ static void parse_args(int argc, char *argv[])
         case 'n':
             n_threads = atoi(optarg);
             break;
+        case 'm':
+            use_mutex = true;
+            break;
         case 'r':
             range = pow2ceil(atoi(optarg));
             break;
-- 
2.14.3

[Qemu-devel] [PATCH v4 2/4] qemu-thread: introduce qemu-thread-common.h

[Peter Xu]

Introduce some hooks for the shared part of qemu thread between POSIX
and Windows implementations.  Note that in qemu_mutex_unlock_impl() we
moved the call before unlock operation which should make more sense.
And we don't need qemu_mutex_post_unlock() hook.

Currently the hooks only calls the tracepoints.

Put all these shared hooks into the header files.  It should be internal
to qemu-thread but not for qemu-thread users, hence put into util/
directory.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 util/qemu-thread-common.h | 38 ++++++++++++++++++++++++++++++++++++++
 util/qemu-thread-posix.c  | 17 +++++++----------
 util/qemu-thread-win32.c  | 15 +++++++--------
 3 files changed, 52 insertions(+), 18 deletions(-)
 create mode 100644 util/qemu-thread-common.h

diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
new file mode 100644
index 0000000000..324915fb80
--- /dev/null
+++ b/util/qemu-thread-common.h
@@ -0,0 +1,38 @@
+/*
+ * Common qemu-thread implementation header file.
+ *
+ * Copyright Red Hat, Inc. 2018
+ *
+ * Authors:
+ *  Peter Xu <peterx@redhat.com>,
+ *
+ * This work is licensed under the terms of the GNU GPL, version 2 or later.
+ * See the COPYING file in the top-level directory.
+ */
+
+#ifndef __QEMU_THREAD_COMMON_H__
+#define __QEMU_THREAD_COMMON_H__
+
+#include "qemu/typedefs.h"
+#include "qemu/thread.h"
+#include "trace.h"
+
+static inline void qemu_mutex_pre_lock(QemuMutex *mutex,
+                                       const char *file, int line)
+{
+    trace_qemu_mutex_lock(mutex, file, line);
+}
+
+static inline void qemu_mutex_post_lock(QemuMutex *mutex,
+                                        const char *file, int line)
+{
+    trace_qemu_mutex_locked(mutex, file, line);
+}
+
+static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
+                                         const char *file, int line)
+{
+    trace_qemu_mutex_unlock(mutex, file, line);
+}
+
+#endif
diff --git a/util/qemu-thread-posix.c b/util/qemu-thread-posix.c
index b789cf32e9..b0e7008db3 100644
--- a/util/qemu-thread-posix.c
+++ b/util/qemu-thread-posix.c
@@ -14,7 +14,7 @@
 #include "qemu/thread.h"
 #include "qemu/atomic.h"
 #include "qemu/notify.h"
-#include "trace.h"
+#include "qemu-thread-common.h"
 
 static bool name_threads;
 
@@ -62,13 +62,11 @@ void qemu_mutex_lock_impl(QemuMutex *mutex, const char *file, const int line)
     int err;
 
     assert(mutex->initialized);
-    trace_qemu_mutex_lock(mutex, file, line);
-
+    qemu_mutex_pre_lock(mutex, file, line);
     err = pthread_mutex_lock(&mutex->lock);
     if (err)
         error_exit(err, __func__);
-
-    trace_qemu_mutex_locked(mutex, file, line);
+    qemu_mutex_post_lock(mutex, file, line);
 }
 
 int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
@@ -78,7 +76,7 @@ int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
     assert(mutex->initialized);
     err = pthread_mutex_trylock(&mutex->lock);
     if (err == 0) {
-        trace_qemu_mutex_locked(mutex, file, line);
+        qemu_mutex_post_lock(mutex, file, line);
         return 0;
     }
     if (err != EBUSY) {
@@ -92,11 +90,10 @@ void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int line)
     int err;
 
     assert(mutex->initialized);
+    qemu_mutex_pre_unlock(mutex, file, line);
     err = pthread_mutex_unlock(&mutex->lock);
     if (err)
         error_exit(err, __func__);
-
-    trace_qemu_mutex_unlock(mutex, file, line);
 }
 
 void qemu_rec_mutex_init(QemuRecMutex *mutex)
@@ -160,9 +157,9 @@ void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, con
     int err;
 
     assert(cond->initialized);
-    trace_qemu_mutex_unlock(mutex, file, line);
+    qemu_mutex_pre_unlock(mutex, file, line);
     err = pthread_cond_wait(&cond->cond, &mutex->lock);
-    trace_qemu_mutex_locked(mutex, file, line);
+    qemu_mutex_post_lock(mutex, file, line);
     if (err)
         error_exit(err, __func__);
 }
diff --git a/util/qemu-thread-win32.c b/util/qemu-thread-win32.c
index ab60c0d557..f9fb0581f2 100644
--- a/util/qemu-thread-win32.c
+++ b/util/qemu-thread-win32.c
@@ -19,7 +19,7 @@
 #include "qemu-common.h"
 #include "qemu/thread.h"
 #include "qemu/notify.h"
-#include "trace.h"
+#include "qemu-thread-common.h"
 #include <process.h>
 
 static bool name_threads;
@@ -59,10 +59,9 @@ void qemu_mutex_destroy(QemuMutex *mutex)
 void qemu_mutex_lock_impl(QemuMutex *mutex, const char *file, const int line)
 {
     assert(mutex->initialized);
-    trace_qemu_mutex_lock(mutex, file, line);
-
+    qemu_mutex_pre_lock(mutex, file, line);
     AcquireSRWLockExclusive(&mutex->lock);
-    trace_qemu_mutex_locked(mutex, file, line);
+    qemu_mutex_post_lock(mutex, file, line);
 }
 
 int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
@@ -72,7 +71,7 @@ int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
     assert(mutex->initialized);
     owned = TryAcquireSRWLockExclusive(&mutex->lock);
     if (owned) {
-        trace_qemu_mutex_locked(mutex, file, line);
+        qemu_mutex_post_lock(mutex, file, line);
         return 0;
     }
     return -EBUSY;
@@ -81,7 +80,7 @@ int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
 void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int line)
 {
     assert(mutex->initialized);
-    trace_qemu_mutex_unlock(mutex, file, line);
+    qemu_mutex_pre_unlock(mutex, file, line);
     ReleaseSRWLockExclusive(&mutex->lock);
 }
 
@@ -145,9 +144,9 @@ void qemu_cond_broadcast(QemuCond *cond)
 void qemu_cond_wait_impl(QemuCond *cond, QemuMutex *mutex, const char *file, const int line)
 {
     assert(cond->initialized);
-    trace_qemu_mutex_unlock(mutex, file, line);
+    qemu_mutex_pre_unlock(mutex, file, line);
     SleepConditionVariableSRW(&cond->var, &mutex->lock, INFINITE, 0);
-    trace_qemu_mutex_locked(mutex, file, line);
+    qemu_mutex_post_lock(mutex, file, line);
 }
 
 void qemu_sem_init(QemuSemaphore *sem, int init)
-- 
2.14.3

[Qemu-devel] [PATCH v4 3/4] QemuMutex: support --enable-debug-mutex

[Peter Xu]

We have had some tracing tools for mutex but it's not easy to use them
for e.g. dead locks.  Let's provide "--enable-debug-mutex" parameter
when configure to allow QemuMutex to store the last owner that took
specific lock.  It will be easy to use this tool to debug deadlocks
since we can directly know who took the lock then as long as we can have
a debugger attached to the process.

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 configure                   | 10 ++++++++++
 include/qemu/thread-posix.h |  4 ++++
 include/qemu/thread-win32.h |  4 ++++
 util/qemu-thread-common.h   |  8 ++++++++
 4 files changed, 26 insertions(+)

diff --git a/configure b/configure
index 0a19b033bc..a80af735b2 100755
--- a/configure
+++ b/configure
@@ -451,6 +451,7 @@ jemalloc="no"
 replication="yes"
 vxhs=""
 libxml2=""
+debug_mutex="no"
 
 supported_cpu="no"
 supported_os="no"
@@ -1374,6 +1375,10 @@ for opt do
   ;;
   --disable-git-update) git_update=no
   ;;
+  --enable-debug-mutex) debug_mutex=yes
+  ;;
+  --disable-debug-mutex) debug_mutex=no
+  ;;
   *)
       echo "ERROR: unknown option $opt"
       echo "Try '$0 --help' for more information"
@@ -1631,6 +1636,7 @@ disabled with --disable-FEATURE, default is enabled if available:
   crypto-afalg    Linux AF_ALG crypto backend driver
   vhost-user      vhost-user support
   capstone        capstone disassembler support
+  debug-mutex     mutex debugging support
 
 NOTE: The object files are built at the place where configure is launched
 EOF
@@ -5874,6 +5880,7 @@ echo "avx2 optimization $avx2_opt"
 echo "replication support $replication"
 echo "VxHS block device $vxhs"
 echo "capstone          $capstone"
+echo "mutex debugging   $debug_mutex"
 
 if test "$sdl_too_old" = "yes"; then
 echo "-> Your SDL version is too old - please upgrade to have SDL support"
@@ -6602,6 +6609,9 @@ fi
 if test "$capstone" != "no" ; then
   echo "CONFIG_CAPSTONE=y" >> $config_host_mak
 fi
+if test "$debug_mutex" = "yes" ; then
+  echo "CONFIG_DEBUG_MUTEX=y" >> $config_host_mak
+fi
 
 # Hold two types of flag:
 #   CONFIG_THREAD_SETNAME_BYTHREAD  - we've got a way of setting the name on
diff --git a/include/qemu/thread-posix.h b/include/qemu/thread-posix.h
index f3f47e426f..fd27b34128 100644
--- a/include/qemu/thread-posix.h
+++ b/include/qemu/thread-posix.h
@@ -12,6 +12,10 @@ typedef QemuMutex QemuRecMutex;
 
 struct QemuMutex {
     pthread_mutex_t lock;
+#ifdef CONFIG_DEBUG_MUTEX
+    const char *file;
+    int line;
+#endif
     bool initialized;
 };
 
diff --git a/include/qemu/thread-win32.h b/include/qemu/thread-win32.h
index 3a05e3b3aa..d668d789b4 100644
--- a/include/qemu/thread-win32.h
+++ b/include/qemu/thread-win32.h
@@ -5,6 +5,10 @@
 
 struct QemuMutex {
     SRWLOCK lock;
+#ifdef CONFIG_DEBUG_MUTEX
+    const char *file;
+    int line;
+#endif
     bool initialized;
 };
 
diff --git a/util/qemu-thread-common.h b/util/qemu-thread-common.h
index 324915fb80..90845842f2 100644
--- a/util/qemu-thread-common.h
+++ b/util/qemu-thread-common.h
@@ -26,12 +26,20 @@ static inline void qemu_mutex_pre_lock(QemuMutex *mutex,
 static inline void qemu_mutex_post_lock(QemuMutex *mutex,
                                         const char *file, int line)
 {
+#ifdef CONFIG_DEBUG_MUTEX
+    mutex->file = file;
+    mutex->line = line;
+#endif
     trace_qemu_mutex_locked(mutex, file, line);
 }
 
 static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
                                          const char *file, int line)
 {
+#ifdef CONFIG_DEBUG_MUTEX
+    mutex->file = NULL;
+    mutex->line = 0;
+#endif
     trace_qemu_mutex_unlock(mutex, file, line);
 }
 
-- 
2.14.3

[Qemu-devel] [PATCH v4 4/4] configure: enable debug-mutex if debug enabled

[Peter Xu]

Signed-off-by: Peter Xu <peterx@redhat.com>
---
 configure | 1 +
 1 file changed, 1 insertion(+)

diff --git a/configure b/configure
index a80af735b2..87453edd88 100755
--- a/configure
+++ b/configure
@@ -1005,6 +1005,7 @@ for opt do
   --enable-debug)
       # Enable debugging options that aren't excessively noisy
       debug_tcg="yes"
+      debug_mutex="yes"
       debug="yes"
       strip_opt="no"
       fortify_source="no"
-- 
2.14.3

Re: [Qemu-devel] [PATCH v4 2/4] qemu-thread: introduce qemu-thread-common.h

[Emilio G. Cota]

On Mon, Apr 23, 2018 at 13:39:25 +0800, Peter Xu wrote:
> Introduce some hooks for the shared part of qemu thread between POSIX
> and Windows implementations.  Note that in qemu_mutex_unlock_impl() we
> moved the call before unlock operation which should make more sense.
> And we don't need qemu_mutex_post_unlock() hook.
> 
> Currently the hooks only calls the tracepoints.
> 
> Put all these shared hooks into the header files.  It should be internal
> to qemu-thread but not for qemu-thread users, hence put into util/
> directory.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
(snip)
> @@ -92,11 +90,10 @@ void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int line)
>      int err;
>  
>      assert(mutex->initialized);
> +    qemu_mutex_pre_unlock(mutex, file, line);
>      err = pthread_mutex_unlock(&mutex->lock);
>      if (err)
>          error_exit(err, __func__);
> -
> -    trace_qemu_mutex_unlock(mutex, file, line);
>  }
(snip)
> @@ -81,7 +80,7 @@ int qemu_mutex_trylock_impl(QemuMutex *mutex, const char *file, const int line)
>  void qemu_mutex_unlock_impl(QemuMutex *mutex, const char *file, const int line)
>  {
>      assert(mutex->initialized);
> -    trace_qemu_mutex_unlock(mutex, file, line);
> +    qemu_mutex_pre_unlock(mutex, file, line);
>      ReleaseSRWLockExclusive(&mutex->lock);
>  }

Note that in posix we're moving the unlock tracepoint before the actual
unlock happens. But that makes it consistent with that we're doing in win32,
and AFAICT it seems a better place to do it, so:

Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		E.

Re: [Qemu-devel] [PATCH v4 3/4] QemuMutex: support --enable-debug-mutex

[Emilio G. Cota]

On Mon, Apr 23, 2018 at 13:39:26 +0800, Peter Xu wrote:
> We have had some tracing tools for mutex but it's not easy to use them
> for e.g. dead locks.  Let's provide "--enable-debug-mutex" parameter
> when configure to allow QemuMutex to store the last owner that took
> specific lock.  It will be easy to use this tool to debug deadlocks
> since we can directly know who took the lock then as long as we can have
> a debugger attached to the process.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
(snip)
>  static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
>                                           const char *file, int line)
>  {
> +#ifdef CONFIG_DEBUG_MUTEX
> +    mutex->file = NULL;
> +    mutex->line = 0;
> +#endif
>      trace_qemu_mutex_unlock(mutex, file, line);
>  }

I'd also initialize to NULL/0 the file/line pair at
qemu_mutex_init time for both posix and win32. Other than that:

Reviewed-by: Emilio G. Cota <cota@braap.org>

Thanks,

		E.

Re: [Qemu-devel] [PATCH v4 4/4] configure: enable debug-mutex if debug enabled

[Emilio G. Cota]

On Mon, Apr 23, 2018 at 13:39:27 +0800, Peter Xu wrote:
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Emilio G. Cota <cota@braap.org>

		E.

Re: [Qemu-devel] [PATCH v4 3/4] QemuMutex: support --enable-debug-mutex

[Peter Xu]

On Mon, Apr 23, 2018 at 02:11:07PM -0400, Emilio G. Cota wrote:
> On Mon, Apr 23, 2018 at 13:39:26 +0800, Peter Xu wrote:
> > We have had some tracing tools for mutex but it's not easy to use them
> > for e.g. dead locks.  Let's provide "--enable-debug-mutex" parameter
> > when configure to allow QemuMutex to store the last owner that took
> > specific lock.  It will be easy to use this tool to debug deadlocks
> > since we can directly know who took the lock then as long as we can have
> > a debugger attached to the process.
> > 
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> (snip)
> >  static inline void qemu_mutex_pre_unlock(QemuMutex *mutex,
> >                                           const char *file, int line)
> >  {
> > +#ifdef CONFIG_DEBUG_MUTEX
> > +    mutex->file = NULL;
> > +    mutex->line = 0;
> > +#endif
> >      trace_qemu_mutex_unlock(mutex, file, line);
> >  }
> 
> I'd also initialize to NULL/0 the file/line pair at
> qemu_mutex_init time for both posix and win32. Other than that:
> 
> Reviewed-by: Emilio G. Cota <cota@braap.org>

I did the same trick for mutex init in patch 2 & 3 and reposted, while
I picked up your r-b for patch 4 only.  Please have a look.  Thanks,

-- 
Peter Xu