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

[Stefan Hajnoczi <stefanha@gmail.com>]

On Tue, Aug 06, 2013 at 03:52:37PM +0100, Alex Bligh wrote:
> 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?

Those wrappers are not necessary.  Once the caller has their QEMUTimer
pointer they should use the QEMUTimer APIs directly.

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

Okay, great.  This makes the conversion from legacy QEMUClock functions
pretty straightforward.  It can be done mechanically in a single big
patch that converts the tree.

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

Yes, I'm convinced now that the it's worth having.

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

If you convert both that's good too.

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

Thanks!