Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1692220 > unrolled thread
| Started by | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| First post | 2017-07-19 23:40 +0200 |
| Last post | 2017-07-26 19:30 +0200 |
| Articles | 20 on this page of 23 — 5 participants |
Back to article view | Back to linux.kernel
[PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-19 23:40 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-20 21:20 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-21 11:50 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-21 13:30 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-24 04:00 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-24 19:40 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-24 23:30 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-25 16:50 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-25 18:00 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-25 18:20 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-26 14:00 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-26 22:20 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Taeung Song <treeze.taeung@gmail.com> - 2017-07-27 17:20 +0200
[tip:perf/core] perf annotate stdio: Fix column header when using --show-total-period tip-bot for Taeung Song <tipbot@zytor.com> - 2017-07-26 19:30 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-21 17:00 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Namhyung Kim <namhyung@kernel.org> - 2017-07-23 00:50 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Andi Kleen <andi@firstfloor.org> - 2017-07-23 17:50 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-24 19:40 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Andi Kleen <andi@firstfloor.org> - 2017-07-24 19:50 +0200
Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-07-24 20:10 +0200
[tip:perf/core] perf annotate: Store the sample period in each histogram bucket tip-bot for Taeung Song <tipbot@zytor.com> - 2017-07-26 19:30 +0200
[tip:perf/core] perf annotate: Do not overwrite sample->period tip-bot for Taeung Song <tipbot@zytor.com> - 2017-07-26 19:30 +0200
[tip:perf/core] perf hists: Pass perf_sample to __symbol__inc_addr_samples() tip-bot for Taeung Song <tipbot@zytor.com> - 2017-07-26 19:30 +0200
Page 1 of 2 [1] 2 Next page →
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-19 23:40 +0200 |
| Subject | [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u54Rl-5Zi-47@gated-at.bofh.it> |
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 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 <namhyung@kernel.org>
Cc: Milian Wolff <milian.wolff@kdab.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
---
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(¬es->lock))
return;
- err = hist_entry__inc_addr_samples(he, counter, ip);
+ err = hist_entry__inc_addr_samples(he, sample, counter, ip);
pthread_mutex_unlock(¬es->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
[toc] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-20 21:20 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u5p9o-380-27@gated-at.bofh.it> |
| In reply to | #1692220 |
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 <namhyung@kernel.org>
> Cc: Milian Wolff <milian.wolff@kdab.com>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
> ---
> 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(¬es->lock))
> return;
>
> - err = hist_entry__inc_addr_samples(he, counter, ip);
> + err = hist_entry__inc_addr_samples(he, sample, counter, ip);
>
> pthread_mutex_unlock(¬es->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
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-21 11:50 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u5CJk-37O-21@gated-at.bofh.it> |
| In reply to | #1693184 |
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 <namhyung@kernel.org>
>> Cc: Milian Wolff <milian.wolff@kdab.com>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>> ---
>> 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(¬es->lock))
>> return;
>>
>> - err = hist_entry__inc_addr_samples(he, counter, ip);
>> + err = hist_entry__inc_addr_samples(he, sample, counter, ip);
>>
>> pthread_mutex_unlock(¬es->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
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-21 13:30 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u5Ei6-4ce-13@gated-at.bofh.it> |
| In reply to | #1693538 |
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
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-24 04:00 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6AP7-6Hr-3@gated-at.bofh.it> |
| In reply to | #1693622 |
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 <ak@linux.intel.com> Date: Fri Jul 21 12:25:57 2017 -0700 $ git show tmp_perf.core | head -3 commit 3331778eb08a50cec959da933c040bd7fbdde396 Author: Adrian Hunter <adrian.hunter@intel.com> Date: Thu Jul 31 09:00:56 2014 +0300 Thanks, Taeung
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-24 19:40 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6PuP-83g-51@gated-at.bofh.it> |
| In reply to | #1694398 |
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 <ak@linux.intel.com> > 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 <adrian.hunter@intel.com> > Date: Thu Jul 31 09:00:56 2014 +0300 > > > Thanks, > Taeung
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-24 23:30 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6T5o-25X-7@gated-at.bofh.it> |
| In reply to | #1694939 |
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 <ak@linux.intel.com>
>> 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 <treeze.taeung@gmail.com>
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 <milian.wolff@kdab.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Cc: Jiri Olsa <jolsa@redhat.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
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 <adrian.hunter@intel.com>
>> Date: Thu Jul 31 09:00:56 2014 +0300
>>
>>
>> Thanks,
>> Taeung
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-25 16:50 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u79jQ-420-3@gated-at.bofh.it> |
| In reply to | #1695111 |
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 <treeze.taeung@gmail.com>
> 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 <milian.wolff@kdab.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> Cc: Jiri Olsa <jolsa@redhat.com>
> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>
> 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
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-25 18:00 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u7apA-4Hw-21@gated-at.bofh.it> |
| In reply to | #1695798 |
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 <treeze.taeung@gmail.com>
>> 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 <milian.wolff@kdab.com>
>> Cc: Namhyung Kim <namhyung@kernel.org>
>> Cc: Jiri Olsa <jolsa@redhat.com>
>> Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
>>
>> 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
>
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-25 18:20 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u7aIV-539-11@gated-at.bofh.it> |
| In reply to | #1695897 |
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
> >
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-26 14:00 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u7t8R-8gu-3@gated-at.bofh.it> |
| In reply to | #1695926 |
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
>>>
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-26 22:20 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u7AWK-4Xd-13@gated-at.bofh.it> |
| In reply to | #1697015 |
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
[toc] | [prev] | [next] | [standalone]
| From | Taeung Song <treeze.taeung@gmail.com> |
|---|---|
| Date | 2017-07-27 17:20 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u7SJX-7Iw-1@gated-at.bofh.it> |
| In reply to | #1697545 |
On 07/27/2017 05:17 AM, Arnaldo Carvalho de Melo wrote:
> 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.
Understood !! :)
Thanks,
Taeung
>
>> $ 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
[toc] | [prev] | [next] | [standalone]
| From | tip-bot for Taeung Song <tipbot@zytor.com> |
|---|---|
| Date | 2017-07-26 19:30 +0200 |
| Subject | [tip:perf/core] perf annotate stdio: Fix column header when using --show-total-period |
| Message-ID | <u7yie-3eE-1@gated-at.bofh.it> |
| In reply to | #1695111 |
Commit-ID: 38d2dcd0ccf85b55d783edbfc14fd8dea4d55b73
Gitweb: http://git.kernel.org/tip/38d2dcd0ccf85b55d783edbfc14fd8dea4d55b73
Author: Taeung Song <treeze.taeung@gmail.com>
AuthorDate: Tue, 25 Jul 2017 06:28:42 +0900
Committer: Arnaldo Carvalho de Melo <acme@redhat.com>
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 <milian.wolff@kdab.com>
Signed-off-by: Taeung Song <treeze.taeung@gmail.com>
Cc: Jiri Olsa <jolsa@redhat.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Link: http://lkml.kernel.org/r/c3c902e7-95bc-16d4-366f-12eb034c5c8d@gmail.com
[ Extracted from a larger patch ]
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
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);
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-21 17:00 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u5Hzk-663-7@gated-at.bofh.it> |
| In reply to | #1692220 |
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 <treeze.taeung@gmail.com>
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 <jolsa@redhat.com>
Cc: Milian Wolff <milian.wolff@kdab.com>
Cc: Namhyung Kim <namhyung@kernel.org>
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 <acme@redhat.com>
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);
[toc] | [prev] | [next] | [standalone]
| From | Namhyung Kim <namhyung@kernel.org> |
|---|---|
| Date | 2017-07-23 00:50 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6bnH-7Nr-1@gated-at.bofh.it> |
| In reply to | #1693768 |
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 <treeze.taeung@gmail.com>
> 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 <jolsa@redhat.com>
> Cc: Milian Wolff <milian.wolff@kdab.com>
> Cc: Namhyung Kim <namhyung@kernel.org>
> 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 <acme@redhat.com>
>
> 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);
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <andi@firstfloor.org> |
|---|---|
| Date | 2017-07-23 17:50 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6riN-Qt-3@gated-at.bofh.it> |
| In reply to | #1694220 |
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
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-24 19:40 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6PuP-83g-39@gated-at.bofh.it> |
| In reply to | #1694326 |
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
[toc] | [prev] | [next] | [standalone]
| From | Andi Kleen <andi@firstfloor.org> |
|---|---|
| Date | 2017-07-24 19:50 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6PEu-87k-11@gated-at.bofh.it> |
| In reply to | #1694934 |
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
[toc] | [prev] | [next] | [standalone]
| From | Arnaldo Carvalho de Melo <acme@kernel.org> |
|---|---|
| Date | 2017-07-24 20:10 +0200 |
| Subject | Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples |
| Message-ID | <u6PXP-8ue-5@gated-at.bofh.it> |
| In reply to | #1694947 |
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
[toc] | [prev] | [next] | [standalone]
Page 1 of 2 [1] 2 Next page →
Back to top | Article view | linux.kernel
csiph-web