On 07/03/2018 07:37 AM, Marc Hartmayer wrote:
> 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@xxxxxxxxxxxxx>
> Reviewed-by: Boris Fiuczynski <fiuczy@xxxxxxxxxxxxx>
> Reviewed-by: Bjoern Walk <bwalk@xxxxxxxxxxxxx>
> ---
> 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(-)
>
So it seems there's multiple things going on in this patch - maybe
they're semantically tied and I'm missing it ;-)...
The virNetServerNew to not inhibit the call to virThreadPoolNew if
max_workers == 0 would seem to be easily separable with the other places
that would check if srv->workers == NULL before continuing.
I don't understand why virNetServerDispatchNewMessage needs anything
more than if (virThreadPoolGetMaxWorkers(srv->workers)
If I think back to the previous patch, then why not:
size_t workers = 0;
...
if (srv->workers)
workers = virThreadPoolGetMaxWorkers(srv->workers);
/* we can unlock @srv since @prog can only become invalid in case
* of disposing @srv, but let's grab a ref first to ensure nothing
* else disposes of it before we use it. */
virObjectRef(srv);
virObjectUnlock(srv);
if (workers > 0) {
...
In the long run, I don't think it's been the desire to expose so many
virthreadpool.c API's - especially the locks.
John
> 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,
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
