From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752855AbbC3Mtu (ORCPT ); Mon, 30 Mar 2015 08:49:50 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:34400 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752503AbbC3Mtr (ORCPT ); Mon, 30 Mar 2015 08:49:47 -0400 Date: Mon, 30 Mar 2015 21:48:52 +0900 From: Namhyung Kim To: Jiri Olsa Cc: David Ahern , Arnaldo Carvalho de Melo , Jiri Olsa , Stephane Eranian , LKML Subject: Re: [BUG] segfault in perf-top -- thread refcnt Message-ID: <20150330124852.GA4507@danjae.kornet> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20150330114907.GH1413@krava> 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 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. :-/ Thanks, Namhyung > > /* > * We have to initialize map_groups separately > @@ -394,7 +398,9 @@ static struct thread *__machine__findnew_thread(struct machine *machine, > * leader and that would screwed the rb tree. > */ > if (thread__init_map_groups(th, machine)) { > + pthread_mutex_lock(&machine->threads_lock); > rb_erase(&th->rb_node, &machine->threads); > + pthread_mutex_unlock(&machine->threads_lock); > thread__delete(th); > return NULL; > } > @@ -1258,7 +1264,6 @@ static void machine__remove_thread(struct machine *machine, struct thread *th) > if (machine->last_match == th) > thread__zput(machine->last_match); > > - rb_erase(&th->rb_node, &machine->threads); > /* > * 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 e2faf3b..e3468d6 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_mutex_t threads_lock; > struct list_head dead_threads; > struct thread *last_match; > struct vdso_info *vdso_info; > diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c > index 1c8fbc9..4592fc4 100644 > --- a/tools/perf/util/thread.c > +++ b/tools/perf/util/thread.c > @@ -26,7 +26,7 @@ int thread__init_map_groups(struct thread *thread, struct machine *machine) > return thread->mg ? 0 : -1; > } > > -struct thread *thread__new(pid_t pid, pid_t tid) > +struct thread *thread__new(struct machine *machine, pid_t pid, pid_t tid) > { > char *comm_str; > struct comm *comm; > @@ -38,6 +38,7 @@ struct thread *thread__new(pid_t pid, pid_t tid) > thread->ppid = -1; > thread->cpu = -1; > INIT_LIST_HEAD(&thread->comm_list); > + thread->machine = machine; > > if (unwind__prepare_access(thread) < 0) > goto err_thread; > @@ -91,7 +92,14 @@ struct thread *thread__get(struct thread *thread) > void thread__put(struct thread *thread) > { > if (thread && --thread->refcnt == 0) { > + struct machine *machine = thread->machine; > + > list_del_init(&thread->node); > + > + pthread_mutex_lock(&machine->threads_lock); > + rb_erase(&thread->rb_node, &machine->threads); > + pthread_mutex_unlock(&machine->threads_lock); > + > thread__delete(thread); > } > } > diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h > index 9b8a54d..df6fb69 100644 > --- a/tools/perf/util/thread.h > +++ b/tools/perf/util/thread.h > @@ -31,12 +31,13 @@ struct thread { > > void *priv; > struct thread_stack *ts; > + struct machine *machine; > }; > > struct machine; > struct comm; > > -struct thread *thread__new(pid_t pid, pid_t tid); > +struct thread *thread__new(struct machine *machine, pid_t pid, pid_t tid); > int thread__init_map_groups(struct thread *thread, struct machine *machine); > void thread__delete(struct thread *thread); >