Re: [libvirt] [PATCH] threadpool: Switch to detached threads

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