Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751208AbdGMHXN (ORCPT ); Thu, 13 Jul 2017 03:23:13 -0400 Received: from mail-pg0-f65.google.com ([74.125.83.65]:34115 "EHLO mail-pg0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750969AbdGMHXM (ORCPT ); Thu, 13 Jul 2017 03:23:12 -0400 Subject: Re: [PATCH 1/4] 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: <1499811244-9261-1-git-send-email-treeze.taeung@gmail.com> <20170712200910.GQ27350@kernel.org> From: Taeung Song Message-ID: Date: Thu, 13 Jul 2017 16:23:07 +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: <20170712200910.GQ27350@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: 18450 Lines: 500 Hi Arnaldo :) On 07/13/2017 05:09 AM, Arnaldo Carvalho de Melo wrote: > Em Wed, Jul 12, 2017 at 07:14:04AM +0900, Taeung Song escreveu: >> Currently the --show-total-period option of perf-annotate >> is different from perf-report's. >> >> For example, perf-report ordinarily shows period and number of samples. >> >> # Overhead Samples Period Command Shared Object Symbol >> # ........ ............ ............ ....... .............. ............ >> # >> 9.83% 102 84813704 test test [.] knapsack >> >> But --show-total-period of perf-annotate has two problem like below: >> >> 1) Wrong column i.e. 'Percent' >> 2) Show number of samples, not period >> >> So fix this option to show period instead of number of samples. > > so you have multiple bugs here, please fix one per patch, i.e. if using > --show-total-period the header should not be "Percent". Okey, I'll separate this patch. > > Then, you need a patch to introduce that "struct sym_sample" and use it, > but please rename it to "struct sym_hist_entry". > I got it. > You can do it and just update the sym_hist_entry->period field, before > the change to pass 'struct sample' around. > Understood. > That disasm__calc_percent() function could receive a pointer to a struct > sym_hist_entry instead of two pointers. > Okey. > Small, self contained patches that do one thing make reviewing easier, > by yourself and others. > Yep. > Please give this a try, if you didn't understand something here I can do > it for you, as this needs doing to fix real bugs. > > Thanks, > > - Arnaldo > I understood what you said! I'll send v2 in accordance with your comment. Thanks, Taeung >> Before: >> >> Percent | Source code & Disassembly of old for cycles:ppp (102 samples) >> ----------------------------------------------------------------------------- >> : >> : >> : >> : Disassembly of section .text: >> : >> : 0000000000400816 : >> : knapsack(): >> 1 : 400816: push %rbp >> >> After: >> >> Event count | Source code & Disassembly of old for cycles:ppp (84813704 event count) >> -------------------------------------------------------------------------------------- >> : >> : >> : >> : Disassembly of section .text: >> : >> : 0000000000400816 : >> : knapsack(): >> 743737 : 400816: push %rbp >> >> Cc: 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/ui/browsers/annotate.c | 4 +- >> tools/perf/ui/gtk/annotate.c | 4 +- >> tools/perf/util/annotate.c | 92 +++++++++++++++++++++++++++++---------- >> tools/perf/util/annotate.h | 12 +++-- >> 7 files changed, 96 insertions(+), 39 deletions(-) >> >> diff --git a/tools/perf/builtin-annotate.c b/tools/perf/builtin-annotate.c >> index 7a5dc7e..f314661 100644 >> --- a/tools/perf/builtin-annotate.c >> +++ b/tools/perf/builtin-annotate.c >> @@ -177,14 +177,12 @@ static int perf_evsel__add_sample(struct perf_evsel *evsel, >> */ >> process_branch_stack(sample->branch_stack, al, sample); >> >> - sample->period = 1; >> sample->weight = 1; >> - > > Please do not remove this line. > >> he = hists__add_entry(hists, al, NULL, NULL, NULL, sample, true); >> if (he == NULL) >> return -ENOMEM; >> >> - ret = hist_entry__inc_addr_samples(he, evsel->idx, al->addr); >> + ret = hist_entry__inc_addr_samples(he, sample, evsel->idx, al->addr); >> hists__inc_nr_samples(hists, true); >> return ret; >> } >> diff --git a/tools/perf/builtin-report.c b/tools/perf/builtin-report.c >> index 79a33eb..5f1894c 100644 >> --- a/tools/perf/builtin-report.c >> +++ b/tools/perf/builtin-report.c >> @@ -113,13 +113,14 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter, >> struct report *rep = arg; >> struct hist_entry *he = iter->he; >> struct perf_evsel *evsel = iter->evsel; >> + struct perf_sample *sample = iter->sample; >> struct mem_info *mi; >> struct branch_info *bi; >> >> if (!ui__has_annotation()) >> return 0; >> >> - hist__account_cycles(iter->sample->branch_stack, al, iter->sample, >> + hist__account_cycles(sample->branch_stack, al, sample, >> rep->nonany_branch_mode); >> >> if (sort__mode == SORT_MODE__BRANCH) { >> @@ -136,14 +137,16 @@ static int hist_iter__report_callback(struct hist_entry_iter *iter, >> if (err) >> goto out; >> >> - err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr); >> + err = hist_entry__inc_addr_samples(he, sample, >> + evsel->idx, al->addr); > > why break this into two lines? > >> >> } else if (symbol_conf.cumulate_callchain) { >> if (single) >> - err = hist_entry__inc_addr_samples(he, evsel->idx, >> - al->addr); >> + err = hist_entry__inc_addr_samples(he, sample, >> + evsel->idx, al->addr); >> } else { >> - err = hist_entry__inc_addr_samples(he, evsel->idx, al->addr); >> + err = hist_entry__inc_addr_samples(he, sample, >> + evsel->idx, al->addr); >> } >> >> out: >> diff --git a/tools/perf/builtin-top.c b/tools/perf/builtin-top.c >> index 022486d..09885a4 100644 >> --- a/tools/perf/builtin-top.c >> +++ b/tools/perf/builtin-top.c >> @@ -183,6 +183,7 @@ static void ui__warn_map_erange(struct map *map, struct symbol *sym, u64 ip) >> >> static void perf_top__record_precise_ip(struct perf_top *top, >> struct hist_entry *he, >> + struct perf_sample *sample, >> int counter, u64 ip) >> { >> struct annotation *notes; >> @@ -199,7 +200,7 @@ static void perf_top__record_precise_ip(struct perf_top *top, >> if (pthread_mutex_trylock(¬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/ui/browsers/annotate.c b/tools/perf/ui/browsers/annotate.c >> index b376637..73e5ae2 100644 >> --- a/tools/perf/ui/browsers/annotate.c >> +++ b/tools/perf/ui/browsers/annotate.c >> @@ -449,13 +449,13 @@ static void annotate_browser__calc_percent(struct annotate_browser *browser, >> next = disasm__get_next_ip_line(¬es->src->source, pos); >> >> for (i = 0; i < browser->nr_events; i++) { >> - u64 nr_samples; >> + u64 nr_samples, period; >> >> bpos->samples[i].percent = disasm__calc_percent(notes, >> evsel->idx + i, >> pos->offset, >> next ? next->offset : len, >> - &path, &nr_samples); >> + &path, &nr_samples, &period); >> bpos->samples[i].nr = nr_samples; >> >> if (max_percent < bpos->samples[i].percent) >> diff --git a/tools/perf/ui/gtk/annotate.c b/tools/perf/ui/gtk/annotate.c >> index 87e3760..d736fd5 100644 >> --- a/tools/perf/ui/gtk/annotate.c >> +++ b/tools/perf/ui/gtk/annotate.c >> @@ -34,10 +34,10 @@ static int perf_gtk__get_percent(char *buf, size_t size, struct symbol *sym, >> return 0; >> >> symhist = annotation__histogram(symbol__annotation(sym), evidx); >> - if (!symbol_conf.event_group && !symhist->addr[dl->offset]) >> + if (!symbol_conf.event_group && !symhist->addr[dl->offset].nr_samples) >> return 0; >> >> - percent = 100.0 * symhist->addr[dl->offset] / symhist->sum; >> + percent = 100.0 * symhist->addr[dl->offset].nr_samples / symhist->sum; >> >> markup = perf_gtk__get_percent_color(percent); >> if (markup) >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c >> index ef434b5..f7aeb5f 100644 >> --- a/tools/perf/util/annotate.c >> +++ b/tools/perf/util/annotate.c >> @@ -610,10 +610,10 @@ int symbol__alloc_hist(struct symbol *sym) >> size_t sizeof_sym_hist; >> >> /* Check for overflow when calculating sizeof_sym_hist */ >> - if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(u64)) >> + if (size > (SIZE_MAX - sizeof(struct sym_hist)) / sizeof(struct sym_sample)) >> return -1; >> >> - sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(u64)); >> + sizeof_sym_hist = (sizeof(struct sym_hist) + size * sizeof(struct sym_sample)); >> >> /* Check for overflow in zalloc argument */ >> if (sizeof_sym_hist > (SIZE_MAX - sizeof(*notes->src)) >> @@ -714,11 +714,11 @@ static int __symbol__inc_addr_samples(struct symbol *sym, struct map *map, >> offset = addr - sym->start; >> h = annotation__histogram(notes, evidx); >> h->sum++; >> - h->addr[offset]++; >> + h->addr[offset].nr_samples++; >> >> pr_debug3("%#" PRIx64 " %s: period++ [addr: %#" PRIx64 ", %#" PRIx64 >> ", evidx=%d] => %" PRIu64 "\n", sym->start, sym->name, >> - addr, addr - sym->start, evidx, h->addr[offset]); >> + addr, addr - sym->start, evidx, h->addr[offset].nr_samples); >> return 0; >> } >> >> @@ -816,9 +816,33 @@ int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx) >> return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr); >> } >> >> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip) >> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample, >> + int evidx, u64 addr) >> { >> - return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip); >> + struct symbol *sym = he->ms.sym; >> + struct annotation *notes; >> + struct sym_hist *h; >> + s64 offset; >> + >> + if (sym == NULL) >> + return 0; >> + >> + notes = symbol__get_annotation(sym, false); >> + if (notes == NULL) >> + return -ENOMEM; >> + >> + if ((addr < sym->start || addr >= sym->end) && >> + (addr != sym->end || sym->start != sym->end)) >> + return -ERANGE; >> + >> + offset = addr - sym->start; >> + h = annotation__histogram(notes, evidx); >> + h->sum++; >> + h->addr[offset].nr_samples++; >> + h->total_period += sample->period; >> + h->addr[offset].period += sample->period; >> + >> + return 0; >> } >> >> static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map) >> @@ -928,11 +952,13 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa >> } >> >> double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset, >> - s64 end, const char **path, u64 *nr_samples) >> + s64 end, const char **path, u64 *nr_samples, u64 *period) >> { >> struct source_line *src_line = notes->src->lines; >> double percent = 0.0; >> + >> *nr_samples = 0; >> + *period = 0; >> >> if (src_line) { >> size_t sizeof_src_line = sizeof(*src_line) + >> @@ -952,12 +978,16 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset, >> } else { >> struct sym_hist *h = annotation__histogram(notes, evidx); >> unsigned int hits = 0; >> + unsigned int p = 0; >> >> - while (offset < end) >> - hits += h->addr[offset++]; >> + while (offset < end) { >> + hits += h->addr[offset].nr_samples; >> + p += h->addr[offset++].period; >> + } >> >> if (h->sum) { >> *nr_samples = hits; >> + *period = p; >> percent = 100.0 * hits / h->sum; >> } >> } >> @@ -1057,10 +1087,11 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> >> if (dl->offset != -1) { >> const char *path = NULL; >> - u64 nr_samples; >> + u64 nr_samples, period; >> double percent, max_percent = 0.0; >> double *ppercents = &percent; >> u64 *psamples = &nr_samples; >> + u64 *pperiod = . >> int i, nr_percent = 1; >> const char *color; >> struct annotation *notes = symbol__annotation(sym); >> @@ -1075,9 +1106,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> nr_percent = evsel->nr_members; >> ppercents = calloc(nr_percent, sizeof(double)); >> psamples = calloc(nr_percent, sizeof(u64)); >> - if (ppercents == NULL || psamples == NULL) { >> + pperiod = calloc(nr_percent, sizeof(u64)); >> + if (ppercents == NULL || psamples == NULL || pperiod == NULL) >> return -1; >> - } >> } >> >> for (i = 0; i < nr_percent; i++) { >> @@ -1085,10 +1116,11 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> notes->src->lines ? i : evsel->idx + i, >> offset, >> next ? next->offset : (s64) len, >> - &path, &nr_samples); >> + &path, &nr_samples, &period); >> >> ppercents[i] = percent; >> psamples[i] = nr_samples; >> + pperiod[i] = period; >> if (percent > max_percent) >> max_percent = percent; >> } >> @@ -1127,11 +1159,12 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> for (i = 0; i < nr_percent; i++) { >> percent = ppercents[i]; >> nr_samples = psamples[i]; >> + period = pperiod[i]; >> color = get_percent_color(percent); >> >> if (symbol_conf.show_total_period) >> - color_fprintf(stdout, color, " %7" PRIu64, >> - nr_samples); >> + color_fprintf(stdout, color, " %11" PRIu64, >> + period); >> else >> color_fprintf(stdout, color, " %7.2f", percent); >> } >> @@ -1150,6 +1183,9 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> if (psamples != &nr_samples) >> free(psamples); >> >> + if (pperiod != &period) >> + free(pperiod); >> + >> } else if (max_lines && printed >= max_lines) >> return 1; >> else { >> @@ -1161,6 +1197,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st >> if (perf_evsel__is_group_event(evsel)) >> width *= evsel->nr_members; >> >> + if (symbol_conf.show_total_period) >> + width += perf_evsel__is_group_event(evsel) ? >> + 4 * evsel->nr_members : 4; >> + >> if (!*dl->line) >> printf(" %*s:\n", width, " "); >> else >> @@ -1702,7 +1742,7 @@ static int symbol__get_source_line(struct symbol *sym, struct map *map, >> double percent = 0.0; >> >> h = annotation__histogram(notes, evidx + k); >> - nr_samples = h->addr[i]; >> + nr_samples = h->addr[i].nr_samples; >> if (h->sum) >> percent = 100.0 * nr_samples / h->sum; >> >> @@ -1773,9 +1813,9 @@ static void symbol__annotate_hits(struct symbol *sym, struct perf_evsel *evsel) >> u64 len = symbol__size(sym), offset; >> >> for (offset = 0; offset < len; ++offset) >> - if (h->addr[offset] != 0) >> + if (h->addr[offset].nr_samples != 0) >> printf("%*" PRIx64 ": %" PRIu64 "\n", BITS_PER_LONG / 2, >> - sym->start + offset, h->addr[offset]); >> + sym->start + offset, h->addr[offset].nr_samples); >> printf("%*s: %" PRIu64 "\n", BITS_PER_LONG / 2, "h->sum", h->sum); >> } >> >> @@ -1811,8 +1851,16 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map, >> if (perf_evsel__is_group_event(evsel)) >> width *= evsel->nr_members; >> >> - graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n", >> - width, width, "Percent", d_filename, evsel_name, h->sum); >> + if (symbol_conf.show_total_period) >> + width += perf_evsel__is_group_event(evsel) ? >> + 4 * evsel->nr_members : 4; >> + >> + graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " %s)\n", >> + width, width, >> + symbol_conf.show_total_period ? "Event count" : "Percent", >> + d_filename, evsel_name, >> + symbol_conf.show_total_period ? h->total_period : h->sum, >> + symbol_conf.show_total_period ? "event count" : "samples"); >> >> printf("%-*.*s----\n", >> graph_dotted_len, graph_dotted_len, graph_dotted_line); >> @@ -1878,8 +1926,8 @@ void symbol__annotate_decay_histogram(struct symbol *sym, int evidx) >> >> h->sum = 0; >> for (offset = 0; offset < len; ++offset) { >> - h->addr[offset] = h->addr[offset] * 7 / 8; >> - h->sum += h->addr[offset]; >> + h->addr[offset].nr_samples = h->addr[offset].nr_samples * 7 / 8; >> + h->sum += h->addr[offset].nr_samples; >> } >> } >> >> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h >> index bac698d..6b2e645 100644 >> --- a/tools/perf/util/annotate.h >> +++ b/tools/perf/util/annotate.h >> @@ -79,11 +79,16 @@ struct disasm_line *disasm__get_next_ip_line(struct list_head *head, struct disa >> int disasm_line__scnprintf(struct disasm_line *dl, char *bf, size_t size, bool raw); >> size_t disasm__fprintf(struct list_head *head, FILE *fp); >> double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset, >> - s64 end, const char **path, u64 *nr_samples); >> + s64 end, const char **path, u64 *nr_samples, u64 *period); >> +struct sym_sample { >> + u64 nr_samples; >> + u64 period; >> +}; >> >> struct sym_hist { >> u64 sum; >> - u64 addr[0]; >> + u64 total_period; >> + struct sym_sample addr[0]; >> }; >> >> struct cyc_hist { >> @@ -155,7 +160,8 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams, >> struct addr_map_symbol *start, >> unsigned cycles); >> >> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr); >> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample, >> + int evidx, u64 addr); >> >> int symbol__alloc_hist(struct symbol *sym); >> void symbol__annotate_zero_histograms(struct symbol *sym); >> -- >> 2.7.4