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
