[libvirt] [PATCH] threadpool: Switch to detached threads
Using joinable threads does not help anything, but it can lead to memory leaks. When a worker thread exits, it decreases nWorkers or nPrioWorkers and once both nWorkers and nPrioWorkers are zero (i.e., the last worker is gone), quit_cond is signaled. When freeing the pool we first tell all threads to die and then we are waiting for both nWorkers and nPrioWorkers to become zero. At this point we already know all threads are gone. So the only reason for calling virThreadJoin of all workers is to free the memory allocated for joinable threads. If we avoid allocating this memory, we don't need to take care of freeing it. Moreover, any memory associated with a worker thread which died before we asked it to die (e.g., because virCondWait failed in the thread) would be lost anyway since virThreadPoolFree calls virThreadJoin only for threads which were running at the time virThreadPoolFree was called. Signed-off-by: Jiri Denemark <jdenemar@redhat.com> --- src/util/virthreadpool.c | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/src/util/virthreadpool.c b/src/util/virthreadpool.c index ed22486..f640448 100644 --- a/src/util/virthreadpool.c +++ b/src/util/virthreadpool.c @@ -201,7 +201,7 @@ virThreadPoolNewFull(size_t minWorkers, data->cond = &pool->cond; if (virThreadCreateFull(&pool->workers[i], - true, + false, virThreadPoolWorker, pool->jobFuncName, true, @@ -225,7 +225,7 @@ virThreadPoolNewFull(size_t minWorkers, data->priority = true; if (virThreadCreateFull(&pool->prioWorkers[i], - true, + false, virThreadPoolWorker, pool->jobFuncName, true, @@ -249,16 +249,11 @@ void virThreadPoolFree(virThreadPoolPtr pool) { virThreadPoolJobPtr job; bool priority = false; - size_t i; - size_t nWorkers; - size_t nPrioWorkers; if (!pool) return; virMutexLock(&pool->mutex); - nWorkers = pool->nWorkers; - nPrioWorkers = pool->nPrioWorkers; pool->quit = true; if (pool->nWorkers > 0) virCondBroadcast(&pool->cond); @@ -275,12 +270,6 @@ void virThreadPoolFree(virThreadPoolPtr pool) VIR_FREE(job); } - for (i = 0; i < nWorkers; i++) - virThreadJoin(&pool->workers[i]); - - for (i = 0; i < nPrioWorkers; i++) - virThreadJoin(&pool->prioWorkers[i]); - VIR_FREE(pool->workers); virMutexUnlock(&pool->mutex); virMutexDestroy(&pool->mutex); @@ -338,7 +327,7 @@ int virThreadPoolSendJob(virThreadPoolPtr pool, data->cond = &pool->cond; if (virThreadCreateFull(&pool->workers[pool->nWorkers - 1], - true, + false, virThreadPoolWorker, pool->jobFuncName, true, -- 2.4.1
On Thu, May 21, 2015 at 11:57:52AM +0200, Jiri Denemark wrote:
Using joinable threads does not help anything, but it can lead to memory leaks.
When a worker thread exits, it decreases nWorkers or nPrioWorkers and once both nWorkers and nPrioWorkers are zero (i.e., the last worker is gone), quit_cond is signaled. When freeing the pool we first tell all threads to die and then we are waiting for both nWorkers and nPrioWorkers to become zero. At this point we already know all threads are gone. So the only reason for calling virThreadJoin of all workers is to free the memory allocated for joinable threads. If we avoid allocating this memory, we don't need to take care of freeing it.
The idea behind thread join was to make debugging with valgrind better. By waiting for the threads to shutdown we ensure that all memory they are using has been free before libvirtd exits, so valgrind doesn't report bogus leaks. Another reason for using Join is that it allows the threads to finish processing whatever RPC/API call they were currently handling. If we daemonize all threads, then I believe that would allow libvirtd to exit while we were in the middle of performing some change on QEMU. This seems like it could well lead to inconsistent state where we had updated QEMU, but not our state XML, or vica-verca. For sake of automated restarts in RPM %post, I think allowing threads to finish their RPC/API call is quite desirable in general.
Moreover, any memory associated with a worker thread which died before we asked it to die (e.g., because virCondWait failed in the thread) would be lost anyway since virThreadPoolFree calls virThreadJoin only for threads which were running at the time virThreadPoolFree was called.
Hmm, that is a bug - we should be calling Join on all threads we ever created, not just those that appear to be running. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, May 21, 2015 at 11:19:05 +0100, Daniel P. Berrange wrote:
On Thu, May 21, 2015 at 11:57:52AM +0200, Jiri Denemark wrote:
Using joinable threads does not help anything, but it can lead to memory leaks.
When a worker thread exits, it decreases nWorkers or nPrioWorkers and once both nWorkers and nPrioWorkers are zero (i.e., the last worker is gone), quit_cond is signaled. When freeing the pool we first tell all threads to die and then we are waiting for both nWorkers and nPrioWorkers to become zero. At this point we already know all threads are gone. So the only reason for calling virThreadJoin of all workers is to free the memory allocated for joinable threads. If we avoid allocating this memory, we don't need to take care of freeing it.
The idea behind thread join was to make debugging with valgrind better. By waiting for the threads to shutdown we ensure that all memory they are using has been free before libvirtd exits, so valgrind doesn't report bogus leaks.
Detached threads don't consume any memory by themselves (while joinable threads do), so there's no memory that needs to be freed. And if we are concerned about the memory allocated by the code the thread is executing, we are safe too, since we already wait for all threads to die. A few lines above the virThreadJoin, the following loop handles it: while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex));
Another reason for using Join is that it allows the threads to finish processing whatever RPC/API call they were currently handling. If we daemonize all threads, then I believe that would allow libvirtd to exit while we were in the middle of performing some change on QEMU. This seems like it could well lead to inconsistent state where we had updated QEMU, but not our state XML, or vica-verca.
We wait for all thread to finish their processing anyway, so when the code gets to virThreadJoin, we already know all threads are gone.
For sake of automated restarts in RPM %post, I think allowing threads to finish their RPC/API call is quite desirable in general.
Definitely.
Moreover, any memory associated with a worker thread which died before we asked it to die (e.g., because virCondWait failed in the thread) would be lost anyway since virThreadPoolFree calls virThreadJoin only for threads which were running at the time virThreadPoolFree was called.
Hmm, that is a bug - we should be calling Join on all threads we ever created, not just those that appear to be running.
And this bug if fixed in this patch, although in a different way. Jirka
On Thu, May 21, 2015 at 01:09:37PM +0200, Jiri Denemark wrote:
On Thu, May 21, 2015 at 11:19:05 +0100, Daniel P. Berrange wrote:
On Thu, May 21, 2015 at 11:57:52AM +0200, Jiri Denemark wrote:
Using joinable threads does not help anything, but it can lead to memory leaks.
When a worker thread exits, it decreases nWorkers or nPrioWorkers and once both nWorkers and nPrioWorkers are zero (i.e., the last worker is gone), quit_cond is signaled. When freeing the pool we first tell all threads to die and then we are waiting for both nWorkers and nPrioWorkers to become zero. At this point we already know all threads are gone. So the only reason for calling virThreadJoin of all workers is to free the memory allocated for joinable threads. If we avoid allocating this memory, we don't need to take care of freeing it.
The idea behind thread join was to make debugging with valgrind better. By waiting for the threads to shutdown we ensure that all memory they are using has been free before libvirtd exits, so valgrind doesn't report bogus leaks.
Detached threads don't consume any memory by themselves (while joinable threads do), so there's no memory that needs to be freed. And if we are concerned about the memory allocated by the code the thread is executing, we are safe too, since we already wait for all threads to die. A few lines above the virThreadJoin, the following loop handles it:
while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex));
Another reason for using Join is that it allows the threads to finish processing whatever RPC/API call they were currently handling. If we daemonize all threads, then I believe that would allow libvirtd to exit while we were in the middle of performing some change on QEMU. This seems like it could well lead to inconsistent state where we had updated QEMU, but not our state XML, or vica-verca.
We wait for all thread to finish their processing anyway, so when the code gets to virThreadJoin, we already know all threads are gone.
Ok, yes, i see that now looking more closely at the code. ACK Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On Thu, May 21, 2015 at 12:18:32 +0100, Daniel P. Berrange wrote:
On Thu, May 21, 2015 at 01:09:37PM +0200, Jiri Denemark wrote:
On Thu, May 21, 2015 at 11:19:05 +0100, Daniel P. Berrange wrote:
On Thu, May 21, 2015 at 11:57:52AM +0200, Jiri Denemark wrote:
Using joinable threads does not help anything, but it can lead to memory leaks.
When a worker thread exits, it decreases nWorkers or nPrioWorkers and once both nWorkers and nPrioWorkers are zero (i.e., the last worker is gone), quit_cond is signaled. When freeing the pool we first tell all threads to die and then we are waiting for both nWorkers and nPrioWorkers to become zero. At this point we already know all threads are gone. So the only reason for calling virThreadJoin of all workers is to free the memory allocated for joinable threads. If we avoid allocating this memory, we don't need to take care of freeing it.
The idea behind thread join was to make debugging with valgrind better. By waiting for the threads to shutdown we ensure that all memory they are using has been free before libvirtd exits, so valgrind doesn't report bogus leaks.
Detached threads don't consume any memory by themselves (while joinable threads do), so there's no memory that needs to be freed. And if we are concerned about the memory allocated by the code the thread is executing, we are safe too, since we already wait for all threads to die. A few lines above the virThreadJoin, the following loop handles it:
while (pool->nWorkers > 0 || pool->nPrioWorkers > 0) ignore_value(virCondWait(&pool->quit_cond, &pool->mutex));
Another reason for using Join is that it allows the threads to finish processing whatever RPC/API call they were currently handling. If we daemonize all threads, then I believe that would allow libvirtd to exit while we were in the middle of performing some change on QEMU. This seems like it could well lead to inconsistent state where we had updated QEMU, but not our state XML, or vica-verca.
We wait for all thread to finish their processing anyway, so when the code gets to virThreadJoin, we already know all threads are gone.
Ok, yes, i see that now looking more closely at the code.
ACK
Pushed, thanks. Jirka
participants (2)
-
Daniel P. Berrange -
Jiri Denemark