* [PATCH 0/2] perf report: Fix kallsyms parsing
@ 2018-01-19 16:11 Jiri Olsa
2018-01-19 16:11 ` [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse Jiri Olsa
2018-01-19 16:11 ` [PATCH 2/2] perf tools: Skip read of kernel maps once it failed Jiri Olsa
0 siblings, 2 replies; 10+ messages in thread
From: Jiri Olsa @ 2018-01-19 16:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Ingo Molnar, Namhyung Kim, David Ahern, Andi Kleen,
Alexander Shishkin, Peter Zijlstra
hi,
I noticed slow perf report on my system and found some
issues described in patches. However I haven't got much
time for testing.. please review/test carefully ;-)
thanks,
jirka
---
tools/lib/symbol/kallsyms.c | 8 ++++++--
tools/perf/util/machine.c | 6 ++++--
tools/perf/util/machine.h | 1 +
3 files changed, 11 insertions(+), 4 deletions(-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-19 16:11 [PATCH 0/2] perf report: Fix kallsyms parsing Jiri Olsa
@ 2018-01-19 16:11 ` Jiri Olsa
2018-01-26 6:36 ` Namhyung Kim
2018-01-26 17:22 ` Andy Shevchenko
2018-01-19 16:11 ` [PATCH 2/2] perf tools: Skip read of kernel maps once it failed Jiri Olsa
1 sibling, 2 replies; 10+ messages in thread
From: Jiri Olsa @ 2018-01-19 16:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Ingo Molnar, Namhyung Kim, David Ahern, Andi Kleen,
Alexander Shishkin, Peter Zijlstra
Current kallsyms__parse uses hex2u64, which gives
no indication of error. Using strtoul to checkup
on failed attempt to parse the number and stop the
rest of the kallsyms__parse processing early.
Link: http://lkml.kernel.org/n/tip-djqwni3p6lgctf6o7xhhwpmw@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/lib/symbol/kallsyms.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tools/lib/symbol/kallsyms.c b/tools/lib/symbol/kallsyms.c
index 914cb8e3d40b..dd5bb1dfd2b6 100644
--- a/tools/lib/symbol/kallsyms.c
+++ b/tools/lib/symbol/kallsyms.c
@@ -29,6 +29,7 @@ int kallsyms__parse(const char *filename, void *arg,
int line_len, len;
char symbol_type;
char *symbol_name;
+ char *endptr;
line_len = getline(&line, &n, file);
if (line_len < 0 || !line)
@@ -36,9 +37,12 @@ int kallsyms__parse(const char *filename, void *arg,
line[--line_len] = '\0'; /* \n */
- len = hex2u64(line, &start);
+ start = strtoul(line, &endptr, 16);
+ if (line == endptr)
+ continue;
+
+ len = endptr - line + 1;
- len++;
if (len + 2 >= line_len)
continue;
--
2.13.6
^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/2] perf tools: Skip read of kernel maps once it failed
2018-01-19 16:11 [PATCH 0/2] perf report: Fix kallsyms parsing Jiri Olsa
2018-01-19 16:11 ` [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse Jiri Olsa
@ 2018-01-19 16:11 ` Jiri Olsa
2018-01-26 6:48 ` Namhyung Kim
1 sibling, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2018-01-19 16:11 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo
Cc: lkml, Ingo Molnar, Namhyung Kim, David Ahern, Andi Kleen,
Alexander Shishkin, Peter Zijlstra
Current perf report is real slow on newer kernels,
with following commit:
c0f3ea158939 ("stop using '%pK' for /proc/kallsyms pointer values")
which prevent pointers in /proc/kallsyms, in case
kernel.perf_event_paranoid=2.
That makes perf to fail in finding kernel map details,
and keep parsing it again for every kernel sample.
Adding and setting a new machine::vmlinux_maps_failed
flag after first failed parsing attempt and using it
to prevent new pointless parsing.
TODO We might want to add some perf report warning
about that.
Link: http://lkml.kernel.org/n/tip-ld2kp994rhz6i341igt8f98y@git.kernel.org
Signed-off-by: Jiri Olsa <jolsa@kernel.org>
---
tools/perf/util/machine.c | 6 ++++--
tools/perf/util/machine.h | 1 +
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
index b05a67464c03..5e9648817077 100644
--- a/tools/perf/util/machine.c
+++ b/tools/perf/util/machine.c
@@ -1222,13 +1222,15 @@ int machine__create_kernel_maps(struct machine *machine)
u64 addr = 0;
int ret;
- if (kernel == NULL)
+ if (kernel == NULL || machine->vmlinux_maps_failed)
return -1;
ret = __machine__create_kernel_maps(machine, kernel);
dso__put(kernel);
- if (ret < 0)
+ if (ret < 0) {
+ machine->vmlinux_maps_failed = true;
return -1;
+ }
if (symbol_conf.use_modules && machine__create_modules(machine) < 0) {
if (machine__is_host(machine))
diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
index 5ce860b64c74..edcb007333cb 100644
--- a/tools/perf/util/machine.h
+++ b/tools/perf/util/machine.h
@@ -42,6 +42,7 @@ struct machine {
u16 id_hdr_size;
bool comm_exec;
bool kptr_restrict_warned;
+ bool vmlinux_maps_failed;
char *root_dir;
struct threads threads[THREADS__TABLE_SIZE];
struct vdso_info *vdso_info;
--
2.13.6
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-19 16:11 ` [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse Jiri Olsa
@ 2018-01-26 6:36 ` Namhyung Kim
2018-01-26 17:22 ` Andy Shevchenko
1 sibling, 0 replies; 10+ messages in thread
From: Namhyung Kim @ 2018-01-26 6:36 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, David Ahern,
Andi Kleen, Alexander Shishkin, Peter Zijlstra, kernel-team
On Fri, Jan 19, 2018 at 05:11:02PM +0100, Jiri Olsa wrote:
> Current kallsyms__parse uses hex2u64, which gives
> no indication of error. Using strtoul to checkup
> on failed attempt to parse the number and stop the
> rest of the kallsyms__parse processing early.
>
> Link: http://lkml.kernel.org/n/tip-djqwni3p6lgctf6o7xhhwpmw@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
Acked-by: Namhyung Kim <namhyung@kernel.org>
Thanks,
Namhyung
> ---
> tools/lib/symbol/kallsyms.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/symbol/kallsyms.c b/tools/lib/symbol/kallsyms.c
> index 914cb8e3d40b..dd5bb1dfd2b6 100644
> --- a/tools/lib/symbol/kallsyms.c
> +++ b/tools/lib/symbol/kallsyms.c
> @@ -29,6 +29,7 @@ int kallsyms__parse(const char *filename, void *arg,
> int line_len, len;
> char symbol_type;
> char *symbol_name;
> + char *endptr;
>
> line_len = getline(&line, &n, file);
> if (line_len < 0 || !line)
> @@ -36,9 +37,12 @@ int kallsyms__parse(const char *filename, void *arg,
>
> line[--line_len] = '\0'; /* \n */
>
> - len = hex2u64(line, &start);
> + start = strtoul(line, &endptr, 16);
> + if (line == endptr)
> + continue;
> +
> + len = endptr - line + 1;
>
> - len++;
> if (len + 2 >= line_len)
> continue;
>
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf tools: Skip read of kernel maps once it failed
2018-01-19 16:11 ` [PATCH 2/2] perf tools: Skip read of kernel maps once it failed Jiri Olsa
@ 2018-01-26 6:48 ` Namhyung Kim
2018-01-30 11:21 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Namhyung Kim @ 2018-01-26 6:48 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, David Ahern,
Andi Kleen, Alexander Shishkin, Peter Zijlstra, kernel-team
On Fri, Jan 19, 2018 at 05:11:03PM +0100, Jiri Olsa wrote:
> Current perf report is real slow on newer kernels,
> with following commit:
> c0f3ea158939 ("stop using '%pK' for /proc/kallsyms pointer values")
>
> which prevent pointers in /proc/kallsyms, in case
> kernel.perf_event_paranoid=2.
>
> That makes perf to fail in finding kernel map details,
> and keep parsing it again for every kernel sample.
>
> Adding and setting a new machine::vmlinux_maps_failed
> flag after first failed parsing attempt and using it
> to prevent new pointless parsing.
Hmm.. is it because it's called from machine__resolve() right?
/*
* Have we already created the kernel maps for this machine?
*
* This should have happened earlier, when we processed the kernel MMAP
* events, but for older perf.data files there was no such thing, so do
* it now.
*/
It seems that it's only to be compatible with ancient versions.
Do we still need it? I guess they are recorded many years ago (with
the ancient version) so using addresses of current kernel is just
meaningless. If one still uses the ancient version, [s]he really
needs to update it.
Thanks,
Namhyung
>
> TODO We might want to add some perf report warning
> about that.
>
> Link: http://lkml.kernel.org/n/tip-ld2kp994rhz6i341igt8f98y@git.kernel.org
> Signed-off-by: Jiri Olsa <jolsa@kernel.org>
> ---
> tools/perf/util/machine.c | 6 ++++--
> tools/perf/util/machine.h | 1 +
> 2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c
> index b05a67464c03..5e9648817077 100644
> --- a/tools/perf/util/machine.c
> +++ b/tools/perf/util/machine.c
> @@ -1222,13 +1222,15 @@ int machine__create_kernel_maps(struct machine *machine)
> u64 addr = 0;
> int ret;
>
> - if (kernel == NULL)
> + if (kernel == NULL || machine->vmlinux_maps_failed)
> return -1;
>
> ret = __machine__create_kernel_maps(machine, kernel);
> dso__put(kernel);
> - if (ret < 0)
> + if (ret < 0) {
> + machine->vmlinux_maps_failed = true;
> return -1;
> + }
>
> if (symbol_conf.use_modules && machine__create_modules(machine) < 0) {
> if (machine__is_host(machine))
> diff --git a/tools/perf/util/machine.h b/tools/perf/util/machine.h
> index 5ce860b64c74..edcb007333cb 100644
> --- a/tools/perf/util/machine.h
> +++ b/tools/perf/util/machine.h
> @@ -42,6 +42,7 @@ struct machine {
> u16 id_hdr_size;
> bool comm_exec;
> bool kptr_restrict_warned;
> + bool vmlinux_maps_failed;
> char *root_dir;
> struct threads threads[THREADS__TABLE_SIZE];
> struct vdso_info *vdso_info;
> --
> 2.13.6
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-19 16:11 ` [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse Jiri Olsa
2018-01-26 6:36 ` Namhyung Kim
@ 2018-01-26 17:22 ` Andy Shevchenko
2018-01-26 17:27 ` Andy Shevchenko
1 sibling, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-01-26 17:22 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim,
David Ahern, Andi Kleen, Alexander Shishkin, Peter Zijlstra
On Fri, Jan 19, 2018 at 6:11 PM, Jiri Olsa <jolsa@kernel.org> wrote:
> Current kallsyms__parse uses hex2u64, which gives
> no indication of error. Using strtoul to checkup
> on failed attempt to parse the number and stop the
> rest of the kallsyms__parse processing early.
> + start = strtoul(line, &endptr, 16);
> + if (line == endptr)
> + continue;
> +
> + len = endptr - line + 1;
>
> - len++;
> if (len + 2 >= line_len)
> continue;
https://patchwork.kernel.org/patch/4087681/
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-26 17:22 ` Andy Shevchenko
@ 2018-01-26 17:27 ` Andy Shevchenko
2018-01-29 7:18 ` Jiri Olsa
0 siblings, 1 reply; 10+ messages in thread
From: Andy Shevchenko @ 2018-01-26 17:27 UTC (permalink / raw)
To: Jiri Olsa
Cc: Arnaldo Carvalho de Melo, lkml, Ingo Molnar, Namhyung Kim,
David Ahern, Andi Kleen, Alexander Shishkin, Peter Zijlstra
On Fri, Jan 26, 2018 at 7:22 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Fri, Jan 19, 2018 at 6:11 PM, Jiri Olsa <jolsa@kernel.org> wrote:
>> Current kallsyms__parse uses hex2u64, which gives
>> no indication of error. Using strtoul to checkup
>> on failed attempt to parse the number and stop the
>> rest of the kallsyms__parse processing early.
>
>> + start = strtoul(line, &endptr, 16);
>> + if (line == endptr)
>> + continue;
>> +
>> + len = endptr - line + 1;
>>
>> - len++;
>> if (len + 2 >= line_len)
>> continue;
>
> https://patchwork.kernel.org/patch/4087681/
Even second attempt including recent ping left without consideration.
http://lkml.iu.edu/hypermail/linux/kernel/1407.0/02791.html
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-26 17:27 ` Andy Shevchenko
@ 2018-01-29 7:18 ` Jiri Olsa
2018-01-29 13:05 ` Andy Shevchenko
0 siblings, 1 reply; 10+ messages in thread
From: Jiri Olsa @ 2018-01-29 7:18 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Ingo Molnar,
Namhyung Kim, David Ahern, Andi Kleen, Alexander Shishkin,
Peter Zijlstra
On Fri, Jan 26, 2018 at 07:27:06PM +0200, Andy Shevchenko wrote:
> On Fri, Jan 26, 2018 at 7:22 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Fri, Jan 19, 2018 at 6:11 PM, Jiri Olsa <jolsa@kernel.org> wrote:
> >> Current kallsyms__parse uses hex2u64, which gives
> >> no indication of error. Using strtoul to checkup
> >> on failed attempt to parse the number and stop the
> >> rest of the kallsyms__parse processing early.
> >
> >> + start = strtoul(line, &endptr, 16);
> >> + if (line == endptr)
> >> + continue;
> >> +
> >> + len = endptr - line + 1;
> >>
> >> - len++;
> >> if (len + 2 >= line_len)
> >> continue;
> >
> > https://patchwork.kernel.org/patch/4087681/
>
> Even second attempt including recent ping left without consideration.
>
> http://lkml.iu.edu/hypermail/linux/kernel/1407.0/02791.html
I see, sry we overlooked it.. could you please repost it?
thanks,
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse
2018-01-29 7:18 ` Jiri Olsa
@ 2018-01-29 13:05 ` Andy Shevchenko
0 siblings, 0 replies; 10+ messages in thread
From: Andy Shevchenko @ 2018-01-29 13:05 UTC (permalink / raw)
To: Jiri Olsa
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Ingo Molnar,
Namhyung Kim, David Ahern, Andi Kleen, Alexander Shishkin,
Peter Zijlstra
On Mon, Jan 29, 2018 at 9:18 AM, Jiri Olsa <jolsa@redhat.com> wrote:
> On Fri, Jan 26, 2018 at 07:27:06PM +0200, Andy Shevchenko wrote:
>> On Fri, Jan 26, 2018 at 7:22 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>> > https://patchwork.kernel.org/patch/4087681/
>>
>> Even second attempt including recent ping left without consideration.
>>
>> http://lkml.iu.edu/hypermail/linux/kernel/1407.0/02791.html
>
> I see, sry we overlooked it.. could you please repost it?
Done.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf tools: Skip read of kernel maps once it failed
2018-01-26 6:48 ` Namhyung Kim
@ 2018-01-30 11:21 ` Jiri Olsa
0 siblings, 0 replies; 10+ messages in thread
From: Jiri Olsa @ 2018-01-30 11:21 UTC (permalink / raw)
To: Namhyung Kim
Cc: Jiri Olsa, Arnaldo Carvalho de Melo, lkml, Ingo Molnar,
David Ahern, Andi Kleen, Alexander Shishkin, Peter Zijlstra,
kernel-team
On Fri, Jan 26, 2018 at 03:48:30PM +0900, Namhyung Kim wrote:
> On Fri, Jan 19, 2018 at 05:11:03PM +0100, Jiri Olsa wrote:
> > Current perf report is real slow on newer kernels,
> > with following commit:
> > c0f3ea158939 ("stop using '%pK' for /proc/kallsyms pointer values")
> >
> > which prevent pointers in /proc/kallsyms, in case
> > kernel.perf_event_paranoid=2.
> >
> > That makes perf to fail in finding kernel map details,
> > and keep parsing it again for every kernel sample.
> >
> > Adding and setting a new machine::vmlinux_maps_failed
> > flag after first failed parsing attempt and using it
> > to prevent new pointless parsing.
>
> Hmm.. is it because it's called from machine__resolve() right?
>
> /*
> * Have we already created the kernel maps for this machine?
> *
> * This should have happened earlier, when we processed the kernel MMAP
> * events, but for older perf.data files there was no such thing, so do
> * it now.
> */
>
> It seems that it's only to be compatible with ancient versions.
>
> Do we still need it? I guess they are recorded many years ago (with
> the ancient version) so using addresses of current kernel is just
> meaningless. If one still uses the ancient version, [s]he really
> needs to update it.
right, I think we can remove it.. the reason I see is
that we actualy lookup the kernel maps via the map_group
in machine::kmap, which is static.. so always there
if there were no kernel maps events for some reason,
the map group wil be empty and the search will correctly
fail..
I will send it together with some other fixes later this week
thanks,
jirka
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-01-30 11:22 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-19 16:11 [PATCH 0/2] perf report: Fix kallsyms parsing Jiri Olsa
2018-01-19 16:11 ` [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse Jiri Olsa
2018-01-26 6:36 ` Namhyung Kim
2018-01-26 17:22 ` Andy Shevchenko
2018-01-26 17:27 ` Andy Shevchenko
2018-01-29 7:18 ` Jiri Olsa
2018-01-29 13:05 ` Andy Shevchenko
2018-01-19 16:11 ` [PATCH 2/2] perf tools: Skip read of kernel maps once it failed Jiri Olsa
2018-01-26 6:48 ` Namhyung Kim
2018-01-30 11:21 ` Jiri Olsa
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.