[libvirt] [PATCH] threadpool: Don't wait on condition if pool has no workers
Pool creates new workers dynamically. However, it is possible for a pool to have no workers. If we want to free that pool, we don't want to wait on quit condition as it will never be signaled. --- Pushing under trivial rule src/util/threadpool.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-) diff --git a/src/util/threadpool.c b/src/util/threadpool.c index c16e2af..883d1e5 100644 --- a/src/util/threadpool.c +++ b/src/util/threadpool.c @@ -258,7 +258,8 @@ void virThreadPoolFree(virThreadPoolPtr pool) virCondBroadcast(&pool->prioCond); } - ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); + if (pool->nWorkers > 0 || pool->nPrioWorkers > 0) + ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); while ((job = pool->jobList.head)) { pool->jobList.head = pool->jobList.head->next; -- 1.7.3.4
On 12/09/2011 07:26 AM, Michal Privoznik wrote:
Pool creates new workers dynamically. However, it is possible for a pool to have no workers. If we want to free that pool, we don't want to wait on quit condition as it will never be signaled. --- Pushing under trivial rule
src/util/threadpool.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/src/util/threadpool.c b/src/util/threadpool.c index c16e2af..883d1e5 100644 --- a/src/util/threadpool.c +++ b/src/util/threadpool.c @@ -258,7 +258,8 @@ void virThreadPoolFree(virThreadPoolPtr pool) virCondBroadcast(&pool->prioCond); }
- ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); + if (pool->nWorkers > 0 || pool->nPrioWorkers > 0) + ignore_value(virCondWait(&pool->quit_cond, &pool->mutex));
Shouldn't this be a while loop, instead of an if()? That is, the documentation for pthread_cond_wait (which is what virCondWait uses under the hood) says that any robust implementation must be ready for spurious wakeups, and must re-check the condition and loop until the condition has been met (here, the condition we want is that all workers must have exited prior to us continuing). -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
--- src/util/threads.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-) diff --git a/src/util/threads.h b/src/util/threads.h index b72610c..7d8a6ed 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -88,6 +88,11 @@ void virMutexUnlock(virMutexPtr m); int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c) ATTRIBUTE_RETURN_CHECK; +/* virCondWait, virCondWaitUntil: + * These functions can return without associated predicate + * changing value. Therefore in nearly all cases there + * should be enclosed in a while loop that checks predicate. + */ int virCondWait(virCondPtr c, virMutexPtr m) ATTRIBUTE_RETURN_CHECK; int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) ATTRIBUTE_RETURN_CHECK; void virCondSignal(virCondPtr c); -- 1.7.3.4
On 12/09/2011 09:29 AM, Michal Privoznik wrote:
--- src/util/threads.h | 5 +++++ 1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/src/util/threads.h b/src/util/threads.h index b72610c..7d8a6ed 100644 --- a/src/util/threads.h +++ b/src/util/threads.h @@ -88,6 +88,11 @@ void virMutexUnlock(virMutexPtr m); int virCondInit(virCondPtr c) ATTRIBUTE_RETURN_CHECK; int virCondDestroy(virCondPtr c) ATTRIBUTE_RETURN_CHECK;
+/* virCondWait, virCondWaitUntil: + * These functions can return without associated predicate
s/without/without the/
+ * changing value. Therefore in nearly all cases there
s/there/they/
+ * should be enclosed in a while loop that checks predicate.
s/checks/checks the/
+ */ int virCondWait(virCondPtr c, virMutexPtr m) ATTRIBUTE_RETURN_CHECK; int virCondWaitUntil(virCondPtr c, virMutexPtr m, unsigned long long whenms) ATTRIBUTE_RETURN_CHECK;
I'd also insert a newline here, to make it obvious that the above docs apply just to the two Wait functions, and not also the Signal function.
void virCondSignal(virCondPtr c);
ACK with nits fixed. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
instead of simple 'if' statement as virCondWait can return even if associated condition was not signaled. --- src/util/threadpool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/util/threadpool.c b/src/util/threadpool.c index 883d1e5..e8689d9 100644 --- a/src/util/threadpool.c +++ b/src/util/threadpool.c @@ -258,7 +258,7 @@ void virThreadPoolFree(virThreadPoolPtr pool) virCondBroadcast(&pool->prioCond); } - if (pool->nWorkers > 0 || pool->nPrioWorkers > 0) + while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex)); while ((job = pool->jobList.head)) { -- 1.7.3.4
On 12/09/2011 09:29 AM, Michal Privoznik wrote:
instead of simple 'if' statement as virCondWait can return even if associated condition was not signaled. --- src/util/threadpool.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/src/util/threadpool.c b/src/util/threadpool.c index 883d1e5..e8689d9 100644 --- a/src/util/threadpool.c +++ b/src/util/threadpool.c @@ -258,7 +258,7 @@ void virThreadPoolFree(virThreadPoolPtr pool) virCondBroadcast(&pool->prioCond); }
- if (pool->nWorkers > 0 || pool->nPrioWorkers > 0) + while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex));
ACK. -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Michal Privoznik