Revert "perf report: Append inlines to non-DWARF callchains"

PATCH In review Arnaldo Carvalho de Melo 3 participants 7 messages Aug 1, 2023 – Aug 8, 2023 perf events

lore

Share & tools

lore

Arnaldo Carvalho de Melo

author maintainer

2023-08-01 21:43 UTC #0 lore raw

PATCH 1/1 Revert "perf report: Append inlines to non-DWARF callchains"

Hi Artem,

Can you please double check this? I reproduced with:

git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report

Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?

- Arnaldo

----

This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.

The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.

Cc Andrii Nakryiko <[email protected]>

Cc Artem Savkov <[email protected]>

Cc Namhyung Kim <[email protected]>

Cc Adrian Hunter <[email protected]>

Cc Alexander Shishkin <[email protected]>

Cc Ian Rogers <[email protected]>

Cc Ingo Molnar <[email protected]>

Cc Jiri Olsa <[email protected]>

Cc Mark Rutland <[email protected]>

Cc Masami Hiramatsu <[email protected]>

Cc Milian Wolff <[email protected]>

Cc Peter Zijlstra <[email protected]>

Signed-off-by Arnaldo Carvalho de Melo <[email protected]>

---
 tools/perf/util/machine.c | 5 -----
 1 file changed, 5 deletions(-)

tools/perf/util/machine.c +0 -5

index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
 static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
 				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 
 static struct dso *machine__kernel_dso(struct machine *machine)
 {

@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
 	ms.maps = maps__get(al.maps);
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
 	srcline = callchain_srcline(&ms, al.addr);
 	err = callchain_cursor_append(cursor, ip, &ms,
 				      branch, flags, nr_loop_iter,

Signature · 1 line

2.41.0

Artem Savkov

2023-08-02 07:47 UTC #1 lore raw

Hi Arnaldo,

Quoted 4 lines · Arnaldo Carvalho de Melo

Hi Artem,
Can you please double check this? I reproduced with:

Quoted 8 lines

git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?

With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.

Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.

For 'perf record -a -g sleep 60' (Event count (approx.): 774055090):

with inlines:

$ time ./perf report

real    0m1.477s
user    0m1.324s
sys     0m0.147s

without inlines:

$ time ./perf report

real    0m1.349s
user    0m1.232s
sys     0m0.111s

For 'perf record -a -g sleep 5' (Event count (approx.): 90179399):

with inlines:

$ time ./perf report

real    0m0.657s
user    0m0.555s
sys     0m0.099s

without inlines:

$ time ./perf report

real    0m0.559s
user    0m0.498s
sys     0m0.060s

Quoted 55 lines

- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/machine.c | 5 -----
 1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -5
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
 static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
 				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 
 static struct dso *machine__kernel_dso(struct machine *machine)
 {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
 	ms.maps = maps__get(al.maps);
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
 	srcline = callchain_srcline(&ms, al.addr);
 	err = callchain_cursor_append(cursor, ip, &ms,
 				      branch, flags, nr_loop_iter,
Signature · 1 line
2.41.0

Signature · 1 line

 Artem

Artem Savkov

2023-08-07 11:01 UTC #2 lore raw

Quoted 16 lines · Artem Savkov

Hi Arnaldo,
Quoted 4 lines · Arnaldo Carvalho de Melo
Hi Artem,
Can you please double check this? I reproduced with:
Quoted 8 lines
git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?

Quoted 6 lines

With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.
Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.

My bad, I had wrong debuginfo installed for the kernel I tested. I can reproduce it with the correct one. Looks like vmlinux is just too much for addr2line. Maybe we can skip it but leave other inlines in, like so:

tools/perf/util/machine.c +3 -1

index 11de3ca8d4fa7..fef309cd401f7 100644
@@ -2388,7 +2388,9 @@ static int add_callchain_ip(struct thread *thread,
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
 
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
+	if (!branch && ms.map && ms.map->dso &&
+	    strcmp(ms.map->dso->short_name, "[kernel.vmlinux]") &&
+	    append_inlines(cursor, &ms, ip) == 0)
 		goto out;
 
 	srcline = callchain_srcline(&ms, al.addr);

Quoted 58 lines

Quoted 55 lines
- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/machine.c | 5 -----
 1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -5
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
 static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
 				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 
 static struct dso *machine__kernel_dso(struct machine *machine)
 {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
 	ms.maps = maps__get(al.maps);
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
 	srcline = callchain_srcline(&ms, al.addr);
 	err = callchain_cursor_append(cursor, ip, &ms,
 				      branch, flags, nr_loop_iter,
Signature · 1 line
2.41.0
Signature · 1 line
 Artem

Signature · 1 line

 Artem

Arnaldo Carvalho de Melo

author maintainer

2023-08-07 13:35 UTC #3 lore raw

Em Mon, Aug 07, 2023 at 01:00:08PM +0200, Artem Savkov escreveu:

Quoted 20 lines · Artem Savkov

Quoted 19 lines
Hi Arnaldo,
Quoted 4 lines · Arnaldo Carvalho de Melo
Hi Artem,
Can you please double check this? I reproduced with:
Quoted 8 lines
git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?
With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.

Quoted 7 lines

Quoted 3 lines
Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.
My bad, I had wrong debuginfo installed for the kernel I tested. I can reproduce it with the correct one. Looks like vmlinux is just too much for addr2line. Maybe we can skip it but leave other inlines in, like so:

That is a possibilit, and probably we could make it cheaper by looking at the cpumode, avoiding calling addr2line when we didn't makage to resolve the symbol, etc.

We also may want to have this as an option that has to be explicitely enabled, like --resolve-inlines, as this will add overhead no matter if we stop calling addr2line and do it more efficiently, etc.

Fact is, we're late in the 6.5 schedule, so the best thing now is to just revert the patch and then try again later, ok?

- Arnaldo

Quoted 78 lines

tools/perf/util/machine.c +3 -1
index 11de3ca8d4fa7..fef309cd401f7 100644
@@ -2388,7 +2388,9 @@ static int add_callchain_ip(struct thread *thread,
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
 
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
+	if (!branch && ms.map && ms.map->dso &&
+	    strcmp(ms.map->dso->short_name, "[kernel.vmlinux]") &&
+	    append_inlines(cursor, &ms, ip) == 0)
 		goto out;
 
 	srcline = callchain_srcline(&ms, al.addr);
Quoted 59 lines
Quoted 56 lines
- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/machine.c | 5 -----
 1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -5
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
 static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
 				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 
 static struct dso *machine__kernel_dso(struct machine *machine)
 {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
 	ms.maps = maps__get(al.maps);
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
 	srcline = callchain_srcline(&ms, al.addr);
 	err = callchain_cursor_append(cursor, ip, &ms,
 				      branch, flags, nr_loop_iter,
Signature · 1 line
2.41.0
Signature · 1 line
 Artem
Signature · 1 line
 Artem

Signature · 1 line

- Arnaldo

Artem Savkov

2023-08-07 14:06 UTC #4 lore raw

Quoted 29 lines · Arnaldo Carvalho de Melo

Em Mon, Aug 07, 2023 at 01:00:08PM +0200, Artem Savkov escreveu:
Quoted 20 lines · Artem Savkov
Quoted 19 lines
Hi Arnaldo,
Quoted 4 lines · Arnaldo Carvalho de Melo
Hi Artem,
Can you please double check this? I reproduced with:
Quoted 8 lines
git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?
With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.
Quoted 7 lines
Quoted 3 lines
Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.
My bad, I had wrong debuginfo installed for the kernel I tested. I can reproduce it with the correct one. Looks like vmlinux is just too much for addr2line. Maybe we can skip it but leave other inlines in, like so:

Quoted 7 lines

That is a possibilit, and probably we could make it cheaper by looking at the cpumode, avoiding calling addr2line when we didn't makage to resolve the symbol, etc.
We also may want to have this as an option that has to be explicitely enabled, like --resolve-inlines, as this will add overhead no matter if we stop calling addr2line and do it more efficiently, etc.

Sounds good, I'll look into it.

Quoted 2 lines

Fact is, we're late in the 6.5 schedule, so the best thing now is to just revert the patch and then try again later, ok?

Yes, sure.

Quoted 85 lines

- Arnaldo
Quoted 78 lines
tools/perf/util/machine.c +3 -1
index 11de3ca8d4fa7..fef309cd401f7 100644
@@ -2388,7 +2388,9 @@ static int add_callchain_ip(struct thread *thread,
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
 
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
+	if (!branch && ms.map && ms.map->dso &&
+	    strcmp(ms.map->dso->short_name, "[kernel.vmlinux]") &&
+	    append_inlines(cursor, &ms, ip) == 0)
 		goto out;
 
 	srcline = callchain_srcline(&ms, al.addr);
Quoted 59 lines
Quoted 56 lines
- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
 tools/perf/util/machine.c | 5 -----
 1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -5
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
 static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
 				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 
 static struct dso *machine__kernel_dso(struct machine *machine)
 {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
 	ms.maps = maps__get(al.maps);
 	ms.map = map__get(al.map);
 	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
 	srcline = callchain_srcline(&ms, al.addr);
 	err = callchain_cursor_append(cursor, ip, &ms,
 				      branch, flags, nr_loop_iter,
Signature · 1 line
2.41.0
Signature · 1 line
 Artem
Signature · 1 line
 Artem
Signature · 1 line
- Arnaldo

Signature · 1 line

 Artem

Jesper Dangaard Brouer

Tested-by

2023-08-08 17:13 UTC #5 lore raw

Quoted 19 lines · Artem Savkov

Quoted 18 lines · Arnaldo Carvalho de Melo
Em Mon, Aug 07, 2023 at 01:00:08PM +0200, Artem Savkov escreveu:
Quoted 17 lines · Artem Savkov
Quoted 16 lines
Hi Arnaldo,
Quoted 12 lines · Arnaldo Carvalho de Melo
Hi Artem,
Can you please double check this? I reproduced with:
git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.
Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.

I reported this problem to ACME. It is also possible to reproduce without hitting 'q' via using this cmdline with --stdio like this:

time perf report -v --stdio > /dev/null 2> debug01.stderr

The file 'debug01.stderr' contained a lot of addr2line output, that might help debug the issue further.

Quoted 12 lines

Quoted 11 lines
Quoted 3 lines
My bad, I had wrong debuginfo installed for the kernel I tested. I can reproduce it with the correct one. Looks like vmlinux is just too much for addr2line. Maybe we can skip it but leave other inlines in, like so:
That is a possibilit, and probably we could make it cheaper by looking at the cpumode, avoiding calling addr2line when we didn't makage to resolve the symbol, etc.
We also may want to have this as an option that has to be explicitely enabled, like --resolve-inlines, as this will add overhead no matter if we stop calling addr2line and do it more efficiently, etc.

Quoted 37 lines

Sounds good, I'll look into it.
Quoted 2 lines
Fact is, we're late in the 6.5 schedule, so the best thing now is to just revert the patch and then try again later, ok?
Yes, sure.
Quoted 30 lines
- Arnaldo
Quoted 28 lines
tools/perf/util/machine.c +3 -1
index 11de3ca8d4fa7..fef309cd401f7 100644
@@ -2388,7 +2388,9 @@ static int add_callchain_ip(struct thread *thread,
  	ms.map = map__get(al.map);
  	ms.sym = al.sym;
 
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
+	if (!branch && ms.map && ms.map->dso &&
+	    strcmp(ms.map->dso->short_name, "[kernel.vmlinux]") &&
+	    append_inlines(cursor, &ms, ip) == 0)
  		goto out;
 
  	srcline = callchain_srcline(&ms, al.addr);
Quoted 13 lines
Quoted 13 lines
- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.

Reported-by Jesper Dangaard Brouer <[email protected]>

Quoted 27 lines

Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Quoted 27 lines
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
  tools/perf/util/machine.c | 5 -----
  1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -1
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
 
  static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
  				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
 

Quoted 13 lines

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
tools/perf/util/machine.c +0 -0
  static struct dso *machine__kernel_dso(struct machine *machine)
  {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
  	ms.maps = maps__get(al.maps);
  	ms.map = map__get(al.map);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
tools/perf/util/machine.c static int add_callchain_ip(struct thread *thread, · line 2389
Quoted 8 lines
tools/perf/util/machine.c +0 -4
  	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
  	srcline = callchain_srcline(&ms, al.addr);
  	err = callchain_cursor_append(cursor, ip, &ms,
  				      branch, flags, nr_loop_iter,

Tested-by Jesper Dangaard Brouer <[email protected]>

--Jesper

Arnaldo Carvalho de Melo

author maintainer

2023-08-08 17:35 UTC #6 lore raw

Em Tue, Aug 08, 2023 at 03:38:37PM +0200, Jesper Dangaard Brouer escreveu:

Quoted 80 lines · Artem Savkov

Quoted 19 lines · Arnaldo Carvalho de Melo
Quoted 18 lines
Em Mon, Aug 07, 2023 at 01:00:08PM +0200, Artem Savkov escreveu:
Quoted 17 lines · Artem Savkov
Quoted 16 lines
Hi Arnaldo,
Quoted 12 lines · Arnaldo Carvalho de Melo
Hi Artem,
Can you please double check this? I reproduced with:
git checkout 46d21ec067490ab9cdcc89b9de5aae28786a8b8e build it
perf record -a -g sleep 5s
perf report
Do you get the same slowness and then reverting it, i.e. just
going to HEAD~ and rebuilding getting a fast 'perf report' startup, i.e. without the inlines in the callchains?
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 6 lines
With a simple test like this I definitely get a slowdown, but not sure if it can be called excessive.
Below are the times I got by running 'time perf report' and hitting 'q' during load so that it quits as soon as it is loads up. Tested on a freshly updated fedora 38.
I reported this problem to ACME. It is also possible to reproduce without hitting 'q' via using this cmdline with --stdio like this:
time perf report -v --stdio > /dev/null 2> debug01.stderr
The file 'debug01.stderr' contained a lot of addr2line output, that might help debug the issue further.
Quoted 40 lines
Quoted 11 lines
Quoted 3 lines
My bad, I had wrong debuginfo installed for the kernel I tested. I can reproduce it with the correct one. Looks like vmlinux is just too much for addr2line. Maybe we can skip it but leave other inlines in, like so:
That is a possibilit, and probably we could make it cheaper by looking at the cpumode, avoiding calling addr2line when we didn't makage to resolve the symbol, etc.
We also may want to have this as an option that has to be explicitely enabled, like --resolve-inlines, as this will add overhead no matter if we stop calling addr2line and do it more efficiently, etc.
Sounds good, I'll look into it.
Quoted 2 lines
Fact is, we're late in the 6.5 schedule, so the best thing now is to just revert the patch and then try again later, ok?
Yes, sure.
Quoted 21 lines
- Arnaldo
Quoted 20 lines
tools/perf/util/machine.c +3 -1
index 11de3ca8d4fa7..fef309cd401f7 100644
@@ -2388,7 +2388,9 @@ static int add_callchain_ip(struct thread *thread,
  	ms.map = map__get(al.map);
  	ms.sym = al.sym;
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
+	if (!branch && ms.map && ms.map->dso &&
+	    strcmp(ms.map->dso->short_name, "[kernel.vmlinux]") &&
+	    append_inlines(cursor, &ms, ip) == 0)
  		goto out;
  	srcline = callchain_srcline(&ms, al.addr);
 
Quoted 6 lines
Quoted 6 lines
- Arnaldo
----
This reverts commit 46d21ec067490ab9cdcc89b9de5aae28786a8b8e.

Quoted 8 lines

Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
The tests were made with a specific workload, further tests on a recently updated fedora 38 system with a system wide perf.data file shows 'perf report' taking excessive time, so lets revert this until a full investigation and improvement on the addr2line support code is made.
Reported-by Jesper Dangaard Brouer <[email protected]>

Thanks, I did add that locally, will keep it and add your Tested-by, thanks a lot!

- Arnaldo

Quoted 42 lines

Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Quoted 30 lines
Cc Andrii Nakryiko <[email protected]>
Cc Artem Savkov <[email protected]>
Cc Namhyung Kim <[email protected]>
Cc Adrian Hunter <[email protected]>
Cc Alexander Shishkin <[email protected]>
Cc Ian Rogers <[email protected]>
Cc Ingo Molnar <[email protected]>
Cc Jiri Olsa <[email protected]>
Cc Mark Rutland <[email protected]>
Cc Masami Hiramatsu <[email protected]>
Cc Milian Wolff <[email protected]>
Cc Peter Zijlstra <[email protected]>
Signed-off-by Arnaldo Carvalho de Melo <[email protected]>
---
  tools/perf/util/machine.c | 5 -----
  1 file changed, 5 deletions(-)
tools/perf/util/machine.c +0 -1
index 4e62843d51b7dbf9..f4cb41ee23cdbcfc 100644
@@ -45,7 +45,6 @@
  static void __machine__remove_thread(struct machine *machine, struct thread_rb_node *nd,
  				     struct thread *th, bool lock);
-static int append_inlines(struct callchain_cursor *cursor, struct map_symbol *ms, u64 ip);
  static struct dso *machine__kernel_dso(struct machine *machine)
  {
@@ -2385,10 +2384,6 @@ static int add_callchain_ip(struct thread *thread,
  	ms.maps = maps__get(al.maps);
  	ms.map = map__get(al.map);
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
tools/perf/util/machine.c static int add_callchain_ip(struct thread *thread, · line 2389
Quoted 8 lines
tools/perf/util/machine.c +0 -4
  	ms.sym = al.sym;
-
-	if (!branch && append_inlines(cursor, &ms, ip) == 0)
-		goto out;
-
  	srcline = callchain_srcline(&ms, al.addr);
  	err = callchain_cursor_append(cursor, ip, &ms,
  				      branch, flags, nr_loop_iter,
Tested-by Jesper Dangaard Brouer <[email protected]>
--Jesper

Signature · 1 line

- Arnaldo