Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList

[Stefan Hajnoczi]

On Tue, Aug 06, 2013 at 10:16:21AM +0100, Alex Bligh wrote:
> Split QEMUClock into QEMUClock and QEMUTimerList so that we can
> have more than one QEMUTimerList associated with the same clock.
> 
> Introduce a default_timerlist concept and make existing
> qemu_clock_* calls that actually should operate on a QEMUTimerList
> call the relevant QEMUTimerList implementations, using the clock's
> default timerlist. This vastly reduces the invasiveness of this
> change and means the API stays constant for existing users.
> 
> Introduce a list of QEMUTimerLists associated with each clock
> so that reenabling the clock can cause all the notifiers
> to be called. Note the code to do the notifications is added
> in a later patch.
> 
> Switch QEMUClockType to an enum. Remove global variables vm_clock,
> host_clock and rt_clock and add compatibility defines. Do not
> fix qemu_next_alarm_deadline as it's going to be deleted.
> 
> Signed-off-by: Alex Bligh <address@hidden>
> ---
>  include/qemu/timer.h |   91 +++++++++++++++++++++++--
>  qemu-timer.c         |  185 
> ++++++++++++++++++++++++++++++++++++++------------
>  2 files changed, 224 insertions(+), 52 deletions(-)

Although in the short time it's easier to keep the old API where
QEMUClock also acts as a timer list, I don't think it's a good idea to
do that.

It makes timers harder to understand for everyone because we now have
timerlists but an extra layer to sort of make QEMUClock like a
timerlist.

The API we should end up with has two concepts: clock sources (rt, vm,
host) and timer lists.  This patch is adding unnecessary indirections
and making the clock source look like a timer list.  I think it's worth
converting existing code once and for all instead of carrying this
baggage forever.

> +extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
> +
> +static inline QEMUClock *qemu_get_clock(QEMUClockType type)
> +{
> +    return qemu_clocks[type];
> +}
> +
> +/* These three clocks are maintained here with separate variable
> +   names for compatibility only.
> +*/
> +
>  /* The real time clock should be used only for stuff which does not
>     change the virtual machine state, as it is run even if the virtual
>     machine is stopped. The real time clock has a frequency of 1000
>     Hz. */
> -extern QEMUClock *rt_clock;
> +#define rt_clock (qemu_get_clock(QEMU_CLOCK_REALTIME))
>  
>  /* The virtual clock is only run during the emulation. It is stopped
>     when the virtual machine is stopped. Virtual timers use a high
>     precision clock, usually cpu cycles (use ticks_per_sec). */
> -extern QEMUClock *vm_clock;
> +#define vm_clock (qemu_get_clock(QEMU_CLOCK_VIRTUAL))
>  
>  /* The host clock should be use for device models that emulate accurate
>     real time sources. It will continue to run when the virtual machine
>     is suspended, and it will reflect system time changes the host may
>     undergo (e.g. due to NTP). The host clock has the same precision as
>     the virtual clock. */
> -extern QEMUClock *host_clock;
> +#define host_clock (qemu_get_clock(QEMU_CLOCK_HOST))

What is the point of this change?  It's not clear how using
qemu_clocks[] is an improvement over rt_clock, vm_clock, and host_clock.

Or in other words: why is timerlist_new necessary?

>  struct QEMUTimer {
>      int64_t expire_time;     /* in nanoseconds */
> -    QEMUClock *clock;
> +    QEMUTimerList *tl;

'timer_list' is easier to read than just 'tl'.

> -QEMUClock *qemu_new_clock(int type)
> +void timerlist_free(QEMUTimerList *tl)
> +{

Assert that active_timers is empty.

> +bool qemu_clock_use_for_deadline(QEMUClock *clock)
> +{
> +    return !(use_icount && (clock->type = QEMU_CLOCK_VIRTUAL));
> +}

Please use doc comments (see include/object/qom.h for example doc
comment syntax).  No idea why this function is needed or what it will be
used for.

Also, should it be '==' instead of '='?