From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:51294) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6ghW-0003J6-S9 for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:49:25 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V6ghR-0008TV-PI for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:49:22 -0400 Received: from mail.avalus.com ([2001:41c8:10:1dd::10]:54741) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V6ghR-0008S7-Gl for qemu-devel@nongnu.org; Tue, 06 Aug 2013 08:49:17 -0400 Date: Tue, 06 Aug 2013 13:49:03 +0100 From: Alex Bligh Message-ID: <5AE028D8D3CBDB45EFBBD20A@nimrod.local> In-Reply-To: <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> <20130806122633.GC30812@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit Content-Disposition: inline Subject: Re: [Qemu-devel] [RFC] [PATCHv6 05/16] aio / timers: Split QEMUClock into QEMUClock and QEMUTimerList Reply-To: Alex Bligh List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Anthony Liguori , qemu-devel@nongnu.org, liu ping fan , Alex Bligh , Paolo Bonzini , MORITA Kazutaka , rth@twiddle.net Stefan, > 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. 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. However it touches so much (it touches just about every driver) that it's going to be really hard for people to maintain a driver that works with two versions of qemu. This is really useful (having just backported the modern ceph driver to qemu 1.0 and 1.3). Moreover it's going to break any developer with an existing non-upstreamed branch The change is pretty mechanical, so what I'd suggest is that we keep the old API around too (as deprecated) for a little while. At some stage we can fix existing code and after that point not accept patches that use the existing API. Even if the period is just a month (i.e. the old API goes before 1.7), why break things unnecessarily? > 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. Because you can iterate through all the clocks with a for() loop as is done in six places in qemu-timer.c by the end of the patch series (look for QEMU_CLOCK_MAX). This also allows us to use a timerlistgroup (a set of timerlists, one for each clock) so each AioContext can have a clock of each type, as Paolo requested in response to v4. > Or in other words: why is timerlist_new necessary? I think that question is entirely orthogonal. This generates a new timerlist. In v4 each AioContext had its own timerlist. Now it has its own timerlistgroup, with one timerlist of each clock type. The constructor timerlist_new is there to initialise the timerlist which broadly is a malloc(), setting ->clock, and inserting it onto the list of timerlists associated with that clock. How would we avoid this if we still want multiple timerlists per clock? >> 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. >> -QEMUClock *qemu_new_clock(int type) >> +void timerlist_free(QEMUTimerList *tl) >> +{ > > Assert that active_timers is empty. OK >> +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. > Also, should it be '==' instead of '='? Good catch! -- Alex Bligh