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

[Alex Bligh]

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