[PATCH 0/2] perf report: Fix kallsyms parsing

[PATCH 0/2] perf report: Fix kallsyms parsing

[Jiri Olsa]

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(-)

[PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Jiri Olsa]

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

[PATCH 2/2] perf tools: Skip read of kernel maps once it failed

[Jiri Olsa]

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

Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Namhyung Kim]

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
> 

Re: [PATCH 2/2] perf tools: Skip read of kernel maps once it failed

[Namhyung Kim]

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
> 

Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Andy Shevchenko]

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

Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Andy Shevchenko]

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

Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Jiri Olsa]

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

Re: [PATCH 1/2] tools lib symbol: Use strtoul instead of hex2u64 in kallsyms__parse

[Andy Shevchenko]

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

Re: [PATCH 2/2] perf tools: Skip read of kernel maps once it failed

[Jiri Olsa]

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