Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754128AbdGUJlj (ORCPT ); Fri, 21 Jul 2017 05:41:39 -0400 Received: from mail-pf0-f195.google.com ([209.85.192.195]:33244 "EHLO mail-pf0-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754056AbdGUJld (ORCPT ); Fri, 21 Jul 2017 05:41:33 -0400 Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples To: Arnaldo Carvalho de Melo Cc: linux-kernel@vger.kernel.org, Namhyung Kim , Milian Wolff , Jiri Olsa References: <1500500215-16646-1-git-send-email-treeze.taeung@gmail.com> <20170720191934.GD4134@kernel.org> From: Taeung Song Message-ID: <51c3c55a-d43d-427f-53fb-56b3ab275dd9@gmail.com> Date: Fri, 21 Jul 2017 18:41:29 +0900 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <20170720191934.GD4134@kernel.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 11110 Lines: 295 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(): >> 1 : 400816: push %rbp >> >> After: >> >> Percent | Source code & Disassembly of old for cycles:ppp (84813704 event count) >> ---------------------------------------------------------------------------------- >> : >> : >> : >> : Disassembly of section .text: >> : >> : 0000000000400816 : >> : knapsack(): >> 743737 : 400816: push %rbp >> >> Reported-by: Namhyung Kim >> Cc: Milian Wolff >> Cc: Jiri Olsa >> Signed-off-by: Taeung Song >> --- >> 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