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

["Venkateswararao Jujjuri (JV)" <jvrao@linux.vnet.ibm.com>]

On 10/15/2010 2:52 AM, Stefan Hajnoczi wrote:
> On Thu, Oct 14, 2010 at 10:17 PM, Venkateswararao Jujjuri (JV)
> <jvrao@linux.vnet.ibm.com> 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.

Correct...I too missed that. :)

JV

> 
> Stefan