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

[Paolo Bonzini <pbonzini@redhat.com>]

On 5/13/22 13:56, Nicolas Saenz Julienne wrote:
>>               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 ||
>
> I think, you need to check for 'pool->stopping' upon exiting wait_cond().
> Otherwise it'll blindly try to dequeue a request from a list that is otherwise
> empty.

Good point, thanks.

>> -    if (elem->state == THREAD_QUEUED &&
>> -        /* No thread has yet started working on elem. we can try to
>> "steal"
>> -         * the item from the worker if we can get a signal from the
>> -         * semaphore.  Because this is non-blocking, we can do it
>> with
>> -         * the lock taken and ensure that elem will remain
>> THREAD_QUEUED.
>> -         */
>> -        qemu_sem_timedwait(&pool->sem, 0) == 0) {
>> +    if (elem->state == THREAD_QUEUED) {
>>           QTAILQ_REMOVE(&pool->request_list, elem, reqs);
>>           qemu_bh_schedule(pool->completion_bh);
> 
> The 'thread-pool cancel' unit test fails.
> 
> I think it's because there is an assumption in worker_thread() that if you get
> woken up, you'll have a pending request. And you're now 'stealing' work
> requests, without 'stealing' a wakeup (what qemu_sem_timedwait(sem, 0) achieved
> in the past).

You don't need to steal a wakeup because cond_wait does not "count", but 
yeah it's essentially the same issue that you mentioned above.

Paolo