| 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: <c3c59d7f-ead6-ae8d-d7d5-2842406048fd@intel.com>
Date: Mon, 24 Apr 2023 08:25:16 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Changbin Du <changbin.du@...wei.com>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>,
Namhyung Kim <namhyung@...nel.org>,
Ian Rogers <irogers@...gle.com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
Hui Wang <hw.huiwang@...wei.com>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>
Subject: Re: [PATCH v5 2/3] perf: add helper map__fprintf_dsoname_dsoff
On 23/04/23 07:32, Changbin Du wrote:
> On Fri, Apr 21, 2023 at 10:30:45AM +0300, Adrian Hunter wrote:
>> On 21/04/23 08:04, Changbin Du wrote:
>>>> What do you get if you try below diff on top of
>>>> your patches:
>>>>
>>>> diff --git a/tools/perf/builtin-script.c b/tools/perf/builtin-script.c
>>>> index c7bf1ac14e90..df0d21141185 100644
>>>> --- a/tools/perf/builtin-script.c
>>>> +++ b/tools/perf/builtin-script.c
>>>> @@ -576,8 +576,11 @@ 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(DSOFF))
>>>> + if (PRINT_FIELD(DSOFF)) {
>>>> output[type].print_ip_opts |= EVSEL__PRINT_DSOFF;
>>>> + /* DSO offset is relative to dso->longname */
>>>> + symbol_conf.show_kernel_path = true;
>>>> + }
>>>>
>>>> if (PRINT_FIELD(SYMOFFSET))
>>>> output[type].print_ip_opts |= EVSEL__PRINT_SYMOFFSET;
>>>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>>>> index a86614599269..19ebfd3468cc 100644
>>>> --- a/tools/perf/util/dso.c
>>>> +++ b/tools/perf/util/dso.c
>>>> @@ -67,6 +67,42 @@ char dso__symtab_origin(const struct dso *dso)
>>>> return origin[dso->symtab_type];
>>>> }
>>>>
>>>> +bool dso__is_file(const struct dso *dso)
>>>> +{
>>>> + switch (dso->binary_type) {
>>>> + case DSO_BINARY_TYPE__KALLSYMS:
>>>> + case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>>>> + return false;
>>>> + case DSO_BINARY_TYPE__VMLINUX:
>>>> + case DSO_BINARY_TYPE__GUEST_VMLINUX:
>>>> + return true;
>>>> + case DSO_BINARY_TYPE__JAVA_JIT:
>>>> + return false;
>>>> + case DSO_BINARY_TYPE__DEBUGLINK:
>>>> + case DSO_BINARY_TYPE__BUILD_ID_CACHE:
>>>> + case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
>>>> + case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
>>>> + case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
>>>> + case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
>>>> + case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
>>>> + case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
>>>> + case DSO_BINARY_TYPE__GUEST_KMODULE:
>>>> + case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
>>>> + case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
>>>> + case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP:
>>>> + case DSO_BINARY_TYPE__KCORE:
>>>> + case DSO_BINARY_TYPE__GUEST_KCORE:
>>>> + case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
>>>> + return true;
>>>> + case DSO_BINARY_TYPE__BPF_PROG_INFO:
>>>> + case DSO_BINARY_TYPE__BPF_IMAGE:
>>>> + case DSO_BINARY_TYPE__OOL:
>>>> + case DSO_BINARY_TYPE__NOT_FOUND:
>>>> + default:
>>>> + return false;
>>>> + }
>>>> +}
>>>> +
>>>> int dso__read_binary_type_filename(const struct dso *dso,
>>>> enum dso_binary_type type,
>>>> char *root_dir, char *filename, size_t size)
>>>> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
>>>> index 0b7c7633b9f6..fb33f5224fb6 100644
>>>> --- a/tools/perf/util/dso.h
>>>> +++ b/tools/perf/util/dso.h
>>>> @@ -396,6 +396,8 @@ static inline bool dso__is_kallsyms(struct dso *dso)
>>>> return dso->kernel && dso->long_name[0] != '/';
>>>> }
>>>>
>>>> +bool dso__is_file(const struct dso *dso);
>>>> +
>>>> void dso__free_a2l(struct dso *dso);
>>>>
>>>> enum dso_type dso__type(struct dso *dso, struct machine *machine);
>>>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>>>> index 7da96b41100f..9b79f88d371c 100644
>>>> --- a/tools/perf/util/map.c
>>>> +++ b/tools/perf/util/map.c
>>>> @@ -447,11 +447,12 @@ size_t map__fprintf_dsoname(struct map *map, FILE *fp)
>>>>
>>>> size_t map__fprintf_dsoname_dsoff(struct map *map, bool print_off, u64 addr, FILE *fp)
>>>> {
>>>> + const struct dso *dso = map ? map__dso(map) : NULL;
>>>> int printed = 0;
>>>>
>>>> printed += fprintf(fp, " (");
>>>> printed += map__fprintf_dsoname(map, fp);
>>>> - if (print_off && map && map__dso(map) && !map__dso(map)->kernel)
>>>> + if (print_off && dso && dso__is_file(dso))
>>>> printed += fprintf(fp, "+0x%" PRIx64, addr);
>>>> printed += fprintf(fp, ")");
>>>>
>>>>
>>>
>>> Here are the outputs with above change.
>>>
>>> For elf in build-id cache, it works as expected.
>>> $ sudo perf script -F +dsoff
>>> perf-exec 12768 135.648023: 1 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/home/changbin/.debug/.build-id/5e/2fa721660d663f38b6e1aa98d6fa3776974b54/elf+0x28ee44)
>>> perf-exec 12768 135.648028: 1 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/home/changbin/.debug/.build-id/5e/2fa721660d663f38b6e1aa98d6fa3776974b54/elf+0x28ee44)
>>> perf-exec 12768 135.648030: 11 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/home/changbin/.debug/.build-id/5e/2fa721660d663f38b6e1aa98d6fa3776974b54/elf+0x28ee44)
>>> perf-exec 12768 135.648031: 295 cycles: ffffffff96c8ee46 native_write_msr+0x6 (/home/changbin/.debug/.build-id/5e/2fa721660d663f38b6e1aa98d6fa3776974b54/elf+0x28ee46)
>>> perf-exec 12768 135.648032: 8850 cycles: ffffffff96c4c686 native_sched_clock+0x66 (/home/changbin/.debug/.build-id/5e/2fa721660d663f38b6e1aa98d6fa3776974b54/elf+0x24c686)
>>
>> A bit messy though. User can use option --show-kernel-path
>> so let's not force that after all.
>>
>>> ls 27521 501.120978: 4309123 cycles: 7f31cb51c591 _dl_sort_maps+0x301 (/usr/lib/x86_64-linux-gnu/ld-2.31.so)
>>>
>>> But when I specify my vmlinux: (the binary_type is DSO_BINARY_TYPE__SYSTEM_PATH_DSO)
>>> $ sudo perf script -k linux/vmlinux -F +dsoff
>>> perf-exec 12768 135.648023: 1 cycles: ffffffff96c8ee44 [unknown] (/lib/modules/6.2.12/build/vmlinux)
>>> perf-exec 12768 135.648028: 1 cycles: ffffffff96c8ee44 [unknown] (/lib/modules/6.2.12/build/vmlinux+0xffffffff96c8ee44)
>>> perf-exec 12768 135.648030: 11 cycles: ffffffff96c8ee44 [unknown] (/lib/modules/6.2.12/build/vmlinux+0xffffffff96c8ee44)
>>> perf-exec 12768 135.648031: 295 cycles: ffffffff96c8ee46 [unknown] (/lib/modules/6.2.12/build/vmlinux+0xffffffff96c8ee46)
>>>
>>> This is for kcore file:
>>> $ sudo perf script --kallsyms /proc/kallsyms -F +dsoff
>>> perf-exec 18922 267.284368: 1 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/proc/kcore+0x7fff96c91e44)
>>> perf-exec 18922 267.284372: 1 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/proc/kcore+0x7fff96c91e44)
>>> perf-exec 18922 267.284374: 11 cycles: ffffffff96c8ee44 native_write_msr+0x4 (/proc/kcore+0x7fff96c91e44)
>>>
>>> For ko, it's wierd that the binary_type of first one is DSO_BINARY_TYPE__NOT_FOUND.
>>> $ sudo perf script -F +dsoff | grep '.ko'
>>> gedit 37410 769.927194: 199304 cycles: ffffffffc0a3d050 ipt_do_table+0x0 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko)
>>> gedit 37410 770.459667: 271035 cycles: ffffffffc0a3d050 ipt_do_table+0x0 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0xf0)
>>> gedit 37410 770.649838: 271878 cycles: ffffffffc0a3d050 ipt_do_table+0x0 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0xf0)
>>> gedit 37410 771.239221: 216084 cycles: ffffffffc0a3d13b ipt_do_table+0xeb (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0x1db)
>>> gedit 37410 771.257816: 219469 cycles: ffffffffc0a3d165 ipt_do_table+0x115 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0x205)
>>> gedit 37410 771.531158: 288970 cycles: ffffffffc0a3d151 ipt_do_table+0x101 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0x1f1)
>>> gedit 37410 771.816916: 321215 cycles: ffffffffc0a3d151 ipt_do_table+0x101 (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0x1f1)
>>> gedit 37410 773.624786: 332528 cycles: ffffffffc0a3d2ea ipt_do_table+0x29a (/lib/modules/6.2.12/kernel/net/ipv4/netfilter/ip_tables.ko+0x38a)
>>>
>>> Maybe we can just filter by name? e.g. '[kernel.kallsyms]', '[guest.kernel.kallsyms]'.
>>
>> Using the dso->binary_type deals with other cases also.
>> We can just change case DSO_BINARY_TYPE__NOT_FOUND to return
>> true.
>>
>> How about this:
>>
>> commit abaf1cbf5be5d50b1b3682511b92794394b72178
>> Author: Adrian Hunter <adrian.hunter@...el.com>
>> Date: Fri Apr 21 10:15:14 2023 +0300
>>
>> perf script: Refine printing of dso offset
>>
>> Print dso offset only for object files, and in those cases force using the
>> dso->long_name if the dso->name starts with '[' or the dso is kcore, in
>> order to avoid special names such as [vdso], or mixing up kcore with
>> vmlinux.
>>
>> Signed-off-by: Adrian Hunter <adrian.hunter@...el.com>
>>
>> diff --git a/tools/perf/util/dso.c b/tools/perf/util/dso.c
>> index a86614599269..046fbfcfdaab 100644
>> --- a/tools/perf/util/dso.c
>> +++ b/tools/perf/util/dso.c
>> @@ -67,6 +67,39 @@ char dso__symtab_origin(const struct dso *dso)
>> return origin[dso->symtab_type];
>> }
>>
>> +bool dso__is_object_file(const struct dso *dso)
>> +{
>> + switch (dso->binary_type) {
>> + case DSO_BINARY_TYPE__KALLSYMS:
>> + case DSO_BINARY_TYPE__GUEST_KALLSYMS:
>> + case DSO_BINARY_TYPE__JAVA_JIT:
>> + case DSO_BINARY_TYPE__BPF_PROG_INFO:
>> + case DSO_BINARY_TYPE__BPF_IMAGE:
>> + case DSO_BINARY_TYPE__OOL:
>> + return false;
>> + case DSO_BINARY_TYPE__VMLINUX:
>> + case DSO_BINARY_TYPE__GUEST_VMLINUX:
>> + case DSO_BINARY_TYPE__DEBUGLINK:
>> + case DSO_BINARY_TYPE__BUILD_ID_CACHE:
>> + case DSO_BINARY_TYPE__BUILD_ID_CACHE_DEBUGINFO:
>> + case DSO_BINARY_TYPE__FEDORA_DEBUGINFO:
>> + case DSO_BINARY_TYPE__UBUNTU_DEBUGINFO:
>> + case DSO_BINARY_TYPE__MIXEDUP_UBUNTU_DEBUGINFO:
>> + case DSO_BINARY_TYPE__BUILDID_DEBUGINFO:
>> + case DSO_BINARY_TYPE__SYSTEM_PATH_DSO:
>> + case DSO_BINARY_TYPE__GUEST_KMODULE:
>> + case DSO_BINARY_TYPE__GUEST_KMODULE_COMP:
>> + case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE:
>> + case DSO_BINARY_TYPE__SYSTEM_PATH_KMODULE_COMP:
>> + case DSO_BINARY_TYPE__KCORE:
>> + case DSO_BINARY_TYPE__GUEST_KCORE:
>> + case DSO_BINARY_TYPE__OPENEMBEDDED_DEBUGINFO:
>> + case DSO_BINARY_TYPE__NOT_FOUND:
>> + default:
>> + return true;
>> + }
>> +}
>> +
>> int dso__read_binary_type_filename(const struct dso *dso,
>> enum dso_binary_type type,
>> char *root_dir, char *filename, size_t size)
>> diff --git a/tools/perf/util/dso.h b/tools/perf/util/dso.h
>> index 0b7c7633b9f6..b23a157c914d 100644
>> --- a/tools/perf/util/dso.h
>> +++ b/tools/perf/util/dso.h
>> @@ -379,23 +379,25 @@ void dso__reset_find_symbol_cache(struct dso *dso);
>> size_t dso__fprintf_symbols_by_name(struct dso *dso, FILE *fp);
>> size_t dso__fprintf(struct dso *dso, FILE *fp);
>>
>> -static inline bool dso__is_vmlinux(struct dso *dso)
>> +static inline bool dso__is_vmlinux(const struct dso *dso)
>> {
>> return dso->binary_type == DSO_BINARY_TYPE__VMLINUX ||
>> dso->binary_type == DSO_BINARY_TYPE__GUEST_VMLINUX;
>> }
>>
>> -static inline bool dso__is_kcore(struct dso *dso)
>> +static inline bool dso__is_kcore(const struct dso *dso)
>> {
>> return dso->binary_type == DSO_BINARY_TYPE__KCORE ||
>> dso->binary_type == DSO_BINARY_TYPE__GUEST_KCORE;
>> }
>>
>> -static inline bool dso__is_kallsyms(struct dso *dso)
>> +static inline bool dso__is_kallsyms(const struct dso *dso)
>> {
>> return dso->kernel && dso->long_name[0] != '/';
>> }
>>
>> +bool dso__is_object_file(const struct dso *dso);
>> +
>> void dso__free_a2l(struct dso *dso);
>>
>> enum dso_type dso__type(struct dso *dso, struct machine *machine);
>> diff --git a/tools/perf/util/map.c b/tools/perf/util/map.c
>> index 7da96b41100f..5e7808b0bc87 100644
>> --- a/tools/perf/util/map.c
>> +++ b/tools/perf/util/map.c
>> @@ -424,14 +424,21 @@ size_t map__fprintf(struct map *map, FILE *fp)
>> map__start(map), map__end(map), map__pgoff(map), dso->name);
>> }
>>
>> -size_t map__fprintf_dsoname(struct map *map, FILE *fp)
>> +static bool prefer_dso_long_name(const struct dso *dso, bool print_off)
>> +{
>> + return dso->long_name &&
>> + (symbol_conf.show_kernel_path ||
>> + (print_off && (dso->name[0] == '[' || dso__is_kcore(dso))));
>> +}
>> +
>> +static size_t __map__fprintf_dsoname(struct map *map, bool print_off, FILE *fp)
>> {
>> char buf[symbol_conf.pad_output_len_dso + 1];
>> const char *dsoname = "[unknown]";
>> const struct dso *dso = map ? map__dso(map) : NULL;
>>
>> if (dso) {
>> - if (symbol_conf.show_kernel_path && dso->long_name)
>> + if (prefer_dso_long_name(dso, print_off))
>> dsoname = dso->long_name;
>> else
>> dsoname = dso->name;
>> @@ -445,13 +452,21 @@ size_t map__fprintf_dsoname(struct map *map, FILE *fp)
>> return fprintf(fp, "%s", dsoname);
>> }
>>
>> +size_t map__fprintf_dsoname(struct map *map, FILE *fp)
>> +{
>> + return __map__fprintf_dsoname(map, false, fp);
>> +}
>> +
>> size_t map__fprintf_dsoname_dsoff(struct map *map, bool print_off, u64 addr, FILE *fp)
>> {
>> + const struct dso *dso = map ? map__dso(map) : NULL;
>> int printed = 0;
>>
>> + if (print_off && (!dso || !dso__is_object_file(dso)))
>> + print_off = false;
>> printed += fprintf(fp, " (");
>> - printed += map__fprintf_dsoname(map, fp);
>> - if (print_off && map && map__dso(map) && !map__dso(map)->kernel)
>> + printed += __map__fprintf_dsoname(map, print_off, fp);
>> + if (print_off)
>> printed += fprintf(fp, "+0x%" PRIx64, addr);
>> printed += fprintf(fp, ")");
>>
>>
>>
> I am fine with this polish patch. So will send this standalone or append to this
> thread?
I will send my patch separately.
Powered by blists - more mailing lists