[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v3 2/3] thread-pool: replace semaphore with condition variabl
From: |
Nicolas Saenz Julienne |
Subject: |
Re: [PATCH v3 2/3] thread-pool: replace semaphore with condition variable |
Date: |
Tue, 17 May 2022 14:46:29 +0200 |
User-agent: |
Evolution 3.44.1 (3.44.1-1.fc36) |
Hi Paolo,
On Sat, 2022-05-14 at 08:50 +0200, Paolo Bonzini wrote:
[...]
> static void *worker_thread(void *opaque)
> {
> ThreadPool *pool = opaque;
> @@ -99,20 +82,25 @@ static void *worker_thread(void *opaque)
> pool->pending_threads--;
> do_spawn_thread(pool);
>
> - while (!pool->stopping) {
> + while (!pool->stopping && pool->cur_threads <= pool->max_threads) {
> ThreadPoolElement *req;
> int ret;
>
> - do {
> + if (QTAILQ_EMPTY(&pool->request_list)) {
> pool->idle_threads++;
> - qemu_mutex_unlock(&pool->lock);
> - ret = qemu_sem_timedwait(&pool->sem, 10000);
> - qemu_mutex_lock(&pool->lock);
> + ret = qemu_cond_timedwait(&pool->request_cond, &pool->lock,
> 10000);
> pool->idle_threads--;
> - } while (back_to_sleep(pool, ret));
> - if (ret == -1 || pool->stopping ||
> - pool->cur_threads > pool->max_threads) {
> - break;
> + if (ret == 0 &&
> + QTAILQ_EMPTY(&pool->request_list) &&
> + pool->cur_threads > pool->min_threads) {
> + /* Timed out + no work to do + no need for warm threads =
> exit. */
> + break;
> + }
Some comments:
- A completely idle pool will now never be able to lose its threads, as the
'pool->cur_threads <= pool->max_threads' condition is only checked after a
non-timeout wakeup.
- You don't take into account the possibility of being woken up with an empty
queue. Which I belive possible:
CPU0: CPU1:
// in worker_thread(), queue
empty
qemu_cond_timedwait();
acb = thread_pool_submit_aio();
...
qemu_cond_signal();
thread_pool_cancel(acb);
// wakes-up, ret == 1
req =
QTAILQ_FIRST(&pool->request_list);
QTAILQ_REMOVE(&pool->request_list, req, reqs);
// segmentation fault....
------------------------------------------------------
CPU0: CPU1:
// in worker_thread(), queue
empty
qemu_cond_timedwait();
thread_pool_free()
...
qemu_cond_broadcast();
// wakes-up, ret == 1
req =
QTAILQ_FIRST(&pool->request_list);
QTAILQ_REMOVE(&pool->request_list, req, reqs);
// segmentation fault....
Regards,
--
Nicolás Sáenz
[PATCH v3 3/3] thread-pool: remove stopping variable, Paolo Bonzini, 2022/05/14
Re: [PATCH v3 0/3] thread-pool: fix performance regression, Nicolas Saenz Julienne, 2022/05/17