Re: [Qemu-devel] [RFC] [PATCHv4 10/13] aio / timers: Convert mainloop to use timeout

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

Paolo,

(apologies for taking a little time to reply to this one)

--On 1 August 2013 16:14:11 +0200 Paolo Bonzini <pbonzini@redhat.com> wrote:

>> >> @@ -449,6 +460,7 @@ int main_loop_wait(int nonblocking)
>> >> {
>> >>     int ret;
>> >>     uint32_t timeout = UINT32_MAX;
>> >> +    int64_t timeout_ns;
>> >>
>> >>     if (nonblocking) {
>> >>         timeout = 0;
>> >> @@ -462,7 +474,21 @@ int main_loop_wait(int nonblocking)
>> >>     slirp_pollfds_fill(gpollfds);
>> >> # endif
>> >>     qemu_iohandler_fill(gpollfds);
>> >> -    ret = os_host_main_loop_wait(timeout);
>> >> +
>> >> +    if (timeout == UINT32_MAX) {
>> >> +        timeout_ns = -1;
>> >> +    } else {
>> >> +        timeout_ns = (uint64_t)timeout * (int64_t)(SCALE_MS);
>> >> +    }
>> >> +
>> >> +    timeout_ns = qemu_soonest_timeout(timeout_ns,
>> >> +
>> >> qemu_clock_deadline_ns(rt_clock)); +    timeout_ns =
>> >> qemu_soonest_timeout(timeout_ns,
>> >> +
>> >> qemu_clock_deadline_ns(vm_clock));
>> >
>> > This must not be included if use_icount.
>>
>> Really? qemu_run_all_timers was running all 3 timers irrespective of
>> use_icount, and doing a qemu_notify if anything expired, so I'm merely
>> mimicking the existing behaviour here.
>
> Maybe I'm misreading the code.  If it is a replacement of
> qemu_next_alarm_deadline, then it is indeed omitting vm_clock.

Well, qemu_next_alarm_deadline calculated the time at which the
next alarm signal would be sent. What I'm calculating is the
wait timeout for poll(), either in the mainloop or in AioContext.

In the mainloop, what this appears to do is to ignore the vm_clock
(whether or not it is enabled) in calculating the timeout if
use_icount is set. However qemu_run_timers is running the timers
attached to the vm_clock whether or not use_icount is set.

If use_icount is set, should be assuming vm_timers have an infinite
timeout for the purpose of calculating timeouts? What the the timer
has already expired (i.e. qemu_run_timers would run it immediately)?

In AioContext, there were previously no timers, so I'm not sure
whether or not this should be doing the same thing.

>> I'm not quite sure what use_icount does. Does vm_clock get disabled
>> if it is set? (in which case it won't matter).
>
> It doesn't count nanoseconds anymore.

"it" being the vm_timer clock source I presume.

> The clock is updated by the
> VCPU thread.  When the VCPU thread notices that the clock is past the
> earliest timers, it does a qemu_notify_event().  That exits the g_poll
> and qemu_run_all_timers then can process the callbacks.

So the first problem is that this will not cause the other threads
to have aio_notify called, which they should do.

The second question is whether this approach is consistent with using
a timeout for poll at all. I think the answer to that is "yes"
PROVIDED THAT the VCPU thread is never the one doing the poll,
or we'll go in with an infinite timeout.

I am presuming use_icount never changes at runtime? If not, I
think the way to address this is to add a 'lineartime' member
to QEMUClock, and set that false on the vm_timer QEMUClock if
use_icount is true. I can then ignore it when looping through
clocksources bases on that rather than special casing vm_clock.

>> Yep - per the above that's really intrusive (I think I touched well over
>> a hundred files). The problem is that lots of things refer to vm_clock
>> to set a timer (when it's a clock so should use a timer list) and
>> also to vm_clock to read the timer (which is a clock function).
>
> Yes, that's fine.  You can still keep the shorter invocation,
> but instead of using clock->timerlist it would use
> qemu_aio_context->clocks[clock->type].

Note that the aio context contains a QEMUTimerList and not
a clock. So it would have 3 QEMUTimerLists, and not 3 QEMUClocks.

I might see if there is a neat way to encapsulate these
(QEMUTimerListGroup or similar?) but yes I get the idea.

Do you want this in a v5 or are we polishing the ball a bit
much here?

> Related to this, a better name for the "full" functions taking
> a timerlist could be simply timer_new_ns etc.  And I would remove
> the allocation step for these functions.  It is shameful how little
> we use qemu_free_timer, and removing allocation would "fix" the
> problem completely for users of the QEMUTimerList-based functions.
>
> It's already a convention to use qemu_* only for functions that use some
> global state, for example qemu_notify_event() vs. aio_notify().

Agree

>> > And because all timerlists have an AioContext,
>>
>> Well old callers, particularly those not using an AioContext, would
>> not have an AioContext would they?
>
> It would be qemu_aio_context.

If this were the case, then the timeout for the mainloop would be
the same as the timeout for the qemu_aio_context. Is that an improvement?

What about a (putative) thread that wants a timer but has no AioContext?

>> I was trying hard to avoid anything having to iterate over all
>> timerlists, and leave the timerlist to be per-thread where possible.
>> This may well fail for the clock warp stuff. I probably need to
>> exactly the same as on qemu_clock_enable() here if use_icount is
>> true. WDYT?
>
> Yes.  This:
>
>         qemu_mod_timer(icount_warp_timer, vm_clock_warp_start + deadline);
>
> would have to use the earliest deadline of all vm_clock timerlists.
>
> And this:
>
>         if (qemu_clock_expired(vm_clock)) {
>             qemu_notify_event();
>         }
>
> would also have to walk all timerlists for vm_clock, and notify
> those that have expired.  But you would not need one warp timer
> per timerlist.

OK looks like I need to review the patch looking for use_icount.

-- 
Alex Bligh