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

[Paolo Bonzini <pbonzini@redhat.com>]

On 5/17/22 14:46, Nicolas Saenz Julienne wrote:
>> -    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.

Are you sure?  The full code is:

             ret = qemu_cond_timedwait(&pool->request_cond, &pool->lock, 10000);
             pool->idle_threads--;
             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;
             }
             /*
              * Even if there was some work to do, check if there aren't
              * too many worker threads before picking it up.
              */
             continue;

That is, it won't immediately pick up the job after _any_ wait,
whether successful or not.  It will first of all "continue" to
check pool->cur_threads <= pool->max_threads.

This is also the reason why I had to add a qemu_cond_signal() at the
bottom of the worker thread (because maybe it got a signal to act on a
non-empty queue, but decided to exit instead).

> - You don't take into account the possibility of being woken up with an empty
>    queue. Which I belive possible:

It's absolutely possible, but the difference between v2 and v3 _should_
be the fix.  Of course I could have screwed up, but it seems correct
this time.

Paolo