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

[Paolo Bonzini]

Il 09/08/2013 16:27, Alex Bligh ha scritto:
> 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.

Perhaps, but do we really need it to work outside AioContexts?  Using a
second AioContext for bottom halves has been mentioned for a while.  It
would fix issues where a bottom half runs spuriously before QEMU starts,
just because something uses qemu_aio_wait().  Extending the same
approach to timers is natural.

At some point Stefan was planning to add a per-BlockDriverState
AioContext.  Once that is done, using two different AioContext for
"default block layer context" and "default main loop" context would be
very easily done.

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

There's also some more cleanup to do, for example you would also get
rid of the notify callback.

The point is that you can get rid altogether of TimerListGroup if you
just stick the array in the AioContext.  There's no use in adding a
concept with an easy plan to delete it, only waiting for someone willing
to do the work.  It is not related to anything that Stefan is
doing---the concept of TimerListGroup is introduced by this series.

Paolo

> 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;
>>>         }
>>>     }
>>>
>>>
>>
>>
>