[PATCH] thread-pool: signal condition variable while holding lock

[PATCH] thread-pool: signal condition variable while holding lock

[Stepan Popov]

Move qemu_cond_signal() inside the critical section protected by pool->lock.
Signaling while holding the lock imposes more predictable scheduling behavior.

Signed-off-by: Stepan Popov <Stepan.Popov@kaspersky.com>
---
 util/thread-pool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/util/thread-pool.c b/util/thread-pool.c
index 8f8cb38d5c..8e55aefd07 100644
--- a/util/thread-pool.c
+++ b/util/thread-pool.c
@@ -265,8 +265,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
         spawn_thread(pool);
     }
     QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
-    qemu_mutex_unlock(&pool->lock);
     qemu_cond_signal(&pool->request_cond);
+    qemu_mutex_unlock(&pool->lock);
     return &req->common;
 }
 
-- 
2.34.1

Re: [PATCH] thread-pool: signal condition variable while holding lock

[Daniel P. Berrangé]

On Mon, Mar 30, 2026 at 04:01:04PM +0300, Stepan Popov wrote:
> Move qemu_cond_signal() inside the critical section protected by pool->lock.
> Signaling while holding the lock imposes more predictable scheduling behavior.
> 
> Signed-off-by: Stepan Popov <Stepan.Popov@kaspersky.com>
> ---
>  util/thread-pool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/thread-pool.c b/util/thread-pool.c
> index 8f8cb38d5c..8e55aefd07 100644
> --- a/util/thread-pool.c
> +++ b/util/thread-pool.c
> @@ -265,8 +265,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
>          spawn_thread(pool);
>      }
>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> -    qemu_mutex_unlock(&pool->lock);
>      qemu_cond_signal(&pool->request_cond);
> +    qemu_mutex_unlock(&pool->lock);
>      return &req->common;
>  }

Doesn't this order mean that when the signal wakes up the waiting
thread, that thread will get temporarily re-blocked waiting for
'lock' to be released by the original thread too.

With regards,
Daniel
-- 
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

RE: [PATCH] thread-pool: signal condition variable while holding lock

[Stepan Popov]

When pthread_cond_signal wakes the waiter, the kernel requeues the waiter directly onto the mutex's PI futex and transfers the lock ownership atomically. So the waiter returns from the syscall already holding the mutex. No extra pthread_mutex_lock call, no re-blocking.
So with a PI mutex, moving the signal under the lock is fine – the waiter won't get stuck waiting for the lock again.

Glibc has supported PI mutexes with FUTEX_REQUEUE_PI.
Here are glibc commits:
x86: https://sourceware.org/git/?p=glibc.git;a=commit;h=42e69bcf1137fccfd7a95645a9d316c6490b9ff9
non-x86 : https://sourceware.org/git/?p=glibc.git;a=commit;h=8313cb997d2da2465c8560d3164358a68ea1e9ad

Looking forward to your reply,
Stepan.

-----Original Message-----
From: Daniel P. Berrangé <berrange@redhat.com> 
Sent: Monday, March 30, 2026 4:43 PM
To: Stepan Popov <Stepan.Popov@kaspersky.com>
Cc: qemu-devel@nongnu.org; qemu-trivial@nongnu.org; Paolo Bonzini <pbonzini@redhat.com>
Subject: Re: [PATCH] thread-pool: signal condition variable while holding lock



On Mon, Mar 30, 2026 at 04:01:04PM +0300, Stepan Popov wrote:
> Move qemu_cond_signal() inside the critical section protected by pool->lock.
> Signaling while holding the lock imposes more predictable scheduling behavior.
>
> Signed-off-by: Stepan Popov <Stepan.Popov@kaspersky.com>
> ---
>  util/thread-pool.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/util/thread-pool.c b/util/thread-pool.c index 
> 8f8cb38d5c..8e55aefd07 100644
> --- a/util/thread-pool.c
> +++ b/util/thread-pool.c
> @@ -265,8 +265,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
>          spawn_thread(pool);
>      }
>      QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> -    qemu_mutex_unlock(&pool->lock);
>      qemu_cond_signal(&pool->request_cond);
> +    qemu_mutex_unlock(&pool->lock);
>      return &req->common;
>  }

Doesn't this order mean that when the signal wakes up the waiting thread, that thread will get temporarily re-blocked waiting for 'lock' to be released by the original thread too.

With regards,
Daniel
--
|: https://berrange.com       ~~        https://hachyderm.io/@berrange :|
|: https://libvirt.org          ~~          https://entangle-photo.org :|
|: https://pixelfed.art/berrange   ~~    https://fstop138.berrange.com :|

Re: [PATCH] thread-pool: signal condition variable while holding lock

[Paolo Bonzini]

On 3/30/26 15:01, Stepan Popov wrote:
> Move qemu_cond_signal() inside the critical section protected by pool->lock.
> Signaling while holding the lock imposes more predictable scheduling behavior.
> 
> Signed-off-by: Stepan Popov <Stepan.Popov@kaspersky.com>
> ---
>   util/thread-pool.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/util/thread-pool.c b/util/thread-pool.c
> index 8f8cb38d5c..8e55aefd07 100644
> --- a/util/thread-pool.c
> +++ b/util/thread-pool.c
> @@ -265,8 +265,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
>           spawn_thread(pool);
>       }
>       QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> -    qemu_mutex_unlock(&pool->lock);
>       qemu_cond_signal(&pool->request_cond);
> +    qemu_mutex_unlock(&pool->lock);
>       return &req->common;
>   }

In this code, in the past even very small changes had big impact on 
performance.  So, without some measurement I am wary of taking this change.

What is "more predictable scheduling behavior" is unclear, too.  If 
thead_pool_submit_aio() is preempted between qemu_cond_signal() and 
qemu_mutex_unlock(), then not only the waiting thread cannot start the 
work, but the mutex is taken and no one else can submit other requests.

Paolo

RE: [PATCH] thread-pool: signal condition variable while holding lock

[Stepan Popov]

Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 3/30/26 15:01, Stepan Popov wrote:
> > Move qemu_cond_signal() inside the critical section protected by pool->lock.
> > Signaling while holding the lock imposes more predictable scheduling behavior.
> >
> > Signed-off-by: Stepan Popov <Stepan.Popov@kaspersky.com>
> > ---
> >   util/thread-pool.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/util/thread-pool.c b/util/thread-pool.c index 
> > 8f8cb38d5c..8e55aefd07 100644
> > --- a/util/thread-pool.c
> > +++ b/util/thread-pool.c
> > @@ -265,8 +265,8 @@ BlockAIOCB *thread_pool_submit_aio(ThreadPoolFunc *func, void *arg,
> >           spawn_thread(pool);
> >       }
> >       QTAILQ_INSERT_TAIL(&pool->request_list, req, reqs);
> > -    qemu_mutex_unlock(&pool->lock);
> >       qemu_cond_signal(&pool->request_cond);
> > +    qemu_mutex_unlock(&pool->lock);
> >       return &req->common;
> >   }
>
> In this code, in the past even very small changes had big impact on 
> performance.  So, without some measurement I am wary of taking this change.
>
> What is "more predictable scheduling behavior" is unclear, too.  If 
> thead_pool_submit_aio() is preempted between qemu_cond_signal() and 
> qemu_mutex_unlock(), then not only the waiting thread cannot start the 
> work, but the mutex is taken and no one else can submit other requests.

Thank you for your feedback. I understand your concern about performance.

To explain the motivation: we have seen repetitive deadlocks in production after running qemu-img info --output=json image.qcow. 
Submitter is stuck in pthread_cond_signal (trying to __condvar_quiesce_and_switch_g1) and waiter in __pthread_cond_wait_common (trying to __condvar_cancel_waiting). 
Our glibc version has a known bug leading to deadlock (Ubuntu GLIBC 2.35-0ubuntu3.13):
https://sourceware.org/bugzilla/show_bug.cgi?id=25847 (pthread_cond_signal failed to wake up due to a bug in undoing stealing).

We are not sure if this change will fix it, 
but holding the mutex while signaling is preferred for predictable scheduling behaviour, 
as stated in the pthread man page (https://linux.die.net/man/3/pthread_cond_signal). 
Theoretically, it could help with similar problems.

To move forward, could you suggest a set of benchmarks that would be sensitive to this change? 
Maybe there are existing scripts or tools that you would recommend running? 
Which QEMU use cases (block I/O, virtio-scsi, file-backed disks, etc.) have historically shown performance changes with similar changes?

Thank you again.

Best regards,
Stepan