Re: [PATCH 1/4] perf tools: Check availability of annotate when processing samples

[Arnaldo Carvalho de Melo]

Em Thu, Feb 20, 2014 at 10:32:53AM +0900, Namhyung Kim escreveu:
> The TUI of perf report and top support annotation, but stdio and GTK
> don't.  So it should be checked before calling hist_entry__inc_addr_
> samples() since perf annotate need it regardless of UI and sort keys.
> 
> It caused perf annotate on ppc64 to produce zero output.

I renamed {top,report}_needs_annotate to ui__has_annotation() and will
make this patch apply on perf/urgent, as it is a bug fix.

While looking at it I noticed several things that can be improved in
this area, like we don't need to map the ip in top if we're not going to
use it, etc. But will leave those to a follow on patch.

Thanks,

- Arnaldo
 
> Reported-by: Anton Blanchard <anton@xxxxxxxxx>
> Cc: Anton Blanchard <anton@xxxxxxxxx>
> Signed-off-by: Namhyung Kim <namhyung@xxxxxxxxxx>
> ---
>  tools/perf/builtin-report.c | 45 +++++++++++++++++++++++++++++----------------
>  tools/perf/builtin-top.c    | 11 +++++++++--
>  tools/perf/util/annotate.c  |  2 +-
>  3 files changed, 39 insertions(+), 19 deletions(-)
> 
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index d882b6f96411..bab762bdeb0d 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -75,6 +75,11 @@ static int report__config(const char *var, const char *value, void *cb)
>  	return perf_default_config(var, value, cb);
>  }
>  
> +static bool report_needs_annotate(void)
> +{
> +	return use_browser == 1 && sort__has_sym;
> +}
> +
>  static int report__add_mem_hist_entry(struct report *rep, struct addr_location *al,
>  				      struct perf_sample *sample, struct perf_evsel *evsel)
>  {
> @@ -110,14 +115,16 @@ static int report__add_mem_hist_entry(struct report *rep, struct addr_location *
>  	if (!he)
>  		return -ENOMEM;
>  
> -	err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> -	if (err)
> -		goto out;
> +	if (report_needs_annotate()) {
> +		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +		if (err)
> +			goto out;
>  
> -	mx = he->mem_info;
> -	err = addr_map_symbol__inc_samples(&mx->daddr, evsel->idx);
> -	if (err)
> -		goto out;
> +		mx = he->mem_info;
> +		err = addr_map_symbol__inc_samples(&mx->daddr, evsel->idx);
> +		if (err)
> +			goto out;
> +	}
>  
>  	evsel->hists.stats.total_period += cost;
>  	hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
> @@ -159,14 +166,18 @@ static int report__add_branch_hist_entry(struct report *rep, struct addr_locatio
>  		he = __hists__add_entry(&evsel->hists, al, parent, &bi[i], NULL,
>  					1, 1, 0);
>  		if (he) {
> -			bx = he->branch_info;
> -			err = addr_map_symbol__inc_samples(&bx->from, evsel->idx);
> -			if (err)
> -				goto out;
> -
> -			err = addr_map_symbol__inc_samples(&bx->to, evsel->idx);
> -			if (err)
> -				goto out;
> +			if (report_needs_annotate()) {
> +				bx = he->branch_info;
> +				err = addr_map_symbol__inc_samples(&bx->from,
> +								   evsel->idx);
> +				if (err)
> +					goto out;
> +
> +				err = addr_map_symbol__inc_samples(&bx->to,
> +								   evsel->idx);
> +				if (err)
> +					goto out;
> +			}
>  
>  			evsel->hists.stats.total_period += 1;
>  			hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
> @@ -199,7 +210,9 @@ static int report__add_hist_entry(struct report *rep, struct perf_evsel *evsel,
>  	if (err)
>  		goto out;
>  
> -	err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +	if (report_needs_annotate())
> +		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +
>  	evsel->hists.stats.total_period += sample->period;
>  	hists__inc_nr_events(&evsel->hists, PERF_RECORD_SAMPLE);
>  out:
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index ed99ec4a309f..a19c3afcfa0e 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -87,6 +87,11 @@ static void perf_top__sig_winch(int sig __maybe_unused,
>  	perf_top__update_print_entries(top);
>  }
>  
> +static bool top_needs_annotate(void)
> +{
> +	return use_browser == 1 && sort__has_sym;
> +}
> +
>  static int perf_top__parse_source(struct perf_top *top, struct hist_entry *he)
>  {
>  	struct symbol *sym;
> @@ -176,7 +181,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  {
>  	struct annotation *notes;
>  	struct symbol *sym;
> -	int err;
> +	int err = 0;
>  
>  	if (he == NULL || he->ms.sym == NULL ||
>  	    ((top->sym_filter_entry == NULL ||
> @@ -190,7 +195,9 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  		return;
>  
>  	ip = he->ms.map->map_ip(he->ms.map, ip);
> -	err = hist_entry__inc_addr_samples(he, counter, ip);
> +
> +	if (top_needs_annotate())
> +		err = hist_entry__inc_addr_samples(he, counter, ip);
>  
>  	pthread_mutex_unlock(&notes->lock);
>  
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 469eb679fb9d..6fcada625c86 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -489,7 +489,7 @@ static int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>  {
>  	struct annotation *notes;
>  
> -	if (sym == NULL || use_browser != 1 || !sort__has_sym)
> +	if (sym == NULL)
>  		return 0;
>  
>  	notes = symbol__annotation(sym);
> -- 
> 1.7.11.7
--
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/