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

[Paolo Bonzini <pbonzini@redhat.com>]

 On Aug 01 2013, Alex Bligh wrote:
> Paolo,
> 
> >>@@ -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.

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

> The way it's done at the moment is that the QEMUTimerList user can
> create as many QEMUTimerLists as he wants. So AioContext asks for one
> of one type. It could equally ask for three - one of each type.
> 
> I think that's probably adequate.
> 
> >Once you do this, you get some complications due to more data structures,
> >but other code is simplified noticeably.  For example, you lose the
> >concept of a default timerlist (it's just the timerlist of the default
> >AioContext).
> 
> 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].

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

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

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

Paolo