Re: [PATCH 2/2] perf ppc64le: Fix probe location when using DWARF

[Masami Hiramatsu <mhiramat@kernel.org>]

On Tue,  9 Aug 2016 01:23:25 -0500
Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com> wrote:

> Powerpc has Global Entry Point and Local Entry Point for functions.
> LEP catches call from both the GEP and the LEP. Symbol table of ELF
> contains GEP and Offset from which we can calculate LEP, but debuginfo
> does not have LEP info.
> 
> Currently, perf prioritize symbol table over dwarf to probe on LEP
> for ppc64le. But when user tries to probe with function parameter,
> we fall back to using dwarf(i.e. GEP) and when function called via
> LEP, probe will never hit.
> 
> For example:
>   $ objdump -d vmlinux
>     ...
>     do_sys_open():
>     c0000000002eb4a0:       e8 00 4c 3c     addis   r2,r12,232
>     c0000000002eb4a4:       60 00 42 38     addi    r2,r2,96
>     c0000000002eb4a8:       a6 02 08 7c     mflr    r0
>     c0000000002eb4ac:       d0 ff 41 fb     std     r26,-48(r1)
> 
>   $ sudo ./perf probe do_sys_open
>   $ sudo cat /sys/kernel/debug/tracing/kprobe_events
>     p:probe/do_sys_open _text+3060904
> 
>   $ sudo ./perf probe 'do_sys_open filename:string'
>   $ sudo cat /sys/kernel/debug/tracing/kprobe_events
>     p:probe/do_sys_open _text+3060896 filename_string=+0(%gpr4):string
> 
> For second case, perf probed on GEP. So when function will be called
> via LEP, probe won't hit.
> 
>   $ sudo ./perf record -a -e probe:do_sys_open ls
>     [ perf record: Woken up 1 times to write data ]
>     [ perf record: Captured and wrote 0.195 MB perf.data ]
> 
> To resolve this issue, let's not prioritize symbol table, let perf
> decide what it wants to use. Perf is already converting GEP to LEP
> when it uses symbol table. When perf uses debuginfo, let it find
> LEP offset form symbol table. This way we fall back to probe on LEP
> for all cases.
> 
> After patch:
>   $ sudo ./perf probe 'do_sys_open filename:string'
>   $ sudo cat /sys/kernel/debug/tracing/kprobe_events
>     p:probe/do_sys_open _text+3060904 filename_string=+0(%gpr4):string
> 
>   $ sudo ./perf record -a -e probe:do_sys_open ls
>     [ perf record: Woken up 1 times to write data ]
>     [ perf record: Captured and wrote 0.197 MB perf.data (11 samples) ]

OK, I see.
And I'd like to add note below.

 This reverts commit d5c2e2c17ae1 ("perf probe ppc64le: Prefer symbol
 table lookup over DWARF").

Acked-by: Masami Hiramatsu <mhiramat@kernel.org>

for this series. 

Thanks,

> 
> Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.vnet.ibm.com>
> ---
>  tools/perf/arch/powerpc/util/sym-handling.c | 27 +++++++++++++++++----
>  tools/perf/util/probe-event.c               | 37 ++++++++++++++++-------------
>  tools/perf/util/probe-event.h               |  6 ++++-
>  3 files changed, 49 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/perf/arch/powerpc/util/sym-handling.c b/tools/perf/arch/powerpc/util/sym-handling.c
> index c6d0f91..8d4dc97 100644
> --- a/tools/perf/arch/powerpc/util/sym-handling.c
> +++ b/tools/perf/arch/powerpc/util/sym-handling.c
> @@ -54,10 +54,6 @@ int arch__compare_symbol_names(const char *namea, const char *nameb)
>  #endif
>  
>  #if defined(_CALL_ELF) && _CALL_ELF == 2
> -bool arch__prefers_symtab(void)
> -{
> -	return true;
> -}
>  
>  #ifdef HAVE_LIBELF_SUPPORT
>  void arch__sym_update(struct symbol *s, GElf_Sym *sym)
> @@ -100,4 +96,27 @@ void arch__fix_tev_from_maps(struct perf_probe_event *pev,
>  			tev->point.offset += lep_offset;
>  	}
>  }
> +
> +void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
> +					   int ntevs)
> +{
> +	struct probe_trace_event *tev;
> +	struct map *map;
> +	struct symbol *sym = NULL;
> +	struct rb_node *tmp;
> +	int i = 0;
> +
> +	map = get_target_map(pev->target, pev->uprobes);
> +	if (!map || map__load(map, NULL) < 0)
> +		return;
> +
> +	for (i = 0; i < ntevs; i++) {
> +		tev = &pev->tevs[i];
> +		map__for_each_symbol(map, sym, tmp) {
> +			if (map->unmap_ip(map, sym->start) == tev->point.address)
> +				arch__fix_tev_from_maps(pev, tev, map, sym);
> +		}
> +	}
> +}
> +
>  #endif
> diff --git a/tools/perf/util/probe-event.c b/tools/perf/util/probe-event.c
> index 4e215e7..5efa535 100644
> --- a/tools/perf/util/probe-event.c
> +++ b/tools/perf/util/probe-event.c
> @@ -178,7 +178,7 @@ static struct map *kernel_get_module_map(const char *module)
>  	return NULL;
>  }
>  
> -static struct map *get_target_map(const char *target, bool user)
> +struct map *get_target_map(const char *target, bool user)
>  {
>  	/* Init maps of given executable or kernel */
>  	if (user)
> @@ -703,19 +703,32 @@ post_process_kernel_probe_trace_events(struct probe_trace_event *tevs,
>  	return skipped;
>  }
>  
> +void __weak
> +arch__post_process_probe_trace_events(struct perf_probe_event *pev __maybe_unused,
> +				      int ntevs __maybe_unused)
> +{
> +}
> +
>  /* Post processing the probe events */
> -static int post_process_probe_trace_events(struct probe_trace_event *tevs,
> +static int post_process_probe_trace_events(struct perf_probe_event *pev,
> +					   struct probe_trace_event *tevs,
>  					   int ntevs, const char *module,
>  					   bool uprobe)
>  {
> -	if (uprobe)
> -		return add_exec_to_probe_trace_events(tevs, ntevs, module);
> +	int ret;
>  
> -	if (module)
> +	if (uprobe)
> +		ret = add_exec_to_probe_trace_events(tevs, ntevs, module);
> +	else if (module)
>  		/* Currently ref_reloc_sym based probe is not for drivers */
> -		return add_module_to_probe_trace_events(tevs, ntevs, module);
> +		ret = add_module_to_probe_trace_events(tevs, ntevs, module);
> +	else
> +		ret = post_process_kernel_probe_trace_events(tevs, ntevs);
>  
> -	return post_process_kernel_probe_trace_events(tevs, ntevs);
> +	if (ret >= 0)
> +		arch__post_process_probe_trace_events(pev, ntevs);
> +
> +	return ret;
>  }
>  
>  /* Try to find perf_probe_event with debuginfo */
> @@ -756,7 +769,7 @@ static int try_to_find_probe_trace_events(struct perf_probe_event *pev,
>  
>  	if (ntevs > 0) {	/* Succeeded to find trace events */
>  		pr_debug("Found %d probe_trace_events.\n", ntevs);
> -		ret = post_process_probe_trace_events(*tevs, ntevs,
> +		ret = post_process_probe_trace_events(pev, *tevs, ntevs,
>  						pev->target, pev->uprobes);
>  		if (ret < 0 || ret == ntevs) {
>  			clear_probe_trace_events(*tevs, ntevs);
> @@ -2943,8 +2956,6 @@ errout:
>  	return err;
>  }
>  
> -bool __weak arch__prefers_symtab(void) { return false; }
> -
>  /* Concatinate two arrays */
>  static void *memcat(void *a, size_t sz_a, void *b, size_t sz_b)
>  {
> @@ -3165,12 +3176,6 @@ static int convert_to_probe_trace_events(struct perf_probe_event *pev,
>  	if (ret > 0 || pev->sdt)	/* SDT can be found only in the cache */
>  		return ret == 0 ? -ENOENT : ret; /* Found in probe cache */
>  
> -	if (arch__prefers_symtab() && !perf_probe_event_need_dwarf(pev)) {
> -		ret = find_probe_trace_events_from_map(pev, tevs);
> -		if (ret > 0)
> -			return ret; /* Found in symbol table */
> -	}
> -
>  	/* Convert perf_probe_event with debuginfo */
>  	ret = try_to_find_probe_trace_events(pev, tevs);
>  	if (ret != 0)
> diff --git a/tools/perf/util/probe-event.h b/tools/perf/util/probe-event.h
> index e18ea9f..f4f45db 100644
> --- a/tools/perf/util/probe-event.h
> +++ b/tools/perf/util/probe-event.h
> @@ -158,7 +158,6 @@ int show_line_range(struct line_range *lr, const char *module, bool user);
>  int show_available_vars(struct perf_probe_event *pevs, int npevs,
>  			struct strfilter *filter);
>  int show_available_funcs(const char *module, struct strfilter *filter, bool user);
> -bool arch__prefers_symtab(void);
>  void arch__fix_tev_from_maps(struct perf_probe_event *pev,
>  			     struct probe_trace_event *tev, struct map *map,
>  			     struct symbol *sym);
> @@ -173,4 +172,9 @@ int e_snprintf(char *str, size_t size, const char *format, ...)
>  int copy_to_probe_trace_arg(struct probe_trace_arg *tvar,
>  			    struct perf_probe_arg *pvar);
>  
> +struct map *get_target_map(const char *target, bool user);
> +
> +void arch__post_process_probe_trace_events(struct perf_probe_event *pev,
> +					   int ntevs);
> +
>  #endif /*_PROBE_EVENT_H */
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>