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

[Stefan Hajnoczi]

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).

In other words, qemu_cond_wait(&pool->worker_stopped, &pool->lock)
already releases pool->lock. It is unnecessary to release/re-acquire it
manually plus it probably suffers from a race condition in case the
other thread signals the condvar between the time when we unlock and
before we block on the condvar.

Please check that this patch really solves a problem and if so, please
explain the root cause.

Stefan

signature.asc
Description: PGP signature