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