From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50228) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VNzbm-00024D-Vn for qemu-devel@nongnu.org; Mon, 23 Sep 2013 02:27:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VNzbf-0006Dv-Io for qemu-devel@nongnu.org; Mon, 23 Sep 2013 02:26:58 -0400 Received: from thoth.sbs.de ([192.35.17.2]:24409) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VNzbf-0006Ca-9T for qemu-devel@nongnu.org; Mon, 23 Sep 2013 02:26:51 -0400 Message-ID: <523FDF24.8060208@siemens.com> Date: Mon, 23 Sep 2013 08:26:44 +0200 From: Jan Kiszka MIME-Version: 1.0 References: <1379837479-8419-1-git-send-email-pingfank@linux.vnet.ibm.com> <1379837479-8419-6-git-send-email-pingfank@linux.vnet.ibm.com> In-Reply-To: <1379837479-8419-6-git-send-email-pingfank@linux.vnet.ibm.com> Content-Type: text/plain; charset=ISO-8859-15 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH v4 4/4] timer: make qemu_clock_enable sync between disable and timer's cb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Liu Ping Fan Cc: Alex Bligh , Paolo Bonzini , Kevin Wolf , qemu-devel@nongnu.org, Stefan Hajnoczi On 2013-09-22 10:11, Liu Ping Fan wrote: > After disabling the QemuClock, we should make sure that no QemuTimers > are still in flight. To implement that with light overhead, we resort > to QemuEvent. The caller of disabling will wait on QemuEvent of each > timerlist. > > Note, qemu_clock_enable(foo,false) can _not_ be called from timer's cb. > And the callers of qemu_clock_enable() should be sync by themselves, > not protected by this patch. > > Signed-off-by: Liu Ping Fan > --- > include/qemu/timer.h | 4 ++++ > qemu-timer.c | 20 +++++++++++++++++++- > 2 files changed, 23 insertions(+), 1 deletion(-) > > diff --git a/include/qemu/timer.h b/include/qemu/timer.h > index e4934dd..b26909a 100644 > --- a/include/qemu/timer.h > +++ b/include/qemu/timer.h > @@ -185,6 +185,10 @@ void qemu_clock_notify(QEMUClockType type); > * @enabled: true to enable, false to disable > * > * Enable or disable a clock > + * Disabling the clock will wait for related timerlists to stop > + * executing qemu_run_timers. Thus, this functions should not > + * be used from the callback of a timer that is based on @clock. > + * Doing so would cause a deadlock. > */ > void qemu_clock_enable(QEMUClockType type, bool enabled); > > diff --git a/qemu-timer.c b/qemu-timer.c > index 95ff47f..c500a76 100644 > --- a/qemu-timer.c > +++ b/qemu-timer.c > @@ -45,6 +45,7 @@ > /* timers */ > > typedef struct QEMUClock { > + /* We rely on BQL to protect the timerlists */ > QLIST_HEAD(, QEMUTimerList) timerlists; > > NotifierList reset_notifiers; > @@ -70,6 +71,8 @@ struct QEMUTimerList { > QLIST_ENTRY(QEMUTimerList) list; > QEMUTimerListNotifyCB *notify_cb; > void *notify_opaque; > + /* light weight method to mark the end of timerlist's running */ > + QemuEvent ev; What about "timers_done_ev"? > }; > > /** > @@ -98,6 +101,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type, > QEMUClock *clock = qemu_clock_ptr(type); > > timer_list = g_malloc0(sizeof(QEMUTimerList)); > + qemu_event_init(&timer_list->ev, false); > timer_list->clock = clock; > timer_list->notify_cb = cb; > timer_list->notify_opaque = opaque; > @@ -140,13 +144,24 @@ void qemu_clock_notify(QEMUClockType type) > } > } > > +/* Disabling the clock will wait for related timerlists to stop > + * executing qemu_run_timers. Thus, this functions should not > + * be used from the callback of a timer that is based on @clock. > + * Doing so would cause a deadlock. > + */ > void qemu_clock_enable(QEMUClockType type, bool enabled) > { > QEMUClock *clock = qemu_clock_ptr(type); > + QEMUTimerList *tl; > bool old = clock->enabled; > clock->enabled = enabled; > if (enabled && !old) { > qemu_clock_notify(type); > + } else if (!enabled && old) { > + /* We rely on BQL to protect the timerlists */ So the caller of qemu_clock_enable has to hold the BQL? Then please add that to the function description above instead. Jan > + QLIST_FOREACH(tl, &clock->timerlists, list) { > + qemu_event_wait(&tl->ev); > + } > } > } > > @@ -373,8 +388,10 @@ bool timerlist_run_timers(QEMUTimerList *timer_list) > QEMUTimer *ts; > int64_t current_time; > bool progress = false; > - > + > + qemu_event_reset(&timer_list->ev); > if (!timer_list->clock->enabled) { > + qemu_event_set(&timer_list->ev); > return progress; > } > > @@ -392,6 +409,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list) > ts->cb(ts->opaque); > progress = true; > } > + qemu_event_set(&timer_list->ev); > return progress; > } > > -- Siemens AG, Corporate Technology, CT RTC ITP SES-DE Corporate Competence Center Embedded Linux