Date:   Wed, 12 Jul 2017 17:09:10 -0300
From:   Arnaldo Carvalho de Melo <acme@...nel.org>
To:     Taeung Song <treeze.taeung@...il.com>
Cc:     linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
        Milian Wolff <milian.wolff@...b.com>,
        Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH 1/4] perf annotate: Fix wrong --show-total-period option
 showing number of samples

Em Wed, Jul 12, 2017 at 07:14:04AM +0900, Taeung Song escreveu:
> Currently the --show-total-period option of perf-annotate
> is different from perf-report's.
> 
> For example, perf-report ordinarily shows period and number of samples.
> 
>  # Overhead       Samples        Period  Command  Shared Object   Symbol
>  # ........  ............  ............  .......  ..............  ............
>  #
>       9.83%           102      84813704  test     test            [.] knapsack
> 
> But --show-total-period of perf-annotate has two problem like below:
> 
>   1) Wrong column i.e. 'Percent'
>   2) Show number of samples, not period
> 
> So fix this option to show period instead of number of samples.

so you have multiple bugs here, please fix one per patch, i.e. if using
--show-total-period the header should not be "Percent".

Then, you need a patch to introduce that "struct sym_sample" and use it,
but please rename it to "struct sym_hist_entry".

You can do it and just update the sym_hist_entry->period field, before
the change to pass 'struct sample' around.

That disasm__calc_percent() function could receive a pointer to a struct
sym_hist_entry instead of two pointers.

Small, self contained patches that do one thing make reviewing easier,
by yourself and others.

Please give this a try, if you didn't understand something here I can do
it for you, as this needs doing to fix real bugs.

Thanks,

- Arnaldo
 
> Before:
> 
>   Percent |      Source code & Disassembly of old for cycles:ppp (102 samples)
>  -----------------------------------------------------------------------------
>           :
>           :
>           :
>           :      Disassembly of section .text:
>           :
>           :      0000000000400816 <knapsack>:
>           :      knapsack():
>         1 :        400816:       push   %rbp
> 
> After:
> 
>   Event count |  Source code & Disassembly of old for cycles:ppp (84813704 event count)
>  --------------------------------------------------------------------------------------
>               :
>               :
>               :
>               :  Disassembly of section .text:
>               :
>               :  0000000000400816 <knapsack>:
>               :  knapsack():
>        743737 :    400816:       push   %rbp
> 
> Cc: Namhyung Kim <namhyung@...nel.org>
> Cc: Milian Wolff <milian.wolff@...b.com>
> Cc: Jiri Olsa <jolsa@...hat.com>
> Signed-off-by: Taeung Song <treeze.taeung@...il.com>
> ---
>  tools/perf/builtin-annotate.c     |  4 +-
>  tools/perf/builtin-report.c       | 13 +++---
>  tools/perf/builtin-top.c          |  6 ++-
>  tools/perf/ui/browsers/annotate.c |  4 +-
>  tools/perf/ui/gtk/annotate.c      |  4 +-
>  tools/perf/util/annotate.c        | 92 +++++++++++++++++++++++++++++----------
>  tools/perf/util/annotate.h        | 12 +++--
>  7 files changed, 96 insertions(+), 39 deletions(-)
> 
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 7a5dc7e..f314661 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -177,14 +177,12 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
>  	 */
>  	process_branch_stack(sample->branch_stack, al, sample);
>  
> -	sample->period = 1;
>  	sample->weight = 1;
> -

Please do not remove this line.

>  	he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
>  	if (he == NULL)
>  		return -ENOMEM;
>  
> -	ret = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +	ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr);
>  	hists__inc_nr_samples(hists, true);
>  	return ret;
>  }
> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c
> index 79a33eb..5f1894c 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -113,13 +113,14 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
>  	struct report *rep = arg;
>  	struct hist_entry *he = iter->he;
>  	struct perf_evsel *evsel = iter->evsel;
> +	struct perf_sample *sample = iter->sample;
>  	struct mem_info *mi;
>  	struct branch_info *bi;
>  
>  	if (!ui__has_annotation())
>  		return 0;
>  
> -	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
> +	hist__account_cycles(sample->branch_stack, al, sample,
>  			     rep->nonany_branch_mode);
>  
>  	if (sort__mode == SORT_MODE__BRANCH) {
> @@ -136,14 +137,16 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter,
>  		if (err)
>  			goto out;
>  
> -		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +		err = hist_entry__inc_addr_samples(he, sample,
> +						   evsel->idx, al->addr);

why break this into two lines?

>  
>  	} else if (symbol_conf.cumulate_callchain) {
>  		if (single)
> -			err = hist_entry__inc_addr_samples(he, evsel->idx,
> -							   al->addr);
> +			err = hist_entry__inc_addr_samples(he, sample,
> +							   evsel->idx, al->addr);
>  	} else {
> -		err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr);
> +		err = hist_entry__inc_addr_samples(he, sample,
> +						   evsel->idx, al->addr);
>  	}
>  
>  out:
> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c
> index 022486d..09885a4 100644
> --- a/tools/perf/builtin-top.c
> +++ b/tools/perf/builtin-top.c
> @@ -183,6 +183,7 @@ static void ui__warn_map_erange(struct map *map, struct symbol *sym, u64 ip)
>  
>  static void perf_top__record_precise_ip(struct perf_top *top,
>  					struct hist_entry *he,
> +					struct perf_sample *sample,
>  					int counter, u64 ip)
>  {
>  	struct annotation *notes;
> @@ -199,7 +200,7 @@ static void perf_top__record_precise_ip(struct perf_top *top,
>  	if (pthread_mutex_trylock(&notes->lock))
>  		return;
>  
> -	err = hist_entry__inc_addr_samples(he, counter, ip);
> +	err = hist_entry__inc_addr_samples(he, sample, counter, ip);
>  
>  	pthread_mutex_unlock(&notes->lock);
>  
> @@ -671,7 +672,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
>  	struct perf_evsel *evsel = iter->evsel;
>  
>  	if (perf_hpp_list.sym && single)
> -		perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
> +		perf_top__record_precise_ip(top, he, iter->sample,
> +					    evsel->idx, al->addr);
>  
>  	hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
>  		     !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));
> diff --git a/tools/perf/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c
> index b376637..73e5ae2 100644
> --- a/tools/perf/ui/browsers/annotate.c
> +++ b/tools/perf/ui/browsers/annotate.c
> @@ -449,13 +449,13 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser,
>  		next = disasm__get_next_ip_line(&notes->src->source, pos);
>  
>  		for (i = 0; i < browser->nr_events; i++) {
> -			u64 nr_samples;
> +			u64 nr_samples, period;
>  
>  			bpos->samples[i].percent = disasm__calc_percent(notes,
>  						evsel->idx + i,
>  						pos->offset,
>  						next ? next->offset : len,
> -						&path, &nr_samples);
> +						&path, &nr_samples, &period);
>  			bpos->samples[i].nr = nr_samples;
>  
>  			if (max_percent < bpos->samples[i].percent)
> diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c
> index 87e3760..d736fd5 100644
> --- a/tools/perf/ui/gtk/annotate.c
> +++ b/tools/perf/ui/gtk/annotate.c
> @@ -34,10 +34,10 @@ static int perf_gtk__get_percent(char *buf, size_t size, struct symbol *sym,
>  		return 0;
>  
>  	symhist = annotation__histogram(symbol__annotation(sym), evidx);
> -	if (!symbol_conf.event_group && !symhist->addr[dl->offset])
> +	if (!symbol_conf.event_group && !symhist->addr[dl->offset].nr_samples)
>  		return 0;
>  
> -	percent = 100.0 * symhist->addr[dl->offset] / symhist->sum;
> +	percent = 100.0 * symhist->addr[dl->offset].nr_samples / symhist->sum;
>  
>  	markup = perf_gtk__get_percent_color(percent);
>  	if (markup)
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index ef434b5..f7aeb5f 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -610,10 +610,10 @@ int symbol__alloc_hist(struct symbol *sym)
>  	size_t sizeof_sym_hist;
>  
>  	/* Check for overflow when calculating sizeof_sym_hist */
> -	if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(u64))
> +	if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct sym_sample))
>  		return -1;
>  
> -	sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(u64));
> +	sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(struct sym_sample));
>  
>  	/* Check for overflow in zalloc argument */
>  	if (sizeof_sym_hist > (SIZE_MAX - sizeof(*notes->src))
> @@ -714,11 +714,11 @@ static int __symbol__inc_addr_samples(struct symbol *sym, struct map *map,
>  	offset = addr - sym->start;
>  	h = annotation__histogram(notes, evidx);
>  	h->sum++;
> -	h->addr[offset]++;
> +	h->addr[offset].nr_samples++;
>  
>  	pr_debug3("%#" PRIx64 " %s: period++ [addr: %#" PRIx64 ", %#" PRIx64
>  		  ", evidx=%d] => %" PRIu64 "\n", sym->start, sym->name,
> -		  addr, addr - sym->start, evidx, h->addr[offset]);
> +		  addr, addr - sym->start, evidx, h->addr[offset].nr_samples);
>  	return 0;
>  }
>  
> @@ -816,9 +816,33 @@ int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx)
>  	return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr);
>  }
>  
> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip)
> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
> +				 int evidx, u64 addr)
>  {
> -	return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip);
> +	struct symbol *sym = he->ms.sym;
> +	struct annotation *notes;
> +	struct sym_hist *h;
> +	s64 offset;
> +
> +	if (sym == NULL)
> +		return 0;
> +
> +	notes = symbol__get_annotation(sym, false);
> +	if (notes == NULL)
> +		return -ENOMEM;
> +
> +	if ((addr < sym->start || addr >= sym->end) &&
> +	    (addr != sym->end || sym->start != sym->end))
> +		return -ERANGE;
> +
> +	offset = addr - sym->start;
> +	h = annotation__histogram(notes, evidx);
> +	h->sum++;
> +	h->addr[offset].nr_samples++;
> +	h->total_period += sample->period;
> +	h->addr[offset].period += sample->period;
> +
> +	return 0;
>  }
>  
>  static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
> @@ -928,11 +952,13 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa
>  }
>  
>  double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
> -			    s64 end, const char **path, u64 *nr_samples)
> +			    s64 end, const char **path, u64 *nr_samples, u64 *period)
>  {
>  	struct source_line *src_line = notes->src->lines;
>  	double percent = 0.0;
> +
>  	*nr_samples = 0;
> +	*period = 0;
>  
>  	if (src_line) {
>  		size_t sizeof_src_line = sizeof(*src_line) +
> @@ -952,12 +978,16 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>  	} else {
>  		struct sym_hist *h = annotation__histogram(notes, evidx);
>  		unsigned int hits = 0;
> +		unsigned int p = 0;
>  
> -		while (offset < end)
> -			hits += h->addr[offset++];
> +		while (offset < end) {
> +			hits += h->addr[offset].nr_samples;
> +			p += h->addr[offset++].period;
> +		}
>  
>  		if (h->sum) {
>  			*nr_samples = hits;
> +			*period = p;
>  			percent = 100.0 * hits / h->sum;
>  		}
>  	}
> @@ -1057,10 +1087,11 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  
>  	if (dl->offset != -1) {
>  		const char *path = NULL;
> -		u64 nr_samples;
> +		u64 nr_samples, period;
>  		double percent, max_percent = 0.0;
>  		double *ppercents = &percent;
>  		u64 *psamples = &nr_samples;
> +		u64 *pperiod = &period;
>  		int i, nr_percent = 1;
>  		const char *color;
>  		struct annotation *notes = symbol__annotation(sym);
> @@ -1075,9 +1106,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  			nr_percent = evsel->nr_members;
>  			ppercents = calloc(nr_percent, sizeof(double));
>  			psamples = calloc(nr_percent, sizeof(u64));
> -			if (ppercents == NULL || psamples == NULL) {
> +			pperiod = calloc(nr_percent, sizeof(u64));
> +			if (ppercents == NULL || psamples == NULL || pperiod == NULL)
>  				return -1;
> -			}
>  		}
>  
>  		for (i = 0; i < nr_percent; i++) {
> @@ -1085,10 +1116,11 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  					notes->src->lines ? i : evsel->idx + i,
>  					offset,
>  					next ? next->offset : (s64) len,
> -					&path, &nr_samples);
> +					&path, &nr_samples, &period);
>  
>  			ppercents[i] = percent;
>  			psamples[i] = nr_samples;
> +			pperiod[i] = period;
>  			if (percent > max_percent)
>  				max_percent = percent;
>  		}
> @@ -1127,11 +1159,12 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		for (i = 0; i < nr_percent; i++) {
>  			percent = ppercents[i];
>  			nr_samples = psamples[i];
> +			period = pperiod[i];
>  			color = get_percent_color(percent);
>  
>  			if (symbol_conf.show_total_period)
> -				color_fprintf(stdout, color, " %7" PRIu64,
> -					      nr_samples);
> +				color_fprintf(stdout, color, " %11" PRIu64,
> +					      period);
>  			else
>  				color_fprintf(stdout, color, " %7.2f", percent);
>  		}
> @@ -1150,6 +1183,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		if (psamples != &nr_samples)
>  			free(psamples);
>  
> +		if (pperiod != &period)
> +			free(pperiod);
> +
>  	} else if (max_lines && printed >= max_lines)
>  		return 1;
>  	else {
> @@ -1161,6 +1197,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>  		if (perf_evsel__is_group_event(evsel))
>  			width *= evsel->nr_members;
>  
> +		if (symbol_conf.show_total_period)
> +			width += perf_evsel__is_group_event(evsel) ?
> +				4 * evsel->nr_members : 4;
> +
>  		if (!*dl->line)
>  			printf(" %*s:\n", width, " ");
>  		else
> @@ -1702,7 +1742,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map,
>  			double percent = 0.0;
>  
>  			h = annotation__histogram(notes, evidx + k);
> -			nr_samples = h->addr[i];
> +			nr_samples = h->addr[i].nr_samples;
>  			if (h->sum)
>  				percent = 100.0 * nr_samples / h->sum;
>  
> @@ -1773,9 +1813,9 @@ static void symbol__annotate_hits(struct symbol *sym, struct perf_evsel *evsel)
>  	u64 len = symbol__size(sym), offset;
>  
>  	for (offset = 0; offset < len; ++offset)
> -		if (h->addr[offset] != 0)
> +		if (h->addr[offset].nr_samples != 0)
>  			printf("%*" PRIx64 ": %" PRIu64 "\n", BITS_PER_LONG / 2,
> -			       sym->start + offset, h->addr[offset]);
> +			       sym->start + offset, h->addr[offset].nr_samples);
>  	printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2, "h->sum", h->sum);
>  }
>  
> @@ -1811,8 +1851,16 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
>  	if (perf_evsel__is_group_event(evsel))
>  		width *= evsel->nr_members;
>  
> -	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
> -	       width, width, "Percent", d_filename, evsel_name, h->sum);
> +	if (symbol_conf.show_total_period)
> +		width += perf_evsel__is_group_event(evsel) ?
> +			4 * evsel->nr_members : 4;
> +
> +	graph_dotted_len = printf(" %-*.*s|	Source code & Disassembly of %s for %s (%" PRIu64 " %s)\n",
> +				  width, width,
> +				  symbol_conf.show_total_period ? "Event count" : "Percent",
> +				  d_filename, evsel_name,
> +				  symbol_conf.show_total_period ? h->total_period : h->sum,
> +				  symbol_conf.show_total_period ? "event count" : "samples");
>  
>  	printf("%-*.*s----\n",
>  	       graph_dotted_len, graph_dotted_len, graph_dotted_line);
> @@ -1878,8 +1926,8 @@ void symbol__annotate_decay_histogram(struct symbol *sym, int evidx)
>  
>  	h->sum = 0;
>  	for (offset = 0; offset < len; ++offset) {
> -		h->addr[offset] = h->addr[offset] * 7 / 8;
> -		h->sum += h->addr[offset];
> +		h->addr[offset].nr_samples = h->addr[offset].nr_samples * 7 / 8;
> +		h->sum += h->addr[offset].nr_samples;
>  	}
>  }
>  
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index bac698d..6b2e645 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -79,11 +79,16 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa
>  int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw);
>  size_t disasm__fprintf(struct list_head *head, FILE *fp);
>  double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
> -			    s64 end, const char **path, u64 *nr_samples);
> +			    s64 end, const char **path, u64 *nr_samples, u64 *period);
> +struct sym_sample {
> +	u64		nr_samples;
> +	u64		period;
> +};
>  
>  struct sym_hist {
>  	u64		sum;
> -	u64		addr[0];
> +	u64		total_period;
> +	struct sym_sample addr[0];
>  };
>  
>  struct cyc_hist {
> @@ -155,7 +160,8 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
>  				    struct addr_map_symbol *start,
>  				    unsigned cycles);
>  
> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr);
> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
> +				 int evidx, u64 addr);
>  
>  int symbol__alloc_hist(struct symbol *sym);
>  void symbol__annotate_zero_histograms(struct symbol *sym);
> -- 
> 2.7.4

Powered by blists - more mailing lists