Il 14/08/2013 02:34, liu ping fan ha scritto: > On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonz...@redhat.com> wrote: >> >> 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. >> > Here is a ugly pattern issue, we hold events_list_lock and wait for > QemuEvent set. If the modifier reset the QemuEvent and then try to > hold the events_list_lock, then _deadlock_. To eliminate the > possibility, using @events_list_lock, and you can see the modifier > can not reset QemuEvent while trying to own the lock. On the other > handle, if using lock on timerlists, since many entrance to access the > lock, we are not sure of avoiding deadlock
But does timerlists need a lock, or does the BQL suffice? If it doesn't, there is no need for events_list_lock either. Is qemu_clock_enable called outside the BQL? Paolo > Regards, > Pingfan >>> 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; >>> } >>> >>> >>