From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752899AbbC3O6K (ORCPT ); Mon, 30 Mar 2015 10:58:10 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:50678 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752755AbbC3O57 (ORCPT ); Mon, 30 Mar 2015 10:57:59 -0400 Date: Mon, 30 Mar 2015 11:58:05 -0300 From: Arnaldo Carvalho de Melo To: Namhyung Kim Cc: Jiri Olsa , David Ahern , Jiri Olsa , Stephane Eranian , LKML Subject: Re: [BUG] segfault in perf-top -- thread refcnt Message-ID: <20150330145805.GC32560@kernel.org> References: <551593EA.2030201@gmail.com> <20150327201126.GM21510@kernel.org> <5515B9E6.5020007@gmail.com> <20150330080737.GD1413@krava> <20150330102220.GE1413@krava> <20150330112108.GG1413@krava> <20150330114907.GH1413@krava> <20150330124852.GA4507@danjae.kornet> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150330124852.GA4507@danjae.kornet> X-Url: http://acmel.wordpress.com User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Em Mon, Mar 30, 2015 at 09:48:52PM +0900, Namhyung Kim escreveu: > Hi Jiri, > > On Mon, Mar 30, 2015 at 01:49:07PM +0200, Jiri Olsa wrote: > > On Mon, Mar 30, 2015 at 01:21:08PM +0200, Jiri Olsa wrote: > > > On Mon, Mar 30, 2015 at 12:22:20PM +0200, Jiri Olsa wrote: > > > > On Mon, Mar 30, 2015 at 10:07:37AM +0200, Jiri Olsa wrote: > > > > > > > > SNIP > > > > > > > > > > > > > > > > 2 things: > > > > > > 1. let run for a long time. go about using the server. do lots of builds, > > > > > > etc. it takes time > > > > > > > > > > > > 2. use a box with a LOT of cpus (1024 in my case) > > > > > > > > > > > > Make sure ulimit is set to get the core. > > > > > > > > > > reproduced under 24 cpu box with kernel build (make -j25) > > > > > running on background.. will try to look closer > > > > > > > > > > perf: Segmentation fault > > > > > -------- backtrace -------- > > > > > ./perf[0x4fd79b] > > > > > /lib64/libc.so.6(+0x358f0)[0x7f9cbff528f0] > > > > > ./perf(thread__put+0x5b)[0x4b1a7b] > > > > > ./perf(hists__delete_entries+0x70)[0x4c8670] > > > > > ./perf[0x436a88] > > > > > ./perf[0x4fa73d] > > > > > ./perf(perf_evlist__tui_browse_hists+0x97)[0x4fc437] > > > > > ./perf[0x4381d0] > > > > > /lib64/libpthread.so.0(+0x7ee5)[0x7f9cc1ff2ee5] > > > > > /lib64/libc.so.6(clone+0x6d)[0x7f9cc0011b8d] > > > > > [0x0] > > > > > > > > looks like race among __machine__findnew_thread and thread__put > > > > over the machine->threads rb_tree insert/removal > > > > > > > > is there a reason why thread__put does not erase itself from machine->threads? > > > > that was the reason.. we do this separately.. not in thread__put.. > > is there a reason for this? ;-) > > > > testing attached patch.. > > > > jirka > > > > > > --- > > diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c > > index f7fb258..966564a 100644 > > --- a/tools/perf/util/build-id.c > > +++ b/tools/perf/util/build-id.c > > @@ -60,7 +60,6 @@ static int perf_event__exit_del_thread(struct perf_tool *tool __maybe_unused, > > event->fork.ppid, event->fork.ptid); > > > > if (thread) { > > - rb_erase(&thread->rb_node, &machine->threads); > > if (machine->last_match == thread) > > thread__zput(machine->last_match); > > thread__put(thread); > > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > > index e335330..a8443ef 100644 > > --- a/tools/perf/util/machine.c > > +++ b/tools/perf/util/machine.c > > @@ -30,6 +30,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid) > > dsos__init(&machine->kernel_dsos); > > > > machine->threads = RB_ROOT; > > + pthread_mutex_init(&machine->threads_lock, NULL); > > INIT_LIST_HEAD(&machine->dead_threads); > > machine->last_match = NULL; > > > > @@ -380,10 +381,13 @@ static struct thread *__machine__findnew_thread(struct machine *machine, > > if (!create) > > return NULL; > > > > - th = thread__new(pid, tid); > > + th = thread__new(machine, pid, tid); > > if (th != NULL) { > > + > > + pthread_mutex_lock(&machine->threads_lock); > > rb_link_node(&th->rb_node, parent, p); > > rb_insert_color(&th->rb_node, &machine->threads); > > + pthread_mutex_unlock(&machine->threads_lock); > > I think you also need to protect the rb tree traversal above. > > But this makes every sample processing grabs and releases the lock so > might cause high overhead. It can be a problem if such processing is > done parallelly like my multi-thread work. :-/ Still untested, using rw lock, next step is auditing the machine__findnew_thread users that really should be using machine__find_thread, i.e. grabbing just the reader lock, and measuring the overhead of using a pthread rw lock instead of pthread_mutex_t as Jiri is doing. - Arnaldo diff --git a/tools/perf/util/build-id.c b/tools/perf/util/build-id.c index f7fb2587df69..e3c80bab47a3 100644 --- a/tools/perf/util/build-id.c +++ b/tools/perf/util/build-id.c @@ -60,7 +60,9 @@ static int perf_event__exit_del_thread(struct perf_tool *tool __maybe_unused, event->fork.ppid, event->fork.ptid); if (thread) { + pthread_rwlock_wrlock(&machine->threads_lock); rb_erase(&thread->rb_node, &machine->threads); + pthread_rwlock_unlock(&machine->threads_lock); if (machine->last_match == thread) thread__zput(machine->last_match); thread__put(thread); diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index e45c8f33a8fd..b901ed27a793 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -30,6 +30,7 @@ int machine__init(struct machine *machine, const char *root_dir, pid_t pid) dsos__init(&machine->kernel_dsos); machine->threads = RB_ROOT; + pthread_rwlock_init(&machine->threads_lock, NULL); INIT_LIST_HEAD(&machine->dead_threads); machine->last_match = NULL; @@ -111,6 +112,7 @@ void machine__exit(struct machine *machine) vdso__exit(machine); zfree(&machine->root_dir); zfree(&machine->current_tid); + pthread_rwlock_destroy(&machines->threads_lock); } void machine__delete(struct machine *machine) @@ -411,13 +413,22 @@ static struct thread *__machine__findnew_thread(struct machine *machine, struct thread *machine__findnew_thread(struct machine *machine, pid_t pid, pid_t tid) { - return __machine__findnew_thread(machine, pid, tid, true); + struct thread *th; + + pthread_rwlock_wrlock(&machine->threads_lock); + th = __machine__findnew_thread(machine, pid, tid, true); + pthread_rwlock_unlock(&machine->threads_lock); + return th; } struct thread *machine__find_thread(struct machine *machine, pid_t pid, pid_t tid) { - return __machine__findnew_thread(machine, pid, tid, false); + struct thread *th; + pthread_rwlock_rdlock(&machine->threads_lock); + th = __machine__findnew_thread(machine, pid, tid, false); + pthread_rwlock_unlock(&machine->threads_lock); + return th; } struct comm *machine__thread_exec_comm(struct machine *machine, @@ -1258,7 +1269,9 @@ static void machine__remove_thread(struct machine *machine, struct thread *th) if (machine->last_match == th) thread__zput(machine->last_match); + pthread_rwlock_wrlock(&machine->threads_lock); rb_erase(&th->rb_node, &machine->threads); + pthread_rwlock_unlock(&machine->threads_lock); /* * Move it first to the dead_threads list, then drop the reference, * if this is the last reference, then the thread__delete destructor diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h index e2faf3b47e7b..c2b9402921fc 100644 --- a/tools/perf/util/machine.h +++ b/tools/perf/util/machine.h @@ -30,6 +30,7 @@ struct machine { bool comm_exec; char *root_dir; struct rb_root threads; + pthread_rwlock_t threads_lock; struct list_head dead_threads; struct thread *last_match; struct vdso_info *vdso_info;