Re: [PATCH 19/20] perf tools: Reference count struct thread

[Namhyung Kim <namhyung@kernel.org>]

Hi Arnaldo,

On Tue, Mar 03, 2015 at 12:26:08AM -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> We need to do that to stop accumulating entries in the dead_threads
> linked list, i.e. we were keeping references to threads in struct hists
> that continue to exist even after a thread exited and was removed from
> the machine threads rbtree.
> 
> We still keep the dead_threads list, but just for debugging, allowing us
> to iterate at any given point over the threads that still are referenced
> by things like struct hist_entry.

[SNIP]
> diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
> index 70b48a65064c..95f5ab707b74 100644
> --- a/tools/perf/util/hist.c
> +++ b/tools/perf/util/hist.c
> @@ -355,6 +355,7 @@ static struct hist_entry *hist_entry__new(struct hist_entry *template,
>  			callchain_init(he->callchain);
>  
>  		INIT_LIST_HEAD(&he->pairs.node);
> +		thread__get(he->thread);
>  	}
>  
>  	return he;
> @@ -941,6 +942,7 @@ hist_entry__collapse(struct hist_entry *left, struct hist_entry *right)
>  
>  void hist_entry__delete(struct hist_entry *he)
>  {
> +	thread__zput(he->thread);
>  	zfree(&he->branch_info);
>  	zfree(&he->mem_info);
>  	zfree(&he->stat_acc);

[SNIP]
> diff --git a/tools/perf/util/thread.c b/tools/perf/util/thread.c
> index 9ebc8b1f9be5..a5dbba95107f 100644
> --- a/tools/perf/util/thread.c
> +++ b/tools/perf/util/thread.c
> @@ -82,6 +82,20 @@ void thread__delete(struct thread *thread)
>  	free(thread);
>  }
>  
> +struct thread *thread__get(struct thread *thread)
> +{
> +	++thread->refcnt;
> +	return thread;
> +}
> +
> +void thread__put(struct thread *thread)
> +{
> +	if (thread && --thread->refcnt == 0) {
> +		list_del_init(&thread->node);
> +		thread__delete(thread);
> +	}
> +}

I think we need to protect refcnt from concurrent accesses from
multiple threads.  Not to mention my multi-thread work, perf top
already uses two threads.

For perf top case, hist_entry__new() will be called from main thread
and hist_entry__delete() might be called from display thread.

Thanks,
Namhyung


> +
>  struct comm *thread__comm(const struct thread *thread)
>  {
>  	if (list_empty(&thread->comm_list))
> diff --git a/tools/perf/util/thread.h b/tools/perf/util/thread.h
> index 160fd066a7d1..783b6688d2f7 100644
> --- a/tools/perf/util/thread.h
> +++ b/tools/perf/util/thread.h
> @@ -20,6 +20,7 @@ struct thread {
>  	pid_t			tid;
>  	pid_t			ppid;
>  	int			cpu;
> +	int			refcnt;
>  	char			shortname[3];
>  	bool			comm_set;
>  	bool			dead; /* if set thread has exited */
> @@ -37,6 +38,18 @@ struct comm;
>  struct thread *thread__new(pid_t pid, pid_t tid);
>  int thread__init_map_groups(struct thread *thread, struct machine *machine);
>  void thread__delete(struct thread *thread);
> +
> +struct thread *thread__get(struct thread *thread);
> +void thread__put(struct thread *thread);
> +
> +static inline void __thread__zput(struct thread **thread)
> +{
> +	thread__put(*thread);
> +	*thread = NULL;
> +}
> +
> +#define thread__zput(thread) __thread__zput(&thread)
> +
>  static inline void thread__exited(struct thread *thread)
>  {
>  	thread->dead = true;
> -- 
> 1.9.3
>