Re: [Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb
On Tue, Aug 27, 2013 at 11:32 PM, Alex Bligh wrote:
>
> On 27 Aug 2013, at 04:21, 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.
>
> Presumably they should also be called with the BQL held?
>
Yes, will document this.
Thx,
Pingfan
>> */
>> 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;
>> };
>>
>> /**
>> @@ -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 */
>> +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;
>> }
>>
>> --
>> 1.8.1.4
>>
>>
>>
>
> --
> Alex Bligh
>
>
>
>
Re: [Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb
On 27 Aug 2013, at 04:21, 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.
Presumably they should also be called with the BQL held?
> */
> 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;
> };
>
> /**
> @@ -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 */
> +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;
> }
>
> --
> 1.8.1.4
>
>
>
--
Alex Bligh
[Qemu-devel] [PATCH v3 4/4] timer: make qemu_clock_enable sync between disable and timer's cb
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;
};
/**
@@ -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 */
+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;
}
--
1.8.1.4
