From: Adrian Hunter <adrian.hunter@intel.com>
To: Changbin Du <changbin.du@huawei.com>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Jiri Olsa <jolsa@kernel.org>, Namhyung Kim <namhyung@kernel.org>,
Ian Rogers <irogers@google.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org,
Hui Wang <hw.huiwang@huawei.com>
Subject: Re: [PATCH] perf: script: add new output field 'dsoip'
Date: Mon, 3 Apr 2023 11:54:48 +0300 [thread overview]
Message-ID: <3ae7d0d9-32c2-3a5f-4f5a-4ba974ebef2b@intel.com> (raw)
In-Reply-To: <f2eb0299-05cf-72f5-4bb1-c09ebb0a55a1@intel.com>
On 3/04/23 08:28, Adrian Hunter wrote:
> On 31/03/23 11:44, Changbin Du wrote:
>> The goal of this change is to achieve consistent output for address. Before
>> this, the raw ip is printed for non-callchain and binary offset for
>> callchain. Mostly what we expect is the raw ip.
>>
>> This patch does two changes:
>> - Always print raw ip for symbols.
>> - Add a new 'dsoip' field if we really need the binary offset, and the
>
> IP stands for instruction pointer, which is always a memory address,
> so 'dsoip' does not seem like a good name. What about 'dso_offset'?
>
>> offset is appended to dso name.
>>
>> Default output now:
>> $ perf script
>> perf-exec 171459 5380927.621499: 1 cycles: ffffffff8c66e1b8 [unknown] ([unknown])
>> perf-exec 171459 5380927.621524: 1 cycles: ffffffff8c66e1b8 [unknown] ([unknown])
>> perf-exec 171459 5380927.621527: 2 cycles: ffffffff8c66e1b8 [unknown] ([unknown])
>> perf-exec 171459 5380927.621530: 17 cycles: ffffffff8c66e1b8 [unknown] ([unknown])
>> perf-exec 171459 5380927.621533: 194 cycles: ffffffff8c66e1bf [unknown] ([unknown])
>> perf-exec 171459 5380927.621536: 2153 cycles: ffffffff8c6375fe [unknown] ([unknown])
>> perf-exec 171459 5380927.621540: 23135 cycles: ffffffff8d001ac4 [unknown] ([unknown])
>> perf-exec 171459 5380927.621555: 224703 cycles: ffffffff8c7a5153 [unknown] ([unknown])
>> ls 171459 5380927.621686: 587397 cycles: ffffffff8c7c43de [unknown] ([unknown])
>> ls 171459 5380927.622016: 653693 cycles: 7fe1e47278c7 _dl_map_object_deps+0x307 (/lib/x86_64-linux-gnu/ld-2.27.so)
>> ls 171459 5380927.622382: 633896 cycles: 7fe1e41bd0d0 __wmemchr_ifunc+0x0 (/lib/x86_64-linux-gnu/libc-2.27.so)
>> ls 171459 5380927.622737: 608856 cycles: 7fe1e4194594 _int_malloc+0x264 (/lib/x86_64-linux-gnu/libc-2.27.so)
>> ls 171459 5380927.623078: 586376 cycles: 55ff5293d27c [unknown] (/bin/ls)
>> ls 171459 5380927.623405: 566812 cycles: 55ff5293b202 [unknown] (/bin/ls)
>> ls 171459 5380927.623723: 549960 cycles: ffffffff8c82b138 [unknown] ([unknown])
>>
>> Display 'dsoip' field:
>> $ perf script -F +dsoip
>> perf-exec 171459 5380927.621499: 1 cycles: ffffffff8c66e1b8 [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621524: 1 cycles: ffffffff8c66e1b8 [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621527: 2 cycles: ffffffff8c66e1b8 [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621530: 17 cycles: ffffffff8c66e1b8 [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621533: 194 cycles: ffffffff8c66e1bf [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621536: 2153 cycles: ffffffff8c6375fe [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621540: 23135 cycles: ffffffff8d001ac4 [unknown] ([unknown]+0x0)
>> perf-exec 171459 5380927.621555: 224703 cycles: ffffffff8c7a5153 [unknown] ([unknown]+0x0)
>> ls 171459 5380927.621686: 587397 cycles: ffffffff8c7c43de [unknown] ([unknown]+0x0)
>> ls 171459 5380927.622016: 653693 cycles: 7fe1e47278c7 _dl_map_object_deps+0x307 (/lib/x86_64-linux-gnu/ld-2.27.so+0xe8c7)
>> ls 171459 5380927.622382: 633896 cycles: 7fe1e41bd0d0 __wmemchr_ifunc+0x0 (/lib/x86_64-linux-gnu/libc-2.27.so+0xbd0d0)
>> ls 171459 5380927.622737: 608856 cycles: 7fe1e4194594 _int_malloc+0x264 (/lib/x86_64-linux-gnu/libc-2.27.so+0x94594)
>> ls 171459 5380927.623078: 586376 cycles: 55ff5293d27c [unknown] (/bin/ls+0x1227c)
>> ls 171459 5380927.623405: 566812 cycles: 55ff5293b202 [unknown] (/bin/ls+0x10202)
>> ls 171459 5380927.623723: 549960 cycles: ffffffff8c82b138 [unknown] ([unknown]+0x0)
>>
>> Signed-off-by: Changbin Du <changbin.du@huawei.com>
>> ---
>> tools/perf/Documentation/perf-script.txt | 2 +-
>> tools/perf/builtin-script.c | 5 +++++
>> tools/perf/util/evsel_fprintf.c | 26 ++++++++++++------------
>> tools/perf/util/evsel_fprintf.h | 13 ++++++------
>> 4 files changed, 26 insertions(+), 20 deletions(-)
>>
>> diff --git a/tools/perf/Documentation/perf-script.txt b/tools/perf/Documentation/perf-script.txt
>> index 777a0d8ba7d1..ef392785f776 100644
>> --- a/tools/perf/Documentation/perf-script.txt
>> +++ b/tools/perf/Documentation/perf-script.txt
>> @@ -130,7 +130,7 @@ OPTIONS
>> -F::
>> --fields::
>> Comma separated list of fields to print. Options are:
>> - comm, tid, pid, time, cpu, event, trace, ip, sym, dso, addr, symoff,
>> + comm, tid, pid, time, cpu, event, trace, ip, sym, dso, dsoip, addr, symoff,
>> srcline, period, iregs, uregs, brstack, brstacksym, flags, bpf-output,
>> brstackinsn, brstackinsnlen, brstackoff, callindent, insn, insnlen, synth,
>> phys_addr, metric, misc, srccode, ipc, data_page_size, code_page_size, ins_lat,
>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>> index a792214d1af8..a181decf7922 100644
>> --- a/tools/perf/builtin-script.c
>> +++ b/tools/perf/builtin-script.c
>> @@ -133,6 +133,7 @@ enum perf_output_field {
>> PERF_OUTPUT_VCPU = 1ULL << 38,
>> PERF_OUTPUT_CGROUP = 1ULL << 39,
>> PERF_OUTPUT_RETIRE_LAT = 1ULL << 40,
>> + PERF_OUTPUT_DSOIP = 1ULL << 41,
>> };
>>
>> struct perf_script {
>> @@ -174,6 +175,7 @@ struct output_option {
>> {.str = "ip", .field = PERF_OUTPUT_IP},
>> {.str = "sym", .field = PERF_OUTPUT_SYM},
>> {.str = "dso", .field = PERF_OUTPUT_DSO},
>> + {.str = "dsoip", .field = PERF_OUTPUT_DSOIP},
>> {.str = "addr", .field = PERF_OUTPUT_ADDR},
>> {.str = "symoff", .field = PERF_OUTPUT_SYMOFFSET},
>> {.str = "srcline", .field = PERF_OUTPUT_SRCLINE},
>> @@ -574,6 +576,9 @@ static void set_print_ip_opts(struct perf_event_attr *attr)
>> if (PRINT_FIELD(DSO))
>> output[type].print_ip_opts |= EVSEL__PRINT_DSO;
>>
>> + if (PRINT_FIELD(DSOIP))
>> + output[type].print_ip_opts |= EVSEL__PRINT_DSOIP;
>> +
>> if (PRINT_FIELD(SYMOFFSET))
>> output[type].print_ip_opts |= EVSEL__PRINT_SYMOFFSET;
>>
>> diff --git a/tools/perf/util/evsel_fprintf.c b/tools/perf/util/evsel_fprintf.c
>> index bd22c4932d10..fd1791fab051 100644
>> --- a/tools/perf/util/evsel_fprintf.c
>> +++ b/tools/perf/util/evsel_fprintf.c
>> @@ -116,6 +116,7 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>> int print_ip = print_opts & EVSEL__PRINT_IP;
>> int print_sym = print_opts & EVSEL__PRINT_SYM;
>> int print_dso = print_opts & EVSEL__PRINT_DSO;
>> + int print_dsoip = print_opts & EVSEL__PRINT_DSOIP;
>> int print_symoffset = print_opts & EVSEL__PRINT_SYMOFFSET;
>> int print_oneline = print_opts & EVSEL__PRINT_ONELINE;
>> int print_srcline = print_opts & EVSEL__PRINT_SRCLINE;
>> @@ -133,7 +134,7 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>> while (1) {
>> struct symbol *sym;
>> struct map *map;
>> - u64 addr = 0;
>> + u64 dso_ip = 0;
>
> Renaming 'addr' just makes the patch more complicated.
> Just leave it as 'addr'
>
>>
>> node = callchain_cursor_current(cursor);
>> if (!node)
>> @@ -141,6 +142,8 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>>
>> sym = node->ms.sym;
>> map = node->ms.map;
>> + if (map)
>> + dso_ip = map->map_ip(map, node->ip);
>
> There does not seem to be any reason to move these lines of code.
>
>>
>> if (sym && sym->ignore && print_skip_ignored)
>> goto next;
>> @@ -150,20 +153,12 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>> if (print_arrow && !first)
>> printed += fprintf(fp, " <-");
>>
>> - if (map)
>> - addr = map->map_ip(map, node->ip);
>> -
>> - if (print_ip) {
>> - /* Show binary offset for userspace addr */
>> - if (map && !map->dso->kernel)
>
> I would have expected to keep existing functionality unchanged i.e.
>
> - if (map && !map->dso->kernel)
> + if (!print_dsoip && map && !map->dso->kernel)
>
>> - printed += fprintf(fp, "%c%16" PRIx64, s, addr);
>> - else
>> - printed += fprintf(fp, "%c%16" PRIx64, s, node->ip);
>> - }
>> + if (print_ip)
>> + printed += fprintf(fp, "%c%16" PRIx64, s, node->ip);
>>
>> if (print_sym) {
>> printed += fprintf(fp, " ");
>> - node_al.addr = addr;
>> + node_al.addr = dso_ip;
>> node_al.map = map;
>>
>> if (print_symoffset) {
>> @@ -179,11 +174,13 @@ int sample__fprintf_callchain(struct perf_sample *sample, int left_alignment,
>> if (print_dso && (!sym || !sym->inlined)) {
>> printed += fprintf(fp, " (");
>> printed += map__fprintf_dsoname(map, fp);
>> + if (print_dsoip)
>
> Better to avoid '[unknown]+0x0' i.e.
>
> if (print_dsoip && map && map->dso)
>
>> + printed += fprintf(fp, "+0x%lx", (map && map->dso) ? dso_ip : 0);
>> printed += fprintf(fp, ")");
>> }
>>
>> if (print_srcline)
>> - printed += map__fprintf_srcline(map, addr, "\n ", fp);
>> + printed += map__fprintf_srcline(map, dso_ip, "\n ", fp);
>>
>> if (sym && sym->inlined)
>> printed += fprintf(fp, " (inlined)");
>> @@ -214,6 +211,7 @@ int sample__fprintf_sym(struct perf_sample *sample, struct addr_location *al,
>> int print_ip = print_opts & EVSEL__PRINT_IP;
>> int print_sym = print_opts & EVSEL__PRINT_SYM;
>> int print_dso = print_opts & EVSEL__PRINT_DSO;
>> + int print_dsoip = print_opts & EVSEL__PRINT_DSOIP;
>> int print_symoffset = print_opts & EVSEL__PRINT_SYMOFFSET;
>> int print_srcline = print_opts & EVSEL__PRINT_SRCLINE;
>> int print_unknown_as_addr = print_opts & EVSEL__PRINT_UNKNOWN_AS_ADDR;
>> @@ -242,6 +240,8 @@ int sample__fprintf_sym(struct perf_sample *sample, struct addr_location *al,
>> if (print_dso) {
>> printed += fprintf(fp, " (");
>> printed += map__fprintf_dsoname(al->map, fp);
>> + if (print_dsoip)
>
> Better to avoid '[unknown]+0x0' i.e.
>
> if (print_dsoip && al->map && al->map->dso)
>
>
>> + printed += fprintf(fp, "+0x%lx", (al->map && al->map->dso) ? al->addr : 0);
>> printed += fprintf(fp, ")");
>> }
>>
>> diff --git a/tools/perf/util/evsel_fprintf.h b/tools/perf/util/evsel_fprintf.h
>> index 3093d096c29f..3f282c5840da 100644
>> --- a/tools/perf/util/evsel_fprintf.h
>> +++ b/tools/perf/util/evsel_fprintf.h
>> @@ -20,12 +20,13 @@ int evsel__fprintf(struct evsel *evsel, struct perf_attr_details *details, FILE
>> #define EVSEL__PRINT_IP (1<<0)
>> #define EVSEL__PRINT_SYM (1<<1)
>> #define EVSEL__PRINT_DSO (1<<2)
>> -#define EVSEL__PRINT_SYMOFFSET (1<<3)
>> -#define EVSEL__PRINT_ONELINE (1<<4)
>> -#define EVSEL__PRINT_SRCLINE (1<<5)
>> -#define EVSEL__PRINT_UNKNOWN_AS_ADDR (1<<6)
>> -#define EVSEL__PRINT_CALLCHAIN_ARROW (1<<7)
>> -#define EVSEL__PRINT_SKIP_IGNORED (1<<8)
>> +#define EVSEL__PRINT_DSOIP (1<<3)
>> +#define EVSEL__PRINT_SYMOFFSET (1<<4)
>> +#define EVSEL__PRINT_ONELINE (1<<5)
>> +#define EVSEL__PRINT_SRCLINE (1<<6)
>> +#define EVSEL__PRINT_UNKNOWN_AS_ADDR (1<<7)
>> +#define EVSEL__PRINT_CALLCHAIN_ARROW (1<<8)
>> +#define EVSEL__PRINT_SKIP_IGNORED (1<<9)
Also, do these really need to be renumbered? Why not just
put:
#define EVSEL__PRINT_DSOIP (1<<9)
>>
>> struct addr_location;
>> struct perf_event_attr;
>
next prev parent reply other threads:[~2023-04-03 8:54 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-03-31 8:44 [PATCH] perf: script: add new output field 'dsoip' Changbin Du
2023-04-03 5:28 ` Adrian Hunter
2023-04-03 8:54 ` Adrian Hunter [this message]
2023-04-04 2:26 ` Changbin Du
2023-04-04 6:38 ` Adrian Hunter
2023-04-04 6:54 ` Changbin Du
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=3ae7d0d9-32c2-3a5f-4f5a-4ba974ebef2b@intel.com \
--to=adrian.hunter@intel.com \
--cc=acme@kernel.org \
--cc=alexander.shishkin@linux.intel.com \
--cc=changbin.du@huawei.com \
--cc=hw.huiwang@huawei.com \
--cc=irogers@google.com \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).