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

[Paolo Bonzini <pbonzini@redhat.com>]

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