On Thu, Jul 19, 2018 at 04:51 PM +0200, John Ferlan <jferlan@redhat.com> wrote:

On 07/03/2018 07:37 AM, Marc Hartmayer wrote:

...

@srv must be unlocked for the call virNetServerProcessMsg otherwise a deadlock can occur.

Since the pointer 'srv->workers' will never be changed after initialization and the thread pool has it's own locking we can release the lock of 'srv' earlier. This also fixes the deadlock.

Signed-off-by: Marc Hartmayer <mhartmay@linux.ibm.com> Reviewed-by: Boris Fiuczynski <fiuczy@linux.ibm.com> Reviewed-by: Bjoern Walk <bwalk@linux.ibm.com> --- src/rpc/virnetserver.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/src/rpc/virnetserver.c b/src/rpc/virnetserver.c index 5c7f7dd08fe1..091e3ef23406 100644 --- a/src/rpc/virnetserver.c +++ b/src/rpc/virnetserver.c @@ -51,6 +51,7 @@ struct _virNetServer {

char *name;

+ /* Immutable pointer, self-locking APIs */ virThreadPoolPtr workers;

char *mdnsGroupName; @@ -177,9 +178,11 @@ static void virNetServerHandleJob(void *jobOpaque, void *opaque) VIR_FREE(job); }

-static void virNetServerDispatchNewMessage(virNetServerClientPtr client, - virNetMessagePtr msg, - void *opaque) + +static void +virNetServerDispatchNewMessage(virNetServerClientPtr client, + virNetMessagePtr msg, + void *opaque) { virNetServerPtr srv = opaque; virNetServerProgramPtr prog = NULL; @@ -196,6 +199,9 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr client, break; } }

Should we grab an object reference then to avoid the chance that something disposes srv right after it's unlocked? And then Unref instead of Unlock later on?

Following virThreadPoolSendJob finds that it'll grab the srv->workers pool mutex and check the quit flag before adding a job to and of course as you point out virNetServerProcessMsg would eventually assume that the server is unlocked in virNetServerProgramDispatchCall before calling dispatcher->func.

With a Ref/Unref, I'd feel better and thus consider this,

Yep, I’m fine with that.

Reviewed-by: John Ferlan <jferlan@redhat.com>

Thanks!

John

...

+ /* we can unlock @srv since @prog can only become invalid in case + * of disposing @srv */ + virObjectUnlock(srv);

if (srv->workers) { virNetServerJobPtr job; @@ -223,15 +229,14 @@ static void virNetServerDispatchNewMessage(virNetServerClientPtr client, goto error; }

- virObjectUnlock(srv); return;

error: virNetMessageFree(msg); virNetServerClientClose(client); - virObjectUnlock(srv); }

+ /** * virNetServerCheckLimits: * @srv: server to check limits on

-- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294

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