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

[Namhyung Kim <namhyung@kernel.org>]

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