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

[Namhyung Kim]

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);
>  
--
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/