Re: [PATCH 1/8] perf db-export: Add calls parent_id

[Arnaldo Carvalho de Melo <arnaldo.melo@gmail.com>]

Em Thu, Feb 28, 2019 at 03:00:24PM +0200, Adrian Hunter escreveu:
> The call_path can be used to find the parent symbol for a call but not the
> exact parent call. To do that add parent_id to the call_return export. This
> enables the creation of a call tree from the exported data.

Thanks, applied.

- Arnaldo
 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/util/db-export.c                      | 15 ++++++++++-----
>  tools/perf/util/db-export.h                      |  3 ++-
>  .../util/scripting-engines/trace-event-python.c  |  8 +++++---
>  tools/perf/util/thread-stack.c                   | 16 ++++++++++++++--
>  tools/perf/util/thread-stack.h                   |  6 ++++--
>  5 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/db-export.c b/tools/perf/util/db-export.c
> index de9b4769d06c..d7315a00c731 100644
> --- a/tools/perf/util/db-export.c
> +++ b/tools/perf/util/db-export.c
> @@ -510,18 +510,23 @@ int db_export__call_path(struct db_export *dbe, struct call_path *cp)
>  	return 0;
>  }
>  
> -int db_export__call_return(struct db_export *dbe, struct call_return *cr)
> +int db_export__call_return(struct db_export *dbe, struct call_return *cr,
> +			   u64 *parent_db_id)
>  {
>  	int err;
>  
> -	if (cr->db_id)
> -		return 0;
> -
>  	err = db_export__call_path(dbe, cr->cp);
>  	if (err)
>  		return err;
>  
> -	cr->db_id = ++dbe->call_return_last_db_id;
> +	if (!cr->db_id)
> +		cr->db_id = ++dbe->call_return_last_db_id;
> +
> +	if (parent_db_id) {
> +		if (!*parent_db_id)
> +			*parent_db_id = ++dbe->call_return_last_db_id;
> +		cr->parent_db_id = *parent_db_id;
> +	}
>  
>  	if (dbe->export_call_return)
>  		return dbe->export_call_return(dbe, cr);
> diff --git a/tools/perf/util/db-export.h b/tools/perf/util/db-export.h
> index 67bc6b8ad2d6..4e2424c89df9 100644
> --- a/tools/perf/util/db-export.h
> +++ b/tools/perf/util/db-export.h
> @@ -104,6 +104,7 @@ int db_export__sample(struct db_export *dbe, union perf_event *event,
>  int db_export__branch_types(struct db_export *dbe);
>  
>  int db_export__call_path(struct db_export *dbe, struct call_path *cp);
> -int db_export__call_return(struct db_export *dbe, struct call_return *cr);
> +int db_export__call_return(struct db_export *dbe, struct call_return *cr,
> +			   u64 *parent_db_id);
>  
>  #endif
> diff --git a/tools/perf/util/scripting-engines/trace-event-python.c b/tools/perf/util/scripting-engines/trace-event-python.c
> index 0e17db41b49b..09604c6508f0 100644
> --- a/tools/perf/util/scripting-engines/trace-event-python.c
> +++ b/tools/perf/util/scripting-engines/trace-event-python.c
> @@ -1173,7 +1173,7 @@ static int python_export_call_return(struct db_export *dbe,
>  	u64 comm_db_id = cr->comm ? cr->comm->db_id : 0;
>  	PyObject *t;
>  
> -	t = tuple_new(11);
> +	t = tuple_new(12);
>  
>  	tuple_set_u64(t, 0, cr->db_id);
>  	tuple_set_u64(t, 1, cr->thread->db_id);
> @@ -1186,6 +1186,7 @@ static int python_export_call_return(struct db_export *dbe,
>  	tuple_set_u64(t, 8, cr->return_ref);
>  	tuple_set_u64(t, 9, cr->cp->parent->db_id);
>  	tuple_set_s32(t, 10, cr->flags);
> +	tuple_set_u64(t, 11, cr->parent_db_id);
>  
>  	call_object(tables->call_return_handler, t, "call_return_table");
>  
> @@ -1194,11 +1195,12 @@ static int python_export_call_return(struct db_export *dbe,
>  	return 0;
>  }
>  
> -static int python_process_call_return(struct call_return *cr, void *data)
> +static int python_process_call_return(struct call_return *cr, u64 *parent_db_id,
> +				      void *data)
>  {
>  	struct db_export *dbe = data;
>  
> -	return db_export__call_return(dbe, cr);
> +	return db_export__call_return(dbe, cr, parent_db_id);
>  }
>  
>  static void python_process_general_event(struct perf_sample *sample,
> diff --git a/tools/perf/util/thread-stack.c b/tools/perf/util/thread-stack.c
> index a8b45168513c..41942c2aaa18 100644
> --- a/tools/perf/util/thread-stack.c
> +++ b/tools/perf/util/thread-stack.c
> @@ -49,6 +49,7 @@ enum retpoline_state_t {
>   * @timestamp: timestamp (if known)
>   * @ref: external reference (e.g. db_id of sample)
>   * @branch_count: the branch count when the entry was created
> + * @db_id: id used for db-export
>   * @cp: call path
>   * @no_call: a 'call' was not seen
>   * @trace_end: a 'call' but trace ended
> @@ -59,6 +60,7 @@ struct thread_stack_entry {
>  	u64 timestamp;
>  	u64 ref;
>  	u64 branch_count;
> +	u64 db_id;
>  	struct call_path *cp;
>  	bool no_call;
>  	bool trace_end;
> @@ -280,12 +282,14 @@ static int thread_stack__call_return(struct thread *thread,
>  		.comm = ts->comm,
>  		.db_id = 0,
>  	};
> +	u64 *parent_db_id;
>  
>  	tse = &ts->stack[idx];
>  	cr.cp = tse->cp;
>  	cr.call_time = tse->timestamp;
>  	cr.return_time = timestamp;
>  	cr.branch_count = ts->branch_count - tse->branch_count;
> +	cr.db_id = tse->db_id;
>  	cr.call_ref = tse->ref;
>  	cr.return_ref = ref;
>  	if (tse->no_call)
> @@ -295,7 +299,14 @@ static int thread_stack__call_return(struct thread *thread,
>  	if (tse->non_call)
>  		cr.flags |= CALL_RETURN_NON_CALL;
>  
> -	return crp->process(&cr, crp->data);
> +	/*
> +	 * The parent db_id must be assigned before exporting the child. Note
> +	 * it is not possible to export the parent first because its information
> +	 * is not yet complete because its 'return' has not yet been processed.
> +	 */
> +	parent_db_id = idx ? &(tse - 1)->db_id : NULL;
> +
> +	return crp->process(&cr, parent_db_id, crp->data);
>  }
>  
>  static int __thread_stack__flush(struct thread *thread, struct thread_stack *ts)
> @@ -484,7 +495,7 @@ void thread_stack__sample(struct thread *thread, int cpu,
>  }
>  
>  struct call_return_processor *
> -call_return_processor__new(int (*process)(struct call_return *cr, void *data),
> +call_return_processor__new(int (*process)(struct call_return *cr, u64 *parent_db_id, void *data),
>  			   void *data)
>  {
>  	struct call_return_processor *crp;
> @@ -537,6 +548,7 @@ static int thread_stack__push_cp(struct thread_stack *ts, u64 ret_addr,
>  	tse->no_call = no_call;
>  	tse->trace_end = trace_end;
>  	tse->non_call = false;
> +	tse->db_id = 0;
>  
>  	return 0;
>  }
> diff --git a/tools/perf/util/thread-stack.h b/tools/perf/util/thread-stack.h
> index b7c04e19ad41..9c45f947f5a9 100644
> --- a/tools/perf/util/thread-stack.h
> +++ b/tools/perf/util/thread-stack.h
> @@ -55,6 +55,7 @@ enum {
>   * @call_ref: external reference to 'call' sample (e.g. db_id)
>   * @return_ref:  external reference to 'return' sample (e.g. db_id)
>   * @db_id: id used for db-export
> + * @parent_db_id: id of parent call used for db-export
>   * @flags: Call/Return flags
>   */
>  struct call_return {
> @@ -67,6 +68,7 @@ struct call_return {
>  	u64 call_ref;
>  	u64 return_ref;
>  	u64 db_id;
> +	u64 parent_db_id;
>  	u32 flags;
>  };
>  
> @@ -79,7 +81,7 @@ struct call_return {
>   */
>  struct call_return_processor {
>  	struct call_path_root *cpr;
> -	int (*process)(struct call_return *cr, void *data);
> +	int (*process)(struct call_return *cr, u64 *parent_db_id, void *data);
>  	void *data;
>  };
>  
> @@ -93,7 +95,7 @@ void thread_stack__free(struct thread *thread);
>  size_t thread_stack__depth(struct thread *thread, int cpu);
>  
>  struct call_return_processor *
> -call_return_processor__new(int (*process)(struct call_return *cr, void *data),
> +call_return_processor__new(int (*process)(struct call_return *cr, u64 *parent_db_id, void *data),
>  			   void *data);
>  void call_return_processor__free(struct call_return_processor *crp);
>  int thread_stack__process(struct thread *thread, struct comm *comm,
> -- 
> 2.17.1

-- 

- Arnaldo