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

[Alex Bligh]

Stefan,

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

I think I disagree here.

At the very least we should put the conversion to use the new API
into a separate patch (possibly a separate patch set). It's fantastically
intrusive.

However it touches so much (it touches just about every driver) that
it's going to be really hard for people to maintain a driver that
works with two versions of qemu. This is really useful (having just
backported the modern ceph driver to qemu 1.0 and 1.3). Moreover it's
going to break any developer with an existing non-upstreamed branch

The change is pretty mechanical, so what I'd suggest is that we keep
the old API around too (as deprecated) for a little while. At some
stage we can fix existing code and after that point not accept patches
that use the existing API.

Even if the period is just a month (i.e. the old API goes before 1.7),
why break things unnecessarily?

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

Because you can iterate through all the clocks with a for() loop as
is done in six places in qemu-timer.c by the end of the patch
series (look for QEMU_CLOCK_MAX). This also allows us to use
a timerlistgroup (a set of timerlists, one for each clock) so
each AioContext can have a clock of each type, as Paolo requested
in response to v4.

> Or in other words: why is timerlist_new necessary?

I think that question is entirely orthogonal. This generates a new
timerlist. In v4 each AioContext had its own timerlist. Now it has
its own timerlistgroup, with one timerlist of each clock type.
The constructor timerlist_new is there to initialise the timerlist
which broadly is a malloc(), setting ->clock, and inserting it
onto the list of timerlists associated with that clock. How would
we avoid this if we still want multiple timerlists per clock?

> >  struct QEMUTimer {
> >      int64_t expire_time;       /* in nanoseconds */
> > -    QEMUClock *clock;
> > +    QEMUTimerList *tl;
>
> 'timer_list' is easier to read than just 'tl'.

It caused a pile of line wrap issues which made the patch harder
to read, so I shortened it. I can put it back if you like.

> > -QEMUClock *qemu_new_clock(int type)
> > +void timerlist_free(QEMUTimerList *tl)
> > +{
>
> Assert that active_timers is empty.

OK

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

I will comment it, but it mostly does what it says in the tin. Per
Paolo's comment, the vm_clock should not be used for calculation of
deadlines to ppoll etc. if use_icount is true, because it's not actually
in nanoseconds; rather qemu_notify() or aio_notify() get called by the
vm cpu thread when the relevant instruction counter is exceeded.

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

Good catch!

--
Alex Bligh