Re: [Qemu-devel] [RFC PATCH] main-loop: Unconditionally unlock iothread

[Anthony Liguori <aliguori@us.ibm.com>]

Paolo Bonzini <pbonzini@redhat.com> writes:

> Il 04/04/2013 01:58, Peter Crosthwaite ha scritto:
>> 
>> I think there may be a flaw in that "any of the descriptors being
>> pollable" is not a good definition of progress. stdin is blocked by
>> the fact that the device and mux cannot accept their data anymore so
>> even though its readable, no meaningful read will happen. That leaves
>> us with having to devise more elaborate code to define progress, or we
>> simplify by just removing this nonblocking optimisation altogether
>> (original patch).
>
> If stdin is blocked, it shouldn't be polled at all.  That is the purpose
> of the can_read callback.  Unfortunately, return FALSE from the prepare
> callback still leaves the poll handler.
>
> So your original patch fixes the symptom, but leaves the busy waiting
> unfixed.
>
> The right thing to use would be g_source_add_child_source() and
> g_source_remove_child_source(), but that is only present since glib 2.28
> and we currently require 2.12 (2.20 on Windows).

I don't think a child source fixes the problem.  The backend definitely
has work to do.  What we don't know is whether the front end is capable
of processing the work.

The problem here is that we use polling on the front-end.  IOW:

1) Char backend has data and is ready to write.
2) Asks front end if it can write
3) Front end says no
4) Goto (1)

It does this without dropping the BQL which means that TCG never gets to
run.

Ideally, we would do:

1) Char backend has data and is ready to write.
2) Asks front end if it can write
3) Backend stops asking front end until front end says it can receive
   data again
4) Drop BQL because we have no more work to do.

But this requires eliminating all users of can_read() which isn't going
to happen any time soon.

The solution therefore needs to be:

1) Char backend has data and is ready to write.
2) Asks front end if it can write
3) Front end says no
3.5) Drop the BQL and give the VCPU a chance to run
4) Goto (1)

Which is essentially what Peter's patch does.  In fact, you can imagine
other scenarios where a very busy VNC client essentially blocks the
BQL.  We need to regularly give up the BQL in order to prevent
starvation regardless of the polling in the char layer.

So I think this is a long way of saying:

Reviewed-by: Anthony Liguori <aliguori@us.ibm.com>

Regards,

Anthony Liguori

>
> Anthony, Amit, can you look at it?
>
> Paolo