Re: [Qemu-devel] qemu_cond_signal() taking a long time to complete.

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

On 10/4/2010 8:49 AM, Stefan Hajnoczi wrote:
> On Mon, Oct 4, 2010 at 4:37 PM, Anthony Liguori<anthony@codemonkey.ws>  wrote:
>> On 10/04/2010 05:40 AM, Arun R Bharadwaj wrote:
>>>
>>> Hi,
>>>
>>> I am working on introducing threading model into Qemu. This introduces
>>> the Threadlets infrastructure which allows subsystems to offload possibly
>>> blocking work to a queue to be processed by a pool of threads
>>> asynchrnonously.
>>> Threadlets are useful when there are operations that can be performed
>>> outside the context of the VCPU/IO threads inorder to free these latter
>>> to service any other guest requests. As of now we have converted a few
>>> v9fs calls like v9fs_read, v9fs_write etc to this model to test the
>>> working nature of this model.
>>>
>>> I observed that performance is degrading in the threading model for the
>>> following reason:
>>>
>>> Taking the example of v9fs_read call: We submit the blocking work in
>>> v9fs_read to a queue and do a qemu_cond_signal(). the work will be picked
>>> up by a worker thread which is waiting on the condition variable to go
>>> true. I measured the time taken to execute the v9fs_read call; in the
>>> case without the threading model, it takes around 15microseconds to
>>> complete, while in the threading model, it takes around 30microsends
>>> to complete. Most of this extra time (around 22microsends) is spent in
>>> completing the qemu_cond_signal() call. I suspect this is the reason why
>>> I am seeing performance hit with the threading model, because this
>>> time is much more than the time needed to complete the entire
>>> v9fs_read call in the non threading model case.
>>>
>>> I need advice on how to proceed from this situation. Pasting relevant
>>> code snippets below.
>>>
>>> thanks
>>> arun.
>>> ---
>>>
>>> /* Code to sumbit work to the queue */
>>> void submit_threadlet_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_cond_signal(&(queue->cond));
>>>      qemu_mutex_unlock(&(queue->lock));
>>> }
>>>
>>
>> Try moving qemu_cond_signal past qemu_mutex_unlock().
>
> 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

Since it is timedwait, it is either the time expiry or more work is added to the 
queue.

- JV


> the code correctly this bug is currently present!
> 2. Moving qemu_cond_signal() outside queue->lock is dangerous for the
> same reason: you need to be careful not to qemu_cond_signal() when the
> thread isn't inside qemu_cond_timedwait().
>
> Stefan
>
> Stefan