Stefan,
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.
Yes, it should be a separate patch.
...
Even if the period is just a month (i.e. the old API goes before 1.7),
why break things unnecessarily?
Nothing upstream breaks. Only out-of-tree code breaks but that's life.
What's important is that upstream will be clean and easy to understand
or debug. Given how undocumented the QEMU codebase is, leaving legacy
API layers around just does more to confuse new contributors.
That's why I'd really like to transition now instead of leaving things
in a more complex state than before.
OK. I'm just concerned about the potential fall out. If that's
what everyone wants, I will do a monster patch to fix this up. Need
that be part of this series? I can't help thinking it would be
better to have the series applied first.
We end up with:
AioContext->tlg and default_timerlistgroup.
Regarding naming, I think default_timerlistgroup should be called
main_loop_timerlistgroup instead. The meaning of "default" is not
obvious.
I agree. I will change this.
Now let's think about how callers will create QEMUTimers:
1. AioContext
timer_new(ctx->tlg[QEMU_CLOCK_RT], SCALE_NS, cb, opaque);
Or with a wrapper:
QEMUTimer *aio_new_timer(AioContext *ctx, QEMUClockType type, int
scale, QEMUTimerCB *cb, void *opaque)
{
return timer_new(ctx->tlg[type], scale, cb, opaque);
}
aio_new_timer(ctx, QEMU_CLOCK_RT, SCALE_NS, cb, opaque);
I was actually thinking about adding that wrapper anyway.
Do you think we need to wrap timer_mod, timer_del, timer_free
etc. to make aio_timer_mod and so forth, even though they are
a straight pass through?
2. main-loop
/* without legacy qemu_timer_new() */
timer_new(main_loop_tlg[QEMU_CLOCK_RT], SCALE_NS, cb, opaque);
Or with a wrapper:
QEMUTimer *qemu_new_timer(QEMUClockType type, int scale,
QEMUTimerCB *cb, void *opaque)
{
return timer_new(main_loop_tlg[type], scale, cb, opaque);
}
qemu_new_timer(QEMU_CLOCK_RT, SCALE_NS, cb, opaque);
Is this what you have in mind too?
Yes.
Certainly qemu_timer_new (as opposed to qemu_new_timer)
would be a good addition.
But this doesn't allow for the array indexing that you do in
TimerListGroup later. I didn't know that at this point in the patch
series.
Yep. I'll leave that if that's OK.
>> 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.
Are you sure it's the QEMUTimer->tl field that causes line wraps?
I took a quick look and it seemed like only the QEMUTimerList *tl
function argument to the deadline functions could cause line wrap. The
argument variable is unrelated and can stay short since it has a very
narrow context - the reader can be expected to remember the tl argument
while reading the code for the function.
From memory it was lots of ->tl expanding within the code the issue
rather than the arguments to functions. I'll try again. To be honest
it's probably easier to change tl to timer_list throughout.
>> +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.
I didn't know that but the explanation makes sense. Definitely
something that could be in a comment. Perhaps its best to introduce
this small helper function in the patch that actually calls it.
It's preparation for the next patch. I will add a comment in the
commit message.
--
Alex Bligh
