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 <qemul...@gmail.com> wrote:
> > On Thu, Aug 15, 2013 at 4:01 PM, Stefan Hajnoczi <stefa...@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 <stefa...@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
