Re: [Qemu-devel] [RFC] [PATCHv8 09/30] aio / timers: Add QEMUTimerListGroup and helper functions

[Alex Bligh]

Paolo,

>> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
>> index 1baa0e2..3b46f60 100644
>> --- a/include/qemu/timer.h
>> +++ b/include/qemu/timer.h
>> @@ -52,8 +52,10 @@ typedef enum {
>> 
>> typedef struct QEMUClock QEMUClock;
>> typedef struct QEMUTimerList QEMUTimerList;
>> +typedef QEMUTimerList *QEMUTimerListGroup[QEMU_CLOCK_MAX];
> 
> Please wrap this in a struct for easier future extensibility.

OK

> I'm not a big fan of the TimerListGroup name, but I cannot think of
> anything better...

Ditto

> ... except if we get rid of TimerListGroup completely and put it in
> AioContext.  To do so, we can have _two_ AioContexts in main-loop.c.
> One is for the block layer, the other is only used for timers.  Later we
> could move bottom halves from the first to the second, too.

I don't really want to do this, or at least I don't want to do it yet.
Partly my concern is about initialising a dummy AioContext and having
it hang around. Partly the point is that the timer stuff is meant to
work without any AioContexts. But mostly I think Stefan is already
looking at reworking the thing into a single event loop so this
problem will go away of its own accord.

Apart from the fact I will have just wrapped it in a struct ( :-) ),
the entire change required then will be:

1. delete main_loop_ltg
2. rewrite the wrappers to use a default AioContext
3. delete the typedef and put the raw array into the AioContext
  struct (having deleted main_loop_tlg as unused.

This is all pretty trivial and I think best done after Stefan's
work than before.

Alex

> Sorry for not thinking of this before.
> 
> Paolo
> 
>> typedef void QEMUTimerCB(void *opaque);
>> 
>> +extern QEMUTimerListGroup main_loop_tlg;
>> extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
>> 
>> /**
>> @@ -211,6 +213,49 @@ QEMUClock *timerlist_get_clock(QEMUTimerList 
>> *timer_list);
>> bool timerlist_run_timers(QEMUTimerList *timer_list);
>> 
>> /**
>> + * timerlistgroup_init:
>> + * @tlg: the timer list group
>> + *
>> + * Initialise a timer list group. This must already be
>> + * allocated in memory and zeroed.
>> + */
>> +void timerlistgroup_init(QEMUTimerListGroup tlg);
>> +
>> +/**
>> + * timerlistgroup_deinit:
>> + * @tlg: the timer list group
>> + *
>> + * Deinitialise a timer list group. This must already be
>> + * initialised. Note the memory is not freed.
>> + */
>> +void timerlistgroup_deinit(QEMUTimerListGroup tlg);
>> +
>> +/**
>> + * timerlistgroup_run_timers:
>> + * @tlg: the timer list group
>> + *
>> + * Run the timers associated with a timer list group.
>> + * This will run timers on multiple clocks.
>> + *
>> + * Returns: true if any timer callback ran
>> + */
>> +bool timerlistgroup_run_timers(QEMUTimerListGroup tlg);
>> +
>> +/**
>> + * timerlistgroup_deadline_ns
>> + * @tlg: the timer list group
>> + *
>> + * Determine the deadline of the soonest timer to
>> + * expire associated with any timer list linked to
>> + * the timer list group. Only clocks suitable for
>> + * deadline calculation are included.
>> + *
>> + * Returns: the deadline in nanoseconds or -1 if no
>> + * timers are to expire.
>> + */
>> +int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
>> +
>> +/**
>>  * qemu_timeout_ns_to_ms:
>>  * @ns: nanosecond timeout value
>>  *
>> diff --git a/qemu-timer.c b/qemu-timer.c
>> index 1a0a4b1..e151d24 100644
>> --- a/qemu-timer.c
>> +++ b/qemu-timer.c
>> @@ -59,6 +59,7 @@ struct QEMUClock {
>>     bool enabled;
>> };
>> 
>> +QEMUTimerListGroup main_loop_tlg;
>> QEMUClock *qemu_clocks[QEMU_CLOCK_MAX];
>> 
>> /* A QEMUTimerList is a list of timers attached to a clock. More
>> @@ -575,6 +576,45 @@ bool qemu_run_timers(QEMUClock *clock)
>>     return timerlist_run_timers(clock->main_loop_timerlist);
>> }
>> 
>> +void timerlistgroup_init(QEMUTimerListGroup tlg)
>> +{
>> +    QEMUClockType type;
>> +    for (type = 0; type < QEMU_CLOCK_MAX; type++) {
>> +        tlg[type] = timerlist_new(type);
>> +    }
>> +}
>> +
>> +void timerlistgroup_deinit(QEMUTimerListGroup tlg)
>> +{
>> +    QEMUClockType type;
>> +    for (type = 0; type < QEMU_CLOCK_MAX; type++) {
>> +        timerlist_free(tlg[type]);
>> +    }
>> +}
>> +
>> +bool timerlistgroup_run_timers(QEMUTimerListGroup tlg)
>> +{
>> +    QEMUClockType type;
>> +    bool progress = false;
>> +    for (type = 0; type < QEMU_CLOCK_MAX; type++) {
>> +        progress |= timerlist_run_timers(tlg[type]);
>> +    }
>> +    return progress;
>> +}
>> +
>> +int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg)
>> +{
>> +    int64_t deadline = -1;
>> +    QEMUClockType type;
>> +    for (type = 0; type < QEMU_CLOCK_MAX; type++) {
>> +        if (qemu_clock_use_for_deadline(tlg[type]->clock)) {
>> +            deadline = qemu_soonest_timeout(deadline,
>> +                                            
>> timerlist_deadline_ns(tlg[type]));
>> +        }
>> +    }
>> +    return deadline;
>> +}
>> +
>> int64_t qemu_get_clock_ns(QEMUClock *clock)
>> {
>>     int64_t now, last;
>> @@ -616,6 +656,7 @@ void init_clocks(void)
>>     for (type = 0; type < QEMU_CLOCK_MAX; type++) {
>>         if (!qemu_clocks[type]) {
>>             qemu_clocks[type] = qemu_clock_new(type);
>> +            main_loop_tlg[type] = qemu_clocks[type]->main_loop_timerlist;
>>         }
>>     }
>> 
>> 
> 
> 

-- 
Alex Bligh