| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <51c3c55a-d43d-427f-53fb-56b3ab275dd9@gmail.com>
Date: Fri, 21 Jul 2017 18:41:29 +0900
From: Taeung Song <treeze.taeung@...il.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: linux-kernel@...r.kernel.org, Namhyung Kim <namhyung@...nel.org>,
Milian Wolff <milian.wolff@...b.com>,
Jiri Olsa <jolsa@...hat.com>
Subject: Re: [PATCH v3 3/9] perf annotate: Fix wrong --show-total-period
option showing number of samples
On 07/21/2017 04:19 AM, Arnaldo Carvalho de Melo wrote:
> Em Thu, Jul 20, 2017 at 06:36:55AM +0900, Taeung Song escreveu:
>> Currently the --show-total-period option of perf-annotate
>> is different from perf-report's.
>>
>> For example, perf-report ordinarily shows period and number of samples.
>>
>> # Overhead Samples Period Command Shared Object Symbol
>> # ........ ............ ............ ....... .............. ............
>> #
>> 9.83% 102 84813704 test test [.] knapsack
>
> But this is not what this patch does, it is still doing too many things,
> it should first add sample to those function signatures, leaving the
> "meat" to a followup patch, where we will not be distracted with
> infrastructure needed to do what you describe in the changelog.
>
> I'm doing it here this time, please look at the result, later.
>
> - Arnaldo
>
ok, I'm waiting for it.
And if you give me some sketchy code, that's fine.
If you do, I'll remake this patch based on the result. :)
Thanks,
Taeung
>> But --show-total-period of perf-annotate has the problem that show
>> number of samples, not period.
>> So fix this option to show period instead of number of samples.
>>
>> Before:
>>
>> Percent | Source code & Disassembly of old for cycles:ppp (102 samples)
>> -----------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> :
>> : 0000000000400816 <knapsack>:
>> : knapsack():
>> 1 : 400816: push %rbp
>>
>> After:
>>
>> Percent | Source code & Disassembly of old for cycles:ppp (84813704 event count)
>> ----------------------------------------------------------------------------------
>> :
>> :
>> :
>> : Disassembly of section .text:
>> :
>> : 0000000000400816 <knapsack>:
>> : knapsack():
>> 743737 : 400816: push %rbp
>>
>> Reported-by: Namhyung Kim <namhyung@...nel.org>
>> Cc: Milian Wolff <milian.wolff@...b.com>
>> Cc: Jiri Olsa <jolsa@...hat.com>
>> Signed-off-by: Taeung Song <treeze.taeung@...il.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
Powered by blists - more mailing lists