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

John Ferlan jferlan at redhat.com
Thu Jul 19 14:52:41 UTC 2018



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 at linux.ibm.com>
> Reviewed-by: Boris Fiuczynski <fiuczy at linux.ibm.com>
> Reviewed-by: Bjoern Walk <bwalk at linux.ibm.com>
> ---
>  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,
> 




More information about the libvir-list mailing list