Il 13/08/2013 07:43, Liu Ping Fan ha scritto:
> 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 | 4 ++++
> qemu-timer.c | 53
> +++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 56 insertions(+), 1 deletion(-)
>
> diff --git a/include/qemu/timer.h b/include/qemu/timer.h
> index 829c005..2b755c9 100644
> --- a/include/qemu/timer.h
> +++ b/include/qemu/timer.h
> @@ -184,6 +184,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 5b9a722..8b32e92 100644
> --- a/qemu-timer.c
> +++ b/qemu-timer.c
> @@ -48,6 +48,12 @@ typedef struct QEMUClock {
> QLIST_HEAD(, QEMUTimerList) timerlists;
>
> NotifierList reset_notifiers;
> + /* While the reader holds this lock, it may block on events_list.
> + * So the modifier should be carefuly not to reset the event before
> + * holding this lock. Otherwise, deadlock.
> + */
> + QemuMutex events_list_lock;
> + GList *events_list;
No need for a separate list. Just use the timerlists list; if
events_list needs a lock, timerlists needs one too.
> int64_t last;
>
> QEMUClockType type;
> @@ -70,6 +76,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;
Also no need to malloc this one.
Paolo
> };
>
> /**
> @@ -90,6 +98,25 @@ static bool timer_expired_ns(QEMUTimer *timer_head,
> int64_t current_time)
> return timer_head && (timer_head->expire_time <= current_time);
> }
>
> +static QemuEvent *qemu_clock_new_qemu_event(QEMUClock *clock)
> +{
> + QemuEvent *ev = g_malloc(sizeof(QemuEvent));
> +
> + qemu_event_init(ev, true);
> + qemu_mutex_lock(&clock->events_list_lock);
> + clock->events_list = g_list_append(clock->events_list, ev);
> + qemu_mutex_unlock(&clock->events_list_lock);
> + return ev;
> +}
> +
> +static void qemu_clock_free_qemu_event(QEMUClock *clock, QemuEvent *ev)
> +{
> + qemu_mutex_lock(&clock->events_list_lock);
> + clock->events_list = g_list_remove(clock->events_list, ev);
> + qemu_mutex_unlock(&clock->events_list_lock);
> + qemu_event_destroy(ev);
> +}
> +
> QEMUTimerList *timerlist_new(QEMUClockType type,
> QEMUTimerListNotifyCB *cb,
> void *opaque)
> @@ -98,6 +125,7 @@ QEMUTimerList *timerlist_new(QEMUClockType type,
> QEMUClock *clock = qemu_clock_ptr(type);
>
> timer_list = g_malloc0(sizeof(QEMUTimerList));
> + timer_list->ev = qemu_clock_new_qemu_event(clock);
> timer_list->clock = clock;
> timer_list->notify_cb = cb;
> timer_list->notify_opaque = opaque;
> @@ -109,6 +137,7 @@ void timerlist_free(QEMUTimerList *timer_list)
> {
> assert(!timerlist_has_timers(timer_list));
> if (timer_list->clock) {
> + qemu_clock_free_qemu_event(timer_list->clock, timer_list->ev);
> QLIST_REMOVE(timer_list, list);
> }
> g_free(timer_list);
> @@ -122,6 +151,7 @@ static void qemu_clock_init(QEMUClockType type)
> clock->enabled = true;
> clock->last = INT64_MIN;
> QLIST_INIT(&clock->timerlists);
> + qemu_mutex_init(&clock->events_list_lock);
> notifier_list_init(&clock->reset_notifiers);
> main_loop_tlg.tl[type] = timerlist_new(type, NULL, NULL);
> }
> @@ -140,6 +170,17 @@ void qemu_clock_notify(QEMUClockType type)
> }
> }
>
> +static void clock_event_wait(gpointer key, gpointer opaque)
> +{
> + QemuEvent *ev = key;
> + qemu_event_wait(ev);
> +}
> +
> +/* 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);
> @@ -147,6 +188,13 @@ void qemu_clock_enable(QEMUClockType type, bool enabled)
> clock->enabled = enabled;
> if (enabled && !old) {
> qemu_clock_notify(type);
> + } else if (!enabled && old) {
> + /* We may block while holding @events_list_lock, but the modifier is
> + * guaranteed not to reset the event. So we can avoid deadlock.
> + */
> + qemu_mutex_lock(&clock->events_list_lock);
> + g_list_foreach(clock->events_list, clock_event_wait, NULL);
> + qemu_mutex_unlock(&clock->events_list_lock);
> }
> }
>
> @@ -373,8 +421,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 +442,7 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> ts->cb(ts->opaque);
> progress = true;
> }
> + qemu_event_set(timer_list->ev);
> return progress;
> }
>
>
