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

[Alex Bligh]

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.

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

> Allowing only one rt_clock clock for each AioContext is a simplification,
> but I'm worried that it will be a problem later.  For example, the block
> layer wants to use vm_clock.  Perhaps QEMUTimerList should really have
> three lists, one for each clock type?

Well currently each QEMUClock has a default QEMUTimerList, so that
wouldn't work well - see below.

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). Changing
vm_clock to vm_timerlist and vm_clock was truly horrible. Hence the
default timer list concept, which I admit is not great but saved a
horribly intrusive patch not all of which I could test. I did this
patch, and scrapped it.

> And because all timerlists have an AioContext,

Well old callers, particularly those not using an AioContext, would
not have an AioContext would they?

> you do not
> need to special case aio_notify() vs. qemu_notify_event().

Well, if I do a v5, I was going to make the constructor for
creating a timerlist specify a callback function to say what should
happen if the clock is enabled etc., and if none was specified
call qemu_notify_event(). The AioContext user would specify a callback
that called aio_notify(). This would be a bit nicer.

> There are a couple of places to be careful about, of course.  For example,
>
>         if (use_icount && qemu_clock_deadline(vm_clock) <= 0) {
>             qemu_notify_event();
>         }
>
> in cpus.c must be changed to iterate over all timerlists.

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?

--
Alex Bligh