From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752989AbbC3NJc (ORCPT ); Mon, 30 Mar 2015 09:09:32 -0400 Received: from bombadil.infradead.org ([198.137.202.9]:53386 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752701AbbC3NJb (ORCPT ); Mon, 30 Mar 2015 09:09:31 -0400 Date: Mon, 30 Mar 2015 10:09:40 -0300 From: Arnaldo Carvalho de Melo To: Jiri Olsa Cc: David Ahern , Jiri Olsa , Namhyung Kim , Stephane Eranian , LKML Subject: Re: [BUG] segfault in perf-top -- thread refcnt Message-ID: <20150330130940.GB24063@kernel.org> References: <551593EA.2030201@gmail.com> <20150327201126.GM21510@kernel.org> <5515B9E6.5020007@gmail.com> <20150330080737.GD1413@krava> <20150330102220.GE1413@krava> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150330102220.GE1413@krava> 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 12:22:20PM +0200, Jiri Olsa escreveu: > 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? Thread put should only remove stuff from the rbtree when the refcount hits zero, and then, it should grab a lock, that is the buggy part, as this lock isn't there, and remove the entry from the rbtree. Now I see what Namhyung was talking about... - Arnaldo > I'm trying attached patch.. so far so gut ;-) > > jirka > > > --- > diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c > index e335330..7e6abc7 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); > > /* > * We have to initialize map_groups separately > @@ -394,8 +398,10 @@ 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); > thread__delete(th); > + pthread_mutex_unlock(&machine->threads_lock); > return NULL; > } > /* > 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); >