From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:44509) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2I9I-0000D6-FZ for qemu-devel@nongnu.org; Thu, 25 Jul 2013 05:47:53 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V2I9H-000078-4R for qemu-devel@nongnu.org; Thu, 25 Jul 2013 05:47:52 -0400 Received: from mail.avalus.com ([2001:41c8:10:1dd::10]:40299) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V2Hz6-0005Cf-Hg for qemu-devel@nongnu.org; Thu, 25 Jul 2013 05:37:20 -0400 Date: Thu, 25 Jul 2013 10:37:06 +0100 From: Alex Bligh Message-ID: In-Reply-To: <20130725091015.GC21033@stefanha-thinkpad.redhat.com> References: <1E8E204.8000201@redhat.com> <1374343603-29183-1-git-send-email-alex@alex.org.uk> <1374343603-29183-2-git-send-email-alex@alex.org.uk> <20130725091015.GC21033@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] [PATCHv2] [RFC 1/7] aio / timers: Remove alarm timers Reply-To: Alex Bligh List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Stefan Hajnoczi Cc: Kevin Wolf , Anthony Liguori , Alex Bligh , qemu-devel@nongnu.org, Stefan Hajnoczi , Paolo Bonzini , rth@twiddle.net Stefan, > This should be one of the last patches so qemu.git remains bisectable. > Only remove the alarm timer once the event loops are already using the > timeout argument. OK >> @@ -245,11 +82,7 @@ static QEMUClock *qemu_new_clock(int type) >> >> void qemu_clock_enable(QEMUClock *clock, bool enabled) >> { >> - bool old = clock->enabled; >> clock->enabled = enabled; >> - if (enabled && !old) { >> - qemu_rearm_alarm_timer(alarm_timer); >> - } > > If this function is supposed to work when called from another thread > (e.g. vcpu thread), then you need to call qemu_notify_event(). For > AioContext clocks that should be aio_notify() with the relevant > AioContext, but we don't need that yet. Each AioContext knows which clock it has but each clock doesn't know if it's part of an AioContext. I suggest this is infrequent enough that always using qemu_notify_event() would be OK. That should interrupt any poll. >> int64_t qemu_clock_has_timers(QEMUClock *clock) >> @@ -340,10 +173,9 @@ void qemu_mod_timer_ns(QEMUTimer *ts, int64_t >> expire_time) >> >> /* Rearm if necessary */ >> if (pt == &ts->clock->active_timers) { >> - if (!alarm_timer->pending) { >> - qemu_rearm_alarm_timer(alarm_timer); >> - } >> - /* Interrupt execution to force deadline recalculation. */ >> + /* Interrupt execution to force deadline recalculation. >> + * FIXME: Do we need to do this now? >> + */ >> qemu_clock_warp(ts->clock); >> if (use_icount) { >> qemu_notify_event(); > > Same here. I think I just need to delete the FIXME comment. >> diff --git a/vl.c b/vl.c >> index 25b8f2f..612c609 100644 >> --- a/vl.c >> +++ b/vl.c >> @@ -3714,7 +3714,10 @@ int main(int argc, char **argv, char **envp) >> old_param = 1; >> break; >> case QEMU_OPTION_clock: >> - configure_alarms(optarg); >> + /* Once upon a time we did: >> + * configure_alarms(optarg); >> + * here. This is stubbed out for compatibility. >> + */ >> break; > > This could be made clearer to say outright that the options don't exist > anymore: > > /* Clock options no longer exist. Keep this option for backward > * compatibility. > */ OK -- Alex Bligh