On Aug 05 2013, 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 <pingf...@linux.vnet.ibm.com>
> ---
> include/qemu/timer.h | 1 +
> qemu-timer.c | 11 +++++++++++
> 2 files changed, 12 insertions(+)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 1363316..ca09ba2 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -85,6 +85,7 @@ int64_t timerlistgroup_deadline_ns(QEMUTimerListGroup tlg);
>
> int qemu_timeout_ns_to_ms(int64_t ns);
> int qemu_poll_ns(GPollFD *fds, uint nfds, int64_t timeout);
> +/* The disable of clock can not be called in timer's cb */
See below for a more verbose version of the comment. For
now leave it only in the .c file, we should add comments to
all of timer.h.
> void qemu_clock_enable(QEMUClock *clock, bool enabled);
> void qemu_clock_warp(QEMUClock *clock);
>
> diff --git a/qemu-timer.c b/qemu-timer.c
> index ebe7597..5828107 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -71,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;
> };
>
> struct QEMUTimer {
> @@ -92,6 +94,7 @@ static QEMUTimerList *timerlist_new_from_clock(QEMUClock
> *clock)
> QEMUTimerList *tl;
>
> tl = g_malloc0(sizeof(QEMUTimerList));
> + qemu_event_init(&tl->ev, false);
The event should start as "set", since "set" means "not inside
qemu_run_timers".
> tl->clock = clock;
> QLIST_INSERT_HEAD(&clock->timerlists, tl, list);
> return tl;
> @@ -145,12 +148,18 @@ void qemu_clock_notify(QEMUClock *clock)
> }
> }
>
> +/* The disable of clock can _not_ be called from timer's cb */
/* 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(QEMUClock *clock, bool enabled)
> {
> + QEMUTimerList *tl;
> bool old = clock->enabled;
> clock->enabled = enabled;
> if (enabled && !old) {
> qemu_clock_notify(clock);
> + } else if (!enabled && old) {
> + QLIST_FOREACH(tl, &clock->timerlists, list) {
> + qemu_event_wait(&tl->ev);
> + }
> }
> }
>
> @@ -419,6 +428,7 @@ bool timerlist_run_timers(QEMUTimerList *tl)
> }
>
> current_time = qemu_get_clock_ns(tl->clock);
> + qemu_event_reset(&tl->ev);
Race condition here. You need to test clock->enabled while the
event is reset. Otherwise you get:
-------------------------------------------------------------------------
thread 1 is running thread 2 is running
qemu_clock_enable(foo, false) qemu_run_timers(tl);
-------------------------------------------------------------------------
** event is initially set **
if (!clock->enabled) return;
clock->enabled = false;
qemu_event_wait(&tl->ev);
return;
qemu_event_reset(&tl->ev);
invokes callback
qemu_event_set(&tl->ev);
-------------------------------------------------------------------------
violating the invariant that no callbacks are invoked after the return from
qemu_clock_enable(foo, false).
Paolo
> for(;;) {
> ts = tl->active_timers;
> if (!qemu_timer_expired_ns(ts, current_time)) {
> @@ -432,6 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *tl)
> ts->cb(ts->opaque);
> progress = true;
> }
> + qemu_event_set(&tl->ev);
> return progress;
> }
>
> --
> 1.8.1.4
>
