Re: [PATCH V6 02/17] perf tools: Ensure thread-stack is flushed

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Fri, May 29, 2015 at 04:33:30PM +0300, Adrian Hunter escreveu:
> The thread-stack represents a thread's current stack.  When
> a thread exits there can still be many functions on the stack
> e.g. exit() can be called many levels deep, so all the callers
> will never return.  To get that information output, the
> thread-stack must be flushed.
> 
> Previously it was assumed the thread-stack would be flushed
> when the struct thread was deleted.  With thread ref-counting
> it is no longer clear when that will be, if ever. So instead

It'll be when the last reference to that thread is released.

- Arnaldo

> explicitly flush all the thread-stacks at the end of a session.

If after the session ends you have no more need for those thread stacks,
that is the right way to do it.

With tools like 'report', after the session ends we should have all the
unreferenced threads deleted.

Previously they were not being deleted at all, i.e. they were simply
moved to the dead_threads list and sat there because I didn't knew if
some hist_entry, say, had a pointer to it.

So, unless I am missing something, this patch is required irrespective
of thread refcounting, no?

I'm applying it to my work branch where I'm trying to test all this.

- Arnaldo
 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/machine.c      | 21 +++++++++++++++++++++
>  tools/perf/util/machine.h      |  3 +++
>  tools/perf/util/session.c      | 20 ++++++++++++++++++++
>  tools/perf/util/thread-stack.c | 18 +++++++++++++-----
>  tools/perf/util/thread-stack.h |  1 +
>  5 files changed, 58 insertions(+), 5 deletions(-)
900380
> 
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index 0c0e61c..c0c29b9 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1845,6 +1845,27 @@ int machine__for_each_thread(struct machine *machine,
>  	return rc;
>  }
>  
> +int machines__for_each_thread(struct machines *machines,
> +			      int (*fn)(struct thread *thread, void *p),
> +			      void *priv)
> +{
> +	struct rb_node *nd;
> +	int rc = 0;
> +
> +	rc = machine__for_each_thread(&machines->host, fn, priv);
> +	if (rc != 0)
> +		return rc;
> +
> +	for (nd = rb_first(&machines->guests); nd; nd = rb_next(nd)) {
> +		struct machine *machine = rb_entry(nd, struct machine, rb_node);
> +
> +		rc = machine__for_each_thread(machine, fn, priv);
> +		if (rc != 0)
> +			return rc;
> +	}
> +	return rc;
> +}
> +
>  int __machine__synthesize_threads(struct machine *machine, struct perf_tool *tool,
>  				  struct target *target, struct thread_map *threads,
>  				  perf_event__handler_t process, bool data_mmap)
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index c7963c6..6b4a6fb 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -213,6 +213,9 @@ size_t machine__fprintf_vmlinux_path(struct machine *machine, FILE *fp);
>  int machine__for_each_thread(struct machine *machine,
>  			     int (*fn)(struct thread *thread, void *p),
>  			     void *priv);
> +int machines__for_each_thread(struct machines *machines,
> +			      int (*fn)(struct thread *thread, void *p),
> +			      void *priv);
>  
>  int __machine__synthesize_threads(struct machine *machine, struct perf_tool *tool,
>  				  struct target *target, struct thread_map *threads,
> diff --git a/tools/perf/util/session.c b/tools/perf/util/session.c
> index 39fe09d..b44bb2a 100644
> --- a/tools/perf/util/session.c
> +++ b/tools/perf/util/session.c
> @@ -16,6 +16,7 @@
>  #include "perf_regs.h"
>  #include "asm/bug.h"
>  #include "auxtrace.h"
> +#include "thread-stack.h"
>  
>  static int perf_session__deliver_event(struct perf_session *session,
>  				       union perf_event *event,
> @@ -1320,6 +1321,19 @@ static void perf_session__warn_about_errors(const struct perf_session *session)
>  	events_stats__auxtrace_error_warn(stats);
>  }
>  
> +static int perf_session__flush_thread_stack(struct thread *thread,
> +					    void *p __maybe_unused)
> +{
> +	return thread_stack__flush(thread);
> +}
> +
> +static int perf_session__flush_thread_stacks(struct perf_session *session)
> +{
> +	return machines__for_each_thread(&session->machines,
> +					 perf_session__flush_thread_stack,
> +					 NULL);
> +}
> +
>  volatile int session_done;
>  
>  static int __perf_session__process_pipe_events(struct perf_session *session)
> @@ -1409,6 +1423,9 @@ done:
>  	if (err)
>  		goto out_err;
>  	err = auxtrace__flush_events(session, tool);
> +	if (err)
> +		goto out_err;
> +	err = perf_session__flush_thread_stacks(session);
>  out_err:
>  	free(buf);
>  	perf_session__warn_about_errors(session);
> @@ -1559,6 +1576,9 @@ out:
>  	if (err)
>  		goto out_err;
>  	err = auxtrace__flush_events(session, tool);
> +	if (err)
> +		goto out_err;
> +	err = perf_session__flush_thread_stacks(session);
>  out_err:
>  	ui_progress__finish();
>  	perf_session__warn_about_errors(session);
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index 9ed59a4..679688e 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -219,7 +219,7 @@ static int thread_stack__call_return(struct thread *thread,
>  	return crp->process(&cr, crp->data);
>  }
>  
> -static int thread_stack__flush(struct thread *thread, struct thread_stack *ts)
> +static int __thread_stack__flush(struct thread *thread, struct thread_stack *ts)
>  {
>  	struct call_return_processor *crp = ts->crp;
>  	int err;
> @@ -242,6 +242,14 @@ static int thread_stack__flush(struct thread *thread, struct thread_stack *ts)
>  	return 0;
>  }
>  
> +int thread_stack__flush(struct thread *thread)
> +{
> +	if (thread->ts)
> +		return __thread_stack__flush(thread, thread->ts);
> +
> +	return 0;
> +}
> +
>  int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>  			u64 to_ip, u16 insn_len, u64 trace_nr)
>  {
> @@ -264,7 +272,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>  	 */
>  	if (trace_nr != thread->ts->trace_nr) {
>  		if (thread->ts->trace_nr)
> -			thread_stack__flush(thread, thread->ts);
> +			__thread_stack__flush(thread, thread->ts);
>  		thread->ts->trace_nr = trace_nr;
>  	}
>  
> @@ -297,7 +305,7 @@ void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr)
>  
>  	if (trace_nr != thread->ts->trace_nr) {
>  		if (thread->ts->trace_nr)
> -			thread_stack__flush(thread, thread->ts);
> +			__thread_stack__flush(thread, thread->ts);
>  		thread->ts->trace_nr = trace_nr;
>  	}
>  }
> @@ -305,7 +313,7 @@ void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr)
>  void thread_stack__free(struct thread *thread)
>  {
>  	if (thread->ts) {
> -		thread_stack__flush(thread, thread->ts);
> +		__thread_stack__flush(thread, thread->ts);
>  		zfree(&thread->ts->stack);
>  		zfree(&thread->ts);
>  	}
> @@ -689,7 +697,7 @@ int thread_stack__process(struct thread *thread, struct comm *comm,
>  
>  	/* Flush stack on exec */
>  	if (ts->comm != comm && thread->pid_ == thread->tid) {
> -		err = thread_stack__flush(thread, ts);
> +		err = __thread_stack__flush(thread, ts);
>  		if (err)
>  			return err;
>  		ts->comm = comm;
> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
> index b843bbe..e1528f1 100644
> --- a/tools/perf/util/thread-stack.h
> +++ b/tools/perf/util/thread-stack.h
> @@ -96,6 +96,7 @@ int thread_stack__event(struct thread *thread, u32 flags, u64 from_ip,
>  void thread_stack__set_trace_nr(struct thread *thread, u64 trace_nr);
>  void thread_stack__sample(struct thread *thread, struct ip_callchain *chain,
>  			  size_t sz, u64 ip);
> +int thread_stack__flush(struct thread *thread);
>  void thread_stack__free(struct thread *thread);
>  
>  struct call_return_processor *
> -- 
> 1.9.1