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

[Anthony Liguori]

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