lkml.org 
[lkml]   [2017]   [Jul]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period option showing number of samples
    From
    Date


    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(&notes->lock))
    >> return;
    >>
    >> - err = hist_entry__inc_addr_samples(he, counter, ip);
    >> + err = hist_entry__inc_addr_samples(he, sample, counter, ip);
    >>
    >> pthread_mutex_unlock(&notes->lock);
    >>
    >> @@ -671,7 +672,8 @@ static int hist_iter__top_callback(struct hist_entry_iter *iter,
    >> struct perf_evsel *evsel = iter->evsel;
    >>
    >> if (perf_hpp_list.sym && single)
    >> - perf_top__record_precise_ip(top, he, evsel->idx, al->addr);
    >> + perf_top__record_precise_ip(top, he, iter->sample,
    >> + evsel->idx, al->addr);
    >>
    >> hist__account_cycles(iter->sample->branch_stack, al, iter->sample,
    >> !(top->record_opts.branch_stack & PERF_SAMPLE_BRANCH_ANY));
    >> diff --git a/tools/perf/util/annotate.c b/tools/perf/util/annotate.c
    >> index a62067a..d4f1a0a 100644
    >> --- a/tools/perf/util/annotate.c
    >> +++ b/tools/perf/util/annotate.c
    >> @@ -816,9 +816,33 @@ int addr_map_symbol__inc_samples(struct addr_map_symbol *ams, int evidx)
    >> return symbol__inc_addr_samples(ams->sym, ams->map, evidx, ams->al_addr);
    >> }
    >>
    >> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 ip)
    >> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
    >> + int evidx, u64 addr)
    >> {
    >> - return symbol__inc_addr_samples(he->ms.sym, he->ms.map, evidx, ip);
    >> + struct symbol *sym = he->ms.sym;
    >> + struct annotation *notes;
    >> + struct sym_hist *h;
    >> + s64 offset;
    >> +
    >> + if (sym == NULL)
    >> + return 0;
    >> +
    >> + notes = symbol__get_annotation(sym, false);
    >> + if (notes == NULL)
    >> + return -ENOMEM;
    >> +
    >> + if ((addr < sym->start || addr >= sym->end) &&
    >> + (addr != sym->end || sym->start != sym->end))
    >> + return -ERANGE;
    >> +
    >> + offset = addr - sym->start;
    >> + h = annotation__histogram(notes, evidx);
    >> + h->total_samples++;
    >> + h->addr[offset].nr_samples++;
    >> + h->total_period += sample->period;
    >> + h->addr[offset].period += sample->period;
    >> +
    >> + return 0;
    >> }
    >>
    >> static void disasm_line__init_ins(struct disasm_line *dl, struct arch *arch, struct map *map)
    >> @@ -934,6 +958,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
    >> double percent = 0.0;
    >>
    >> sample->nr_samples = 0;
    >> + sample->period = 0;
    >>
    >> if (src_line) {
    >> size_t sizeof_src_line = sizeof(*src_line) +
    >> @@ -953,12 +978,16 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
    >> } else {
    >> struct sym_hist *h = annotation__histogram(notes, evidx);
    >> unsigned int hits = 0;
    >> + unsigned int p = 0;
    >>
    >> - while (offset < end)
    >> - hits += h->addr[offset++].nr_samples;
    >> + while (offset < end) {
    >> + hits += h->addr[offset].nr_samples;
    >> + p += h->addr[offset++].period;
    >> + }
    >>
    >> if (h->total_samples) {
    >> sample->nr_samples = hits;
    >> + sample->period = p;
    >> percent = 100.0 * hits / h->total_samples;
    >> }
    >> }
    >> @@ -1131,8 +1160,8 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
    >> color = get_percent_color(percent);
    >>
    >> if (symbol_conf.show_total_period)
    >> - color_fprintf(stdout, color, " %7" PRIu64,
    >> - sample.nr_samples);
    >> + color_fprintf(stdout, color, " %11" PRIu64,
    >> + sample.period);
    >> else
    >> color_fprintf(stdout, color, " %7.2f", percent);
    >> }
    >> @@ -1162,6 +1191,10 @@ static int disasm_line__print(struct disasm_line *dl, struct symbol *sym, u64 st
    >> if (perf_evsel__is_group_event(evsel))
    >> width *= evsel->nr_members;
    >>
    >> + if (symbol_conf.show_total_period)
    >> + width += perf_evsel__is_group_event(evsel) ?
    >> + 4 * evsel->nr_members : 4;
    >> +
    >> if (!*dl->line)
    >> printf(" %*s:\n", width, " ");
    >> else
    >> @@ -1812,8 +1845,14 @@ int symbol__annotate_printf(struct symbol *sym, struct map *map,
    >> if (perf_evsel__is_group_event(evsel))
    >> width *= evsel->nr_members;
    >>
    >> - graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " samples)\n",
    >> - width, width, "Percent", d_filename, evsel_name, h->total_samples);
    >> + if (symbol_conf.show_total_period)
    >> + width += perf_evsel__is_group_event(evsel) ?
    >> + 4 * evsel->nr_members : 4;
    >> +
    >> + graph_dotted_len = printf(" %-*.*s| Source code & Disassembly of %s for %s (%" PRIu64 " %s)\n",
    >> + width, width, "Percent", d_filename, evsel_name,
    >> + symbol_conf.show_total_period ? h->total_period : h->total_samples,
    >> + symbol_conf.show_total_period ? "event count" : "samples");
    >>
    >> printf("%-*.*s----\n",
    >> graph_dotted_len, graph_dotted_len, graph_dotted_line);
    >> diff --git a/tools/perf/util/annotate.h b/tools/perf/util/annotate.h
    >> index bef1d02..4406352 100644
    >> --- a/tools/perf/util/annotate.h
    >> +++ b/tools/perf/util/annotate.h
    >> @@ -88,6 +88,7 @@ double disasm__calc_percent(struct annotation *notes, int evidx, s64 offset,
    >>
    >> struct sym_hist {
    >> u64 total_samples;
    >> + u64 total_period;
    >> struct sym_hist_entry addr[0];
    >> };
    >>
    >> @@ -160,7 +161,8 @@ int addr_map_symbol__account_cycles(struct addr_map_symbol *ams,
    >> struct addr_map_symbol *start,
    >> unsigned cycles);
    >>
    >> -int hist_entry__inc_addr_samples(struct hist_entry *he, int evidx, u64 addr);
    >> +int hist_entry__inc_addr_samples(struct hist_entry *he, struct perf_sample *sample,
    >> + int evidx, u64 addr);
    >>
    >> int symbol__alloc_hist(struct symbol *sym);
    >> void symbol__annotate_zero_histograms(struct symbol *sym);
    >> --
    >> 2.7.4

    \
     
     \ /
      Last update: 2017-07-21 11:42    [W:4.071 / U:0.172 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site