Re: [libvirt] [PATCH v2 2/6] rpc: Initialize a worker pool for max_workers=0 as well

Semantically, there is no difference between an uninitialized worker pool and an initialized worker pool with zero workers. Let's allow the worker pool to be initialized for max_workers=0 as well then which makes the API more symmetric and simplifies code. Validity of the worker pool is delegated to virThreadPoolGetMaxWorkersLocked instead. This patch fixes segmentation faults in virNetServerGetThreadPoolParameters and virNetServerSetThreadPoolParameters for the case when no worker pool is actually initialized (max_workers=0). Signed-off-by: Marc Hartmayer <mhartmay(a)linux.ibm.com&gt; Reviewed-by: Boris Fiuczynski <fiuczy(a)linux.ibm.com&gt; Reviewed-by: Bjoern Walk <bwalk(a)linux.ibm.com&gt; --- src/libvirt_private.syms | 4 +++ src/rpc/virnetserver.c | 13 ++++----- src/util/virthreadpool.c | 73 ++++++++++++++++++++++++++++++++++++------------ src/util/virthreadpool.h | 8 ++++++ 4 files changed, 73 insertions(+), 25 deletions(-)

diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index ffe5dfd19b11..aa496ddf8012 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -3005,11 +3005,15 @@ virThreadPoolGetCurrentWorkers; virThreadPoolGetFreeWorkers; virThreadPoolGetJobQueueDepth; virThreadPoolGetMaxWorkers; +virThreadPoolGetMaxWorkersLocked; virThreadPoolGetMinWorkers; virThreadPoolGetPriorityWorkers; +virThreadPoolLock; virThreadPoolNewFull; virThreadPoolSendJob; +virThreadPoolSendJobLocked; virThreadPoolSetParameters; +virThreadPoolUnlock; # util/virtime.h diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 091e3ef23406..fdee08fee7cd 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -203,7 +203,8 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client, * of disposing @srv */ virObjectUnlock(srv); - if (srv->workers) { + virThreadPoolLock(srv->workers); + if (virThreadPoolGetMaxWorkersLocked(srv->workers) > 0) { virNetServerJobPtr job; if (VIR_ALLOC(job) < 0) @@ -218,7 +219,7 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client, } virObjectRef(client); - if (virThreadPoolSendJob(srv->workers, priority, job) < 0) { + if (virThreadPoolSendJobLocked(srv->workers, priority, job) < 0) { virObjectUnref(client); VIR_FREE(job); virObjectUnref(prog); @@ -228,10 +229,12 @@ virNetServerDispatchNewMessage(virNetServerClientPtr client, if (virNetServerProcessMsg(srv, client, prog, msg) < 0) goto error; } + virThreadPoolUnlock(srv->workers); return; error: + virThreadPoolUnlock(srv->workers); virNetMessageFree(msg); virNetServerClientClose(client); } @@ -363,8 +366,7 @@ virNetServerPtr virNetServerNew(const char *name, if (!(srv = virObjectLockableNew(virNetServerClass))) return NULL; - if (max_workers && - !(srv->workers = virThreadPoolNew(min_workers, max_workers, + if (!(srv->workers = virThreadPoolNew(min_workers, max_workers, priority_workers, virNetServerHandleJob, srv))) @@ -575,21 +577,18 @@ virJSONValuePtr virNetServerPreExecRestart(virNetServerPtr srv) goto error; if (virJSONValueObjectAppendNumberUint(object, "min_workers", - srv->workers == NULL ? 0 : virThreadPoolGetMinWorkers(srv->workers)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot set min_workers data in JSON document")); goto error; } if (virJSONValueObjectAppendNumberUint(object, "max_workers", - srv->workers == NULL ? 0 : virThreadPoolGetMaxWorkers(srv->workers)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot set max_workers data in JSON document")); goto error; } if (virJSONValueObjectAppendNumberUint(object, "priority_workers", - srv->workers == NULL ? 0 : virThreadPoolGetPriorityWorkers(srv->workers)) < 0) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", _("Cannot set priority_workers data in JSON document")); diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index 10f2bd2c3a03..f18eafb35deb 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -318,17 +318,27 @@ size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool) return ret; } -size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool) + +size_t +virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool) +{ + return pool->maxWorkers; +} + + +size_t +virThreadPoolGetMaxWorkers(virThreadPoolPtr pool) { size_t ret; virMutexLock(&pool->mutex); - ret = pool->maxWorkers; + ret = virThreadPoolGetMaxWorkersLocked(pool); virMutexUnlock(&pool->mutex); return ret; } + size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool) { size_t ret; @@ -373,27 +383,38 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool) return ret; } -/* - * @priority - job priority - * Return: 0 on success, -1 otherwise - */ -int virThreadPoolSendJob(virThreadPoolPtr pool, - unsigned int priority, - void *jobData) + +void +virThreadPoolLock(virThreadPoolPtr pool) +{ + virMutexLock(&pool->mutex); +} + + +void +virThreadPoolUnlock(virThreadPoolPtr pool) +{ + virMutexUnlock(&pool->mutex); +} + + +int +virThreadPoolSendJobLocked(virThreadPoolPtr pool, + unsigned int priority, + void *jobData) { virThreadPoolJobPtr job; - virMutexLock(&pool->mutex); 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; if (VIR_ALLOC(job) < 0) - goto error; + return -1; job->data = jobData; job->priority = priority; @@ -415,14 +436,30 @@ int virThreadPoolSendJob(virThreadPoolPtr pool, if (priority) virCondSignal(&pool->prioCond); - virMutexUnlock(&pool->mutex); return 0; - - error: - virMutexUnlock(&pool->mutex); - return -1; } + +/* + * @priority - job priority + * Return: 0 on success, -1 otherwise + */ +int +virThreadPoolSendJob(virThreadPoolPtr pool, + unsigned int priority, + void *jobData) +{ + int ret; + + virMutexLock(&pool->mutex); + ret = virThreadPoolSendJobLocked(pool, + priority, + jobData); + virMutexUnlock(&pool->mutex); + return ret; +} + + int virThreadPoolSetParameters(virThreadPoolPtr pool, long long int minWorkers, diff --git a/src/util/virthreadpool.h b/src/util/virthreadpool.h index e1f362f5bb60..2d65c2bc4ac3 100644 --- a/src/util/virthreadpool.h +++ b/src/util/virthreadpool.h @@ -45,6 +45,7 @@ virThreadPoolPtr virThreadPoolNewFull(size_t minWorkers, size_t virThreadPoolGetMinWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetMaxWorkers(virThreadPoolPtr pool); +size_t virThreadPoolGetMaxWorkersLocked(virThreadPoolPtr pool); size_t virThreadPoolGetPriorityWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetCurrentWorkers(virThreadPoolPtr pool); size_t virThreadPoolGetFreeWorkers(virThreadPoolPtr pool); @@ -52,10 +53,17 @@ size_t virThreadPoolGetJobQueueDepth(virThreadPoolPtr pool); void virThreadPoolFree(virThreadPoolPtr pool); +void virThreadPoolLock(virThreadPoolPtr pool); +void virThreadPoolUnlock(virThreadPoolPtr pool); + int virThreadPoolSendJob(virThreadPoolPtr pool, unsigned int priority, void *jobdata) ATTRIBUTE_NONNULL(1) ATTRIBUTE_RETURN_CHECK; +int virThreadPoolSendJobLocked(virThreadPoolPtr pool, + unsigned int priority, + void *jobData) ATTRIBUTE_NONNULL(1) + ATTRIBUTE_RETURN_CHECK; int virThreadPoolSetParameters(virThreadPoolPtr pool, long long int minWorkers,