Re: [PATCH v3 2/3] thread-pool: replace semaphore with condition variable

[Nicolas Saenz Julienne]

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