[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock int
From: |
Stefan Hajnoczi |
Subject: |
Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList |
Date: |
Wed, 7 Aug 2013 09:27:36 +0200 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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!
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, (continued)
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, Stefan Hajnoczi, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, Stefan Hajnoczi, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 08/16] aio / timers: Add QEMUTimerListGroup to AioContext, Stefan Hajnoczi, 2013/08/07
- [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Stefan Hajnoczi, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Stefan Hajnoczi, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList,
Stefan Hajnoczi <=
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/07
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList, Stefan Hajnoczi, 2013/08/06
- [Qemu-devel] [RFC] [PATCHv6 06/16] aio / timers: Untangle include files, Alex Bligh, 2013/08/06
- [Qemu-devel] [RFC] [PATCHv6 10/16] aio / timers: aio_ctx_prepare sets timeout from AioContext timers, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 10/16] aio / timers: aio_ctx_prepare sets timeout from AioContext timers, Stefan Hajnoczi, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 10/16] aio / timers: aio_ctx_prepare sets timeout from AioContext timers, Alex Bligh, 2013/08/06
- [Qemu-devel] [RFC] [PATCHv6 09/16] aio / timers: Add a notify callback to QEMUTimerList, Alex Bligh, 2013/08/06
- Re: [Qemu-devel] [RFC] [PATCHv6 09/16] aio / timers: Add a notify callback to QEMUTimerList, Stefan Hajnoczi, 2013/08/06