* [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
* 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 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
* [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 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 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.