[PATCH 0/2] thread: add lock guard macros

[PATCH 0/2] thread: add lock guard macros

[Stefan Hajnoczi]

Lock guards automatically call qemu_(rec_)mutex_unlock() when returning from a
function or leaving leaving a lexical scope.  This simplifies code and
eliminates leaks (especially in error code paths).

This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
convert the entire tree but includes example conversions.

Stefan Hajnoczi (2):
  thread: add QemuRecMutex lock guards
  thread: add QemuMutex lock guards

 include/qemu/thread.h | 52 +++++++++++++++++++++++++++++++++++++++++++
 plugins/core.c        |  6 ++---
 plugins/loader.c      | 15 ++++++-------
 util/qemu-timer.c     | 22 +++++++++---------
 4 files changed, 71 insertions(+), 24 deletions(-)

-- 
2.24.1

[PATCH 1/2] thread: add QemuRecMutex lock guards

[Stefan Hajnoczi]

This patch introduces two lock guard macros that automatically unlock a
QemuRecMutex:

  void f(void) {
      QEMU_REC_MUTEX_LOCK_GUARD(&mutex);
      if (!may_fail()) {
          return; /* automatically unlocks mutex */
      }
      ...
  }

and:

  WITH_QEMU_REC_MUTEX_LOCK_GUARD(&mutex) {
      if (!may_fail()) {
          return; /* automatically unlocks mutex */
      }
  }
  /* automatically unlocks mutex here */
  ...

Convert TCG plugins functions that benefit from these macros.  Manual
qemu_rec_mutex_lock/unlock() callers are left unmodified in cases where
clarity would not improve by switching to the macros.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/thread.h | 26 ++++++++++++++++++++++++++
 plugins/core.c        |  6 ++----
 plugins/loader.c      | 15 +++++++--------
 3 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 047db0307e..3993ab7b25 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -132,6 +132,32 @@ static inline int (qemu_rec_mutex_trylock)(QemuRecMutex *mutex)
 /* Prototypes for other functions are in thread-posix.h/thread-win32.h.  */
 void qemu_rec_mutex_init(QemuRecMutex *mutex);
 
+static inline QemuRecMutex *qemu_rec_mutex_auto_lock(QemuRecMutex *mutex)
+{
+    qemu_rec_mutex_lock(mutex);
+    return mutex;
+}
+
+static inline void qemu_rec_mutex_auto_unlock(QemuRecMutex *mutex)
+{
+    if (mutex) {
+        qemu_rec_mutex_unlock(mutex);
+    }
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuRecMutex, qemu_rec_mutex_auto_unlock)
+
+#define WITH_QEMU_REC_MUTEX_LOCK_GUARD_(mutex, var) \
+    for (g_autoptr(QemuRecMutex) var = qemu_rec_mutex_auto_lock((mutex)); \
+         var; qemu_rec_mutex_auto_unlock(var), var = NULL)
+
+#define WITH_QEMU_REC_MUTEX_LOCK_GUARD(mutex) \
+    WITH_QEMU_REC_MUTEX_LOCK_GUARD_((mutex), qemu_rec_mutex_auto##__COUNTER__)
+
+#define QEMU_REC_MUTEX_LOCK_GUARD(mutex) \
+    g_autoptr(QemuRecMutex) qemu_rec_mutex_auto##__COUNTER__ = \
+            qemu_rec_mutex_auto_lock((mutex))
+
 void qemu_cond_init(QemuCond *cond);
 void qemu_cond_destroy(QemuCond *cond);
 
diff --git a/plugins/core.c b/plugins/core.c
index ed863011ba..cf8c85de9c 100644
--- a/plugins/core.c
+++ b/plugins/core.c
@@ -150,11 +150,11 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
 {
     struct qemu_plugin_ctx *ctx;
 
-    qemu_rec_mutex_lock(&plugin.lock);
+    QEMU_REC_MUTEX_LOCK_GUARD(&plugin.lock);
     ctx = plugin_id_to_ctx_locked(id);
     /* if the plugin is on its way out, ignore this request */
     if (unlikely(ctx->uninstalling)) {
-        goto out_unlock;
+        return;
     }
     if (func) {
         struct qemu_plugin_cb *cb = ctx->callbacks[ev];
@@ -178,8 +178,6 @@ do_plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
     } else {
         plugin_unregister_cb__locked(ctx, ev);
     }
- out_unlock:
-    qemu_rec_mutex_unlock(&plugin.lock);
 }
 
 void plugin_register_cb(qemu_plugin_id_t id, enum qemu_plugin_event ev,
diff --git a/plugins/loader.c b/plugins/loader.c
index 15fc7e5515..9742c8d41d 100644
--- a/plugins/loader.c
+++ b/plugins/loader.c
@@ -367,15 +367,14 @@ void plugin_reset_uninstall(qemu_plugin_id_t id,
     struct qemu_plugin_reset_data *data;
     struct qemu_plugin_ctx *ctx;
 
-    qemu_rec_mutex_lock(&plugin.lock);
-    ctx = plugin_id_to_ctx_locked(id);
-    if (ctx->uninstalling || (reset && ctx->resetting)) {
-        qemu_rec_mutex_unlock(&plugin.lock);
-        return;
+    WITH_QEMU_REC_MUTEX_LOCK_GUARD(&plugin.lock) {
+        ctx = plugin_id_to_ctx_locked(id);
+        if (ctx->uninstalling || (reset && ctx->resetting)) {
+            return;
+        }
+        ctx->resetting = reset;
+        ctx->uninstalling = !reset;
     }
-    ctx->resetting = reset;
-    ctx->uninstalling = !reset;
-    qemu_rec_mutex_unlock(&plugin.lock);
 
     data = g_new(struct qemu_plugin_reset_data, 1);
     data->ctx = ctx;
-- 
2.24.1

[PATCH 2/2] thread: add QemuMutex lock guards

[Stefan Hajnoczi]

This patch introduces two lock guard macros that automatically unlock a
QemuMutex:

  void f(void) {
      QEMU_MUTEX_LOCK_GUARD(&mutex);
      if (!may_fail()) {
          return; /* automatically unlocks mutex */
      }
      ...
  }

and:

  WITH_QEMU_MUTEX_LOCK_GUARD(&mutex) {
      if (!may_fail()) {
          return; /* automatically unlocks mutex */
      }
  }
  /* automatically unlocks mutex here */
  ...

Convert qemu-timer.c functions that benefit from these macros as an
example.  Manual qemu_mutex_lock/unlock() callers are left unmodified in
cases where clarity would not improve by switching to the macros.

Many other QemuMutex users remain in the codebase that might benefit
from lock guards.  Over time they can be converted, if that is
desirable.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 include/qemu/thread.h | 26 ++++++++++++++++++++++++++
 util/qemu-timer.c     | 22 ++++++++++------------
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/include/qemu/thread.h b/include/qemu/thread.h
index 3993ab7b25..8e8254f94f 100644
--- a/include/qemu/thread.h
+++ b/include/qemu/thread.h
@@ -119,6 +119,32 @@ static inline void (qemu_mutex_unlock)(QemuMutex *mutex)
     qemu_mutex_unlock(mutex);
 }
 
+static inline QemuMutex *qemu_mutex_auto_lock(QemuMutex *mutex)
+{
+    qemu_mutex_lock(mutex);
+    return mutex;
+}
+
+static inline void qemu_mutex_auto_unlock(QemuMutex *mutex)
+{
+    if (mutex) {
+        qemu_mutex_unlock(mutex);
+    }
+}
+
+G_DEFINE_AUTOPTR_CLEANUP_FUNC(QemuMutex, qemu_mutex_auto_unlock)
+
+#define WITH_QEMU_MUTEX_LOCK_GUARD_(mutex, var) \
+    for (g_autoptr(QemuMutex) var = qemu_mutex_auto_lock((mutex)); \
+         var; qemu_mutex_auto_unlock(var), var = NULL)
+
+#define WITH_QEMU_MUTEX_LOCK_GUARD(mutex) \
+    WITH_QEMU_MUTEX_LOCK_GUARD_(mutex, qemu_mutex_auto##__COUNTER__)
+
+#define QEMU_MUTEX_LOCK_GUARD(mutex) \
+    g_autoptr(QemuMutex) qemu_mutex_auto##__COUNTER__ = \
+            qemu_mutex_auto_lock((mutex))
+
 static inline void (qemu_rec_mutex_lock)(QemuRecMutex *mutex)
 {
     qemu_rec_mutex_lock(mutex);
diff --git a/util/qemu-timer.c b/util/qemu-timer.c
index ef52d28d37..4c42d33cf5 100644
--- a/util/qemu-timer.c
+++ b/util/qemu-timer.c
@@ -186,13 +186,12 @@ bool timerlist_expired(QEMUTimerList *timer_list)
         return false;
     }
 
-    qemu_mutex_lock(&timer_list->active_timers_lock);
-    if (!timer_list->active_timers) {
-        qemu_mutex_unlock(&timer_list->active_timers_lock);
-        return false;
+    WITH_QEMU_MUTEX_LOCK_GUARD(&timer_list->active_timers_lock) {
+        if (!timer_list->active_timers) {
+            return false;
+        }
+        expire_time = timer_list->active_timers->expire_time;
     }
-    expire_time = timer_list->active_timers->expire_time;
-    qemu_mutex_unlock(&timer_list->active_timers_lock);
 
     return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
 }
@@ -225,13 +224,12 @@ int64_t timerlist_deadline_ns(QEMUTimerList *timer_list)
      * value but ->notify_cb() is called when the deadline changes.  Therefore
      * the caller should notice the change and there is no race condition.
      */
-    qemu_mutex_lock(&timer_list->active_timers_lock);
-    if (!timer_list->active_timers) {
-        qemu_mutex_unlock(&timer_list->active_timers_lock);
-        return -1;
+    WITH_QEMU_MUTEX_LOCK_GUARD(&timer_list->active_timers_lock) {
+        if (!timer_list->active_timers) {
+            return -1;
+        }
+        expire_time = timer_list->active_timers->expire_time;
     }
-    expire_time = timer_list->active_timers->expire_time;
-    qemu_mutex_unlock(&timer_list->active_timers_lock);
 
     delta = expire_time - qemu_clock_get_ns(timer_list->clock->type);
 
-- 
2.24.1

Re: [PATCH 0/2] thread: add lock guard macros

[Paolo Bonzini]

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

Il mer 11 mar 2020, 13:38 Stefan Hajnoczi <stefanha@redhat.com> ha scritto:

> Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> from a
> function or leaving leaving a lexical scope.  This simplifies code and
> eliminates leaks (especially in error code paths).
>
> This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> convert the entire tree but includes example conversions.
>

Thanks for picking this up! It should be possible to use QemuLockable to
introduce a single set of lock guard macros that work for mutexes,
spinlocks and CoMutexes. Would you look into that?

(C++ also has unique_lock, a kind of lock guard that can be unlocked early
and won't cause a double unlock, and also can be created unlocked. However
it makes sense to not implement that unless one has a killer application of
it in the tree).

Paolo


> Stefan Hajnoczi (2):
>   thread: add QemuRecMutex lock guards
>   thread: add QemuMutex lock guards
>
>  include/qemu/thread.h | 52 +++++++++++++++++++++++++++++++++++++++++++
>  plugins/core.c        |  6 ++---
>  plugins/loader.c      | 15 ++++++-------
>  util/qemu-timer.c     | 22 +++++++++---------
>  4 files changed, 71 insertions(+), 24 deletions(-)
>
> --
> 2.24.1
>
>

[-- Attachment #2: Type: text/html, Size: 1946 bytes --]

Re: [PATCH 0/2] thread: add lock guard macros

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200311123624.277221-1-stefanha@redhat.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      util/hexdump.o
  CC      util/crc32c.o
/tmp/qemu-test/src/util/qemu-timer.c: In function 'timerlist_expired':
/tmp/qemu-test/src/util/qemu-timer.c:196:5: error: 'expire_time' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
     ^
cc1: all warnings being treated as errors
  CC      util/uuid.o
make: *** [util/qemu-timer.o] Error 1
make: *** Waiting for unfinished jobs....
rm tests/qemu-iotests/socket_scm_helper.o
Traceback (most recent call last):
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=92163d9ae7a944769ca832267d70514c', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-ijn0d86j/src/docker-src.2020-03-11-09.18.18.28503:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=92163d9ae7a944769ca832267d70514c
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-ijn0d86j/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    1m59.199s
user    0m7.765s


The full log is available at
http://patchew.org/logs/20200311123624.277221-1-stefanha@redhat.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] thread: add lock guard macros

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200311123624.277221-1-stefanha@redhat.com/



Hi,

This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#! /bin/bash
export ARCH=x86_64
make docker-image-fedora V=1 NETWORK=1
time make docker-test-mingw@fedora J=14 NETWORK=1
=== TEST SCRIPT END ===

  CC      util/uri.o
  CC      util/notify.o
/tmp/qemu-test/src/util/qemu-timer.c: In function 'timerlist_expired':
/tmp/qemu-test/src/util/qemu-timer.c:196:24: error: 'expire_time' may be used uninitialized in this function [-Werror=maybe-uninitialized]
     return expire_time <= qemu_clock_get_ns(timer_list->clock->type);
            ~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make: *** [/tmp/qemu-test/src/rules.mak:69: util/qemu-timer.o] Error 1
make: *** Waiting for unfinished jobs....
Traceback (most recent call last):
  File "./tests/docker/docker.py", line 664, in <module>
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=bbe7ec3619e2401a97060d5d9ab55bac', '-u', '1003', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew2/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-jdtnwksa/src/docker-src.2020-03-11-09.20.54.2854:/var/tmp/qemu:z,ro', 'qemu:fedora', '/var/tmp/qemu/run', 'test-mingw']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=bbe7ec3619e2401a97060d5d9ab55bac
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-jdtnwksa/src'
make: *** [docker-run-test-mingw@fedora] Error 2

real    1m36.654s
user    0m7.856s


The full log is available at
http://patchew.org/logs/20200311123624.277221-1-stefanha@redhat.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH 0/2] thread: add lock guard macros

[Markus Armbruster]

Stefan Hajnoczi <stefanha@redhat.com> writes:

> Lock guards automatically call qemu_(rec_)mutex_unlock() when returning from a
> function or leaving leaving a lexical scope.  This simplifies code and
> eliminates leaks (especially in error code paths).
>
> This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> convert the entire tree but includes example conversions.

I support the move towards automatic cleanup, but I'm wary of
incremental conversion.  Experience tells that outdated examples
invariably get copied / imitated, with incremental conversion struggling
to keep up.

Are we resigned to having both kinds of mutex cleanup forever?

If not, what's the plan to get the job finished, and until when?

Re: [PATCH 0/2] thread: add lock guard macros

[Paolo Bonzini]

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

Il mer 11 mar 2020, 15:50 Markus Armbruster <armbru@redhat.com> ha scritto:

> Stefan Hajnoczi <stefanha@redhat.com> writes:
>
> > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> from a
> > function or leaving leaving a lexical scope.  This simplifies code and
> > eliminates leaks (especially in error code paths).
> >
> > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > convert the entire tree but includes example conversions.
>
> I support the move towards automatic cleanup, but I'm wary of
> incremental conversion.  Experience tells that outdated examples
> invariably get copied / imitated, with incremental conversion struggling
> to keep up.
>
> Are we resigned to having both kinds of mutex cleanup forever?
>

There are cases where the legibility benefits of guards are debatable, or
they require more complex functionality in the guards (see my other answer
to Stefan). So, yes. We don't have that many mutexes so incremental
conversion should be doable without taking forever.

Paolo


> If not, what's the plan to get the job finished, and until when?
>
>

[-- Attachment #2: Type: text/html, Size: 1855 bytes --]

Re: [PATCH 0/2] thread: add lock guard macros

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2020 at 04:06:02PM +0100, Paolo Bonzini wrote:
> Il mer 11 mar 2020, 15:50 Markus Armbruster <armbru@redhat.com> ha scritto:
> 
> > Stefan Hajnoczi <stefanha@redhat.com> writes:
> >
> > > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> > from a
> > > function or leaving leaving a lexical scope.  This simplifies code and
> > > eliminates leaks (especially in error code paths).
> > >
> > > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > > convert the entire tree but includes example conversions.
> >
> > I support the move towards automatic cleanup, but I'm wary of
> > incremental conversion.  Experience tells that outdated examples
> > invariably get copied / imitated, with incremental conversion struggling
> > to keep up.
> >
> > Are we resigned to having both kinds of mutex cleanup forever?
> >
> 
> There are cases where the legibility benefits of guards are debatable, or
> they require more complex functionality in the guards (see my other answer
> to Stefan). So, yes. We don't have that many mutexes so incremental
> conversion should be doable without taking forever.

I will add this to the BiteSizedTasks wiki page when the patch is
merged, together with guidelines on how to convert code (it requires
case-by-case evaluation and is not a simple mechanical change).

We will continue to have raw qemu_(rec_)mutex_lock/unlock() calls in
cases where a complex locking scheme is used or lock guards would make
the code less clear.

Stefan

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

Re: [PATCH 0/2] thread: add lock guard macros

[Stefan Hajnoczi]

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

On Wed, Mar 11, 2020 at 01:52:35PM +0100, Paolo Bonzini wrote:
> Il mer 11 mar 2020, 13:38 Stefan Hajnoczi <stefanha@redhat.com> ha scritto:
> 
> > Lock guards automatically call qemu_(rec_)mutex_unlock() when returning
> > from a
> > function or leaving leaving a lexical scope.  This simplifies code and
> > eliminates leaks (especially in error code paths).
> >
> > This series adds lock guards for QemuMutex and QemuRecMutex.  It does not
> > convert the entire tree but includes example conversions.
> >
> 
> Thanks for picking this up! It should be possible to use QemuLockable to
> introduce a single set of lock guard macros that work for mutexes,
> spinlocks and CoMutexes. Would you look into that?
> 
> (C++ also has unique_lock, a kind of lock guard that can be unlocked early
> and won't cause a double unlock, and also can be created unlocked. However
> it makes sense to not implement that unless one has a killer application of
> it in the tree).

I already looked at lockable.h and to be honest I didn't want to combine
g_autoptr macros with lockable's polymorphism macros.

However, since you have mentioned it I'll take another look and try to
overcome the aversion :).

Stefan

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