[Qemu-devel] Re: [patch 2/7] qemu: separate thread for io

[Anthony Liguori <anthony@codemonkey.ws>]

Marcelo Tosatti wrote:
> On Fri, Mar 20, 2009 at 12:43:49PM -0500, Anthony Liguori wrote:
>   
>>> +    qemu_mutex_lock(&qemu_fair_mutex);
>>> +    qemu_mutex_unlock(&qemu_fair_mutex);
>>>   
>>>       
>> This is extremely subtle.  I think it can be made a bit more explicit  
>> via something like:
>>
>> int generation = qemu_io_generation() + 1;
>>
>> qemu_mutex_unlock(&qemu_global_mutex);
>>
>> if (timeout && !has_work(env))
>>     wait_signal(timeout);
>>
>> qemu_mutex_lock(&qemu_global_mutex)
>>
>> while (qemu_io_generation() < generation)
>>    qemu_cond_wait(&qemu_io_generation_cond, &qemu_global_mutex);
>>
>> Then, in main_loop_wait():
>>
>>     
>>>  void main_loop_wait(int timeout)
>>>  {
>>>      IOHandlerRecord *ioh;
>>> @@ -3660,7 +3806,7 @@ void main_loop_wait(int timeout)
>>>       */
>>>      qemu_mutex_unlock(&qemu_global_mutex);
>>>      ret = select(nfds + 1, &rfds, &wfds, &xfds, &tv);
>>> -    qemu_mutex_lock(&qemu_global_mutex);
>>>   
>>>       
>> qemu_mutex_lock(&qemu_global_mutex);
>> qemu_io_generation_add();
>> qemu_cond_signal(&qemu_io_generation_cond);
>>
>> This will ensure that the TCG loop backs off of qemu_global_mutex for at  
>> least one main_loop_wait() iteration.
>>     
>
> I don't see much benefit. It has the disadvantage of introducing more
> state (the generation counter, the conditional), and the potential
> problems associated with this state such as:
>
> - If there is no event for the iothread to process, TCG will throttle
>   unnecessarily (i can't see how that could happen, but is a
>   possibility) until some event breaks it out of select() thus
>   increasing the generation counter.
>   

Right, you have to couple this with a signal sent from the TCG thread to 
the io thread.  And yeah, signals are not 100% reliable.

> - Its possible to miss signals (again, I can't see in happening 
>   in the scheme you suggest), but..
>
> Also note there is no need to be completly fair. It is fine to
> eventually be unfair.
>
> Do you worry about interaction between the two locks? Can make the lock
> ordering documented.
>   

I need to think about this a bit more.  I understand the idea behind 
qemu_signal_lock() a little better now.

Since we know that the IO thread is trying to run in TCG (because we 
sent a signal to it), I wonder if we can use that as an indicator that 
we have to let the IO thread run for a bit.

BTW, your patches lack commit messages and Signed-off-bys.  Are you not 
ready for having them committed?  I know that we still have to figure 
out Windows support but do you know of any other show stoppers?

Have you thought about how this is going to affect kvm-userspace?  Do 
you think we can eliminate the io threading code in kvm-userspace after 
this goes in?

Regards,

Anthony Liguori

> Will spend some time thinking about this, need to take multiple
> "starved" threads into account.
>
> Thanks.
>
>