From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from eggs.gnu.org ([2001:4830:134:3::10]:56977)
	by lists.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V9sUy-0001lh-DC
	for qemu-devel@nongnu.org; Thu, 15 Aug 2013 04:01:45 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V9sUp-00073x-UD
	for qemu-devel@nongnu.org; Thu, 15 Aug 2013 04:01:36 -0400
Received: from mail-ee0-x22b.google.com ([2a00:1450:4013:c00::22b]:50170)
	by eggs.gnu.org with esmtp (Exim 4.71)
	(envelope-from <stefanha@gmail.com>) id 1V9sUp-00073o-NY
	for qemu-devel@nongnu.org; Thu, 15 Aug 2013 04:01:27 -0400
Received: by mail-ee0-f43.google.com with SMTP id e52so200390eek.16
	for <qemu-devel@nongnu.org>; Thu, 15 Aug 2013 01:01:27 -0700 (PDT)
Date: Thu, 15 Aug 2013 10:01:24 +0200
From: Stefan Hajnoczi <stefanha@gmail.com>
Message-ID: <20130815080124.GC22521@stefanha-thinkpad.redhat.com>
References: <1376311769-29811-1-git-send-email-stefanha@redhat.com>
	<1376311769-29811-3-git-send-email-stefanha@redhat.com>
	<CAJnKYQn5bV-3Pop1TGP51rjB9GRb1F9HB5xrOoDXLYD50v0=hw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAJnKYQn5bV-3Pop1TGP51rjB9GRb1F9HB5xrOoDXLYD50v0=hw@mail.gmail.com>
Subject: Re: [Qemu-devel] [PATCH 2/2] qemu-timer: make qemu_timer_mod_ns()
 and qemu_timer_del() thread-safe
List-Id: <qemu-devel.nongnu.org>
List-Unsubscribe: <https://lists.nongnu.org/mailman/options/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.nongnu.org/archive/html/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <https://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: liu ping fan <qemulist@gmail.com>
Cc: Alex Bligh <alex@alex.org.uk>, Paolo Bonzini <pbonzini@redhat.com>, Ping Fan Liu <pingfank@linux.vnet.ibm.com>, qemu-devel@nongnu.org, Stefan Hajnoczi <stefanha@redhat.com>

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?

Stefan