From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:48704) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6y9s-0000cp-86 for qemu-devel@nongnu.org; Wed, 07 Aug 2013 03:27:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6y9j-0005aN-Qz for qemu-devel@nongnu.org; Wed, 07 Aug 2013 03:27:48 -0400 Received: from mail-wi0-x232.google.com ([2a00:1450:400c:c05::232]:32918) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6y9j-0005aB-Gl for qemu-devel@nongnu.org; Wed, 07 Aug 2013 03:27:39 -0400 Received: by mail-wi0-f178.google.com with SMTP id j17so1318017wiw.11 for ; Wed, 07 Aug 2013 00:27:38 -0700 (PDT) Date: Wed, 7 Aug 2013 09:27:36 +0200 From: Stefan Hajnoczi Message-ID: <20130807072735.GC19825@stefanha-thinkpad.redhat.com> References: <1375639805-1943-1-git-send-email-alex@alex.org.uk> <1375780592-22842-1-git-send-email-alex@alex.org.uk> <1375780592-22842-6-git-send-email-alex@alex.org.uk> <20130806122633.GC30812@stefanha-thinkpad.redhat.com> <5AE028D8D3CBDB45EFBBD20A@nimrod.local> <20130806142940.GB4373@stefanha-thinkpad.redhat.com> <314ACE1E3E8E2438F2A4AB5F@nimrod.local> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <314ACE1E3E8E2438F2A4AB5F@nimrod.local> Subject: Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alex Bligh Cc: Kevin Wolf , Anthony Liguori , qemu-devel@nongnu.org, liu ping fan , Stefan Hajnoczi , Paolo Bonzini , MORITA Kazutaka , rth@twiddle.net 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!