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 <nsaen...@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
