Re: Perf doesn't display kernel symbols anymore (bisected commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses"))

Re: Perf doesn't display kernel symbols anymore (bisected commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses"))

[Christophe Leroy]

Le 03/01/2025 à 17:26, Arnaldo Carvalho de Melo a écrit :
> On Fri, Jan 03, 2025 at 01:40:24PM +0100, Christophe Leroy wrote:
>> Le 03/01/2025 à 02:08, Arnaldo Carvalho de Melo a écrit :
>>>>      PerfTop:     524 irqs/sec  kernel:51.1%  exact:  0.0% lost: 0/0 drop: 0/0
>>>> [4000Hz cpu-clock:ppp],  (all, 1 CPU)
>>>> -------------------------------------------------------------------------------
> 
>>>>       17.18%  [unknown]      [k] 0xc0891c14
>>>>        7.63%  [unknown]      [k] 0xc005f11c
> 
>>> I think I hadn't notice this '[unknown]' one bit before :-\ the [k] is
>>> there, so having unknown is odd
>   
>> Problem found, it's in maps__find_next_entry(), this leads to both
>> map->start and map->end of kernel map being set to 0xc0000000, which leads
>> to the failure of bsearch() in maps__find().
> 
> Right, and since you don't have any module (CONFIG_MODULES not set),
> and most machines do, that is when the buggy function is called:
> 
> machine__create_kernel_maps()
> 	if (!machine__get_running_kernel_start(machine, &name, &start, &end))
> <SNIP>
>          if (end == ~0ULL) {
>                  /* update end address of the kernel map using adjacent module address */
>                  struct map *next = maps__find_next_entry(machine__kernel_maps(machine),
>                                                           machine__kernel_map(machine));
> 
>                  if (next) {
>                          machine__set_kernel_mmap(machine, start, map__start(next));
>                          map__put(next);
>                  }
>          }
> <SNIP>
> 
> So machine__get_running_kernel_start() doesn't manage to fill end with
> either because it doesn't find the ref_reloc_sym, one of:
> 
> const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL}
> 
> And returns -1, so that first if block fails, and then start also
> doesn't get set and remains 0, which doesn't seem to be the case, as you
> get 0xc0000000 in it, or this fails:
> 
>          err = kallsyms__get_symbol_start(filename, "_edata", &addr);
>          if (err)
>                  err = kallsyms__get_function_start(filename, "_etext", &addr);
>          if (!err)
>                  *end = addr;
> 

Indeed yes that one fails, because:

~# grep -e _stext -e _etext -e _edata /proc/kallsyms
c0000000 T _stext
c08b8000 D _etext

So there is no _edata and _etext is not text

$ ppc-linux-objdump -x vmlinux | grep -e _stext -e _etext -e _edata
c0000000 g       .head.text	00000000 _stext
c08b8000 g       .rodata	00000000 _etext
c1378000 g       .sbss	00000000 _edata

Changing

	kallsyms__get_function_start(filename, "_etext", &addr);

to

	kallsyms__get_symbol_start(filename, "_etext", &addr);

works.


The following change works as well:

diff --git a/arch/powerpc/kernel/vmlinux.lds.S 
b/arch/powerpc/kernel/vmlinux.lds.S
index b4c9decc7a75..b7b2cd7e2a20 100644
--- a/arch/powerpc/kernel/vmlinux.lds.S
+++ b/arch/powerpc/kernel/vmlinux.lds.S
@@ -123,10 +123,11 @@ SECTIONS
  		 */
  		*(.sfpr);
  		*(.text.asan.* .text.tsan.*)
+
+		. = ALIGN(PAGE_SIZE);
+		_etext = .;
  	} :text

-	. = ALIGN(PAGE_SIZE);
-	_etext = .;
  	PROVIDE32 (etext = .);

  	/* Read-only data */

As it leads to:

~# grep -e _stext -e _etext -e _edata /proc/kallsyms
c0000000 T _stext
c08b8000 T _etext

$ ppc-linux-objdump -x vmlinux | grep -e _stext -e _etext -e _edata
c0000000 g       .head.text	00000000 _stext
c08b8000 g       .text	00000000 _etext
c1378000 g       .sbss	00000000 _edata

So what is the most correct fix ? Change architectures link script or 
make perf _etext lookup more flexible, allowing non-text ?

Looking at vmlinux.lds.S from various architectures, I have the feeling 
several of them are affected.

Now, regarding _edata, what I see is:

~# tail -2 /proc/kallsyms
c136a000 D __start___bug_table
c1377c14 D __stop___bug_table

And in System.map I have:

c136a000 D __start___bug_table
c1377c14 D __stop___bug_table
c1378000 B __bss_start
c1378000 B _edata
c1378000 B initcall_debug
c1378004 B reset_devices

Should perf try to locate the very last symbol when it doesn't find 
_edata ? Or should architecture's link script be modified ? Otherwise 
commit 69a87a32f5cd ("perf machine: Include data symbols in the kernel 
map") is just pointless.

Christophe

Re: Perf doesn't display kernel symbols anymore (bisected commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses"))

[Namhyung Kim]

Hello,

On Mon, Jan 06, 2025 at 01:38:52PM +0100, Christophe Leroy wrote:
> 
> 
> Le 03/01/2025 à 17:26, Arnaldo Carvalho de Melo a écrit :
> > On Fri, Jan 03, 2025 at 01:40:24PM +0100, Christophe Leroy wrote:
> > > Le 03/01/2025 à 02:08, Arnaldo Carvalho de Melo a écrit :
> > > > >      PerfTop:     524 irqs/sec  kernel:51.1%  exact:  0.0% lost: 0/0 drop: 0/0
> > > > > [4000Hz cpu-clock:ppp],  (all, 1 CPU)
> > > > > -------------------------------------------------------------------------------
> > 
> > > > >       17.18%  [unknown]      [k] 0xc0891c14
> > > > >        7.63%  [unknown]      [k] 0xc005f11c
> > 
> > > > I think I hadn't notice this '[unknown]' one bit before :-\ the [k] is
> > > > there, so having unknown is odd
> > > Problem found, it's in maps__find_next_entry(), this leads to both
> > > map->start and map->end of kernel map being set to 0xc0000000, which leads
> > > to the failure of bsearch() in maps__find().
> > 
> > Right, and since you don't have any module (CONFIG_MODULES not set),
> > and most machines do, that is when the buggy function is called:
> > 
> > machine__create_kernel_maps()
> > 	if (!machine__get_running_kernel_start(machine, &name, &start, &end))
> > <SNIP>
> >          if (end == ~0ULL) {
> >                  /* update end address of the kernel map using adjacent module address */
> >                  struct map *next = maps__find_next_entry(machine__kernel_maps(machine),
> >                                                           machine__kernel_map(machine));
> > 
> >                  if (next) {
> >                          machine__set_kernel_mmap(machine, start, map__start(next));
> >                          map__put(next);
> >                  }
> >          }
> > <SNIP>
> > 
> > So machine__get_running_kernel_start() doesn't manage to fill end with
> > either because it doesn't find the ref_reloc_sym, one of:
> > 
> > const char *ref_reloc_sym_names[] = {"_text", "_stext", NULL}
> > 
> > And returns -1, so that first if block fails, and then start also
> > doesn't get set and remains 0, which doesn't seem to be the case, as you
> > get 0xc0000000 in it, or this fails:
> > 
> >          err = kallsyms__get_symbol_start(filename, "_edata", &addr);
> >          if (err)
> >                  err = kallsyms__get_function_start(filename, "_etext", &addr);
> >          if (!err)
> >                  *end = addr;
> > 
> 
> Indeed yes that one fails, because:
> 
> ~# grep -e _stext -e _etext -e _edata /proc/kallsyms
> c0000000 T _stext
> c08b8000 D _etext
> 
> So there is no _edata and _etext is not text
> 
> $ ppc-linux-objdump -x vmlinux | grep -e _stext -e _etext -e _edata
> c0000000 g       .head.text	00000000 _stext
> c08b8000 g       .rodata	00000000 _etext
> c1378000 g       .sbss	00000000 _edata
> 
> Changing
> 
> 	kallsyms__get_function_start(filename, "_etext", &addr);
> 
> to
> 
> 	kallsyms__get_symbol_start(filename, "_etext", &addr);
> 
> works.
> 
> 
> The following change works as well:
> 
> diff --git a/arch/powerpc/kernel/vmlinux.lds.S
> b/arch/powerpc/kernel/vmlinux.lds.S
> index b4c9decc7a75..b7b2cd7e2a20 100644
> --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -123,10 +123,11 @@ SECTIONS
>  		 */
>  		*(.sfpr);
>  		*(.text.asan.* .text.tsan.*)
> +
> +		. = ALIGN(PAGE_SIZE);
> +		_etext = .;
>  	} :text
> 
> -	. = ALIGN(PAGE_SIZE);
> -	_etext = .;
>  	PROVIDE32 (etext = .);
> 
>  	/* Read-only data */
> 
> As it leads to:
> 
> ~# grep -e _stext -e _etext -e _edata /proc/kallsyms
> c0000000 T _stext
> c08b8000 T _etext
> 
> $ ppc-linux-objdump -x vmlinux | grep -e _stext -e _etext -e _edata
> c0000000 g       .head.text	00000000 _stext
> c08b8000 g       .text	00000000 _etext
> c1378000 g       .sbss	00000000 _edata
> 
> So what is the most correct fix ? Change architectures link script or make
> perf _etext lookup more flexible, allowing non-text ?
> 
> Looking at vmlinux.lds.S from various architectures, I have the feeling
> several of them are affected.
> 
> Now, regarding _edata, what I see is:
> 
> ~# tail -2 /proc/kallsyms
> c136a000 D __start___bug_table
> c1377c14 D __stop___bug_table
> 
> And in System.map I have:
> 
> c136a000 D __start___bug_table
> c1377c14 D __stop___bug_table
> c1378000 B __bss_start
> c1378000 B _edata
> c1378000 B initcall_debug
> c1378004 B reset_devices
> 
> Should perf try to locate the very last symbol when it doesn't find _edata ?
> Or should architecture's link script be modified ? Otherwise commit
> 69a87a32f5cd ("perf machine: Include data symbols in the kernel map") is
> just pointless.

Let's go with kallsyms__get_symbol_start().  I think it's the most
straight-forward and simplest fix.

Thanks,
Namhyung

Re: Perf doesn't display kernel symbols anymore (bisected commit 659ad3492b91 ("perf maps: Switch from rbtree to lazily sorted array for addresses"))

[Christophe Leroy]

Le 06/01/2025 à 22:46, Namhyung Kim a écrit :
>>
>> And in System.map I have:
>>
>> c136a000 D __start___bug_table
>> c1377c14 D __stop___bug_table
>> c1378000 B __bss_start
>> c1378000 B _edata
>> c1378000 B initcall_debug
>> c1378004 B reset_devices
>>
>> Should perf try to locate the very last symbol when it doesn't find _edata ?
>> Or should architecture's link script be modified ? Otherwise commit
>> 69a87a32f5cd ("perf machine: Include data symbols in the kernel map") is
>> just pointless.
> 
> Let's go with kallsyms__get_symbol_start().  I think it's the most
> straight-forward and simplest fix.
> 

Ok, I did that, see patch 
https://lore.kernel.org/linux-perf-users/b3ee1994d95257cb7f2de037c5030ba7d1bed404.1736327613.git.christophe.leroy@csgroup.eu/T/#u

And for the _edata which is sometimes missing, I send patch 
https://lore.kernel.org/linux-perf-users/2fec8c50c271dff59f0177ff0884b6c374486ba5.1736327770.git.christophe.leroy@csgroup.eu/T/#u

Christophe