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

[Stefan Hajnoczi]

On Thu, Oct 14, 2010 at 10:17 PM, Venkateswararao Jujjuri (JV)
<address@hidden> wrote:
> On 10/14/2010 2:02 AM, Stefan Hajnoczi wrote:
>> On Wed, Oct 13, 2010 at 4:31 PM, Arun R Bharadwaj
>>> +static void *threadlet_worker(void *data)
>>> +{
>>> +    ThreadletQueue *queue = data;
>>> +
>>> +    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);
>>> +        }
>>> +
>>> +        assert(queue->idle_threads != 0);
>>> +        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;
>>> +}
>>> +
>>> +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);
>>> +    }
>>> +    QTAILQ_INSERT_TAIL(&(queue->request_list), work, node);
>>> +    qemu_mutex_unlock(&(queue->lock));
>>> +    qemu_cond_signal(&(queue->cond));
>>
>> Worker thread signalling and spawning has race conditions.  See my
>> previous email:
>>
>> "There are race conditions here:
>>
>> 1. When a new threadlet is started because there are no idle threads,
>> qemu_cond_signal() may fire a blank because the threadlet isn't inside
>> qemu_cond_timedwait() yet.  The result, the work item is deadlocked
>> until another thread grabs more work off the queue.  If I'm reading
>> the code correctly this bug is currently present!
>
> Moving QTAILQ_INSERT_TAIL() ahead of spawn_threadlet() should take care of 
> this
> issue
> right?

I didn't read the code correctly.  queue->lock is already held by
submit_threadletwork_to_queue() until after QTAILQ_INSERT_TAIL().
threadlet_worker() will only enter its main loop once it acquires
queue->lock.  Therefore the queue definitely has the work before the
spawned thread begins processing.

The work is on the queue when threadlet_worker() enters its main loop,
so it will not need to wait on queue->cond but can process work
immediately.  There is no spawn race condition.

Stefan