Re: [Qemu-devel] [PATCH 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, Aug 15, 2013 at 04:24:57PM +0800, liu ping fan wrote:
> On Thu, Aug 15, 2013 at 4:22 PM, liu ping fan <qemulist@gmail.com> wrote:
> > On Thu, Aug 15, 2013 at 4:01 PM, Stefan Hajnoczi <stefanha@gmail.com> wrote:
> >> On Thu, Aug 15, 2013 at 08:05:11AM +0800, liu ping fan wrote:
> >>> On Mon, Aug 12, 2013 at 8:49 PM, Stefan Hajnoczi <stefanha@redhat.com> wrote:
> >>> > @@ -376,13 +411,16 @@ bool timerlist_run_timers(QEMUTimerList *timer_list)
> >>> >
> >>> >      current_time = qemu_clock_get_ns(timer_list->clock->type);
> >>> >      for(;;) {
> >>> > +        qemu_mutex_lock(&timer_list->active_timers_lock);
> >>> >          ts = timer_list->active_timers;
> >>> >          if (!timer_expired_ns(ts, current_time)) {
> >>> > +            qemu_mutex_unlock(&timer_list->active_timers_lock);
> >>> >              break;
> >>> >          }
> >>> >          /* remove timer from the list before calling the callback */
> >>> >          timer_list->active_timers = ts->next;
> >>> >          ts->next = NULL;
> >>> > +        qemu_mutex_unlock(&timer_list->active_timers_lock);
> >>> >
> >>> Could we do better than this? lock/unlock around ts->cb always cause extra cost?
> >>> Beside this, others seems good.
> >>
> >> ts->cb() can do anything.  We need to drop the mutex to prevent
> >> recursive locking.
> >>
> >> There is no cheap way to clone the list before the loop (so that we
> >> don't need to hold any lock while iterating), and the list may change
> >> when ts->cb() is called.
> >>
> >> Did you have a specific improvement in mind?
> >>
> > How about new_list for vcpu to add timer, an before walking, splice
> > the new_list to timer_list?
> Of course, qemu_mod_timer_ns() should tell the caller, maybe by TLS?

The common case is that we only check the first timer in
->active_timers.  Usually the first timer has not expired yet and we
return; the lock was taken once only.

I'm not sure it's worth complicating the case where we iterate multiple
times.

Stefan