Re: [BUG] segfault in perf-top -- thread refcnt

[Jiri Olsa]

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?

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);
 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/