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

[Marcelo Tosatti <mtosatti@redhat.com>]

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.

- 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.

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

Thanks.