Messages in this thread | | | Date | Wed, 12 Jul 2023 08:44:14 +0300 | Subject | Re: [PATCH 3/3] perf machine: Include data symbols in the kernel map | From | Adrian Hunter <> |
| |
On 11/07/23 20:30, Namhyung Kim wrote: > Hi Adrian, > > On Tue, Jul 11, 2023 at 8:19 AM Adrian Hunter <adrian.hunter@intel.com> wrote: >> >> On 20/06/23 23:18, Namhyung Kim wrote: >>> When perf record -d is used, it needs data mmaps to symbolize global data. >>> But it missed to collect kernel data maps so it cannot symbolize them. >>> Instead of having a separate map, just increase the kernel map size to >>> include the data section. >>> >>> Probably we can have a separate kernel map for data, but the current >>> code assumes a single kernel map. So it'd require more changes in other >>> places and looks error-prone. I decided not to go that way for now. >>> >>> Also it seems the kernel module size already includes the data section. >>> >>> For example, my system has the following. >>> >>> $ grep -e _stext -e _etext -e _edata /proc/kallsyms >>> ffffffff99800000 T _stext >>> ffffffff9a601ac8 T _etext >>> ffffffff9b446a00 D _edata >>> >>> Size of the text section is (0x9a601ac8 - 0x99800000 = 0xe01ac8) and >>> size of the data section is (0x9b446a00 - 0x99800000 = 0x1c46a00). >>> >>> Before: >>> $ perf record -d true >>> >>> $ perf report -D | grep MMAP | head -1 >>> 0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0xe01ac8) @ 0xffffffff99800000]: x [kernel.kallsyms]_text >>> ^^^^^^^^ >>> here >>> After: >>> $ perf report -D | grep MMAP | head -1 >>> 0 0 0x460 [0x60]: PERF_RECORD_MMAP -1/0: [0xffffffff99800000(0x1c46a00) @ 0xffffffff99800000]: x [kernel.kallsyms]_text >>> ^^^^^^^^^ >>> >>> Signed-off-by: Namhyung Kim <namhyung@kernel.org> >>> --- >>> tools/perf/util/machine.c | 5 ++++- >>> 1 file changed, 4 insertions(+), 1 deletion(-) >>> >>> diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c >>> index ddc0a2130caf..e93a66f6e0b3 100644 >>> --- a/tools/perf/util/machine.c >>> +++ b/tools/perf/util/machine.c >>> @@ -1218,7 +1218,10 @@ static int machine__get_running_kernel_start(struct machine *machine, >>> >>> *start = addr; >>> >>> - err = kallsyms__get_function_start(filename, "_etext", &addr); >>> + if (machine->has_data_mmap) >>> + err = kallsyms__get_symbol_start(filename, "_edata", &addr); >>> + else >>> + err = kallsyms__get_function_start(filename, "_etext", &addr); >> >> What is the downside of just extending it unconditionally? > > I don't know.. maybe some people would argue it needs the > proper protection bits other than 'x' but this patch also breaks it. > But as I said, I'm not sure if we really want to change that now. > > That said, we can make it unconditional. :)
Might as well to start with. Will need a big comment.
Also do we know if all arch's do it like that? Perhaps need to fallback to _etext if _edata is not found?
> > Thanks, > Namhyung > >> >>> if (!err) >>> *end = addr; >>> >>
| |