[v2,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 <pingfank@linux.vnet.ibm.com>
---
 include/qemu/timer.h |  4 ++++
 qemu-timer.c         | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

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 <pingfank@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;
>  }
>  
>

On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@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 <pingfank@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

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;
> >  }
> >
> >
>

Il 14/08/2013 02:34, liu ping fan ha scritto:
> On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@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 <pingfank@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;
>>>  }
>>>
>>>
>>

On Sun, Aug 18, 2013 at 10:54 PM, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Il 14/08/2013 02:34, liu ping fan ha scritto:
>> On Tue, Aug 13, 2013 at 10:53 PM, Paolo Bonzini <pbonzini@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 <pingfank@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?
>
Currently, no such guarantee based on BQL. But if we enforce that an
backend thread holds BQL before it cleans up resources, we can resort
to BQL. (so document this, and save the events_list_lock?)

Regards,
Pingfan

Il 19/08/2013 09:14, liu ping fan ha scritto:
>> > 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?
>> >
> Currently, no such guarantee based on BQL. But if we enforce that an
> backend thread holds BQL before it cleans up resources, we can resort
> to BQL. (so document this, and save the events_list_lock?)

Yes, please.

Paolo