[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]