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

[Nicolas Saenz Julienne]

On Tue, 2022-05-17 at 16:18 +0200, Paolo Bonzini wrote:
> 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;
> 

OK, I somehow missed this 'continue', I wonder if I managed to re-review v2 by
accident... Anyway, it looks fine to me now.

Regards,

-- 
Nicolás Sáenz