From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:49222) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9wKT-0003jp-VO for qemu-devel@nongnu.org; Thu, 15 Aug 2013 08:07:08 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1V9wEv-0003Gw-8n for qemu-devel@nongnu.org; Thu, 15 Aug 2013 08:01:54 -0400 Received: from mx1.redhat.com ([209.132.183.28]:21584) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1V9wEv-0003Gf-0c for qemu-devel@nongnu.org; Thu, 15 Aug 2013 08:01:17 -0400 Date: Thu, 15 Aug 2013 14:00:53 +0200 From: Stefan Hajnoczi Message-ID: <20130815120053.GA30514@stefanha-thinkpad.redhat.com> References: <1376311769-29811-1-git-send-email-stefanha@redhat.com> <1376311769-29811-3-git-send-email-stefanha@redhat.com> <20130815080124.GC22521@stefanha-thinkpad.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-timer: make qemu_timer_mod_ns() and qemu_timer_del() thread-safe List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: liu ping fan Cc: Stefan Hajnoczi , Ping Fan Liu , qemu-devel@nongnu.org, Alex Bligh , Paolo Bonzini 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 wrote: > > On Thu, Aug 15, 2013 at 4:01 PM, Stefan Hajnoczi 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 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