Re: [Qemu-devel] [PATCH 1/3] Introduce threadlets

[Venkateswararao Jujjuri (JV)]

On 10/19/2010 7:22 PM, Balbir Singh wrote:
> * Anthony Liguori <address@hidden> [2010-10-19 16:36:31]:
> 
>> On 10/19/2010 01:36 PM, Balbir Singh wrote:
>>>> +    qemu_mutex_lock(&(queue->lock));
>>>> +    while (1) {
>>>> +        ThreadletWork *work;
>>>> +        int ret = 0;
>>>> +
>>>> +        while (QTAILQ_EMPTY(&(queue->request_list))&&
>>>> +               (ret != ETIMEDOUT)) {
>>>> +            ret = qemu_cond_timedwait(&(queue->cond),
>>>> +                                  &(queue->lock), 10*100000);
>>> Ewww... what is 10*100000, can we use something more meaningful
>>> please?
>>
>> A define is fine but honestly, it's pretty darn obvious what it means...
>>
>>>> +        }
>>>> +
>>>> +        assert(queue->idle_threads != 0);
>>> This assertion holds because we believe one of the idle_threads
>>> actually did the dequeuing, right?
>>
>> An idle thread is a thread is one that is not doing work.  At this
>> point in the code, we are not doing any work (yet) so if
>> idle_threads count is zero, something is horribly wrong.  We're also
>> going to unconditionally decrement in the future code path which
>> means that if idle_threads is 0, it's going to become -1.
>>
>> The use of idle_thread is to detect whether it's necessary to spawn
>> an additional thread.
>>
> 
> We can hit this assert if pthread_cond_signal() is called outside of
> the mutex, let me try and explain below
> 
>>>> +        if (QTAILQ_EMPTY(&(queue->request_list))) {
>>>> +            if (queue->cur_threads>  queue->min_threads) {
>>>> +                /* We retain the minimum number of threads */
>>>> +                break;
>>>> +            }
>>>> +        } else {
>>>> +            work = QTAILQ_FIRST(&(queue->request_list));
>>>> +            QTAILQ_REMOVE(&(queue->request_list), work, node);
>>>> +
>>>> +            queue->idle_threads--;
>>>> +            qemu_mutex_unlock(&(queue->lock));
>>>> +
>>>> +            /* execute the work function */
>>>> +            work->func(work);
>>>> +
>>>> +            qemu_mutex_lock(&(queue->lock));
>>>> +            queue->idle_threads++;
>>>> +        }
>>>> +    }
>>>> +
>>>> +    queue->idle_threads--;
>>>> +    queue->cur_threads--;
>>>> +    qemu_mutex_unlock(&(queue->lock));
>>>> +
>>>> +    return NULL;
>>> Does anybody do a join on the exiting thread from the pool?
>>
>> No.  The thread is created in a detached state.
>>
> 
> That makes sense, thanks for clarifying
> 
>>>> +}
>>>> +
>>>> +static void spawn_threadlet(ThreadletQueue *queue)
>>>> +{
>>>> +    QemuThread thread;
>>>> +
>>>> +    queue->cur_threads++;
>>>> +    queue->idle_threads++;
>>>> +
>>>> +    qemu_thread_create(&thread, threadlet_worker, queue);
>>>> +}
>>>> +
>>>> +/**
>>>> + * submit_threadletwork_to_queue: Submit a new task to a private queue to 
>>>> be
>>>> + *                            executed asynchronously.
>>>> + * @queue: Per-subsystem private queue to which the new task needs
>>>> + *         to be submitted.
>>>> + * @work: Contains information about the task that needs to be submitted.
>>>> + */
>>>> +void submit_threadletwork_to_queue(ThreadletQueue *queue, ThreadletWork 
>>>> *work)
>>>> +{
>>>> +    qemu_mutex_lock(&(queue->lock));
>>>> +    if (queue->idle_threads == 0&&  queue->cur_threads<  
>>>> queue->max_threads) {
>>>> +        spawn_threadlet(queue);
>>> So we hold queue->lock, spawn the thread, the spawned thread tries to
>>> acquire queue->lock
>>
>> Yup.
>>
>>>> +    }
>>>> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>>>> +    qemu_mutex_unlock(&(queue->lock));
>>>> +    qemu_cond_signal(&(queue->cond));
>>> In the case that we just spawned the threadlet, the cond_signal is
>>> spurious. If we need predictable scheduling behaviour,
>>> qemu_cond_signal needs to happen with queue->lock held.
>>
>> It doesn't really affect predictability..
>>
>>> I'd rewrite the function as
>>>
>>> /**
>>>  * submit_threadletwork_to_queue: Submit a new task to a private queue to be
>>>  *                            executed asynchronously.
>>>  * @queue: Per-subsystem private queue to which the new task needs
>>>  *         to be submitted.
>>>  * @work: Contains information about the task that needs to be submitted.
>>>  */
>>> void submit_threadletwork_to_queue(ThreadletQueue *queue, ThreadletWork 
>>> *work)
>>> {
>>>     qemu_mutex_lock(&(queue->lock));
>>>     if (queue->idle_threads == 0&&  (queue->cur_threads<  
>>> queue->max_threads)) {
>>>         spawn_threadlet(queue);
>>>     } else {
>>>         qemu_cond_signal(&(queue->cond));
>>>     }
>>>     QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>>>     qemu_mutex_unlock(&(queue->lock));
>>> }
>>
>> I think this is a lot more fragile.  You're relying on the fact that
>> signal will not cause the signalled thread to actually awaken until
>> we release the lock and doing work after signalling that the
>> signalled thread needs to be completed before it wakes up.
>>
>> I think you're a lot more robust in the long term if you treat
>> condition signalling as a hand off point because it makes the code a
>> lot more explicit about what's happening.
>>
> 
> OK, here is a situation that can happen
> 
> T1                              T2
> ---                             ---
> threadlet                       submit_threadletwork_to_queue
> (sees condition as no work)     mutex_lock
> qemu_cond_timedwait             add_work
> ...                             mutex_unlock
> 
> T3
> --
> cancel_threadlet_work_on_queue
> mutex_lock (grabs it) before T1 can
> cancels the work
> 
> 
>                                 qemu_cond_signal
> 
> T1
> --
> Grabs mutex_lock (from within cond_timedwait)
> Now there is no work to do, the condition
> has changed before the thread wakes up

So what? It won't find any work and goes back to sleep or exits.

idle_threads is decremented only in threadlet_worker(). Given that
we have a threadlet that is not doing anywork the assert should never hit unless
something horribly wrong .

- JV

> 
> 
> The man page also states
> 
> "however, if predictable scheduling behavior is required, then that
> mutex shall be locked by the thread calling pthread_cond_broadcast()
> or pthread_cond_signal()"
> 
>>>> +/**
>>>> + * submit_threadletwork: Submit to the global queue a new task to be 
>>>> executed
>>>> + *                   asynchronously.
>>>> + * @work: Contains information about the task that needs to be submitted.
>>>> + */
>>>> +void submit_threadletwork(ThreadletWork *work)
>>>> +{
>>>> +    if (unlikely(!globalqueue_init)) {
>>>> +        threadlet_queue_init(&globalqueue, MAX_GLOBAL_THREADS,
>>>> +                                MIN_GLOBAL_THREADS);
>>>> +        globalqueue_init = 1;
>>>> +    }
>>> What protects globalqueue_init?
>>
>> qemu_mutex, and that unlikely is almost certainly a premature optimization.
>>
>> Regards,
>>
>> Anthony Liguori
>>
>