Re: [libvirt] [PATCH] threadpool: Switch to detached threads
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