Re: [Qemu-devel] [PATCH v4 4/4] timer: make qemu_clock_enable sync between disable and timer's cb

[liu ping fan]

On Mon, Sep 23, 2013 at 2:26 PM, Jan Kiszka <address@hidden> wrote:
> 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 <address@hidden>
>> ---
>>  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"?
>
fine, thx.
>>  };
>>
>>  /**
>> @@ -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.
>
Ok, thx.

Regards,
Pingfan
>> +        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