[libvirt PATCH 00/11] Automatic mutex management
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management. This made a solitary "virMutexUnlock()" call without previous call to "virMutexLock()" in ch_driver.c obvious, which is removed in patch #8. Tim Wiederhake (11): virthreadpool: Use automatic memory management virthreadpool: Cleanup libxl: Use automatic memory management lxc: Prepare virLXCDriverGetCapabilities for automatic mutex management lxc: Use automatic mutex management ch: Prepare virCHDriverGetCapabilities for automatic mutex management ch: Use automatic mutex management ch: Remove solitary virMutexUnlock network: Use automatic mutex management bhyve_driver: Use automatic mutex management node_device: Use automatic mutex management src/bhyve/bhyve_conf.c | 7 +-- src/bhyve/bhyve_driver.c | 12 ---- src/bhyve/bhyve_utils.h | 3 - src/ch/ch_conf.c | 31 +++++----- src/ch/ch_conf.h | 10 ---- src/ch/ch_driver.c | 29 +++++---- src/libxl/libxl_conf.c | 8 +-- src/libxl/libxl_conf.h | 12 ---- src/libxl/libxl_logger.c | 20 +++---- src/lxc/lxc_conf.c | 41 ++++++------- src/lxc/lxc_conf.h | 9 --- src/lxc/lxc_controller.c | 25 ++++---- src/lxc/lxc_fuse.c | 12 ++-- src/network/bridge_driver.c | 41 ++++--------- src/node_device/node_device_driver.c | 23 ++----- src/node_device/node_device_udev.c | 52 +++++++--------- src/util/virthreadpool.c | 90 ++++++++-------------------- 17 files changed, 138 insertions(+), 287 deletions(-) -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virthreadpool.c | 39 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 26 deletions(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 426840e435..b6d154802a 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -306,15 +306,13 @@ void virThreadPoolFree(virThreadPool *pool) if (!pool) return; - virMutexLock(&pool->mutex); - virThreadPoolDrainLocked(pool); + virThreadPoolDrain(pool); if (pool->identity) g_object_unref(pool->identity); g_free(pool->jobName); g_free(pool->workers); - virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex); virCondDestroy(&pool->quit_cond); virCondDestroy(&pool->cond); @@ -326,66 +324,60 @@ void virThreadPoolFree(virThreadPool *pool) size_t virThreadPoolGetMinWorkers(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->minWorkers; - virMutexUnlock(&pool->mutex); return ret; } size_t virThreadPoolGetMaxWorkers(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->maxWorkers; - virMutexUnlock(&pool->mutex); return ret; } size_t virThreadPoolGetPriorityWorkers(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->nPrioWorkers; - virMutexUnlock(&pool->mutex); return ret; } size_t virThreadPoolGetCurrentWorkers(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->nWorkers; - virMutexUnlock(&pool->mutex); return ret; } size_t virThreadPoolGetFreeWorkers(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->freeWorkers; - virMutexUnlock(&pool->mutex); return ret; } size_t virThreadPoolGetJobQueueDepth(virThreadPool *pool) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t ret; - virMutexLock(&pool->mutex); ret = pool->jobQueueDepth; - virMutexUnlock(&pool->mutex); return ret; } @@ -398,9 +390,9 @@ int virThreadPoolSendJob(virThreadPool *pool, unsigned int priority, void *jobData) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); virThreadPoolJob *job; - virMutexLock(&pool->mutex); if (pool->quit) goto error; @@ -431,11 +423,9 @@ int virThreadPoolSendJob(virThreadPool *pool, if (priority) virCondSignal(&pool->prioCond); - virMutexUnlock(&pool->mutex); return 0; error: - virMutexUnlock(&pool->mutex); return -1; } @@ -445,11 +435,10 @@ virThreadPoolSetParameters(virThreadPool *pool, long long int maxWorkers, long long int prioWorkers) { + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); size_t max; size_t min; - virMutexLock(&pool->mutex); - max = maxWorkers >= 0 ? maxWorkers : pool->maxWorkers; min = minWorkers >= 0 ? minWorkers : pool->minWorkers; if (min > max) { @@ -490,26 +479,24 @@ virThreadPoolSetParameters(virThreadPool *pool, pool->maxPrioWorkers = prioWorkers; } - virMutexUnlock(&pool->mutex); return 0; error: - virMutexUnlock(&pool->mutex); return -1; } void virThreadPoolStop(virThreadPool *pool) { - virMutexLock(&pool->mutex); + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); + virThreadPoolStopLocked(pool); - virMutexUnlock(&pool->mutex); } void virThreadPoolDrain(virThreadPool *pool) { - virMutexLock(&pool->mutex); + VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); + virThreadPoolDrainLocked(pool); - virMutexUnlock(&pool->mutex); } -- 2.31.1
On 2/7/22 14:12, Tim Wiederhake wrote:
Signed-off-by: Tim Wiederhake <twiederh@redhat.com>
s/memory/lock/ in $SUBJ.
--- src/util/virthreadpool.c | 39 +++++++++++++-------------------------- 1 file changed, 13 insertions(+), 26 deletions(-)
diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 426840e435..b6d154802a 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -306,15 +306,13 @@ void virThreadPoolFree(virThreadPool *pool) if (!pool) return;
- virMutexLock(&pool->mutex);
Look at this made me realize we need to check pool->quit in virThreadPoolSetParameters(). I was trying to come up with a path where removing this lock acquire would create a problem (because removing a mutex makes all the control lights flash for me), but failed. I'm still trying to convince myself this is safe thing to do. On the other hand, if we used automatic mutex management, then ...
- virThreadPoolDrainLocked(pool); + virThreadPoolDrain(pool);
if (pool->identity) g_object_unref(pool->identity);
g_free(pool->jobName); g_free(pool->workers); - virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex);
.. this would destroy a locked mutex. So I guess your code is right.
virCondDestroy(&pool->quit_cond); virCondDestroy(&pool->cond);
Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/util/virthreadpool.c | 51 ++++++++++------------------------------ 1 file changed, 13 insertions(+), 38 deletions(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index b6d154802a..2f1d49d665 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -325,61 +325,43 @@ void virThreadPoolFree(virThreadPool *pool) size_t virThreadPoolGetMinWorkers(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - ret = pool->minWorkers; - - return ret; + return pool->minWorkers; } size_t virThreadPoolGetMaxWorkers(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - - ret = pool->maxWorkers; - return ret; + return pool->maxWorkers; } size_t virThreadPoolGetPriorityWorkers(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - - ret = pool->nPrioWorkers; - return ret; + return pool->nPrioWorkers; } size_t virThreadPoolGetCurrentWorkers(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - ret = pool->nWorkers; - - return ret; + return pool->nWorkers; } size_t virThreadPoolGetFreeWorkers(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - - ret = pool->freeWorkers; - return ret; + return pool->freeWorkers; } size_t virThreadPoolGetJobQueueDepth(virThreadPool *pool) { VIR_LOCK_GUARD lock = virLockGuardLock(&pool->mutex); - size_t ret; - ret = pool->jobQueueDepth; - - return ret; + return pool->jobQueueDepth; } /* @@ -394,12 +376,12 @@ int virThreadPoolSendJob(virThreadPool *pool, virThreadPoolJob *job; if (pool->quit) - goto error; + return -1; if (pool->freeWorkers - pool->jobQueueDepth <= 0 && pool->nWorkers < pool->maxWorkers && virThreadPoolExpand(pool, 1, false) < 0) - goto error; + return -1; job = g_new0(virThreadPoolJob, 1); @@ -424,9 +406,6 @@ int virThreadPoolSendJob(virThreadPool *pool, virCondSignal(&pool->prioCond); return 0; - - error: - return -1; } int @@ -444,7 +423,7 @@ virThreadPoolSetParameters(virThreadPool *pool, if (min > max) { virReportError(VIR_ERR_INVALID_ARG, "%s", _("minWorkers cannot be larger than maxWorkers")); - goto error; + return -1; } if ((maxWorkers == 0 && pool->maxWorkers > 0) || @@ -452,14 +431,13 @@ virThreadPoolSetParameters(virThreadPool *pool, virReportError(VIR_ERR_INVALID_ARG, "%s", _("maxWorkers must not be switched from zero to non-zero" " and vice versa")); - goto error; + return -1; } if (minWorkers >= 0) { if ((size_t) minWorkers > pool->nWorkers && - virThreadPoolExpand(pool, minWorkers - pool->nWorkers, - false) < 0) - goto error; + virThreadPoolExpand(pool, minWorkers - pool->nWorkers, false) < 0) + return -1; pool->minWorkers = minWorkers; } @@ -474,15 +452,12 @@ virThreadPoolSetParameters(virThreadPool *pool, } else if ((size_t) prioWorkers > pool->nPrioWorkers && virThreadPoolExpand(pool, prioWorkers - pool->nPrioWorkers, true) < 0) { - goto error; + return -1; } pool->maxPrioWorkers = prioWorkers; } return 0; - - error: - return -1; } void -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/libxl/libxl_conf.c | 8 ++------ src/libxl/libxl_conf.h | 12 ------------ src/libxl/libxl_logger.c | 20 +++++++++----------- 3 files changed, 11 insertions(+), 29 deletions(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index f062f8e958..a0fc51c74a 100644 --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1815,12 +1815,8 @@ libxlDriverConfigInit(libxlDriverConfig *cfg) libxlDriverConfig * libxlDriverConfigGet(libxlDriverPrivate *driver) { - libxlDriverConfig *cfg; - - libxlDriverLock(driver); - cfg = virObjectRef(driver->config); - libxlDriverUnlock(driver); - return cfg; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->config); } int libxlDriverConfigLoadFile(libxlDriverConfig *cfg, diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h index b74f455b69..16a7195bfd 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -219,15 +219,3 @@ libxlBuildDomainConfig(virPortAllocatorRange *graphicsports, virDomainDef *def, libxlDriverConfig *cfg, libxl_domain_config *d_config); - -static inline void -libxlDriverLock(libxlDriverPrivate *driver) -{ - virMutexLock(&driver->lock); -} - -static inline void -libxlDriverUnlock(libxlDriverPrivate *driver) -{ - virMutexUnlock(&driver->lock); -} diff --git a/src/libxl/libxl_logger.c b/src/libxl/libxl_logger.c index 4708553f29..45cdc32f38 100644 --- a/src/libxl/libxl_logger.c +++ b/src/libxl/libxl_logger.c @@ -79,16 +79,16 @@ libvirt_vmessage(xentoollog_logger *logger_in, /* Should we print to a domain-specific log file? */ if ((start = strstr(message, ": Domain ")) && (end = strstr(start + 9, ":"))) { - FILE *domainLogFile; + FILE *domainLogFile = NULL; VIR_DEBUG("Found domain log message"); start = start + 9; *end = '\0'; - virMutexLock(&lg->tableLock); - domainLogFile = virHashLookup(lg->files, start); - virMutexUnlock(&lg->tableLock); + VIR_WITH_MUTEX_LOCK_GUARD(&lg->tableLock) { + domainLogFile = virHashLookup(lg->files, start); + } if (domainLogFile) logFile = domainLogFile; @@ -197,9 +197,9 @@ libxlLoggerOpenFile(libxlLogger *logger, path, g_strerror(errno)); return; } - virMutexLock(&logger->tableLock); - ignore_value(virHashAddEntry(logger->files, domidstr, logFile)); - virMutexUnlock(&logger->tableLock); + VIR_WITH_MUTEX_LOCK_GUARD(&logger->tableLock) { + ignore_value(virHashAddEntry(logger->files, domidstr, logFile)); + } /* domain_config is non NULL only when starting a new domain */ if (domain_config) { @@ -211,10 +211,8 @@ libxlLoggerOpenFile(libxlLogger *logger, void libxlLoggerCloseFile(libxlLogger *logger, int id) { - g_autofree char *domidstr = NULL; - domidstr = g_strdup_printf("%d", id); + g_autofree char *domidstr = g_strdup_printf("%d", id); + VIR_LOCK_GUARD lock = virLockGuardLock(&logger->tableLock); - virMutexLock(&logger->tableLock); ignore_value(virHashRemoveEntry(logger->files, domidstr)); - virMutexUnlock(&logger->tableLock); } -- 2.31.1
No functional change intended. This change makes the recfatoring to automatic mutex management easier to follow. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/lxc/lxc_conf.c | 30 +++++++++++++++--------------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index 8955578d54..cf6679711a 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -156,28 +156,28 @@ virCaps *virLXCDriverCapsInit(virLXCDriver *driver) virCaps *virLXCDriverGetCapabilities(virLXCDriver *driver, bool refresh) { - virCaps *ret; - if (refresh) { - virCaps *caps = NULL; - if ((caps = virLXCDriverCapsInit(driver)) == NULL) - return NULL; + virCaps *ret = NULL; + virCaps *caps = NULL; + + lxcDriverLock(driver); + if (!refresh && !driver->caps) { + VIR_DEBUG("Capabilities didn't detect any guests. Forcing a refresh."); + refresh = true; + } + lxcDriverUnlock(driver); - lxcDriverLock(driver); + if (refresh && !(caps = virLXCDriverCapsInit(driver))) + return NULL; + + lxcDriverLock(driver); + if (refresh) { virObjectUnref(driver->caps); driver->caps = caps; - } else { - lxcDriverLock(driver); - - if (driver->caps == NULL) { - VIR_DEBUG("Capabilities didn't detect any guests. Forcing a " - "refresh."); - lxcDriverUnlock(driver); - return virLXCDriverGetCapabilities(driver, true); - } } ret = virObjectRef(driver->caps); lxcDriverUnlock(driver); + return ret; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/lxc/lxc_conf.c | 31 ++++++++++++++----------------- src/lxc/lxc_conf.h | 9 --------- src/lxc/lxc_controller.c | 25 ++++++++++--------------- src/lxc/lxc_fuse.c | 12 ++++++------ 4 files changed, 30 insertions(+), 47 deletions(-) diff --git a/src/lxc/lxc_conf.c b/src/lxc/lxc_conf.c index cf6679711a..3ab4d61fe3 100644 --- a/src/lxc/lxc_conf.c +++ b/src/lxc/lxc_conf.c @@ -159,24 +159,24 @@ virCaps *virLXCDriverGetCapabilities(virLXCDriver *driver, virCaps *ret = NULL; virCaps *caps = NULL; - lxcDriverLock(driver); - if (!refresh && !driver->caps) { - VIR_DEBUG("Capabilities didn't detect any guests. Forcing a refresh."); - refresh = true; + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + if (!refresh && !driver->caps) { + VIR_DEBUG("Capabilities didn't detect any guests. Forcing a refresh."); + refresh = true; + } } - lxcDriverUnlock(driver); if (refresh && !(caps = virLXCDriverCapsInit(driver))) return NULL; - lxcDriverLock(driver); - if (refresh) { - virObjectUnref(driver->caps); - driver->caps = caps; - } + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + if (refresh) { + virObjectUnref(driver->caps); + driver->caps = caps; + } - ret = virObjectRef(driver->caps); - lxcDriverUnlock(driver); + ret = virObjectRef(driver->caps); + } return ret; } @@ -248,11 +248,8 @@ virLXCLoadDriverConfig(virLXCDriverConfig *cfg, virLXCDriverConfig *virLXCDriverGetConfig(virLXCDriver *driver) { - virLXCDriverConfig *cfg; - lxcDriverLock(driver); - cfg = virObjectRef(driver->config); - lxcDriverUnlock(driver); - return cfg; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->config); } static void diff --git a/src/lxc/lxc_conf.h b/src/lxc/lxc_conf.h index 5a1351bd63..04e51aa954 100644 --- a/src/lxc/lxc_conf.h +++ b/src/lxc/lxc_conf.h @@ -113,12 +113,3 @@ virCaps *virLXCDriverGetCapabilities(virLXCDriver *driver, bool refresh); virDomainXMLOption *lxcDomainXMLConfInit(virLXCDriver *driver, const char *defsecmodel); - -static inline void lxcDriverLock(virLXCDriver *driver) -{ - virMutexLock(&driver->lock); -} -static inline void lxcDriverUnlock(virLXCDriver *driver) -{ - virMutexUnlock(&driver->lock); -} diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index c4e3b66751..b5289b2448 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -995,7 +995,7 @@ static int lxcControllerClearCapabilities(void) } static bool wantReboot; -static virMutex lock = VIR_MUTEX_INITIALIZER; +static virMutex mutex = VIR_MUTEX_INITIALIZER; static int virLXCControllerEventSendExit(virLXCController *ctrl, @@ -1012,13 +1012,13 @@ static void virLXCControllerSignalChildIO(virNetDaemon *dmn G_GNUC_UNUSED, ret = waitpid(-1, &status, WNOHANG); VIR_DEBUG("Got sig child %d vs %lld", ret, (long long)ctrl->initpid); if (ret == ctrl->initpid) { - virMutexLock(&lock); - if (WIFSIGNALED(status) && - WTERMSIG(status) == SIGHUP) { - VIR_DEBUG("Status indicates reboot"); - wantReboot = true; + VIR_WITH_MUTEX_LOCK_GUARD(&mutex) { + if (WIFSIGNALED(status) && + WTERMSIG(status) == SIGHUP) { + VIR_DEBUG("Status indicates reboot"); + wantReboot = true; + } } - virMutexUnlock(&lock); virLXCControllerEventSendExit(ctrl, wantReboot ? 1 : 0); } } @@ -1132,8 +1132,8 @@ static void virLXCControllerConsoleUpdateWatch(virLXCControllerConsole *console) static void virLXCControllerConsoleEPoll(int watch, int fd, int events, void *opaque) { virLXCControllerConsole *console = opaque; + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); - virMutexLock(&lock); VIR_DEBUG("IO event watch=%d fd=%d events=%d fromHost=%zu fromcont=%zu", watch, fd, events, console->fromHostLen, @@ -1149,7 +1149,7 @@ static void virLXCControllerConsoleEPoll(int watch, int fd, int events, void *op virReportSystemError(errno, "%s", _("Unable to wait on epoll")); virNetDaemonQuit(console->daemon); - goto cleanup; + return; } if (ret == 0) @@ -1171,16 +1171,13 @@ static void virLXCControllerConsoleEPoll(int watch, int fd, int events, void *op break; } } - - cleanup: - virMutexUnlock(&lock); } static void virLXCControllerConsoleIO(int watch, int fd, int events, void *opaque) { virLXCControllerConsole *console = opaque; + VIR_LOCK_GUARD lock = virLockGuardLock(&mutex); - virMutexLock(&lock); VIR_DEBUG("IO event watch=%d fd=%d events=%d fromHost=%zu fromcont=%zu", watch, fd, events, console->fromHostLen, @@ -1254,7 +1251,6 @@ static void virLXCControllerConsoleIO(int watch, int fd, int events, void *opaqu } virLXCControllerConsoleUpdateWatch(console); - virMutexUnlock(&lock); return; error: @@ -1262,7 +1258,6 @@ static void virLXCControllerConsoleIO(int watch, int fd, int events, void *opaqu virEventRemoveHandle(console->hostWatch); console->contWatch = console->hostWatch = -1; virNetDaemonQuit(console->daemon); - virMutexUnlock(&lock); } diff --git a/src/lxc/lxc_fuse.c b/src/lxc/lxc_fuse.c index c2fe8f0e60..2e600e4402 100644 --- a/src/lxc/lxc_fuse.c +++ b/src/lxc/lxc_fuse.c @@ -260,11 +260,11 @@ static struct fuse_operations lxcProcOper = { static void lxcFuseDestroy(struct virLXCFuse *fuse) { - virMutexLock(&fuse->lock); + VIR_LOCK_GUARD lock = virLockGuardLock(&fuse->lock); + fuse_unmount(fuse->mountpoint, fuse->ch); fuse_destroy(fuse->fuse); fuse->fuse = NULL; - virMutexUnlock(&fuse->lock); } static void lxcFuseRun(void *opaque) @@ -346,10 +346,10 @@ void lxcFreeFuse(struct virLXCFuse **f) if (fuse) { /* exit fuse_loop, lxcFuseRun thread may try to destroy * fuse->fuse at the same time,so add a lock here. */ - virMutexLock(&fuse->lock); - if (fuse->fuse) - fuse_exit(fuse->fuse); - virMutexUnlock(&fuse->lock); + VIR_WITH_MUTEX_LOCK_GUARD(&fuse->lock) { + if (fuse->fuse) + fuse_exit(fuse->fuse); + } g_free(fuse->mountpoint); g_free(*f); -- 2.31.1
No functional change intended. This change makes the refactoring to automatic mutex management easier to follow. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/ch/ch_conf.c | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index be12934dcd..cdf69e3e70 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -87,21 +87,22 @@ virCaps *virCHDriverCapsInit(void) virCaps *virCHDriverGetCapabilities(virCHDriver *driver, bool refresh) { - virCaps *ret; - if (refresh) { - virCaps *caps = NULL; - if ((caps = virCHDriverCapsInit()) == NULL) - return NULL; + virCaps *ret = NULL; + virCaps *caps = NULL; - chDriverLock(driver); + if (refresh && !(caps = virCHDriverCapsInit())) + return NULL; + + chDriverLock(driver); + + if (refresh) { virObjectUnref(driver->caps); driver->caps = caps; - } else { - chDriverLock(driver); } ret = virObjectRef(driver->caps); chDriverUnlock(driver); + return ret; } -- 2.31.1
This leaves a bogus `virMutexUnlock` in `chDomainCreateXML`. Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/ch/ch_conf.c | 20 ++++++++------------ src/ch/ch_conf.h | 10 ---------- src/ch/ch_driver.c | 30 +++++++++++++++--------------- 3 files changed, 23 insertions(+), 37 deletions(-) diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index cdf69e3e70..ccc01ecdab 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -93,16 +93,15 @@ virCaps *virCHDriverGetCapabilities(virCHDriver *driver, if (refresh && !(caps = virCHDriverCapsInit())) return NULL; - chDriverLock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + if (refresh) { + virObjectUnref(driver->caps); + driver->caps = caps; + } - if (refresh) { - virObjectUnref(driver->caps); - driver->caps = caps; + ret = virObjectRef(driver->caps); } - ret = virObjectRef(driver->caps); - chDriverUnlock(driver); - return ret; } @@ -159,11 +158,8 @@ virCHDriverConfigNew(bool privileged) virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver) { - virCHDriverConfig *cfg; - chDriverLock(driver); - cfg = virObjectRef(driver->config); - chDriverUnlock(driver); - return cfg; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->config); } static void diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h index c56caa3f5f..b927621a97 100644 --- a/src/ch/ch_conf.h +++ b/src/ch/ch_conf.h @@ -78,13 +78,3 @@ virDomainXMLOption *chDomainXMLConfInit(virCHDriver *driver); virCHDriverConfig *virCHDriverConfigNew(bool privileged); virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver); int chExtractVersion(virCHDriver *driver); - -static inline void chDriverLock(virCHDriver *driver) -{ - virMutexLock(&driver->lock); -} - -static inline void chDriverUnlock(virCHDriver *driver) -{ - virMutexUnlock(&driver->lock); -} diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 3223f31367..945a1aa225 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -106,9 +106,10 @@ static int chConnectGetVersion(virConnectPtr conn, if (virConnectGetVersionEnsureACL(conn) < 0) return -1; - chDriverLock(driver); - *version = driver->version; - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + *version = driver->version; + } + return 0; } @@ -241,7 +242,7 @@ chDomainCreateXML(virConnectPtr conn, virDomainObjListRemove(driver->domains, vm); } virDomainObjEndAPI(&vm); - chDriverUnlock(driver); + virMutexUnlock(&driver->lock); return dom; } @@ -368,8 +369,8 @@ static int chDomainIsActive(virDomainPtr dom) virCHDriver *driver = dom->conn->privateData; virDomainObj *vm; int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); - chDriverLock(driver); if (!(vm = virCHDomainObjFromDomain(dom))) goto cleanup; @@ -380,7 +381,6 @@ static int chDomainIsActive(virDomainPtr dom) cleanup: virDomainObjEndAPI(&vm); - chDriverUnlock(driver); return ret; } @@ -638,9 +638,9 @@ static virDomainPtr chDomainLookupByID(virConnectPtr conn, virDomainObj *vm; virDomainPtr dom = NULL; - chDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByID(driver->domains, id); + } if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -665,9 +665,9 @@ static virDomainPtr chDomainLookupByName(virConnectPtr conn, virDomainObj *vm; virDomainPtr dom = NULL; - chDriverLock(driver); - vm = virDomainObjListFindByName(driver->domains, name); - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByName(driver->domains, name); + } if (!vm) { virReportError(VIR_ERR_NO_DOMAIN, @@ -692,9 +692,9 @@ static virDomainPtr chDomainLookupByUUID(virConnectPtr conn, virDomainObj *vm; virDomainPtr dom = NULL; - chDriverLock(driver); - vm = virDomainObjListFindByUUID(driver->domains, uuid); - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByUUID(driver->domains, uuid); + } if (!vm) { char uuidstr[VIR_UUID_STRING_BUFLEN]; -- 2.31.1
On 2/7/22 14:12, Tim Wiederhake wrote:
This leaves a bogus `virMutexUnlock` in `chDomainCreateXML`.
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/ch/ch_conf.c | 20 ++++++++------------ src/ch/ch_conf.h | 10 ---------- src/ch/ch_driver.c | 30 +++++++++++++++--------------- 3 files changed, 23 insertions(+), 37 deletions(-)
diff --git a/src/ch/ch_conf.c b/src/ch/ch_conf.c index cdf69e3e70..ccc01ecdab 100644 --- a/src/ch/ch_conf.c +++ b/src/ch/ch_conf.c @@ -93,16 +93,15 @@ virCaps *virCHDriverGetCapabilities(virCHDriver *driver, if (refresh && !(caps = virCHDriverCapsInit())) return NULL;
- chDriverLock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + if (refresh) { + virObjectUnref(driver->caps); + driver->caps = caps; + }
- if (refresh) { - virObjectUnref(driver->caps); - driver->caps = caps; + ret = virObjectRef(driver->caps); }
- ret = virObjectRef(driver->caps); - chDriverUnlock(driver); - return ret; }
@@ -159,11 +158,8 @@ virCHDriverConfigNew(bool privileged)
virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver) { - virCHDriverConfig *cfg; - chDriverLock(driver); - cfg = virObjectRef(driver->config); - chDriverUnlock(driver); - return cfg; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->config); }
static void diff --git a/src/ch/ch_conf.h b/src/ch/ch_conf.h index c56caa3f5f..b927621a97 100644 --- a/src/ch/ch_conf.h +++ b/src/ch/ch_conf.h @@ -78,13 +78,3 @@ virDomainXMLOption *chDomainXMLConfInit(virCHDriver *driver); virCHDriverConfig *virCHDriverConfigNew(bool privileged); virCHDriverConfig *virCHDriverGetConfig(virCHDriver *driver); int chExtractVersion(virCHDriver *driver); - -static inline void chDriverLock(virCHDriver *driver) -{ - virMutexLock(&driver->lock); -} - -static inline void chDriverUnlock(virCHDriver *driver) -{ - virMutexUnlock(&driver->lock); -}
Until here I have no objections. But the code below, I mean the pre-existing one is total mess and we should clean it up first, because it will drop some locking.
diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 3223f31367..945a1aa225 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -106,9 +106,10 @@ static int chConnectGetVersion(virConnectPtr conn, if (virConnectGetVersionEnsureACL(conn) < 0) return -1;
- chDriverLock(driver); - *version = driver->version; - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + *version = driver->version; + } +
I see no point in locking the driver here. Firstly, the @version member is set in chExtractVersion() which is called exactly once from chStateInitialize() and driver init happens in a single thread (for a good reason). And even if it didn't, the initialize function never locks the driver, so this lock here guards nothing.
return 0; }
@@ -241,7 +242,7 @@ chDomainCreateXML(virConnectPtr conn, virDomainObjListRemove(driver->domains, vm); } virDomainObjEndAPI(&vm); - chDriverUnlock(driver); + virMutexUnlock(&driver->lock);
I'm sorry, but what? I know, not your fault, but this is clearly an imbalance in mutex locking. Not to mention that this function doesn't end job properly (typical 'goto error' instead of 'goto endjob' problem).
return dom; }
@@ -368,8 +369,8 @@ static int chDomainIsActive(virDomainPtr dom) virCHDriver *driver = dom->conn->privateData; virDomainObj *vm; int ret = -1; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock);
- chDriverLock(driver);
I'm failing to see the purpose of this locking in the first place. Again, neither of these problems are your fault, I'm just pointing them out.
if (!(vm = virCHDomainObjFromDomain(dom))) goto cleanup;
@@ -380,7 +381,6 @@ static int chDomainIsActive(virDomainPtr dom)
cleanup: virDomainObjEndAPI(&vm); - chDriverUnlock(driver); return ret; }
@@ -638,9 +638,9 @@ static virDomainPtr chDomainLookupByID(virConnectPtr conn, virDomainObj *vm; virDomainPtr dom = NULL;
- chDriverLock(driver); - vm = virDomainObjListFindByID(driver->domains, id); - chDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + vm = virDomainObjListFindByID(driver->domains, id); + }
This is very suboptimal thing to do. I've specifically introduced RW locks to virDomainObjList so that we don't have to lock the whole driver when accessing the list. Alright, let me post patches for these and then get back to you. Patches until here look good. Michal
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/ch/ch_driver.c | 1 - 1 file changed, 1 deletion(-) diff --git a/src/ch/ch_driver.c b/src/ch/ch_driver.c index 945a1aa225..4c1a9bae77 100644 --- a/src/ch/ch_driver.c +++ b/src/ch/ch_driver.c @@ -242,7 +242,6 @@ chDomainCreateXML(virConnectPtr conn, virDomainObjListRemove(driver->domains, vm); } virDomainObjEndAPI(&vm); - virMutexUnlock(&driver->lock); return dom; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/network/bridge_driver.c | 41 +++++++++---------------------------- 1 file changed, 10 insertions(+), 31 deletions(-) diff --git a/src/network/bridge_driver.c b/src/network/bridge_driver.c index 23d9ed4226..3750da7962 100644 --- a/src/network/bridge_driver.c +++ b/src/network/bridge_driver.c @@ -100,28 +100,11 @@ networkGetDriver(void) } -static void -networkDriverLock(virNetworkDriverState *driver) -{ - virMutexLock(&driver->lock); -} - - -static void -networkDriverUnlock(virNetworkDriverState *driver) -{ - virMutexUnlock(&driver->lock); -} - - static dnsmasqCaps * networkGetDnsmasqCaps(virNetworkDriverState *driver) { - dnsmasqCaps *ret; - networkDriverLock(driver); - ret = virObjectRef(driver->dnsmasqCaps); - networkDriverUnlock(driver); - return ret; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->dnsmasqCaps); } @@ -133,10 +116,11 @@ networkDnsmasqCapsRefresh(virNetworkDriverState *driver) if (!(caps = dnsmasqCapsNewFromBinary())) return -1; - networkDriverLock(driver); - virObjectUnref(driver->dnsmasqCaps); - driver->dnsmasqCaps = caps; - networkDriverUnlock(driver); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + virObjectUnref(driver->dnsmasqCaps); + driver->dnsmasqCaps = caps; + } + return 0; } @@ -2725,27 +2709,22 @@ static int networkBridgeNameValidate(virNetworkObjList *nets, virNetworkDef *def) { - virMutexLock(&bridgeNameValidateMutex); + VIR_LOCK_GUARD lock = virLockGuardLock(&bridgeNameValidateMutex); if (def->bridge && !strstr(def->bridge, "%d")) { if (virNetworkObjBridgeInUse(nets, def->bridge, def->name)) { virReportError(VIR_ERR_INTERNAL_ERROR, _("bridge name '%s' already in use."), def->bridge); - goto error; + return -1; } } else { /* Allocate a bridge name */ if (networkFindUnusedBridgeName(nets, def) < 0) - goto error; + return -1; } - virMutexUnlock(&bridgeNameValidateMutex); return 0; - - error: - virMutexUnlock(&bridgeNameValidateMutex); - return -1; } -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/bhyve/bhyve_conf.c | 7 ++----- src/bhyve/bhyve_driver.c | 12 ------------ src/bhyve/bhyve_utils.h | 3 --- 3 files changed, 2 insertions(+), 20 deletions(-) diff --git a/src/bhyve/bhyve_conf.c b/src/bhyve/bhyve_conf.c index 5eca0855f0..28981c90b7 100644 --- a/src/bhyve/bhyve_conf.c +++ b/src/bhyve/bhyve_conf.c @@ -85,11 +85,8 @@ virBhyveLoadDriverConfig(struct _virBhyveDriverConfig *cfg, struct _virBhyveDriverConfig * virBhyveDriverGetConfig(struct _bhyveConn *driver) { - struct _virBhyveDriverConfig *cfg; - bhyveDriverLock(driver); - cfg = virObjectRef(driver->config); - bhyveDriverUnlock(driver); - return cfg; + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + return virObjectRef(driver->config); } static void diff --git a/src/bhyve/bhyve_driver.c b/src/bhyve/bhyve_driver.c index 47ee98e650..f580e87419 100644 --- a/src/bhyve/bhyve_driver.c +++ b/src/bhyve/bhyve_driver.c @@ -70,18 +70,6 @@ VIR_LOG_INIT("bhyve.bhyve_driver"); struct _bhyveConn *bhyve_driver = NULL; -void -bhyveDriverLock(struct _bhyveConn *driver) -{ - virMutexLock(&driver->lock); -} - -void -bhyveDriverUnlock(struct _bhyveConn *driver) -{ - virMutexUnlock(&driver->lock); -} - static int bhyveAutostartDomain(virDomainObj *vm, void *opaque) { diff --git a/src/bhyve/bhyve_utils.h b/src/bhyve/bhyve_utils.h index af7b15486a..5d6e198b09 100644 --- a/src/bhyve/bhyve_utils.h +++ b/src/bhyve/bhyve_utils.h @@ -73,6 +73,3 @@ struct bhyveAutostartData { struct _bhyveConn *driver; virConnectPtr conn; }; - -void bhyveDriverLock(struct _bhyveConn *driver); -void bhyveDriverUnlock(struct _bhyveConn *driver); -- 2.31.1
Signed-off-by: Tim Wiederhake <twiederh@redhat.com> --- src/node_device/node_device_driver.c | 23 +++--------- src/node_device/node_device_udev.c | 52 ++++++++++++---------------- 2 files changed, 26 insertions(+), 49 deletions(-) diff --git a/src/node_device/node_device_driver.c b/src/node_device/node_device_driver.c index d19ed7d948..7b2fb3d953 100644 --- a/src/node_device/node_device_driver.c +++ b/src/node_device/node_device_driver.c @@ -156,33 +156,18 @@ nodeDeviceUpdateDriverName(virNodeDeviceDef *def G_GNUC_UNUSED) #endif -void -nodeDeviceLock(void) -{ - virMutexLock(&driver->lock); -} - - -void -nodeDeviceUnlock(void) -{ - virMutexUnlock(&driver->lock); -} - - static int nodeDeviceInitWait(void) { - nodeDeviceLock(); + VIR_LOCK_GUARD lock = virLockGuardLock(&driver->lock); + while (!driver->initialized) { if (virCondWait(&driver->initCond, &driver->lock) < 0) { - virReportSystemError(errno, "%s", - _("failed to wait on condition")); - nodeDeviceUnlock(); + virReportSystemError(errno, "%s", _("failed to wait on condition")); return -1; } } - nodeDeviceUnlock(); + return 0; } diff --git a/src/node_device/node_device_udev.c b/src/node_device/node_device_udev.c index a9e8bf10da..3d5e25424a 100644 --- a/src/node_device/node_device_udev.c +++ b/src/node_device/node_device_udev.c @@ -93,9 +93,9 @@ udevEventDataDispose(void *obj) udev_monitor_unref(priv->udev_monitor); udev_unref(udev); - virMutexLock(&priv->mdevctlLock); - g_list_free_full(priv->mdevctlMonitors, g_object_unref); - virMutexUnlock(&priv->mdevctlLock); + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + g_list_free_full(priv->mdevctlMonitors, g_object_unref); + } virMutexDestroy(&priv->mdevctlLock); virCondDestroy(&priv->threadCond); @@ -348,13 +348,9 @@ udevTranslatePCIIds(unsigned int vendor, m.match_data = 0; /* pci_get_strings returns void and unfortunately is not thread safe. */ - virMutexLock(&pciaccessMutex); - pci_get_strings(&m, - &device_name, - &vendor_name, - NULL, - NULL); - virMutexUnlock(&pciaccessMutex); + VIR_WITH_MUTEX_LOCK_GUARD(&pciaccessMutex) { + pci_get_strings(&m, &device_name, &vendor_name, NULL, NULL); + } *vendor_string = g_strdup(vendor_name); *product_string = g_strdup(device_name); @@ -373,11 +369,11 @@ udevProcessPCI(struct udev_device *device, virPCIDeviceAddress devAddr; int ret = -1; char *p; - bool privileged; + bool privileged = false; - nodeDeviceLock(); - privileged = driver->privileged; - nodeDeviceUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + privileged = driver->privileged; + } pci_dev->klass = -1; if (udevGetIntProperty(device, "PCI_CLASS", &pci_dev->klass, 16) < 0) @@ -1989,10 +1985,10 @@ nodeStateInitializeEnumerate(void *opaque) goto error; cleanup: - nodeDeviceLock(); - driver->initialized = true; - virCondBroadcast(&driver->initCond); - nodeDeviceUnlock(); + VIR_WITH_MUTEX_LOCK_GUARD(&driver->lock) { + driver->initialized = true; + virCondBroadcast(&driver->initCond); + } return; @@ -2036,12 +2032,10 @@ static void mdevctlHandlerThread(void *opaque G_GNUC_UNUSED) { udevEventData *priv = driver->privateData; + VIR_LOCK_GUARD lock = virLockGuardLock(&priv->mdevctlLock); - /* ensure only a single thread can query mdevctl at a time */ - virMutexLock(&priv->mdevctlLock); if (nodeDeviceUpdateMediatedDevices() < 0) VIR_WARN("mdevctl failed to updated mediated devices"); - virMutexUnlock(&priv->mdevctlLock); } @@ -2143,13 +2137,10 @@ mdevctlEnableMonitor(udevEventData *priv) * mdevctl configuration is stored in a directory tree within * /etc/mdevctl.d/. There is a directory for each parent device, which * contains a file defining each mediated device */ - virMutexLock(&priv->mdevctlLock); - if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, - mdevctlConfigDir))) { - virMutexUnlock(&priv->mdevctlLock); - return -1; + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + if (!(priv->mdevctlMonitors = monitorFileRecursively(priv, mdevctlConfigDir))) + return -1; } - virMutexUnlock(&priv->mdevctlLock); return 0; } @@ -2171,9 +2162,10 @@ mdevctlEventHandleCallback(GFileMonitor *monitor G_GNUC_UNUSED, if (file_type == G_FILE_TYPE_DIRECTORY) { GList *newmonitors = monitorFileRecursively(priv, file); - virMutexLock(&priv->mdevctlLock); - priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, newmonitors); - virMutexUnlock(&priv->mdevctlLock); + VIR_WITH_MUTEX_LOCK_GUARD(&priv->mdevctlLock) { + priv->mdevctlMonitors = g_list_concat(priv->mdevctlMonitors, + newmonitors); + } } } -- 2.31.1
On 2/7/22 14:12, Tim Wiederhake wrote:
Use the recently implemented VIR_LOCK_GUARD and VIR_WITH_MUTEX_LOCK_GUARD to simplify mutex management.
This made a solitary "virMutexUnlock()" call without previous call to "virMutexLock()" in ch_driver.c obvious, which is removed in patch #8.
Tim Wiederhake (11): virthreadpool: Use automatic memory management virthreadpool: Cleanup libxl: Use automatic memory management lxc: Prepare virLXCDriverGetCapabilities for automatic mutex management lxc: Use automatic mutex management ch: Prepare virCHDriverGetCapabilities for automatic mutex management ch: Use automatic mutex management ch: Remove solitary virMutexUnlock network: Use automatic mutex management bhyve_driver: Use automatic mutex management node_device: Use automatic mutex management
src/bhyve/bhyve_conf.c | 7 +-- src/bhyve/bhyve_driver.c | 12 ---- src/bhyve/bhyve_utils.h | 3 - src/ch/ch_conf.c | 31 +++++----- src/ch/ch_conf.h | 10 ---- src/ch/ch_driver.c | 29 +++++---- src/libxl/libxl_conf.c | 8 +-- src/libxl/libxl_conf.h | 12 ---- src/libxl/libxl_logger.c | 20 +++---- src/lxc/lxc_conf.c | 41 ++++++------- src/lxc/lxc_conf.h | 9 --- src/lxc/lxc_controller.c | 25 ++++---- src/lxc/lxc_fuse.c | 12 ++-- src/network/bridge_driver.c | 41 ++++--------- src/node_device/node_device_driver.c | 23 ++----- src/node_device/node_device_udev.c | 52 +++++++--------- src/util/virthreadpool.c | 90 ++++++++-------------------- 17 files changed, 138 insertions(+), 287 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> And sorry for the conflict, but it's trivial enough to resolve. Michal
participants (2)
-
Michal Prívozník -
Tim Wiederhake