LinuxLists.cc - [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
it should first add sample to those function signatures, leaving the
"meat" to a followup patch, where we will not be distracted with
infrastructure needed to do what you describe in the changelog.

I'm doing it here this time, please look at the result, later.

- Arnaldo

> But --show-total-period of perf-annotate has the problem that show
> number of samples, not period.
> So fix this option to show period instead of number of samples.
>
> Before:
>
> Percent | Source code & Disassembly of old for cycles:ppp (102 samples)
> -----------------------------------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> :
> : 0000000000400816 <knapsack>:
> : knapsack():
> 1 : 400816: push %rbp
>
> After:
>
> Percent | Source code & Disassembly of old for cycles:ppp (84813704 event count)
> ----------------------------------------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> :
> : 0000000000400816 <knapsack>:
> : knapsack():
> 743737 : 400816: push %rbp
>
> Reported-by: Namhyung Kim <[email protected]>
> Cc: Milian Wolff <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Taeung Song <[email protected]>
> ---
> tools/perf/builtin-annotate.c | 4 +---
> tools/perf/builtin-report.c | 13 ++++++----
> tools/perf/builtin-top.c | 6 +++--
> tools/perf/util/annotate.c | 55 ++++++++++++++++++++++++++++++++++++-------
> tools/perf/util/annotate.h | 4 +++-
> 5 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 5205408..0381408 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;
> -
> 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 cea25d0..a9bd011 100644
> --- a/tools/perf/builtin-report.c
> +++ b/tools/perf/builtin-report.c
> @@ -115,13 +115,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) {
> @@ -138,14 +139,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);
>
> } 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/util/annotate.c b/tools/perf/util/annotate.c
> index a62067a..d4f1a0a 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -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->total_samples++;
> + 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)
> @@ -934,6 +958,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
> double percent = 0.0;
>
> sample->nr_samples = 0;
> + sample->period = 0;
>
> if (src_line) {
> size_t sizeof_src_line = sizeof(*src_line) +
> @@ -953,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++].nr_samples;
> + while (offset < end) {
> + hits += h->addr[offset].nr_samples;
> + p += h->addr[offset++].period;
> + }
>
> if (h->total_samples) {
> sample->nr_samples = hits;
> + sample->period = p;
> percent = 100.0 * hits / h->total_samples;
> }
> }
> @@ -1131,8 +1160,8 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
> color = get_percent_color(percent);
>
> if (symbol_conf.show_total_period)
> - color_fprintf(stdout, color, " %7" PRIu64,
> - sample.nr_samples);
> + color_fprintf(stdout, color, " %11" PRIu64,
> + sample.period);
> else
> color_fprintf(stdout, color, " %7.2f", percent);
> }
> @@ -1162,6 +1191,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
> @@ -1812,8 +1845,14 @@ 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->total_samples);
> + 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, "Percent", d_filename, evsel_name,
> + symbol_conf.show_total_period ? h->total_period : h->total_samples,
> + symbol_conf.show_total_period ? "event count" : "samples");
>
> printf("%-*.*s----\n",
> graph_dotted_len, graph_dotted_len, graph_dotted_line);
> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
> index bef1d02..4406352 100644
> --- a/tools/perf/util/annotate.h
> +++ b/tools/perf/util/annotate.h
> @@ -88,6 +88,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>
> struct sym_hist {
> u64 total_samples;
> + u64 total_period;
> struct sym_hist_entry addr[0];
> };
>
> @@ -160,7 +161,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

2017-07-21 09:41:39

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
> it should first add sample to those function signatures, leaving the
> "meat" to a followup patch, where we will not be distracted with
> infrastructure needed to do what you describe in the changelog.
>
> I'm doing it here this time, please look at the result, later.
>
> - Arnaldo
>

ok, I'm waiting for it.
And if you give me some sketchy code, that's fine.
If you do, I'll remake this patch based on the result. :)

Thanks,
Taeung

>> But --show-total-period of perf-annotate has the problem that show
>> number of samples, not period.
>> So fix this option to show period instead of number of samples.
>>
>> Before:
>>
>> Percent | Source code & Disassembly of old for cycles:ppp (102 samples)
>> -----------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> :
>> : 0000000000400816 <knapsack>:
>> : knapsack():
>> 1 : 400816: push %rbp
>>
>> After:
>>
>> Percent | Source code & Disassembly of old for cycles:ppp (84813704 event count)
>> ----------------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> :
>> : 0000000000400816 <knapsack>:
>> : knapsack():
>> 743737 : 400816: push %rbp
>>
>> Reported-by: Namhyung Kim <[email protected]>
>> Cc: Milian Wolff <[email protected]>
>> Cc: Jiri Olsa <[email protected]>
>> Signed-off-by: Taeung Song <[email protected]>
>> ---
>> tools/perf/builtin-annotate.c | 4 +---
>> tools/perf/builtin-report.c | 13 ++++++----
>> tools/perf/builtin-top.c | 6 +++--
>> tools/perf/util/annotate.c | 55 ++++++++++++++++++++++++++++++++++++-------
>> tools/perf/util/annotate.h | 4 +++-
>> 5 files changed, 63 insertions(+), 19 deletions(-)
>>
>> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
>> index 5205408..0381408 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;
>> -
>> 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 cea25d0..a9bd011 100644
>> --- a/tools/perf/builtin-report.c
>> +++ b/tools/perf/builtin-report.c
>> @@ -115,13 +115,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) {
>> @@ -138,14 +139,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);
>>
>> } 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/util/annotate.c b/tools/perf/util/annotate.c
>> index a62067a..d4f1a0a 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -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->total_samples++;
>> + 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)
>> @@ -934,6 +958,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>> double percent = 0.0;
>>
>> sample->nr_samples = 0;
>> + sample->period = 0;
>>
>> if (src_line) {
>> size_t sizeof_src_line = sizeof(*src_line) +
>> @@ -953,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++].nr_samples;
>> + while (offset < end) {
>> + hits += h->addr[offset].nr_samples;
>> + p += h->addr[offset++].period;
>> + }
>>
>> if (h->total_samples) {
>> sample->nr_samples = hits;
>> + sample->period = p;
>> percent = 100.0 * hits / h->total_samples;
>> }
>> }
>> @@ -1131,8 +1160,8 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
>> color = get_percent_color(percent);
>>
>> if (symbol_conf.show_total_period)
>> - color_fprintf(stdout, color, " %7" PRIu64,
>> - sample.nr_samples);
>> + color_fprintf(stdout, color, " %11" PRIu64,
>> + sample.period);
>> else
>> color_fprintf(stdout, color, " %7.2f", percent);
>> }
>> @@ -1162,6 +1191,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
>> @@ -1812,8 +1845,14 @@ 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->total_samples);
>> + 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, "Percent", d_filename, evsel_name,
>> + symbol_conf.show_total_period ? h->total_period : h->total_samples,
>> + symbol_conf.show_total_period ? "event count" : "samples");
>>
>> printf("%-*.*s----\n",
>> graph_dotted_len, graph_dotted_len, graph_dotted_line);
>> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
>> index bef1d02..4406352 100644
>> --- a/tools/perf/util/annotate.h
>> +++ b/tools/perf/util/annotate.h
>> @@ -88,6 +88,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
>>
>> struct sym_hist {
>> u64 total_samples;
>> + u64 total_period;
>> struct sym_hist_entry addr[0];
>> };
>>
>> @@ -160,7 +161,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

2017-07-21 11:24:25

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Fri, Jul 21, 2017 at 06:41:29PM +0900, Taeung Song escreveu:
> On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
> > it should first add sample to those function signatures, leaving the
> > "meat" to a followup patch, where we will not be distracted with
> > infrastructure needed to do what you describe in the changelog.

> > I'm doing it here this time, please look at the result, later.

> ok, I'm waiting for it.
> And if you give me some sketchy code, that's fine.
> If you do, I'll remake this patch based on the result. :)

Take a look at the acme/tmp_perf/core, that is where I got yesterday.

- Arnaldo

2017-07-21 14:49:23

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> +++ 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;
> -
> he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> if (he == NULL)
> return -ENOMEM;

I split the hunk above into a separate patch, as a fix, Namhyung, can
you take a look at why need to unconditionally overwrite what is in
sample->weight as well?

Looks fishy as it may come with a value from the kernel, parsed in
perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
perf_event_attr->sample_type.

Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
isn't requested in sample_type?

The resulting cset is below.

- Arnaldo

commit a935e8cd8d5d4b7936c4b4cf27c2d0e87d1a6a66
Author: Taeung Song <[email protected]>
Date: Fri Jul 21 11:38:48 2017 -0300

perf annotate: Do not overwrite sample->period

In fixing the --show-total-period option it was noticed that the value
of sample->period was being overwritten, fix it.

Cc: Jiri Olsa <[email protected]>
Cc: Milian Wolff <[email protected]>
Cc: Namhyung Kim <[email protected]>
Fixes: fd36f3dd7933 ("perf hist: Pass struct sample to __hists__add_entry()")
[ split from a larger patch, added the Fixes tag ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 96fe1a88c1e5..7e33278eff67 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -177,7 +177,6 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
*/
process_branch_stack(sample->branch_stack, al, sample);

- sample->period = 1;
sample->weight = 1;

he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);

2017-07-22 22:47:05

by Namhyung Kim

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Hi Arnaldo and Taeung,

(+ Andi)

On Fri, Jul 21, 2017 at 11:47:48AM -0300, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> > +++ 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;
> > -
> > he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> > if (he == NULL)
> > return -ENOMEM;
>
> I split the hunk above into a separate patch, as a fix, Namhyung, can
> you take a look at why need to unconditionally overwrite what is in
> sample->weight as well?
>
> Looks fishy as it may come with a value from the kernel, parsed in
> perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
> perf_event_attr->sample_type.
>
> Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
> isn't requested in sample_type?

It was Andi added that code originally (05484298cbfe). IIUC the
weight is only meaningful for some CPUs with Intel TSX and he used a
dummy value.

AFAIK the hists code doesn't care of it unless weight sort key is used
(for report). As it's not used by annotate code, I think it'd be
better leaving it as is (like period).

Thanks,
Namhyung

>
> The resulting cset is below.
>
> - Arnaldo
>
> commit a935e8cd8d5d4b7936c4b4cf27c2d0e87d1a6a66
> Author: Taeung Song <[email protected]>
> Date: Fri Jul 21 11:38:48 2017 -0300
>
> perf annotate: Do not overwrite sample->period
>
> In fixing the --show-total-period option it was noticed that the value
> of sample->period was being overwritten, fix it.
>
> Cc: Jiri Olsa <[email protected]>
> Cc: Milian Wolff <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Fixes: fd36f3dd7933 ("perf hist: Pass struct sample to __hists__add_entry()")
> [ split from a larger patch, added the Fixes tag ]
> Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
>
> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
> index 96fe1a88c1e5..7e33278eff67 100644
> --- a/tools/perf/builtin-annotate.c
> +++ b/tools/perf/builtin-annotate.c
> @@ -177,7 +177,6 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
> */
> process_branch_stack(sample->branch_stack, al, sample);
>
> - sample->period = 1;
> sample->weight = 1;
>
> he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);

2017-07-23 15:46:25

by Andi Kleen

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

On Sun, Jul 23, 2017 at 07:46:05AM +0900, Namhyung Kim wrote:
> Hi Arnaldo and Taeung,
>
> (+ Andi)
>
> On Fri, Jul 21, 2017 at 11:47:48AM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> > > +++ 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;
> > > -
> > > he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> > > if (he == NULL)
> > > return -ENOMEM;
> >
> > I split the hunk above into a separate patch, as a fix, Namhyung, can
> > you take a look at why need to unconditionally overwrite what is in
> > sample->weight as well?
> >
> > Looks fishy as it may come with a value from the kernel, parsed in
> > perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
> > perf_event_attr->sample_type.
> >
> > Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
> > isn't requested in sample_type?
>
> It was Andi added that code originally (05484298cbfe). IIUC the
> weight is only meaningful for some CPUs with Intel TSX and he used a
> dummy value.

It's used for more than TSX. e.g. perf mem uses it for memory latencies.

> AFAIK the hists code doesn't care of it unless weight sort key is used
> (for report). As it's not used by annotate code, I think it'd be
> better leaving it as is (like period).

Right, it's needed when weight is specified as a sort key. But we need
a fallback in case the user specified weight in perf report, but
didn't enable it for perf record.

-Andi

2017-07-24 01:52:12

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Hi Arnaldo,

Sorry, I'm too late.

On 07/21/2017 08:24 PM, Arnaldo Carvalho de Melo wrote:
> Em Fri, Jul 21, 2017 at 06:41:29PM +0900, Taeung Song escreveu:
>> On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
>>> it should first add sample to those function signatures, leaving the
>>> "meat" to a followup patch, where we will not be distracted with
>>> infrastructure needed to do what you describe in the changelog.
>
>>> I'm doing it here this time, please look at the result, later.
>
>> ok, I'm waiting for it.
>> And if you give me some sketchy code, that's fine.
>> If you do, I'll remake this patch based on the result. :)
>
> Take a look at the acme/tmp_perf/core, that is where I got yesterday.
>
> - Arnaldo
>

I fetched all branch of your repository, but it don't seem to be pushed.

$ git remote get-url acme
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git

$ git branch -a | grep acme | grep tmp | grep core
remotes/acme/tmp.perf/core
remotes/acme/tmp_perf.core

$ git show tmp.perf/core | head -3
commit 4827c52cd001e208704ab733a389c96ae2e70e5b
Author: Andi Kleen <[email protected]>
Date: Fri Jul 21 12:25:57 2017 -0700

$ git show tmp_perf.core | head -3
commit 3331778eb08a50cec959da933c040bd7fbdde396
Author: Adrian Hunter <[email protected]>
Date: Thu Jul 31 09:00:56 2014 +0300

Thanks,
Taeung

2017-07-24 17:35:32

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Sun, Jul 23, 2017 at 08:46:20AM -0700, Andi Kleen escreveu:
> On Sun, Jul 23, 2017 at 07:46:05AM +0900, Namhyung Kim wrote:
> > Hi Arnaldo and Taeung,
> >
> > (+ Andi)
> >
> > On Fri, Jul 21, 2017 at 11:47:48AM -0300, Arnaldo Carvalho de Melo wrote:
> > > Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> > > > +++ 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;
> > > > -
> > > > he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> > > > if (he == NULL)
> > > > return -ENOMEM;
> > >
> > > I split the hunk above into a separate patch, as a fix, Namhyung, can
> > > you take a look at why need to unconditionally overwrite what is in
> > > sample->weight as well?
> > >
> > > Looks fishy as it may come with a value from the kernel, parsed in
> > > perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
> > > perf_event_attr->sample_type.
> > >
> > > Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
> > > isn't requested in sample_type?
> >
> > It was Andi added that code originally (05484298cbfe). IIUC the
> > weight is only meaningful for some CPUs with Intel TSX and he used a
> > dummy value.
>
> It's used for more than TSX. e.g. perf mem uses it for memory latencies.
>
> > AFAIK the hists code doesn't care of it unless weight sort key is used
> > (for report). As it's not used by annotate code, I think it'd be
> > better leaving it as is (like period).
>
> Right, it's needed when weight is specified as a sort key. But we need
> a fallback in case the user specified weight in perf report, but
> didn't enable it for perf record.

Humm, shouldn't we fail in that case? I.e. user asks for per-sample
property not collected at 'perf record' time?

That or the weight sort order handler should see that
perf_sample->weight is zero and assume it wasn't collected then turn it
into a 1? Or just look at evsel->attr.sample_type & PERF_SAMPLE_WEIGHT?

- Arnaldo

2017-07-24 17:37:55

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Mon, Jul 24, 2017 at 10:51:58AM +0900, Taeung Song escreveu:
> Hi Arnaldo,
>
> Sorry, I'm too late.
>
> On 07/21/2017 08:24 PM, Arnaldo Carvalho de Melo wrote:
> > Em Fri, Jul 21, 2017 at 06:41:29PM +0900, Taeung Song escreveu:
> > > On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
> > > > Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
> > > > it should first add sample to those function signatures, leaving the
> > > > "meat" to a followup patch, where we will not be distracted with
> > > > infrastructure needed to do what you describe in the changelog.
> >
> > > > I'm doing it here this time, please look at the result, later.
> > > ok, I'm waiting for it.
> > > And if you give me some sketchy code, that's fine.
> > > If you do, I'll remake this patch based on the result. :)
> >
> > Take a look at the acme/tmp_perf/core, that is where I got yesterday.
> >
> > - Arnaldo
> >
>
> I fetched all branch of your repository, but it don't seem to be pushed.
>
> $ git remote get-url acme
> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
>
> $ git branch -a | grep acme | grep tmp | grep core
> remotes/acme/tmp.perf/core
> remotes/acme/tmp_perf.core
>
> $ git show tmp.perf/core | head -3
> commit 4827c52cd001e208704ab733a389c96ae2e70e5b
> Author: Andi Kleen <[email protected]>
> Date: Fri Jul 21 12:25:57 2017 -0700

The one above, look further down, from 896bccd3cb8d to 0321d0281cbb,
there are some more missing, but the ones below should be ready for
sending to Ingo, which I plan to do today.

4827c52cd001 perf jevents: Make build fail on JSON parse error
768750b250b4 perf top: Support lookup of symbols in other mount namespaces.
bd09c1d5f473 perf stat: Use group read for event groups
b9830d226aae perf evsel: Add read_counter() method
e9c58e8b74b8 perf evsel: Add read_size method
b1f952a065d6 perf evsel: Add verbose output for sys_perf_event_open fallback
1321cb6fb1ac perf jvmti: Fix linker error when libelf config is disabled
27cf8ae9ac36 perf annotate: Process tracing data in pipe mode
76dd07b982e0 perf tools: Add EXCLUDE_EXTLIBS and EXTRA_PERFLIBS to makefile
f246e8b7f34e perf cgroup: Fix refcount usage
60cdc09e7d77 perf report: Fix kernel symbol adjustment for s390x
0321d0281cbb perf annotate stdio: Fix --show-total-period
ecd5f9959d2c perf annotate: Do not overwrite sample->period
461c17f00f40 perf annotate: Store the sample period in each histogram bucket
bab89f6aed7e perf hists: Pass perf_sample to __symbol__inc_addr_samples()
8158683da3d3 perf annotate: Rename 'sum' to 'nr_samples' in struct sym_hist
896bccd3cb8d perf annotate: Introduce struct sym_hist_entry

> $ git show tmp_perf.core | head -3
> commit 3331778eb08a50cec959da933c040bd7fbdde396
> Author: Adrian Hunter <[email protected]>
> Date: Thu Jul 31 09:00:56 2014 +0300
>
>
> Thanks,
> Taeung

2017-07-24 17:47:00

by Andi Kleen

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

On Mon, Jul 24, 2017 at 02:34:37PM -0300, Arnaldo Carvalho de Melo wrote:
> Em Sun, Jul 23, 2017 at 08:46:20AM -0700, Andi Kleen escreveu:
> > On Sun, Jul 23, 2017 at 07:46:05AM +0900, Namhyung Kim wrote:
> > > Hi Arnaldo and Taeung,
> > >
> > > (+ Andi)
> > >
> > > On Fri, Jul 21, 2017 at 11:47:48AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> > > > > +++ 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;
> > > > > -
> > > > > he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> > > > > if (he == NULL)
> > > > > return -ENOMEM;
> > > >
> > > > I split the hunk above into a separate patch, as a fix, Namhyung, can
> > > > you take a look at why need to unconditionally overwrite what is in
> > > > sample->weight as well?
> > > >
> > > > Looks fishy as it may come with a value from the kernel, parsed in
> > > > perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
> > > > perf_event_attr->sample_type.
> > > >
> > > > Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
> > > > isn't requested in sample_type?
> > >
> > > It was Andi added that code originally (05484298cbfe). IIUC the
> > > weight is only meaningful for some CPUs with Intel TSX and he used a
> > > dummy value.
> >
> > It's used for more than TSX. e.g. perf mem uses it for memory latencies.
> >
> > > AFAIK the hists code doesn't care of it unless weight sort key is used
> > > (for report). As it's not used by annotate code, I think it'd be
> > > better leaving it as is (like period).
> >
> > Right, it's needed when weight is specified as a sort key. But we need
> > a fallback in case the user specified weight in perf report, but
> > didn't enable it for perf record.
>
> Humm, shouldn't we fail in that case? I.e. user asks for per-sample
> property not collected at 'perf record' time?

Could fail, but it's essentially a no-op
>
> That or the weight sort order handler should see that
> perf_sample->weight is zero and assume it wasn't collected then turn it
> into a 1? Or just look at evsel->attr.sample_type & PERF_SAMPLE_WEIGHT?

Either 0 or 1 works, it just always needs to be the same value.

-Andi

2017-07-24 18:07:45

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Mon, Jul 24, 2017 at 10:46:38AM -0700, Andi Kleen escreveu:
> On Mon, Jul 24, 2017 at 02:34:37PM -0300, Arnaldo Carvalho de Melo wrote:
> > Em Sun, Jul 23, 2017 at 08:46:20AM -0700, Andi Kleen escreveu:
> > > On Sun, Jul 23, 2017 at 07:46:05AM +0900, Namhyung Kim wrote:
> > > > Hi Arnaldo and Taeung,
> > > >
> > > > (+ Andi)
> > > >
> > > > On Fri, Jul 21, 2017 at 11:47:48AM -0300, Arnaldo Carvalho de Melo wrote:
> > > > > Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
> > > > > > +++ 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;
> > > > > > -
> > > > > > he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);
> > > > > > if (he == NULL)
> > > > > > return -ENOMEM;
> > > > >
> > > > > I split the hunk above into a separate patch, as a fix, Namhyung, can
> > > > > you take a look at why need to unconditionally overwrite what is in
> > > > > sample->weight as well?
> > > > >
> > > > > Looks fishy as it may come with a value from the kernel, parsed in
> > > > > perf_evsel__parse_sample(), when PERF_SAMPLE_WEIGHT is in
> > > > > perf_event_attr->sample_type.
> > > > >
> > > > > Is it that the hists code needs a sane value when PERF_SAMPLE_WEIGHT
> > > > > isn't requested in sample_type?
> > > >
> > > > It was Andi added that code originally (05484298cbfe). IIUC the
> > > > weight is only meaningful for some CPUs with Intel TSX and he used a
> > > > dummy value.
> > >
> > > It's used for more than TSX. e.g. perf mem uses it for memory latencies.
> > >
> > > > AFAIK the hists code doesn't care of it unless weight sort key is used
> > > > (for report). As it's not used by annotate code, I think it'd be
> > > > better leaving it as is (like period).
> > >
> > > Right, it's needed when weight is specified as a sort key. But we need
> > > a fallback in case the user specified weight in perf report, but
> > > didn't enable it for perf record.
> >
> > Humm, shouldn't we fail in that case? I.e. user asks for per-sample
> > property not collected at 'perf record' time?
>
> Could fail, but it's essentially a no-op
> >
> > That or the weight sort order handler should see that
> > perf_sample->weight is zero and assume it wasn't collected then turn it
> > into a 1? Or just look at evsel->attr.sample_type & PERF_SAMPLE_WEIGHT?
>
> Either 0 or 1 works, it just always needs to be the same value.

Well, perf_evsel__parse_sample() is where that struct
perf_sample->weight is set, and it sets the whole struct to zero, then
goes on parsing using evsel->attr.sample_type to decide what to set, so
I think I can just nuke that setting to one, thanks.

- Arnaldo

2017-07-24 21:28:58

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

On 07/25/2017 02:37 AM, Arnaldo Carvalho de Melo wrote:
> Em Mon, Jul 24, 2017 at 10:51:58AM +0900, Taeung Song escreveu:
>> Hi Arnaldo,
>>
>> Sorry, I'm too late.
>>
>> On 07/21/2017 08:24 PM, Arnaldo Carvalho de Melo wrote:
>>> Em Fri, Jul 21, 2017 at 06:41:29PM +0900, Taeung Song escreveu:
>>>> On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
>>>>> Em Thu, Jul 20, 2017 at 06:36:55AM +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 this is not what this patch does, it is still doing too many things,
>>>>> it should first add sample to those function signatures, leaving the
>>>>> "meat" to a followup patch, where we will not be distracted with
>>>>> infrastructure needed to do what you describe in the changelog.
>>>
>>>>> I'm doing it here this time, please look at the result, later.
>>>> ok, I'm waiting for it.
>>>> And if you give me some sketchy code, that's fine.
>>>> If you do, I'll remake this patch based on the result. :)
>>>
>>> Take a look at the acme/tmp_perf/core, that is where I got yesterday.
>>>
>>> - Arnaldo
>>>
>>
>> I fetched all branch of your repository, but it don't seem to be pushed.
>>
>> $ git remote get-url acme
>> git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git
>>
>> $ git branch -a | grep acme | grep tmp | grep core
>> remotes/acme/tmp.perf/core
>> remotes/acme/tmp_perf.core
>>
>> $ git show tmp.perf/core | head -3
>> commit 4827c52cd001e208704ab733a389c96ae2e70e5b
>> Author: Andi Kleen <[email protected]>
>> Date: Fri Jul 21 12:25:57 2017 -0700
>
> The one above, look further down, from 896bccd3cb8d to 0321d0281cbb,
> there are some more missing, but the ones below should be ready for
> sending to Ingo, which I plan to do today.

I didn't see them..
Thank you for adjusted patchset.
I checked them a while ago.

Would you add Reported-by: Namhyung Kim
to the commit 0321d0281cbbb404ea73f9e1869ec8db42e8ddfd ?

And we can handle other patches later,
but how about additionally fixing the header
on the annotate stdio browser ?
(If you don't want to handle additional patches anymore today,
I'll send them with next patchset.)

"Percent" -> "Event Count"

Because I think it would be better to show the proper first column.
Moreover there is the below case that is not aligned due to big period
values.

perf annotate --stdio -i milian.data --show-total-period
Percent | Source code & Disassembly of test for cycles:ppp (1442
samples)
-------------------------------------------------------------------------------
:
:
:
: Disassembly of section .text:
...
0 : 40089d: pxor %xmm1,%xmm1
27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
0 : 4008a6: pxor %xmm5,%xmm5

So, I made a patch like below:

commit 6b96b9947e83474bd6e6fd09f93c390536bb435b
Author: Taeung Song <[email protected]>
Date: Tue Jul 25 06:17:59 2017 +0900

perf annotate: Show the proper header when using --show-total-period

Currently a first column is only "Percent",
so fix it to show correct column name based on given options.
(e.g. if using --show-total-period, show "Event count" as a first
column)

Reported-by: Milian Wolff <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Jiri Olsa <[email protected]>
Signed-off-by: Taeung Song <[email protected]>

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 004072f..0224595 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line
*dl, struct symbol *sym, u64 st
color = get_percent_color(percent);

if (symbol_conf.show_total_period)
- color_fprintf(stdout, color, " %7" PRIu64,
+ color_fprintf(stdout, color, " %11" PRIu64,
sample.period);
else
color_fprintf(stdout, color, " %7.2f",
percent);
@@ -1173,6 +1173,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
@@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
struct map *map,
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;
+
graph_dotted_len = printf(" %-*.*s| Source code &
Disassembly of %s for %s (%" PRIu64 " samples)\n",
- width, width, "Percent", d_filename, evsel_name,
h->nr_samples);
+ width, width,
+ symbol_conf.show_total_period ?
"Event count" : "Percent",
+ d_filename, evsel_name, h->nr_samples);

printf("%-*.*s----\n",
graph_dotted_len, graph_dotted_len, graph_dotted_line);

Thanks,
Taeung

>
> 4827c52cd001 perf jevents: Make build fail on JSON parse error
> 768750b250b4 perf top: Support lookup of symbols in other mount namespaces.
> bd09c1d5f473 perf stat: Use group read for event groups
> b9830d226aae perf evsel: Add read_counter() method
> e9c58e8b74b8 perf evsel: Add read_size method
> b1f952a065d6 perf evsel: Add verbose output for sys_perf_event_open fallback
> 1321cb6fb1ac perf jvmti: Fix linker error when libelf config is disabled
> 27cf8ae9ac36 perf annotate: Process tracing data in pipe mode
> 76dd07b982e0 perf tools: Add EXCLUDE_EXTLIBS and EXTRA_PERFLIBS to makefile
> f246e8b7f34e perf cgroup: Fix refcount usage
> 60cdc09e7d77 perf report: Fix kernel symbol adjustment for s390x
> 0321d0281cbb perf annotate stdio: Fix --show-total-period
> ecd5f9959d2c perf annotate: Do not overwrite sample->period
> 461c17f00f40 perf annotate: Store the sample period in each histogram bucket
> bab89f6aed7e perf hists: Pass perf_sample to __symbol__inc_addr_samples()
> 8158683da3d3 perf annotate: Rename 'sum' to 'nr_samples' in struct sym_hist
> 896bccd3cb8d perf annotate: Introduce struct sym_hist_entry
>
>
>> $ git show tmp_perf.core | head -3
>> commit 3331778eb08a50cec959da933c040bd7fbdde396
>> Author: Adrian Hunter <[email protected]>
>> Date: Thu Jul 31 09:00:56 2014 +0300
>>
>>
>> Thanks,
>> Taeung

2017-07-25 14:42:57

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Tue, Jul 25, 2017 at 06:28:42AM +0900, Taeung Song escreveu:
> On 07/25/2017 02:37 AM, Arnaldo Carvalho de Melo wrote:
> > The one above, look further down, from 896bccd3cb8d to 0321d0281cbb,
> > there are some more missing, but the ones below should be ready for
> > sending to Ingo, which I plan to do today.
>
> I didn't see them..
> Thank you for adjusted patchset.
> I checked them a while ago.
>
> Would you add Reported-by: Namhyung Kim
> to the commit 0321d0281cbbb404ea73f9e1869ec8db42e8ddfd ?

Sure, will do.

> And we can handle other patches later,
> but how about additionally fixing the header
> on the annotate stdio browser ?
> (If you don't want to handle additional patches anymore today,
> I'll send them with next patchset.)
>
> "Percent" -> "Event Count"
>
> Because I think it would be better to show the proper first column.

Of course, I was coming to that, trying to figure out if what was left
was just one or even more separate patches, see below

> Moreover there is the below case that is not aligned due to big period
> values.

So, that "moreover" means its not just one patch, but at least two, i.e.
when one selects show-total-period we better have more space for that
column, right?

I'll break the patch below accordingly.

And even then, there is one question left, see below

> perf annotate --stdio -i milian.data --show-total-period
> Percent | Source code & Disassembly of test for cycles:ppp (1442
> samples)
> -------------------------------------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> ...
> 0 : 40089d: pxor %xmm1,%xmm1
> 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
> 0 : 4008a6: pxor %xmm5,%xmm5
>
>
> So, I made a patch like below:
>
> commit 6b96b9947e83474bd6e6fd09f93c390536bb435b
> Author: Taeung Song <[email protected]>
> Date: Tue Jul 25 06:17:59 2017 +0900
>
> perf annotate: Show the proper header when using --show-total-period
>
> Currently a first column is only "Percent",
> so fix it to show correct column name based on given options.
> (e.g. if using --show-total-period, show "Event count" as a first
> column)
>
> Reported-by: Milian Wolff <[email protected]>
> Cc: Namhyung Kim <[email protected]>
> Cc: Jiri Olsa <[email protected]>
> Signed-off-by: Taeung Song <[email protected]>
>
> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
> index 004072f..0224595 100644
> --- a/tools/perf/util/annotate.c
> +++ b/tools/perf/util/annotate.c
> @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
> struct symbol *sym, u64 st
> color = get_percent_color(percent);
>
> if (symbol_conf.show_total_period)
> - color_fprintf(stdout, color, " %7" PRIu64,
> + color_fprintf(stdout, color, " %11" PRIu64,
> sample.period);

this part will be in a separate patch, i.e. something like:

[PATCH] Widen "Period" column when using --show-total-period

> else
> color_fprintf(stdout, color, " %7.2f",
> percent);
> @@ -1173,6 +1173,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;
> +

But what about this one? What is that '4' for? Not obvious at first
sight, can you elaborate on the need for this specific one?

> if (!*dl->line)
> printf(" %*s:\n", width, " ");
> else
> @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
> struct map *map,
> 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;

What about this one?

> +
> graph_dotted_len = printf(" %-*.*s| Source code & Disassembly
> of %s for %s (%" PRIu64 " samples)\n",
> - width, width, "Percent", d_filename, evsel_name,
> h->nr_samples);
> + width, width,
> + symbol_conf.show_total_period ? "Event
> count" : "Percent",
> + d_filename, evsel_name, h->nr_samples);
>

this one will be in a separate patch, with the title you chose:

[PATCH] perf annotate: Show the proper header when using --show-total-period

> printf("%-*.*s----\n",
> graph_dotted_len, graph_dotted_len, graph_dotted_line);
>

- Arnaldo

2017-07-25 15:53:35

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Hi Arnaldo,

On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
> Em Tue, Jul 25, 2017 at 06:28:42AM +0900, Taeung Song escreveu:
>> On 07/25/2017 02:37 AM, Arnaldo Carvalho de Melo wrote:
>>> The one above, look further down, from 896bccd3cb8d to 0321d0281cbb,
>>> there are some more missing, but the ones below should be ready for
>>> sending to Ingo, which I plan to do today.
>>
>> I didn't see them..
>> Thank you for adjusted patchset.
>> I checked them a while ago.
>>
>> Would you add Reported-by: Namhyung Kim
>> to the commit 0321d0281cbbb404ea73f9e1869ec8db42e8ddfd ?
>
> Sure, will do.
>
>> And we can handle other patches later,
>> but how about additionally fixing the header
>> on the annotate stdio browser ?
>> (If you don't want to handle additional patches anymore today,
>> I'll send them with next patchset.)
>>
>> "Percent" -> "Event Count"
>>
>> Because I think it would be better to show the proper first column.
>
> Of course, I was coming to that, trying to figure out if what was left
> was just one or even more separate patches, see below
>
>> Moreover there is the below case that is not aligned due to big period
>> values.
>
> So, that "moreover" means its not just one patch, but at least two, i.e.
> when one selects show-total-period we better have more space for that
> column, right?

I got it. will separate this patch.

>
> I'll break the patch below accordingly.
>
> And even then, there is one question left, see below
>
>> perf annotate --stdio -i milian.data --show-total-period
>> Percent | Source code & Disassembly of test for cycles:ppp (1442
>> samples)
>> -------------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> ...
>> 0 : 40089d: pxor %xmm1,%xmm1
>> 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
>> 0 : 4008a6: pxor %xmm5,%xmm5
>>
>>
>> So, I made a patch like below:
>>
>> commit 6b96b9947e83474bd6e6fd09f93c390536bb435b
>> Author: Taeung Song <[email protected]>
>> Date: Tue Jul 25 06:17:59 2017 +0900
>>
>> perf annotate: Show the proper header when using --show-total-period
>>
>> Currently a first column is only "Percent",
>> so fix it to show correct column name based on given options.
>> (e.g. if using --show-total-period, show "Event count" as a first
>> column)
>>
>> Reported-by: Milian Wolff <[email protected]>
>> Cc: Namhyung Kim <[email protected]>
>> Cc: Jiri Olsa <[email protected]>
>> Signed-off-by: Taeung Song <[email protected]>
>>
>> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
>> index 004072f..0224595 100644
>> --- a/tools/perf/util/annotate.c
>> +++ b/tools/perf/util/annotate.c
>> @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
>> struct symbol *sym, u64 st
>> color = get_percent_color(percent);
>>
>> if (symbol_conf.show_total_period)
>> - color_fprintf(stdout, color, " %7" PRIu64,
>> + color_fprintf(stdout, color, " %11" PRIu64,
>> sample.period);
>
> this part will be in a separate patch, i.e. something like:
>
> [PATCH] Widen "Period" column when using --show-total-period
>

ok.

>> else
>> color_fprintf(stdout, color, " %7.2f",
>> percent);
>> @@ -1173,6 +1173,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;
>> +
>
> But what about this one? What is that '4' for? Not obvious at first
> sight, can you elaborate on the need for this specific one?
>

Yep, if you check the above code lines, like below:

color_fprintf(stdout, color, " %11" PRIu64,
sample.period);

The above number of letters is 12
i.e. 12 = 1 (" ": white space) + 11 (digits of sample.period)

So, I used '4', because the 'width' variable is initialized as '8'.

Additionally this patch handle the width for group event like below:

$ perf annotate --show-total-period -i group_events.data --stdio
Event count | Source code & Disassembly of
old for cycles (529 samples)
-----------------------------------------------------------------------------------------------------
:
:
:
: Disassembly of section .text:
:
: 0000000000400816
<get_cond_maxprice>:
: get_cond_maxprice():
0 0 7144 : 400816: push %rbp
3480988 0 5709 : 400817: mov %rsp,%rbp
0 0 7522 : 40081a: mov
%edi,-0x24(%rbp)

Sorry, I repeatedly failed to adjust a proper patch unit..
I'll remake this patches based on your comment,
and resend next patchset !

>> if (!*dl->line)
>> printf(" %*s:\n", width, " ");
>> else
>> @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
>> struct map *map,
>> 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;
>
> What about this one?
>

ditto.

>> +
>> graph_dotted_len = printf(" %-*.*s| Source code & Disassembly
>> of %s for %s (%" PRIu64 " samples)\n",
>> - width, width, "Percent", d_filename, evsel_name,
>> h->nr_samples);
>> + width, width,
>> + symbol_conf.show_total_period ? "Event
>> count" : "Percent",
>> + d_filename, evsel_name, h->nr_samples);
>>
>
> this one will be in a separate patch, with the title you chose:
>
> [PATCH] perf annotate: Show the proper header when using --show-total-period
>

ok.

Thanks,
Taeung

>> printf("%-*.*s----\n",
>> graph_dotted_len, graph_dotted_len, graph_dotted_line);
>>
>
> - Arnaldo
>

2017-07-25 16:17:26

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Wed, Jul 26, 2017 at 12:53:28AM +0900, Taeung Song escreveu:
> On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
> > > Moreover there is the below case that is not aligned due to big period
> > > values.

> > So, that "moreover" means its not just one patch, but at least two, i.e.
> > when one selects show-total-period we better have more space for that
> > column, right?

> I got it. will separate this patch.

Ok, please continue your work from my perf/core branch that I just
pushed, in it the latest patch is this one:

https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=143e9656aec7c61b9b8e134da5abc5dfb6133cbf

Which is a chunk of what you done below. More comments below.

> > I'll break the patch below accordingly.
> >
> > And even then, there is one question left, see below
> >
> > > perf annotate --stdio -i milian.data --show-total-period
> > > Percent | Source code & Disassembly of test for cycles:ppp (1442
> > > samples)
> > > -------------------------------------------------------------------------------
> > > :
> > > :
> > > :
> > > : Disassembly of section .text:
> > > ...
> > > 0 : 40089d: pxor %xmm1,%xmm1
> > > 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
> > > 0 : 4008a6: pxor %xmm5,%xmm5
> > >
> > >
> > > So, I made a patch like below:
<SNIP>
> > > +++ b/tools/perf/util/annotate.c
> > > @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
> > > struct symbol *sym, u64 st
> > > color = get_percent_color(percent);
> > >
> > > if (symbol_conf.show_total_period)
> > > - color_fprintf(stdout, color, " %7" PRIu64,
> > > + color_fprintf(stdout, color, " %11" PRIu64,
> > > sample.period);
> >
> > this part will be in a separate patch, i.e. something like:
> >
> > [PATCH] Widen "Period" column when using --show-total-period
> >
>
> ok.
>
> > > else
> > > color_fprintf(stdout, color, " %7.2f",
> > > percent);
> > > @@ -1173,6 +1173,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;
> > > +
> >
> > But what about this one? What is that '4' for? Not obvious at first
> > sight, can you elaborate on the need for this specific one?
> >
>
> Yep, if you check the above code lines, like below:
>
> color_fprintf(stdout, color, " %11" PRIu64,
> sample.period);
>
> The above number of letters is 12
> i.e. 12 = 1 (" ": white space) + 11 (digits of sample.period)
>
> So, I used '4', because the 'width' variable is initialized as '8'.

Think that I am 7 years old :o) I'm still not understanding this
logic...

> Additionally this patch handle the width for group event like below:
>
> $ perf annotate --show-total-period -i group_events.data --stdio
> Event count | Source code & Disassembly of old for
> cycles (529 samples)
> -----------------------------------------------------------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> :
> : 0000000000400816
> <get_cond_maxprice>:
> : get_cond_maxprice():
> 0 0 7144 : 400816: push %rbp
> 3480988 0 5709 : 400817: mov %rsp,%rbp
> 0 0 7522 : 40081a: mov %edi,-0x24(%rbp)
>
>
> Sorry, I repeatedly failed to adjust a proper patch unit..
> I'll remake this patches based on your comment,
> and resend next patchset !

It is not a problem, you're making progress, thanks for taking into
accoutn my comments.

The end result may be the same, but having a good patch granularity is
fundamental for bisecting, also for maintainers to cherry-pick parts of
your work that they agree on while making comments about parts that
looks wrong or needing some more work.

> > > if (!*dl->line)
> > > printf(" %*s:\n", width, " ");
> > > else
> > > @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
> > > struct map *map,
> > > 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;
> >
> > What about this one?
> >
>
> ditto.
>
> > > +
> > > graph_dotted_len = printf(" %-*.*s| Source code & Disassembly
> > > of %s for %s (%" PRIu64 " samples)\n",
> > > - width, width, "Percent", d_filename, evsel_name,
> > > h->nr_samples);
> > > + width, width,
> > > + symbol_conf.show_total_period ? "Event
> > > count" : "Percent",
> > > + d_filename, evsel_name, h->nr_samples);
> > >
> >
> > this one will be in a separate patch, with the title you chose:
> >
> > [PATCH] perf annotate: Show the proper header when using --show-total-period
> >
>
> ok.
>
>
> Thanks,
> Taeung
>
> > > printf("%-*.*s----\n",
> > > graph_dotted_len, graph_dotted_len, graph_dotted_line);
> > >
> >
> > - Arnaldo
> >

2017-07-26 11:57:50

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Hello Arnaldo :)

On 07/26/2017 01:17 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Jul 26, 2017 at 12:53:28AM +0900, Taeung Song escreveu:
>> On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
>>>> Moreover there is the below case that is not aligned due to big period
>>>> values.
>
>>> So, that "moreover" means its not just one patch, but at least two, i.e.
>>> when one selects show-total-period we better have more space for that
>>> column, right?
>
>> I got it. will separate this patch.
>
> Ok, please continue your work from my perf/core branch that I just
> pushed, in it the latest patch is this one:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=143e9656aec7c61b9b8e134da5abc5dfb6133cbf
>
> Which is a chunk of what you done below. More comments below.
>

Yes sir, :)
I fetched and checked it.

>>> I'll break the patch below accordingly.
>>>
>>> And even then, there is one question left, see below
>>>
>>>> perf annotate --stdio -i milian.data --show-total-period
>>>> Percent | Source code & Disassembly of test for cycles:ppp (1442
>>>> samples)
>>>> -------------------------------------------------------------------------------
>>>> :
>>>> :
>>>> :
>>>> : Disassembly of section .text:
>>>> ...
>>>> 0 : 40089d: pxor %xmm1,%xmm1
>>>> 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
>>>> 0 : 4008a6: pxor %xmm5,%xmm5
>>>>
>>>>
>>>> So, I made a patch like below:
> <SNIP>
>>>> +++ b/tools/perf/util/annotate.c
>>>> @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
>>>> struct symbol *sym, u64 st
>>>> color = get_percent_color(percent);
>>>>
>>>> if (symbol_conf.show_total_period)
>>>> - color_fprintf(stdout, color, " %7" PRIu64,
>>>> + color_fprintf(stdout, color, " %11" PRIu64,
>>>> sample.period);
>>>
>>> this part will be in a separate patch, i.e. something like:
>>>
>>> [PATCH] Widen "Period" column when using --show-total-period
>>>
>>
>> ok.
>>
>>>> else
>>>> color_fprintf(stdout, color, " %7.2f",
>>>> percent);
>>>> @@ -1173,6 +1173,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;
>>>> +
>>>
>>> But what about this one? What is that '4' for? Not obvious at first
>>> sight, can you elaborate on the need for this specific one?
>>>
>>
>> Yep, if you check the above code lines, like below:
>>
>> color_fprintf(stdout, color, " %11" PRIu64,
>> sample.period);
>>
>> The above number of letters is 12
>> i.e. 12 = 1 (" ": white space) + 11 (digits of sample.period)
>>
>> So, I used '4', because the 'width' variable is initialized as '8'.
>
> Think that I am 7 years old :o) I'm still not understanding this
> logic...
>

Humm.. first of all, we can check the 'width' variable in two function
disasm_line__print() and symbol__annotate_printf() like below:

1063 static int disasm_line__print(struct disasm_line *dl, struct symbol
*sym, u64 start,
1064 struct perf_evsel *evsel, u64 len, int
min_pcnt, int printed,
1065 int max_lines, struct disasm_line *queue)
1066 {

...
1167 else {
1168 int width = 8;
1169
1170 if (queue)
1171 return -1;
1172
1173 if (perf_evsel__is_group_event(evsel))
1174 width *= evsel->nr_members;
1175
1176 if (!*dl->line)
1177 printf(" %*s:\n", width, " ");
1178 else
1179 printf(" %*s: %s\n", width, " ", dl->line);

And,

1794 int symbol__annotate_printf(struct symbol *sym, struct map *map,
1795 struct perf_evsel *evsel, bool full_paths,
1796 int min_pcnt, int max_lines, int context)
1797 {
...

1809 int width = 8;
...
1823 if (perf_evsel__is_group_event(evsel))
1824 width *= evsel->nr_members;
1825
1826 graph_dotted_len = printf(" %-*.*s| Source code &
Disassembly of %s for %s (%" PRIu64 " samples)\n",
1827 width, width,
symbol_conf.show_total_period ? "Event count" : "Percent",
1828 d_filename, evsel_name,
h->nr_samples);

As you can see, currently the 'width' variables are set as 8 letters
But I adjust the width as 12 letters for the first column " Event count"
and period value.

So I do witdh += 4 for 12 letters like below:

$ perf annotate --stdio --show-total-period -i hex2u64
Event count | Source code & Disassembly of old for cycles:ppp (102
samples)
---------------------------------------------------------------------------------
:
:
:
: Disassembly of section .text:
:
: 0000000000400816 <get_cond_maxprice>:
: get_cond_maxprice():
1950346 : 400816: push %rbp
741848 : 400817: mov %rsp,%rbp

We don't need to adjust the 'width' for --show-total-period ?

>> Additionally this patch handle the width for group event like below:
>>
>> $ perf annotate --show-total-period -i group_events.data --stdio
>> Event count | Source code & Disassembly of old for
>> cycles (529 samples)
>> -----------------------------------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> :
>> : 0000000000400816
>> <get_cond_maxprice>:
>> : get_cond_maxprice():
>> 0 0 7144 : 400816: push %rbp
>> 3480988 0 5709 : 400817: mov %rsp,%rbp
>> 0 0 7522 : 40081a: mov %edi,-0x24(%rbp)
>>
>>
>> Sorry, I repeatedly failed to adjust a proper patch unit..
>> I'll remake this patches based on your comment,
>> and resend next patchset !
>
> It is not a problem, you're making progress, thanks for taking into
> accoutn my comments.
>
> The end result may be the same, but having a good patch granularity is
> fundamental for bisecting, also for maintainers to cherry-pick parts of
> your work that they agree on while making comments about parts that
> looks wrong or needing some more work.
>

Thanks for your advice !!

- Taeung

>>>> if (!*dl->line)
>>>> printf(" %*s:\n", width, " ");
>>>> else
>>>> @@ -1823,8 +1827,14 @@ int symbol__annotate_printf(struct symbol *sym,
>>>> struct map *map,
>>>> 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;
>>>
>>> What about this one?
>>>
>>
>> ditto.
>>
>>>> +
>>>> graph_dotted_len = printf(" %-*.*s| Source code & Disassembly
>>>> of %s for %s (%" PRIu64 " samples)\n",
>>>> - width, width, "Percent", d_filename, evsel_name,
>>>> h->nr_samples);
>>>> + width, width,
>>>> + symbol_conf.show_total_period ? "Event
>>>> count" : "Percent",
>>>> + d_filename, evsel_name, h->nr_samples);
>>>>
>>>
>>> this one will be in a separate patch, with the title you chose:
>>>
>>> [PATCH] perf annotate: Show the proper header when using --show-total-period
>>>
>>
>> ok.
>>
>>
>> Thanks,
>> Taeung
>>
>>>> printf("%-*.*s----\n",
>>>> graph_dotted_len, graph_dotted_len, graph_dotted_line);
>>>>
>>>
>>> - Arnaldo
>>>

2017-07-26 17:22:21

[permalink] [raw]

Subject: [tip:perf/core] perf hists: Pass perf_sample to symbolinc_addr_samples()

Commit-ID: bab89f6aed7e745893e009014354d0caaf62acf7
Gitweb: http://git.kernel.org/tip/bab89f6aed7e745893e009014354d0caaf62acf7
Author: Taeung Song <[email protected]>
AuthorDate: Thu, 20 Jul 2017 16:28:53 -0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 21 Jul 2017 08:23:50 -0300

perf hists: Pass perf_sample to __symbol__inc_addr_samples()

To pave the way to use perf_sample fields in the annotate code, storing
sample->period in sym_hist->addr->period and its sum in
sym_hist->period.

Signed-off-by: Taeung Song <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ split and adjusted from a larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-annotate.c | 2 +-
tools/perf/builtin-report.c | 15 ++++++++-------
tools/perf/builtin-top.c | 5 +++--
tools/perf/util/annotate.c | 18 +++++++++++-------
tools/perf/util/annotate.h | 6 ++++--
5 files changed, 27 insertions(+), 19 deletions(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 5205408..96fe1a8 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -184,7 +184,7 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
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 cea25d0..983b238 100644
--- a/tools/perf/builtin-report.c
+++ b/tools/perf/builtin-report.c
@@ -115,37 +115,38 @@ 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) {
bi = he->branch_info;
- err = addr_map_symbol__inc_samples(&bi->from, evsel->idx);
+ err = addr_map_symbol__inc_samples(&bi->from, sample, evsel->idx);
if (err)
goto out;

- err = addr_map_symbol__inc_samples(&bi->to, evsel->idx);
+ err = addr_map_symbol__inc_samples(&bi->to, sample, evsel->idx);

} else if (rep->mem_mode) {
mi = he->mem_info;
- err = addr_map_symbol__inc_samples(&mi->daddr, evsel->idx);
+ err = addr_map_symbol__inc_samples(&mi->daddr, sample, evsel->idx);
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);

} else if (symbol_conf.cumulate_callchain) {
if (single)
- err = hist_entry__inc_addr_samples(he, evsel->idx,
+ 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..e5a8f24 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,7 @@ 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/util/annotate.c b/tools/perf/util/annotate.c
index 58c6b63..c2fe16d 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -697,7 +697,8 @@ static int __symbol__account_cycles(struct annotation *notes,
}

static int __symbol__inc_addr_samples(struct symbol *sym, struct map *map,
- struct annotation *notes, int evidx, u64 addr)
+ struct annotation *notes, int evidx, u64 addr,
+ struct perf_sample *sample __maybe_unused)
{
unsigned offset;
struct sym_hist *h;
@@ -738,7 +739,8 @@ static struct annotation *symbol__get_annotation(struct symbol *sym, bool cycles
}

static int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
- int evidx, u64 addr)
+ int evidx, u64 addr,
+ struct perf_sample *sample)
{
struct annotation *notes;

@@ -747,7 +749,7 @@ static int symbol__inc_addr_samples(struct symbol *sym, struct map *map,
notes = symbol__get_annotation(sym, false);
if (notes == NULL)
return -ENOMEM;
- return __symbol__inc_addr_samples(sym, map, notes, evidx, addr);
+ return __symbol__inc_addr_samples(sym, map, notes, evidx, addr, sample);
}

static int symbol__account_cycles(u64 addr, u64 start,
@@ -811,14 +813,16 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
return err;
}

-int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx)
+int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
+ int evidx)
{
- return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr);
+ return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr, sample);
}

-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 ip)
{
- return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip);
+ return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip, sample);
}

static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index e8c246e..720f181 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -154,13 +154,15 @@ static inline struct annotation *symbol__annotation(struct symbol *sym)
return (void *)sym - symbol_conf.priv_size;
}

-int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx);
+int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, struct perf_sample *sample,
+ int evidx);

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);

2017-07-26 17:22:44

[permalink] [raw]

Subject: [tip:perf/core] perf annotate: Store the sample period in each histogram bucket

Commit-ID: 461c17f00f400f95116880d91d20a7fcd84263a9
Gitweb: http://git.kernel.org/tip/461c17f00f400f95116880d91d20a7fcd84263a9
Author: Taeung Song <[email protected]>
AuthorDate: Thu, 20 Jul 2017 17:18:05 -0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 21 Jul 2017 12:02:38 -0300

perf annotate: Store the sample period in each histogram bucket

We'll use it soon, when fixing --show-total-period.

Signed-off-by: Taeung Song <[email protected]>
Cc: Adrian Hunter <[email protected]>
Cc: David Ahern <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Wang Nan <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ split from a larger patch, do the math in __symbol__inc_addr_samples() ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/annotate.c | 17 ++++++++++++-----
tools/perf/util/annotate.h | 1 +
2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c2fe16d..00e085f 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -698,7 +698,7 @@ static int __symbol__account_cycles(struct annotation *notes,

static int __symbol__inc_addr_samples(struct symbol *sym, struct map *map,
struct annotation *notes, int evidx, u64 addr,
- struct perf_sample *sample __maybe_unused)
+ struct perf_sample *sample)
{
unsigned offset;
struct sym_hist *h;
@@ -716,10 +716,13 @@ static int __symbol__inc_addr_samples(struct symbol *sym, struct map *map,
h = annotation__histogram(notes, evidx);
h->nr_samples++;
h->addr[offset].nr_samples++;
+ h->period += sample->period;
+ h->addr[offset].period += sample->period;

pr_debug3("%#" PRIx64 " %s: period++ [addr: %#" PRIx64 ", %#" PRIx64
- ", evidx=%d] => %" PRIu64 "\n", sym->start, sym->name,
- addr, addr - sym->start, evidx, h->addr[offset].nr_samples);
+ ", evidx=%d] => nr_samples: %" PRIu64 ", period: %" PRIu64 "\n",
+ sym->start, sym->name, addr, addr - sym->start, evidx,
+ h->addr[offset].nr_samples, h->addr[offset].period);
return 0;
}

@@ -937,7 +940,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
struct source_line *src_line = notes->src->lines;
double percent = 0.0;

- sample->nr_samples = 0;
+ sample->nr_samples = sample->period = 0;

if (src_line) {
size_t sizeof_src_line = sizeof(*src_line) +
@@ -957,11 +960,15 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
} else {
struct sym_hist *h = annotation__histogram(notes, evidx);
unsigned int hits = 0;
+ u64 period = 0;

- while (offset < end)
+ while (offset < end) {
hits += h->addr[offset++].nr_samples;
+ period += h->addr[offset++].period;
+ }

if (h->nr_samples) {
+ sample->period = period;
sample->nr_samples = hits;
percent = 100.0 * hits / h->nr_samples;
}
diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
index 720f181..9ce575c 100644
--- a/tools/perf/util/annotate.h
+++ b/tools/perf/util/annotate.h
@@ -88,6 +88,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,

struct sym_hist {
u64 nr_samples;
+ u64 period;
struct sym_hist_entry addr[0];
};

2017-07-26 17:23:09

[permalink] [raw]

Subject: [tip:perf/core] perf annotate: Do not overwrite sample->period

Commit-ID: ecd5f9959d2c9540482977ff1208ea67fbfb8cc9
Gitweb: http://git.kernel.org/tip/ecd5f9959d2c9540482977ff1208ea67fbfb8cc9
Author: Taeung Song <[email protected]>
AuthorDate: Fri, 21 Jul 2017 11:38:48 -0300
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Fri, 21 Jul 2017 12:02:52 -0300

perf annotate: Do not overwrite sample->period

In fixing the --show-total-period option it was noticed that the value
of sample->period was being overwritten, fix it.

Signed-off-by: Taeung Song <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Milian Wolff <[email protected]>
Cc: Namhyung Kim <[email protected]>
Fixes: fd36f3dd7933 ("perf hist: Pass struct sample to __hists__add_entry()")
Link: http://lkml.kernel.org/r/[email protected]
[ split from a larger patch, added the Fixes tag ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/builtin-annotate.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c
index 96fe1a8..7e33278 100644
--- a/tools/perf/builtin-annotate.c
+++ b/tools/perf/builtin-annotate.c
@@ -177,7 +177,6 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel,
*/
process_branch_stack(sample->branch_stack, al, sample);

- sample->period = 1;
sample->weight = 1;

he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true);

2017-07-26 17:29:33

[permalink] [raw]

Subject: [tip:perf/core] perf annotate stdio: Fix column header when using --show-total-period

Commit-ID: 38d2dcd0ccf85b55d783edbfc14fd8dea4d55b73
Gitweb: http://git.kernel.org/tip/38d2dcd0ccf85b55d783edbfc14fd8dea4d55b73
Author: Taeung Song <[email protected]>
AuthorDate: Tue, 25 Jul 2017 06:28:42 +0900
Committer: Arnaldo Carvalho de Melo <[email protected]>
CommitDate: Tue, 25 Jul 2017 22:46:37 -0300

perf annotate stdio: Fix column header when using --show-total-period

Currently the first column header is always "Percent", fix it to show
correct column name based on given options, i.e. if using
--show-total-period, show "Event count" as a first column.

Reported-by: Milian Wolff <[email protected]>
Signed-off-by: Taeung Song <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Link: http://lkml.kernel.org/r/[email protected]
[ Extracted from a larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <[email protected]>
---
tools/perf/util/annotate.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index 004072f..c2b4b00 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1824,7 +1824,8 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
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->nr_samples);
+ width, width, symbol_conf.show_total_period ? "Event count" : "Percent",
+ d_filename, evsel_name, h->nr_samples);

printf("%-*.*s----\n",
graph_dotted_len, graph_dotted_len, graph_dotted_line);

2017-07-26 20:18:08

[permalink] [raw]

Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples

Em Wed, Jul 26, 2017 at 08:57:13PM +0900, Taeung Song escreveu:
> On 07/26/2017 01:17 AM, Arnaldo Carvalho de Melo wrote:
> > Em Wed, Jul 26, 2017 at 12:53:28AM +0900, Taeung Song escreveu:
> > > On 07/25/2017 11:42 PM, Arnaldo Carvalho de Melo wrote:
> > > > > Moreover there is the below case that is not aligned due to big period
> > > > > values.
> >
> > > > So, that "moreover" means its not just one patch, but at least two, i.e.
> > > > when one selects show-total-period we better have more space for that
> > > > column, right?
> > > I got it. will separate this patch.
> >
> > Ok, please continue your work from my perf/core branch that I just
> > pushed, in it the latest patch is this one:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git/commit/?h=perf/core&id=143e9656aec7c61b9b8e134da5abc5dfb6133cbf
> >
> > Which is a chunk of what you done below. More comments below.
>
> Yes sir, :)
> I fetched and checked it.
>
> > > > I'll break the patch below accordingly.
> > > >
> > > > And even then, there is one question left, see below
> > > >
> > > > > perf annotate --stdio -i milian.data --show-total-period
> > > > > Percent | Source code & Disassembly of test for cycles:ppp (1442
> > > > > samples)
> > > > > -------------------------------------------------------------------------------
> > > > > :
> > > > > :
> > > > > :
> > > > > : Disassembly of section .text:
> > > > > ...
> > > > > 0 : 40089d: pxor %xmm1,%xmm1
> > > > > 27288350 : 4008a1: cvtsi2sd %rsi,%xmm1
> > > > > 0 : 4008a6: pxor %xmm5,%xmm5
> > > > >
> > > > >
> > > > > So, I made a patch like below:
> > <SNIP>
> > > > > +++ b/tools/perf/util/annotate.c
> > > > > @@ -1142,7 +1142,7 @@ static int disasm_line__print(struct disasm_line *dl,
> > > > > struct symbol *sym, u64 st
> > > > > color = get_percent_color(percent);
> > > > >
> > > > > if (symbol_conf.show_total_period)
> > > > > - color_fprintf(stdout, color, " %7" PRIu64,
> > > > > + color_fprintf(stdout, color, " %11" PRIu64,
> > > > > sample.period);
> > > >
> > > > this part will be in a separate patch, i.e. something like:
> > > >
> > > > [PATCH] Widen "Period" column when using --show-total-period
> > > >
> > >
> > > ok.
> > >
> > > > > else
> > > > > color_fprintf(stdout, color, " %7.2f",
> > > > > percent);
> > > > > @@ -1173,6 +1173,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;
> > > > > +
> > > >
> > > > But what about this one? What is that '4' for? Not obvious at first
> > > > sight, can you elaborate on the need for this specific one?
> > > >
> > >
> > > Yep, if you check the above code lines, like below:
> > >
> > > color_fprintf(stdout, color, " %11" PRIu64,
> > > sample.period);
> > >
> > > The above number of letters is 12
> > > i.e. 12 = 1 (" ": white space) + 11 (digits of sample.period)
> > >
> > > So, I used '4', because the 'width' variable is initialized as '8'.
> >
> > Think that I am 7 years old :o) I'm still not understanding this
> > logic...

> Humm.. first of all, we can check the 'width' variable in two function
> disasm_line__print() and symbol__annotate_printf() like below:
>
> 1063 static int disasm_line__print(struct disasm_line *dl, struct symbol
> *sym, u64 start,
> 1064 struct perf_evsel *evsel, u64 len, int min_pcnt,
> int printed,
> 1065 int max_lines, struct disasm_line *queue)
> 1066 {
>
> ...
> 1167 else {
> 1168 int width = 8;
> 1169
> 1170 if (queue)
> 1171 return -1;
> 1172
> 1173 if (perf_evsel__is_group_event(evsel))
> 1174 width *= evsel->nr_members;
> 1175
> 1176 if (!*dl->line)
> 1177 printf(" %*s:\n", width, " ");
> 1178 else
> 1179 printf(" %*s: %s\n", width, " ", dl->line);
>
>
> And,
>
> 1794 int symbol__annotate_printf(struct symbol *sym, struct map *map,
> 1795 struct perf_evsel *evsel, bool full_paths,
> 1796 int min_pcnt, int max_lines, int context)
> 1797 {
> ...
>
> 1809 int width = 8;
> ...
> 1823 if (perf_evsel__is_group_event(evsel))
> 1824 width *= evsel->nr_members;
> 1825
> 1826 graph_dotted_len = printf(" %-*.*s| Source code &
> Disassembly of %s for %s (%" PRIu64 " samples)\n",
> 1827 width, width,
> symbol_conf.show_total_period ? "Event count" : "Percent",
> 1828 d_filename, evsel_name,
> h->nr_samples);
>
> As you can see, currently the 'width' variables are set as 8 letters
> But I adjust the width as 12 letters for the first column " Event count"
> and period value.
>
> So I do witdh += 4 for 12 letters like below:

Why not fix the initialization of width? I.e.:

diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
index c2b4b00166ed..cc0bf0c1489b 100644
--- a/tools/perf/util/annotate.c
+++ b/tools/perf/util/annotate.c
@@ -1165,7 +1165,7 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
} else if (max_lines && printed >= max_lines)
return 1;
else {
- int width = 8;
+ int width = symbol_conf.show_total_period ? 12 : 8;

if (queue)
return -1;
@@ -1806,7 +1806,7 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
int printed = 2, queue_len = 0;
int more = 0;
u64 len;
- int width = 8;
+ int width = symbol_conf.show_total_period ? 12 : 8;
int graph_dotted_len;

filename = strdup(dso->long_name);

-----------------

the s/7/11/ case is ok, as it is always branching on
symbol_conf.show_total_period.

> $ perf annotate --stdio --show-total-period -i hex2u64
> Event count | Source code & Disassembly of old for cycles:ppp (102
> samples)
> ---------------------------------------------------------------------------------
> :
> :
> :
> : Disassembly of section .text:
> :
> : 0000000000400816 <get_cond_maxprice>:
> : get_cond_maxprice():
> 1950346 : 400816: push %rbp
> 741848 : 400817: mov %rsp,%rbp
>
> We don't need to adjust the 'width' for --show-total-period ?
>
> > > Additionally this patch handle the width for group event like below:
> > >
> > > $ perf annotate --show-total-period -i group_events.data --stdio
> > > Event count | Source code & Disassembly of old for
> > > cycles (529 samples)
> > > -----------------------------------------------------------------------------------------------------
> > > :
> > > :
> > > :
> > > : Disassembly of section .text:
> > > :
> > > : 0000000000400816
> > > <get_cond_maxprice>:
> > > : get_cond_maxprice():
> > > 0 0 7144 : 400816: push %rbp
> > > 3480988 0 5709 : 400817: mov %rsp,%rbp
> > > 0 0 7522 : 40081a: mov %edi,-0x24(%rbp)
> > >
> > >
> > > Sorry, I repeatedly failed to adjust a proper patch unit..
> > > I'll remake this patches based on your comment,
> > > and resend next patchset !
> >
> > It is not a problem, you're making progress, thanks for taking into
> > accoutn my comments.
> >
> > The end result may be the same, but having a good patch granularity is
> > fundamental for bisecting, also for maintainers to cherry-pick parts of
> > your work that they agree on while making comments about parts that
> > looks wrong or needing some more work.
>
> Thanks for your advice !!
>
> - Taeung

2017-07-27 15:15:05