[v2] perf: Fix display of kernel symbols

[PATCH v2] perf: Fix display of kernel symbols

Posted by Christophe Leroy 11 months, 2 weeks ago

Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
sorted array for addresses"), perf doesn't display anymore kernel
symbols on powerpc, allthough it still detects them as kernel addresses.

	# Overhead  Command     Shared Object  Symbol
	# ........  ..........  ............. ......................................
	#
	    80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
	     3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
	     1.72%  Coeur main  [unknown]      [k] 0xc005f11c
	     1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
	     0.44%  Coeur main  libc.so.6      [.] epoll_wait
	     0.38%  Coeur main  [unknown]      [k] 0xc0011718
	     0.36%  Coeur main  [unknown]      [k] 0xc01f45c0

This is because function maps__find_next_entry() now returns current
entry instead of next entry, leading to kernel map end address
getting mis-configured with its own start address instead of the
start address of the following map.

Fix it by really taking the next entry, also make sure that entry
follows current one by making sure entries are sorted.

Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
v2: Make sure the entries are sorted, if not sort them.
---
 tools/perf/util/maps.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
index 432399cbe5dd..09c9cc326c08 100644
--- a/tools/perf/util/maps.c
+++ b/tools/perf/util/maps.c
@@ -1136,8 +1136,13 @@ struct map *maps__find_next_entry(struct maps *maps, struct map *map)
 	struct map *result = NULL;
 
 	down_read(maps__lock(maps));
+	while (!maps__maps_by_address_sorted(maps)) {
+		up_read(maps__lock(maps));
+		maps__sort_by_address(maps);
+		down_read(maps__lock(maps));
+	}
 	i = maps__by_address_index(maps, map);
-	if (i < maps__nr_maps(maps))
+	if (++i < maps__nr_maps(maps))
 		result = map__get(maps__maps_by_address(maps)[i]);
 
 	up_read(maps__lock(maps));
-- 
2.47.0

Re: [PATCH v2] perf: Fix display of kernel symbols

Posted by Ian Rogers 11 months, 2 weeks ago

On Wed, Jan 8, 2025 at 1:54 AM Christophe Leroy
<christophe.leroy@csgroup.eu> wrote:
>
> Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
> sorted array for addresses"), perf doesn't display anymore kernel
> symbols on powerpc, allthough it still detects them as kernel addresses.
>
>         # Overhead  Command     Shared Object  Symbol
>         # ........  ..........  ............. ......................................
>         #
>             80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
>              3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
>              1.72%  Coeur main  [unknown]      [k] 0xc005f11c
>              1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
>              0.44%  Coeur main  libc.so.6      [.] epoll_wait
>              0.38%  Coeur main  [unknown]      [k] 0xc0011718
>              0.36%  Coeur main  [unknown]      [k] 0xc01f45c0
>
> This is because function maps__find_next_entry() now returns current
> entry instead of next entry, leading to kernel map end address
> getting mis-configured with its own start address instead of the
> start address of the following map.
>
> Fix it by really taking the next entry, also make sure that entry
> follows current one by making sure entries are sorted.
>
> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks!
Ian

> ---
> v2: Make sure the entries are sorted, if not sort them.
> ---
>  tools/perf/util/maps.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 432399cbe5dd..09c9cc326c08 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -1136,8 +1136,13 @@ struct map *maps__find_next_entry(struct maps *maps, struct map *map)
>         struct map *result = NULL;
>
>         down_read(maps__lock(maps));
> +       while (!maps__maps_by_address_sorted(maps)) {
> +               up_read(maps__lock(maps));
> +               maps__sort_by_address(maps);
> +               down_read(maps__lock(maps));
> +       }
>         i = maps__by_address_index(maps, map);
> -       if (i < maps__nr_maps(maps))
> +       if (++i < maps__nr_maps(maps))
>                 result = map__get(maps__maps_by_address(maps)[i]);
>
>         up_read(maps__lock(maps));
> --
> 2.47.0
>

Re: [PATCH v2] perf: Fix display of kernel symbols

Posted by Arnaldo Carvalho de Melo 11 months, 2 weeks ago

On Wed, Jan 08, 2025 at 10:54:20AM +0100, Christophe Leroy wrote:
> Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
> sorted array for addresses"), perf doesn't display anymore kernel
> symbols on powerpc, allthough it still detects them as kernel addresses.
> 
> 	# Overhead  Command     Shared Object  Symbol
> 	# ........  ..........  ............. ......................................
> 	#
> 	    80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
> 	     3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
> 	     1.72%  Coeur main  [unknown]      [k] 0xc005f11c
> 	     1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
> 	     0.44%  Coeur main  libc.so.6      [.] epoll_wait
> 	     0.38%  Coeur main  [unknown]      [k] 0xc0011718
> 	     0.36%  Coeur main  [unknown]      [k] 0xc01f45c0
> 
> This is because function maps__find_next_entry() now returns current
> entry instead of next entry, leading to kernel map end address
> getting mis-configured with its own start address instead of the
> start address of the following map.
> 
> Fix it by really taking the next entry, also make sure that entry
> follows current one by making sure entries are sorted.
> 
> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> ---
> v2: Make sure the entries are sorted, if not sort them.

Since you have changed what I reviewed I'll have to re-review :-) Will
try to do it after some calls.

- Arnaldo

> ---
>  tools/perf/util/maps.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/maps.c b/tools/perf/util/maps.c
> index 432399cbe5dd..09c9cc326c08 100644
> --- a/tools/perf/util/maps.c
> +++ b/tools/perf/util/maps.c
> @@ -1136,8 +1136,13 @@ struct map *maps__find_next_entry(struct maps *maps, struct map *map)
>  	struct map *result = NULL;
>  
>  	down_read(maps__lock(maps));
> +	while (!maps__maps_by_address_sorted(maps)) {
> +		up_read(maps__lock(maps));
> +		maps__sort_by_address(maps);
> +		down_read(maps__lock(maps));
> +	}
>  	i = maps__by_address_index(maps, map);
> -	if (i < maps__nr_maps(maps))
> +	if (++i < maps__nr_maps(maps))
>  		result = map__get(maps__maps_by_address(maps)[i]);
>  
>  	up_read(maps__lock(maps));
> -- 
> 2.47.0

Re: [PATCH v2] perf: Fix display of kernel symbols

Posted by Christophe Leroy 11 months, 2 weeks ago


Le 08/01/2025 à 15:53, Arnaldo Carvalho de Melo a écrit :
> On Wed, Jan 08, 2025 at 10:54:20AM +0100, Christophe Leroy wrote:
>> Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
>> sorted array for addresses"), perf doesn't display anymore kernel
>> symbols on powerpc, allthough it still detects them as kernel addresses.
>>
>> 	# Overhead  Command     Shared Object  Symbol
>> 	# ........  ..........  ............. ......................................
>> 	#
>> 	    80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
>> 	     3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
>> 	     1.72%  Coeur main  [unknown]      [k] 0xc005f11c
>> 	     1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
>> 	     0.44%  Coeur main  libc.so.6      [.] epoll_wait
>> 	     0.38%  Coeur main  [unknown]      [k] 0xc0011718
>> 	     0.36%  Coeur main  [unknown]      [k] 0xc01f45c0
>>
>> This is because function maps__find_next_entry() now returns current
>> entry instead of next entry, leading to kernel map end address
>> getting mis-configured with its own start address instead of the
>> start address of the following map.
>>
>> Fix it by really taking the next entry, also make sure that entry
>> follows current one by making sure entries are sorted.
>>
>> Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
>> Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
>> Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
>> ---
>> v2: Make sure the entries are sorted, if not sort them.
> 
> Since you have changed what I reviewed I'll have to re-review :-) Will
> try to do it after some calls.

Ah yes sorry, should have removed your Reviewed-by.

Based on Ian's feedback "Using the next entry in this way won't work if 
the entries aren't sorted", I added the following block in front of the 
initial change:

+	while (!maps__maps_by_address_sorted(maps)) {
+		up_read(maps__lock(maps));
+		maps__sort_by_address(maps);
+		down_read(maps__lock(maps));
+	}

Christophe

Re: [PATCH v2] perf: Fix display of kernel symbols

Posted by Arnaldo Carvalho de Melo 11 months, 2 weeks ago

On Wed, Jan 08, 2025 at 06:06:03PM +0100, Christophe Leroy wrote:
> 
> 
> Le 08/01/2025 à 15:53, Arnaldo Carvalho de Melo a écrit :
> > On Wed, Jan 08, 2025 at 10:54:20AM +0100, Christophe Leroy wrote:
> > > Since commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily
> > > sorted array for addresses"), perf doesn't display anymore kernel
> > > symbols on powerpc, allthough it still detects them as kernel addresses.
> > > 
> > > 	# Overhead  Command     Shared Object  Symbol
> > > 	# ........  ..........  ............. ......................................
> > > 	#
> > > 	    80.49%  Coeur main  [unknown]      [k] 0xc005f0f8
> > > 	     3.91%  Coeur main  gau            [.] engine_loop.constprop.0.isra.0
> > > 	     1.72%  Coeur main  [unknown]      [k] 0xc005f11c
> > > 	     1.09%  Coeur main  [unknown]      [k] 0xc01f82c8
> > > 	     0.44%  Coeur main  libc.so.6      [.] epoll_wait
> > > 	     0.38%  Coeur main  [unknown]      [k] 0xc0011718
> > > 	     0.36%  Coeur main  [unknown]      [k] 0xc01f45c0
> > > 
> > > This is because function maps__find_next_entry() now returns current
> > > entry instead of next entry, leading to kernel map end address
> > > getting mis-configured with its own start address instead of the
> > > start address of the following map.
> > > 
> > > Fix it by really taking the next entry, also make sure that entry
> > > follows current one by making sure entries are sorted.
> > > 
> > > Fixes: 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses")
> > > Signed-off-by: Christophe Leroy <christophe.leroy@csgroup.eu>
> > > Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
> > > ---
> > > v2: Make sure the entries are sorted, if not sort them.
> > 
> > Since you have changed what I reviewed I'll have to re-review :-) Will
> > try to do it after some calls.
> 
> Ah yes sorry, should have removed your Reviewed-by.
> 
> Based on Ian's feedback "Using the next entry in this way won't work if the
> entries aren't sorted", I added the following block in front of the initial
> change:
> 
> +	while (!maps__maps_by_address_sorted(maps)) {
> +		up_read(maps__lock(maps));
> +		maps__sort_by_address(maps);
> +		down_read(maps__lock(maps));
> +	}

Its ok, I'll keep it now that I looked at it.

Thanks!

- Arnaldo