Re: [PATCH v2 1/4] util/thread-pool: Fix thread pool freeing locking

[Nicolas Saenz Julienne]

On Thu, 2022-03-10 at 09:20 +0000, Stefan Hajnoczi wrote:
> On Thu, Mar 03, 2022 at 03:58:19PM +0100, Nicolas Saenz Julienne wrote:
> > Upon freeing a thread pool we need to get rid of any remaining worker.
> > This is achieved by setting the thread pool's topping flag, waking the
> 
> s/topping/stopping/
> 
> > workers up, and waiting for them to exit one by one. The problem is that
> > currently all this process happens with the thread pool lock held,
> > effectively blocking the workers from exiting.
> > 
> > So let's release the thread pool lock after signaling a worker thread
> > that it's time to exit to give it a chance to do so.
> > 
> > Fixes: f7311ccc63 ("threadpool: add thread_pool_new() and 
> > thread_pool_free()")
> > Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> > ---
> >  util/thread-pool.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/util/thread-pool.c b/util/thread-pool.c
> > index d763cea505..fdb43c2d3b 100644
> > --- a/util/thread-pool.c
> > +++ b/util/thread-pool.c
> > @@ -339,7 +339,9 @@ void thread_pool_free(ThreadPool *pool)
> >      pool->stopping = true;
> >      while (pool->cur_threads > 0) {
> >          qemu_sem_post(&pool->sem);
> > +        qemu_mutex_unlock(&pool->lock);
> >          qemu_cond_wait(&pool->worker_stopped, &pool->lock);
> > +        qemu_mutex_lock(&pool->lock);
> 
> This patch looks incorrect. qemu_cond_wait() (and pthread_cond_wait())
> take a mutex as the second argument. This mutex is released and the
> thread blocks to wait (this is done atomically with respect to the
> condvar check so there are no races).

Sorry I completely missed that. It was a late addition, should've thought it
trough. Patch #4 also needs to take this into account as I follow the same
pattern.

Thanks,

-- 
Nicolás Sáenz