[libvirt PATCH v4 0/9] Automatic mutex management
V1: https://listman.redhat.com/archives/libvir-list/2021-August/msg00823.html V2: https://listman.redhat.com/archives/libvir-list/2021-September/msg00249.html V3: https://listman.redhat.com/archives/libvir-list/2021-September/msg00964.html Changes since V3: * Remove not strictly necessary heap allocations from virLockGuard Regards, Tim Tim Wiederhake (9): internal: Add CONCAT macro virthread: Introduce virLockGuard virthread: Introduce VIR_WITH_MUTEX_LOCK_GUARD virobject: Introduce virObjectLockGuard virobject: Introduce VIR_WITH_OBJECT_LOCK_GUARD virChrdevFDStreamCloseCb: Use virLockGuardLock virChrdevFree: Use VIR_WITH_MUTEX_LOCK bhyveAutostartDomain: Use virObjectLockGuard lxcDomainDetachDeviceHostdevUSBLive: Use VIR_WITH_OBJECT_LOCK_GUARD src/bhyve/bhyve_driver.c | 4 ++-- src/conf/virchrdev.c | 12 +++++------- src/internal.h | 3 +++ src/libvirt_private.syms | 3 +++ src/lxc/lxc_driver.c | 6 +++--- src/util/virobject.c | 16 ++++++++++++++++ src/util/virobject.h | 24 ++++++++++++++++++++++++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 30 ++++++++++++++++++++++++++++++ 9 files changed, 101 insertions(+), 12 deletions(-) -- 2.31.1
Using the two-step idiom to force resolution of other macros, e.g.: #define bar BAR CONCAT_(foo, bar) // foobar CONCAT(foo, bar) // fooBAR Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/internal.h | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/internal.h b/src/internal.h index b6e4332542..4cfb022b41 100644 --- a/src/internal.h +++ b/src/internal.h @@ -100,6 +100,9 @@ #define STREQ_NULLABLE(a, b) (g_strcmp0(a, b) == 0) #define STRNEQ_NULLABLE(a, b) (g_strcmp0(a, b) != 0) +#define CONCAT_(a, b) a ## b +#define CONCAT(a, b) CONCAT_(a, b) + #ifdef WIN32 # ifndef O_CLOEXEC # define O_CLOEXEC _O_NOINHERIT -- 2.31.1
Locks a virMutex on creation and unlocks it in its destructor. The VIR_LOCK_GUARD macro is used instead of "g_auto(virLockGuard)" to work around a clang issue (see https://bugs.llvm.org/show_bug.cgi?id=3888 and https://bugs.llvm.org/show_bug.cgi?id=43482). Typical usage: void function(virMutex *m) { VIR_LOCK_GUARD lock = virLockGuardLock(m); /* `m` is locked, and released automatically on scope exit */ ... while (expression) { VIR_LOCK_GUARD lock2 = virLockGuardLock(...); /* similar */ } } Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_private.syms | 2 ++ src/util/virthread.c | 15 +++++++++++++++ src/util/virthread.h | 10 ++++++++++ 3 files changed, 27 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ba3462d849..4cfe6c17eb 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3376,6 +3376,8 @@ virCondInit; virCondSignal; virCondWait; virCondWaitUntil; +virLockGuardLock; +virLockGuardUnlock; virMutexDestroy; virMutexInit; virMutexInitRecursive; diff --git a/src/util/virthread.c b/src/util/virthread.c index e89c1a09fb..5422bb74fd 100644 --- a/src/util/virthread.c +++ b/src/util/virthread.c @@ -96,6 +96,21 @@ void virMutexUnlock(virMutex *m) pthread_mutex_unlock(&m->lock); } +virLockGuard virLockGuardLock(virMutex *m) +{ + virLockGuard l = { m }; + virMutexLock(m); + return l; +} + +void virLockGuardUnlock(virLockGuard *l) +{ + if (!l || !l->mutex) + return; + + virMutexUnlock(g_steal_pointer(&l->mutex)); +} + int virRWLockInit(virRWLock *m) { diff --git a/src/util/virthread.h b/src/util/virthread.h index 55c8263ae6..6cdaf2820e 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -31,6 +31,11 @@ struct virMutex { pthread_mutex_t lock; }; +typedef struct virLockGuard virLockGuard; +struct virLockGuard { + virMutex *mutex; +}; + typedef struct virRWLock virRWLock; struct virRWLock { pthread_rwlock_t lock; @@ -121,6 +126,11 @@ void virMutexLock(virMutex *m); void virMutexUnlock(virMutex *m); +virLockGuard virLockGuardLock(virMutex *m); +void virLockGuardUnlock(virLockGuard *l); +G_DEFINE_AUTO_CLEANUP_CLEAR_FUNC(virLockGuard, virLockGuardUnlock); +#define VIR_LOCK_GUARD g_auto(virLockGuard) G_GNUC_UNUSED + int virRWLockInit(virRWLock *m) G_GNUC_WARN_UNUSED_RESULT; void virRWLockDestroy(virRWLock *m); -- 2.31.1
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h). Uses "__LINE__" instead of "__COUNTER__", as the latter is a GNU extension. See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virthread.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/util/virthread.h b/src/util/virthread.h index 6cdaf2820e..da17780908 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -209,3 +209,23 @@ int virThreadLocalSet(virThreadLocal *l, void*) G_GNUC_WARN_UNUSED_RESULT; return 0; \ } \ struct classname ## EatSemicolon + +/** + * VIR_WITH_MUTEX_LOCK_GUARD: + * + * This macro defines a lock scope such that entering the scope takes the lock + * and leaving the scope releases the lock. Return statements are allowed + * within the scope and release the lock. Break and continue statements leave + * the scope early and release the lock. + * + * virMutex *mutex = ...; + * + * VIR_WITH_MUTEX_LOCK_GUARD(mutex) { + * // `mutex` is locked, and released automatically on scope exit + * ... + * } + */ +#define VIR_WITH_MUTEX_LOCK_GUARD(m) \ + for (g_auto(virLockGuard) CONCAT(var, __LINE__) = virLockGuardLock(m); \ + CONCAT(var, __LINE__).mutex; \ + CONCAT(var, __LINE__).mutex = (virLockGuardUnlock(& CONCAT(var, __LINE__)), NULL)) -- 2.31.1
On Fri, Jan 28, 2022 at 10:59:16AM +0100, Tim Wiederhake wrote:
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h). Uses "__LINE__" instead of "__COUNTER__", as the latter is a GNU extension.
GNU extensions are fine to use, as we explicitly only support GCC or CLang which is compatible with GCC, so I'd stick with __COUNTER__.
See comment for typical usage.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virthread.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+)
diff --git a/src/util/virthread.h b/src/util/virthread.h index 6cdaf2820e..da17780908 100644 --- a/src/util/virthread.h +++ b/src/util/virthread.h @@ -209,3 +209,23 @@ int virThreadLocalSet(virThreadLocal *l, void*) G_GNUC_WARN_UNUSED_RESULT; return 0; \ } \ struct classname ## EatSemicolon + +/** + * VIR_WITH_MUTEX_LOCK_GUARD: + * + * This macro defines a lock scope such that entering the scope takes the lock + * and leaving the scope releases the lock. Return statements are allowed + * within the scope and release the lock. Break and continue statements leave + * the scope early and release the lock. + * + * virMutex *mutex = ...; + * + * VIR_WITH_MUTEX_LOCK_GUARD(mutex) { + * // `mutex` is locked, and released automatically on scope exit + * ... + * } + */ +#define VIR_WITH_MUTEX_LOCK_GUARD(m) \ + for (g_auto(virLockGuard) CONCAT(var, __LINE__) = virLockGuardLock(m); \ + CONCAT(var, __LINE__).mutex; \ + CONCAT(var, __LINE__).mutex = (virLockGuardUnlock(& CONCAT(var, __LINE__)), NULL)) -- 2.31.1
Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Typical usage: void foobar(virObjectLockable *obj) { VIR_LOCK_GUARD lock = virObjectLockGuard(obj); /* `obj` is locked, and released automatically on scope exit */ ... } Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virobject.c | 16 ++++++++++++++++ src/util/virobject.h | 4 ++++ 3 files changed, 21 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 4cfe6c17eb..463bb39de3 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2944,6 +2944,7 @@ virObjectListFree; virObjectListFreeCount; virObjectLock; virObjectLockableNew; +virObjectLockGuard; virObjectNew; virObjectRef; virObjectRWLockableNew; diff --git a/src/util/virobject.c b/src/util/virobject.c index 588b41802c..74cc84205e 100644 --- a/src/util/virobject.c +++ b/src/util/virobject.c @@ -426,6 +426,22 @@ virObjectGetRWLockableObj(void *anyobj) } +/** + * virObjectLockGuard: + * @anyobj: any instance of virObjectLockable + * + * Acquire a lock on @anyobj that will be managed by the virLockGuard object + * returned to the caller. + */ +virLockGuard +virObjectLockGuard(void *anyobj) +{ + virObjectLockable *obj = virObjectGetLockableObj(anyobj); + + return virLockGuardLock(&obj->lock); +} + + /** * virObjectLock: * @anyobj: any instance of virObjectLockable diff --git a/src/util/virobject.h b/src/util/virobject.h index 1e34c77744..dc1ce66a4f 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -118,6 +118,10 @@ void * virObjectRWLockableNew(virClass *klass) ATTRIBUTE_NONNULL(1); +virLockGuard +virObjectLockGuard(void *lockableobj) + ATTRIBUTE_NONNULL(1); + void virObjectLock(void *lockableobj) ATTRIBUTE_NONNULL(1); -- 2.31.1
Modeled after "WITH_QEMU_LOCK_GUARD" (see qemu's include/qemu/lockable.h). Uses "__LINE__" instead of "__COUNTER__", as the latter is a GNU extension. See comment for typical usage. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virobject.h | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/src/util/virobject.h b/src/util/virobject.h index dc1ce66a4f..595076bda7 100644 --- a/src/util/virobject.h +++ b/src/util/virobject.h @@ -148,3 +148,23 @@ virObjectListFree(void *list); void virObjectListFreeCount(void *list, size_t count); + +/** + * VIR_WITH_OBJECT_LOCK_GUARD: + * + * This macro defines a lock scope such that entering the scope takes the lock + * and leaving the scope releases the lock. Return statements are allowed + * within the scope and release the lock. Break and continue statements leave + * the scope early and release the lock. + * + * virObjectLockable *lockable = ...; + * + * VIR_WITH_OBJECT_LOCK_GUARD(lockable) { + * // `lockable` is locked, and released automatically on scope exit + * ... + * } + */ +#define VIR_WITH_OBJECT_LOCK_GUARD(o) \ + for (g_auto(virLockGuard) CONCAT(var, __LINE__) = virObjectLockGuard(o); \ + CONCAT(var, __LINE__).mutex; \ + CONCAT(var, __LINE__).mutex = (virLockGuardUnlock(& CONCAT(var, __LINE__)), NULL)) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virchrdev.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index b5477b03d5..c9b2134e3b 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -237,12 +237,10 @@ static void virChrdevFDStreamCloseCb(virStreamPtr st G_GNUC_UNUSED, void *opaque) { virChrdevStreamInfo *priv = opaque; - virMutexLock(&priv->devs->lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&priv->devs->lock); /* remove entry from hash */ virHashRemoveEntry(priv->devs->hash, priv->path); - - virMutexUnlock(&priv->devs->lock); } /** -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/conf/virchrdev.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/conf/virchrdev.c b/src/conf/virchrdev.c index c9b2134e3b..8610f0ac5c 100644 --- a/src/conf/virchrdev.c +++ b/src/conf/virchrdev.c @@ -291,10 +291,10 @@ void virChrdevFree(virChrdevs *devs) if (!devs) return; - virMutexLock(&devs->lock); - virHashForEachSafe(devs->hash, virChrdevFreeClearCallbacks, NULL); - g_clear_pointer(&devs->hash, g_hash_table_unref); - virMutexUnlock(&devs->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&devs->lock) { + virHashForEachSafe(devs->hash, virChrdevFreeClearCallbacks, NULL); + g_clear_pointer(&devs->hash, g_hash_table_unref); + } virMutexDestroy(&devs->lock); g_free(devs); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/bhyve/bhyve_driver.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index f291f12e50..47ee98e650 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -87,7 +87,8 @@ bhyveAutostartDomain(virDomainObj *vm, void *opaque) { const struct bhyveAutostartData *data = opaque; int ret = 0; - virObjectLock(vm); + VIR_LOCK_GUARD lock = virObjectLockGuard(vm); + if (vm->autostart && !virDomainObjIsActive(vm)) { virResetLastError(); ret = virBhyveProcessStart(data->conn, vm, @@ -98,7 +99,6 @@ bhyveAutostartDomain(virDomainObj *vm, void *opaque) vm->def->name, virGetLastErrorMessage()); } } - virObjectUnlock(vm); return ret; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/lxc/lxc_driver.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lxc/lxc_driver.c b/src/lxc/lxc_driver.c index 7bc39120ee..42053de9c3 100644 --- a/src/lxc/lxc_driver.c +++ b/src/lxc/lxc_driver.c @@ -4045,9 +4045,9 @@ lxcDomainDetachDeviceHostdevUSBLive(virLXCDriver *driver, VIR_WARN("cannot deny device %s for domain %s: %s", dst, vm->def->name, virGetLastErrorMessage()); - virObjectLock(hostdev_mgr->activeUSBHostdevs); - virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, usb); - virObjectUnlock(hostdev_mgr->activeUSBHostdevs); + VIR_WITH_OBJECT_LOCK_GUARD(hostdev_mgr->activeUSBHostdevs) { + virUSBDeviceListDel(hostdev_mgr->activeUSBHostdevs, usb); + } virDomainHostdevRemove(vm->def, idx); virDomainHostdevDefFree(def); -- 2.31.1
participants (2)
-
Daniel P. Berrangé -
Tim Wiederhake