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