From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46259) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6gLn-0000cc-Ci for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:27:02 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6gLi-0007W7-54 for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:26:55 -0400 Received: from mx1.redhat.com ([209.132.183.28]:16455) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6gLh-0007Vd-Sw for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:26:50 -0400 Date: Tue, 6 Aug 2013 14:26:33 +0200 From: Stefan Hajnoczi Message-ID: <20130806122633.GC30812@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1375780592-22842-6-git-send-email-alex@alex.org.uk> 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 , Paolo Bonzini , MORITA Kazutaka , rth@twiddle.net On Tue, Aug 06, 2013 at 10:16:21AM +0100, Alex Bligh wrote: > Split QEMUClock into QEMUClock and QEMUTimerList so that we can > have more than one QEMUTimerList associated with the same clock. > > Introduce a default_timerlist concept and make existing > qemu_clock_* calls that actually should operate on a QEMUTimerList > call the relevant QEMUTimerList implementations, using the clock's > default timerlist. This vastly reduces the invasiveness of this > change and means the API stays constant for existing users. > > Introduce a list of QEMUTimerLists associated with each clock > so that reenabling the clock can cause all the notifiers > to be called. Note the code to do the notifications is added > in a later patch. > > Switch QEMUClockType to an enum. Remove global variables vm_clock, > host_clock and rt_clock and add compatibility defines. Do not > fix qemu_next_alarm_deadline as it's going to be deleted. > > Signed-off-by: Alex Bligh > --- > include/qemu/timer.h | 91 +++++++++++++++++++++++-- > qemu-timer.c | 185 ++++++++++++++++++++++++++++++++++++++------------ > 2 files changed, 224 insertions(+), 52 deletions(-) Although in the short time it's easier to keep the old API where QEMUClock also acts as a timer list, I don't think it's a good idea to do that. It makes timers harder to understand for everyone because we now have timerlists but an extra layer to sort of make QEMUClock like a timerlist. The API we should end up with has two concepts: clock sources (rt, vm, host) and timer lists. This patch is adding unnecessary indirections and making the clock source look like a timer list. I think it's worth converting existing code once and for all instead of carrying this baggage forever. > +extern QEMUClock *qemu_clocks[QEMU_CLOCK_MAX]; > + > +static inline QEMUClock *qemu_get_clock(QEMUClockType type) > +{ > + return qemu_clocks[type]; > +} > + > +/* These three clocks are maintained here with separate variable > + names for compatibility only. > +*/ > + > /* The real time clock should be used only for stuff which does not > change the virtual machine state, as it is run even if the virtual > machine is stopped. The real time clock has a frequency of 1000 > Hz. */ > -extern QEMUClock *rt_clock; > +#define rt_clock (qemu_get_clock(QEMU_CLOCK_REALTIME)) > > /* The virtual clock is only run during the emulation. It is stopped > when the virtual machine is stopped. Virtual timers use a high > precision clock, usually cpu cycles (use ticks_per_sec). */ > -extern QEMUClock *vm_clock; > +#define vm_clock (qemu_get_clock(QEMU_CLOCK_VIRTUAL)) > > /* The host clock should be use for device models that emulate accurate > real time sources. It will continue to run when the virtual machine > is suspended, and it will reflect system time changes the host may > undergo (e.g. due to NTP). The host clock has the same precision as > the virtual clock. */ > -extern QEMUClock *host_clock; > +#define host_clock (qemu_get_clock(QEMU_CLOCK_HOST)) What is the point of this change? It's not clear how using qemu_clocks[] is an improvement over rt_clock, vm_clock, and host_clock. Or in other words: why is timerlist_new necessary? > struct QEMUTimer { > int64_t expire_time; /* in nanoseconds */ > - QEMUClock *clock; > + QEMUTimerList *tl; 'timer_list' is easier to read than just 'tl'. > -QEMUClock *qemu_new_clock(int type) > +void timerlist_free(QEMUTimerList *tl) > +{ Assert that active_timers is empty. > +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. Also, should it be '==' instead of '='?