perf report: Fix kallsyms parsing

PATCH In review Jiri Olsa 4 participants 10 messages Jan 19, 2018 – Jan 30, 2018 perf events

Share & tools

Jiri Olsa

author reviewer

2018-01-19 16:11 UTC #0 lore raw

cover perf report: Fix kallsyms parsing

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

Jiri Olsa

author reviewer

2018-01-19 16:11 UTC #1 lore raw

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

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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/lib/symbol/kallsyms.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/lib/symbol/kallsyms.c +6 -2

index 914cb8e3d40b..dd5bb1dfd2b6 100644
@@ -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;

Signature · 1 line

2.13.6

Namhyung Kim

maintainer Acked-by on v1 1/2

2018-01-26 06:36 UTC #2 lore raw

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

Quoted 7 lines · 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/[email protected] open here
Signed-off-by Jiri Olsa <[email protected]>

Acked-by Namhyung Kim <[email protected]>

Thanks,
Namhyung

Quoted 34 lines

---
 tools/lib/symbol/kallsyms.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

tools/lib/symbol/kallsyms.c +6 -2

index 914cb8e3d40b..dd5bb1dfd2b6 100644
@@ -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;

Signature · 1 line

2.13.6

Andy Shevchenko

2018-01-26 17:22 UTC #3 lore raw

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

In reply to Jiri Olsa

Quoted 5 lines · 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.

tools/lib/symbol/kallsyms.c int kallsyms__parse(const char *filename, void *arg, · line 47

Quoted 9 lines

tools/lib/symbol/kallsyms.c +5 -1

+               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/

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

2018-01-26 17:27 UTC #4 lore raw

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

Quoted 12 lines · Andy Shevchenko

Quoted 11 lines · 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.
tools/lib/symbol/kallsyms.c +5 -0
+               start = strtoul(line, &endptr, 16);
+               if (line == endptr)
+                       continue;
+
+               len = endptr - line + 1;

Quoted 5 lines

Quoted 3 lines
-               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

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jiri Olsa

2018-01-29 07:18 UTC #5 lore raw

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

Quoted 18 lines · Andy Shevchenko

Quoted 16 lines · Andy Shevchenko
Quoted 14 lines · 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.
tools/lib/symbol/kallsyms.c +5 -1
+               start = strtoul(line, &endptr, 16);
+               if (line == endptr)
+                       continue;
+
+               len = endptr - line + 1;
 
-               len++;
                if (len + 2 >= line_len)
                        continue;

Quoted 5 lines

Quoted 1 line
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

Andy Shevchenko

2018-01-29 13:05 UTC #6 lore raw

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

Quoted 11 lines · Jiri Olsa

Quoted 8 lines · Andy Shevchenko
Quoted 1 line · Andy Shevchenko
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.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Jiri Olsa

author reviewer

2018-01-19 16:11 UTC #7 lore raw

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

In reply to 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/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 6 ++++--
 tools/perf/util/machine.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

tools/perf/util/machine.c +4 -2

index b05a67464c03..5e9648817077 100644
@@ -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))

tools/perf/util/machine.h +1 -0

index 5ce860b64c74..edcb007333cb 100644
@@ -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;

Signature · 1 line

2.13.6

Namhyung Kim

maintainer

2018-01-26 06:48 UTC #8 lore raw

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

Quoted 7 lines · 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.

Quoted 6 lines

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

Quoted 48 lines

TODO We might want to add some perf report warning about that.

Link http://lkml.kernel.org/n/[email protected] open here

Signed-off-by Jiri Olsa <[email protected]>

---
 tools/perf/util/machine.c | 6 ++++--
 tools/perf/util/machine.h | 1 +
 2 files changed, 5 insertions(+), 2 deletions(-)

tools/perf/util/machine.c +4 -2

index b05a67464c03..5e9648817077 100644
@@ -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))

tools/perf/util/machine.h +1 -0

index 5ce860b64c74..edcb007333cb 100644
@@ -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;

Signature · 1 line

2.13.6

Jiri Olsa

2018-01-30 11:22 UTC #9 lore raw

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

Quoted 25 lines · Namhyung Kim

Quoted 7 lines · 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.
Quoted 6 lines
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.
*/

Quoted 6 lines

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