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

[Paolo Bonzini]

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