Re: [Qemu-devel] [PATCH] main-loop: Don't lock starve io-threads when main_loop_tlg has pending events

[Alex Bligh <alex@alex.org.uk>]

On 8 Oct 2013, at 21:01, Hans de Goede wrote:

>>> +    /* When not non-blocking always allow io-threads to acquire the lock */
>>> +    if (timeout != 0 && timeout_ns == 0) {
>>> +        timeout_ns = 1;
>>> +    }
>>> +
>>>     ret = os_host_main_loop_wait(timeout_ns);
>>>     qemu_iohandler_poll(gpollfds, ret);
>>> #ifdef CONFIG_SLIRP
>> 
>> I /think/ you might mean "if (!blocking && timeout_ns == 0)"
>> as timeout can be zero on a blocking call at this stage (i.e.
>> when there is a timer which has already expired.
> 
> timeout does not get modified, except by the slirp-stuff, which
> won't ever set it to 0 I think.

Ignoring the slirp stuff (which is hopefully irrelevant):

For your patch to have any effect, timeout_ns must be zero
or your timeout_ns=1 would not execute.

timeout_ns should not be zero frequently for the reason
I described.

If the call to main_loop_wait is blocking (i.e. nonblocking==0),
timeout_ns should start at -1 - this is the case you described.
If the call is non-blocking (as I originally thought)
timeout_ns should start at zero.

Then this line happens:

    timeout_ns = qemu_soonest_timeout(timeout_ns,
                                      timerlistgroup_deadline_ns(
                                          &main_loop_tlg));

If timeout_ns is zero (i.e. non-blocking, caused by timeout=0)
this will do nothing (lack of optimisation and opacity here is
down to slirp stuff).

If timeout_ns is -1 (i.e. blocking, timeout=UINT32_MAX), then
this will use the return from
timerlistgroup_deadline_ns(). You said this was returning
zero, which would cause os_host_main_loop_wait() to
be called with a zero parameter.

This would explain the symptom, and explain why the fix
worked. But the question is why is timerlistgroup_deadline_ns
returning zero.


>  But your right writing
> 
>   if (!nonblocking && timeout_ns == 0)
> 
> would be much clearer, so I'll do that in v2 of the patch.


-- 
Alex Bligh