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

[liu ping fan]

On Thu, Aug 15, 2013 at 4:22 PM, liu ping fan <address@hidden> wrote:
> On Thu, Aug 15, 2013 at 4:01 PM, Stefan Hajnoczi <address@hidden> 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 <address@hidden> 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?
>> Stefan