Arm and arm64 architecture reserve some memory regions prior to the symbol '_stext' and these memory regions later will be used by device module and BPF jit. The current code misses to consider these memory regions thus any address in the regions will be taken as user space mode, but perf cannot find the corresponding dso with the wrong CPU mode so we misses to generate samples for device module and BPF related trace data.
This patch parse the link scripts to get the memory size prior to start address and reduce this size from 'machine>->kernel_start', then can get a fixed up kernel start address which contain memory regions for device module and BPF. Finally, machine__get_kernel_start() can reflect more complete kernel memory regions and perf can successfully generate samples.
The reason for parsing the link scripts is Arm architecture changes text offset dependent on different platforms, which define multiple text offsets in $kernel/arch/arm/Makefile. This offset is decided when build kernel and the final value is extended in the link script, so we can extract the used value from the link script. We use the same way to parse arm64 link script as well. If fail to find the link script, the pre start memory size is assumed as zero, in this case it has no any change caused with this patch.
Below is detailed info for testing this patch:
- Install or build LLVM/Clang;
- Configure perf with ~/.perfconfig:
root@debian:~# cat ~/.perfconfig # this file is auto-generated. [llvm] clang-path = /mnt/build/llvm-build/build/install/bin/clang kbuild-dir = /mnt/linux-kernel/linux-cs-dev/ clang-opt = "-g" dump-obj = true
[trace] show_zeros = yes show_duration = no no_inherit = yes show_timestamp = no show_arg_names = no args_alignment = 40 show_prefix = yes
- Run 'perf trace' command with eBPF event:
root@debian:~# perf trace -e string \ -e $kernel/tools/perf/examples/bpf/augmented_raw_syscalls.c
- Read eBPF program memory mapping in kernel:
root@debian:~# echo 1 > /proc/sys/net/core/bpf_jit_kallsyms root@debian:~# cat /proc/kallsyms | grep -E "bpf_prog_.+_sys_[enter|exit]" ffff00000008a0d0 t bpf_prog_e470211b846088d5_sys_enter [bpf] ffff00000008c6a4 t bpf_prog_29c7ae234d79bd5c_sys_exit [bpf]
- Launch any program which accesses file system frequently so can hit the system calls trace flow with eBPF event;
- Capture CoreSight trace data with filtering eBPF program:
root@debian:~# perf record -e cs_etm/@tmc_etr0/ \ --filter 'filter 0xffff00000008a0d0/0x800' -a sleep 5s
- Decode the eBPF program symbol 'bpf_prog_f173133dc38ccf87_sys_enter':
root@debian:~# perf script -F,ip,sym Frame deformatter: Found 4 FSYNCS 0 [unknown] ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a250 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a124 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter ffff00000008a13c bpf_prog_e470211b846088d5_sys_enter ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a180 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a190 bpf_prog_e470211b846088d5_sys_enter ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a250 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a124 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a180 bpf_prog_e470211b846088d5_sys_enter [...]
Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Alexander Shishkin alexander.shishkin@linux.intel.com Cc: Jiri Olsa jolsa@redhat.com Cc: Namhyung Kim namhyung@kernel.org Cc: Peter Zijlstra peterz@infradead.org Cc: Suzuki Poulouse suzuki.poulose@arm.com Cc: Adrian Hunter adrian.hunter@intel.com Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Leo Yan leo.yan@linaro.org --- tools/perf/Makefile.config | 22 ++++++++++++++++++++++ tools/perf/util/machine.c | 15 ++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index e4988f49ea79..d7ff839d8b20 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -48,9 +48,20 @@ ifeq ($(SRCARCH),x86) NO_PERF_REGS := 0 endif
+ARM_PRE_START_SIZE := 0 + ifeq ($(SRCARCH),arm) NO_PERF_REGS := 0 LIBUNWIND_LIBS = -lunwind -lunwind-arm + ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),) + # Extract info from lds: + # . = ((0xC0000000)) + 0x00208000; + # ARM_PRE_START_SIZE := 0x00208000 + ARM_PRE_START_SIZE := $(shell egrep ' . = ({2}0x[0-9a-fA-F]+){2}' \ + $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \ + sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ +/ /g' -e 's/^[ \t]*//' | \ + awk -F' ' '{printf "0x%x", $$2}' 2>/dev/null) + endif endif
ifeq ($(SRCARCH),arm64) @@ -58,8 +69,19 @@ ifeq ($(SRCARCH),arm64) NO_SYSCALL_TABLE := 0 CFLAGS += -I$(OUTPUT)arch/arm64/include/generated LIBUNWIND_LIBS = -lunwind -lunwind-aarch64 + ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),) + # Extract info from lds: + # . = ((((((((0xffffffffffffffff)) - (((1)) << (48)) + 1) + (0)) + (0x08000000))) + (0x08000000))) + 0x00080000; + # ARM_PRE_START_SIZE := (0x08000000 + 0x08000000 + 0x00080000) = 0x10080000 + ARM_PRE_START_SIZE := $(shell egrep ' . = ({8}0x[0-9a-fA-F]+){2}' \ + $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \ + sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ +/ /g' -e 's/^[ \t]*//' | \ + awk -F' ' '{printf "0x%x", $$6+$$7+$$8}' 2>/dev/null) + endif endif
+CFLAGS += -DARM_PRE_START_SIZE=$(ARM_PRE_START_SIZE) + ifeq ($(SRCARCH),csky) NO_PERF_REGS := 0 endif diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..e993f891bb82 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2687,13 +2687,26 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map); + if (err) + return err; + /* * On x86_64, PTI entry trampolines are less than the * start of kernel text, but still above 2^63. So leave * kernel_start = 1ULL << 63 for x86_64. */ - if (!err && !machine__is(machine, "x86_64")) + if (!machine__is(machine, "x86_64")) machine->kernel_start = map->start; + + /* + * On arm/arm64, the kernel uses some memory regions which are + * prior to '_stext' symbol; to reflect the complete kernel + * address space, compensate these pre-defined regions for + * kernel start address. + */ + if (!strcmp(perf_env__arch(machine->env), "arm") || + !strcmp(perf_env__arch(machine->env), "arm64")) + machine->kernel_start -= ARM_PRE_START_SIZE; } return err; }
On 15/08/19 11:25 AM, Leo Yan wrote:
Arm and arm64 architecture reserve some memory regions prior to the symbol '_stext' and these memory regions later will be used by device module and BPF jit. The current code misses to consider these memory regions thus any address in the regions will be taken as user space mode, but perf cannot find the corresponding dso with the wrong CPU mode so we misses to generate samples for device module and BPF related trace data.
This patch parse the link scripts to get the memory size prior to start address and reduce this size from 'machine>->kernel_start', then can get a fixed up kernel start address which contain memory regions for device module and BPF. Finally, machine__get_kernel_start() can reflect more complete kernel memory regions and perf can successfully generate samples.
The reason for parsing the link scripts is Arm architecture changes text offset dependent on different platforms, which define multiple text offsets in $kernel/arch/arm/Makefile. This offset is decided when build kernel and the final value is extended in the link script, so we can extract the used value from the link script. We use the same way to parse arm64 link script as well. If fail to find the link script, the pre start memory size is assumed as zero, in this case it has no any change caused with this patch.
Below is detailed info for testing this patch:
Install or build LLVM/Clang;
Configure perf with ~/.perfconfig:
root@debian:~# cat ~/.perfconfig # this file is auto-generated. [llvm] clang-path = /mnt/build/llvm-build/build/install/bin/clang kbuild-dir = /mnt/linux-kernel/linux-cs-dev/ clang-opt = "-g" dump-obj = true
[trace] show_zeros = yes show_duration = no no_inherit = yes show_timestamp = no show_arg_names = no args_alignment = 40 show_prefix = yes
Run 'perf trace' command with eBPF event:
root@debian:~# perf trace -e string \ -e $kernel/tools/perf/examples/bpf/augmented_raw_syscalls.c
Read eBPF program memory mapping in kernel:
root@debian:~# echo 1 > /proc/sys/net/core/bpf_jit_kallsyms root@debian:~# cat /proc/kallsyms | grep -E "bpf_prog_.+_sys_[enter|exit]" ffff00000008a0d0 t bpf_prog_e470211b846088d5_sys_enter [bpf] ffff00000008c6a4 t bpf_prog_29c7ae234d79bd5c_sys_exit [bpf]
Launch any program which accesses file system frequently so can hit the system calls trace flow with eBPF event;
Capture CoreSight trace data with filtering eBPF program:
root@debian:~# perf record -e cs_etm/@tmc_etr0/ \ --filter 'filter 0xffff00000008a0d0/0x800' -a sleep 5s
Decode the eBPF program symbol 'bpf_prog_f173133dc38ccf87_sys_enter':
root@debian:~# perf script -F,ip,sym Frame deformatter: Found 4 FSYNCS 0 [unknown] ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a250 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a124 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter ffff00000008a13c bpf_prog_e470211b846088d5_sys_enter ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a180 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a190 bpf_prog_e470211b846088d5_sys_enter ffff00000008a1ac bpf_prog_e470211b846088d5_sys_enter ffff00000008a250 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a124 bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a14c bpf_prog_e470211b846088d5_sys_enter 0 [unknown] ffff00000008a180 bpf_prog_e470211b846088d5_sys_enter [...]
Cc: Mathieu Poirier mathieu.poirier@linaro.org Cc: Alexander Shishkin alexander.shishkin@linux.intel.com Cc: Jiri Olsa jolsa@redhat.com Cc: Namhyung Kim namhyung@kernel.org Cc: Peter Zijlstra peterz@infradead.org Cc: Suzuki Poulouse suzuki.poulose@arm.com Cc: Adrian Hunter adrian.hunter@intel.com Cc: coresight@lists.linaro.org Cc: linux-arm-kernel@lists.infradead.org Signed-off-by: Leo Yan leo.yan@linaro.org
tools/perf/Makefile.config | 22 ++++++++++++++++++++++ tools/perf/util/machine.c | 15 ++++++++++++++- 2 files changed, 36 insertions(+), 1 deletion(-)
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index e4988f49ea79..d7ff839d8b20 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -48,9 +48,20 @@ ifeq ($(SRCARCH),x86) NO_PERF_REGS := 0 endif +ARM_PRE_START_SIZE := 0
ifeq ($(SRCARCH),arm) NO_PERF_REGS := 0 LIBUNWIND_LIBS = -lunwind -lunwind-arm
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((0xC0000000)) + 0x00208000;
- # ARM_PRE_START_SIZE := 0x00208000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({2}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$2}' 2>/dev/null)- endif
endif ifeq ($(SRCARCH),arm64) @@ -58,8 +69,19 @@ ifeq ($(SRCARCH),arm64) NO_SYSCALL_TABLE := 0 CFLAGS += -I$(OUTPUT)arch/arm64/include/generated LIBUNWIND_LIBS = -lunwind -lunwind-aarch64
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((((((((0xffffffffffffffff)) - (((1)) << (48)) + 1) + (0)) + (0x08000000))) + (0x08000000))) + 0x00080000;
- # ARM_PRE_START_SIZE := (0x08000000 + 0x08000000 + 0x00080000) = 0x10080000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({8}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$6+$$7+$$8}' 2>/dev/null)- endif
So, that is not going to work if you take a perf.data file to a non-arm machine?
How come you cannot use kallsyms to get the information?
endif +CFLAGS += -DARM_PRE_START_SIZE=$(ARM_PRE_START_SIZE)
ifeq ($(SRCARCH),csky) NO_PERF_REGS := 0 endif diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..e993f891bb82 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2687,13 +2687,26 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map);
if (err)return err;- /*
*/
- On x86_64, PTI entry trampolines are less than the
- start of kernel text, but still above 2^63. So leave
- kernel_start = 1ULL << 63 for x86_64.
if (!err && !machine__is(machine, "x86_64"))
if (!machine__is(machine, "x86_64")) machine->kernel_start = map->start;/** On arm/arm64, the kernel uses some memory regions which are* prior to '_stext' symbol; to reflect the complete kernel* address space, compensate these pre-defined regions for* kernel start address.*/if (!strcmp(perf_env__arch(machine->env), "arm") ||!strcmp(perf_env__arch(machine->env), "arm64"))machine->kernel_start -= ARM_PRE_START_SIZE;}
Hi Adrian,
On Thu, Aug 15, 2019 at 11:54:54AM +0300, Adrian Hunter wrote:
[...]
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index e4988f49ea79..d7ff839d8b20 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -48,9 +48,20 @@ ifeq ($(SRCARCH),x86) NO_PERF_REGS := 0 endif +ARM_PRE_START_SIZE := 0
ifeq ($(SRCARCH),arm) NO_PERF_REGS := 0 LIBUNWIND_LIBS = -lunwind -lunwind-arm
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((0xC0000000)) + 0x00208000;
- # ARM_PRE_START_SIZE := 0x00208000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({2}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$2}' 2>/dev/null)- endif
endif ifeq ($(SRCARCH),arm64) @@ -58,8 +69,19 @@ ifeq ($(SRCARCH),arm64) NO_SYSCALL_TABLE := 0 CFLAGS += -I$(OUTPUT)arch/arm64/include/generated LIBUNWIND_LIBS = -lunwind -lunwind-aarch64
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((((((((0xffffffffffffffff)) - (((1)) << (48)) + 1) + (0)) + (0x08000000))) + (0x08000000))) + 0x00080000;
- # ARM_PRE_START_SIZE := (0x08000000 + 0x08000000 + 0x00080000) = 0x10080000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({8}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$6+$$7+$$8}' 2>/dev/null)- endif
So, that is not going to work if you take a perf.data file to a non-arm machine?
Yeah, this patch will only allow perf to work correctly when perf run natively on arm/arm64, so it can resolve partial of the issue.
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps(); so I finally gave up to use machine__create_extra_kernel_maps() for tweaking kernel start address and went back to use this patch's approach by parsing lds files.
So for next step, I want to get some guidances:
- One method is to add a new weak function, e.g. arch__fix_kernel_text_start(), then every arch can implement its own function to fixup the kernel start address;
For arm/arm64, can use kallsyms to find the symbols with least address and fixup for kernel start address.
- Another method is to directly parse kallsyms in the function machine__get_kernel_start(), thus the change can be used for all archs;
Seems to me the second method is to address this issue as a common issue crossing all archs. But not sure if this is the requirement for all archs or just this is only required for arm/arm64. Please let me know what's your preference or other thoughts. Thanks a lot!
Leo.
endif +CFLAGS += -DARM_PRE_START_SIZE=$(ARM_PRE_START_SIZE)
ifeq ($(SRCARCH),csky) NO_PERF_REGS := 0 endif diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..e993f891bb82 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2687,13 +2687,26 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map);
if (err)return err;- /*
*/
- On x86_64, PTI entry trampolines are less than the
- start of kernel text, but still above 2^63. So leave
- kernel_start = 1ULL << 63 for x86_64.
if (!err && !machine__is(machine, "x86_64"))
if (!machine__is(machine, "x86_64")) machine->kernel_start = map->start;/** On arm/arm64, the kernel uses some memory regions which are* prior to '_stext' symbol; to reflect the complete kernel* address space, compensate these pre-defined regions for* kernel start address.*/if (!strcmp(perf_env__arch(machine->env), "arm") ||!strcmp(perf_env__arch(machine->env), "arm64"))machine->kernel_start -= ARM_PRE_START_SIZE;}
On 15/08/19 2:32 PM, Leo Yan wrote:
Hi Adrian,
On Thu, Aug 15, 2019 at 11:54:54AM +0300, Adrian Hunter wrote:
[...]
diff --git a/tools/perf/Makefile.config b/tools/perf/Makefile.config index e4988f49ea79..d7ff839d8b20 100644 --- a/tools/perf/Makefile.config +++ b/tools/perf/Makefile.config @@ -48,9 +48,20 @@ ifeq ($(SRCARCH),x86) NO_PERF_REGS := 0 endif +ARM_PRE_START_SIZE := 0
ifeq ($(SRCARCH),arm) NO_PERF_REGS := 0 LIBUNWIND_LIBS = -lunwind -lunwind-arm
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((0xC0000000)) + 0x00208000;
- # ARM_PRE_START_SIZE := 0x00208000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({2}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$2}' 2>/dev/null)- endif
endif ifeq ($(SRCARCH),arm64) @@ -58,8 +69,19 @@ ifeq ($(SRCARCH),arm64) NO_SYSCALL_TABLE := 0 CFLAGS += -I$(OUTPUT)arch/arm64/include/generated LIBUNWIND_LIBS = -lunwind -lunwind-aarch64
- ifneq ($(wildcard $(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds),)
- # Extract info from lds:
- # . = ((((((((0xffffffffffffffff)) - (((1)) << (48)) + 1) + (0)) + (0x08000000))) + (0x08000000))) + 0x00080000;
- # ARM_PRE_START_SIZE := (0x08000000 + 0x08000000 + 0x00080000) = 0x10080000
- ARM_PRE_START_SIZE := $(shell egrep ' . = ({8}0x[0-9a-fA-F]+){2}' \
$(srctree)/arch/$(SRCARCH)/kernel/vmlinux.lds | \sed -e 's/[(|)|.|=|+|<|;|-]//g' -e 's/ \+/ /g' -e 's/^[ \t]*//' | \awk -F' ' '{printf "0x%x", $$6+$$7+$$8}' 2>/dev/null)- endif
So, that is not going to work if you take a perf.data file to a non-arm machine?
Yeah, this patch will only allow perf to work correctly when perf run natively on arm/arm64, so it can resolve partial of the issue.
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
so I finally gave up to usemachine__create_extra_kernel_maps() for tweaking kernel start address and went back to use this patch's approach by parsing lds files.
So for next step, I want to get some guidances:
One method is to add a new weak function, e.g. arch__fix_kernel_text_start(), then every arch can implement its own function to fixup the kernel start address;
For arm/arm64, can use kallsyms to find the symbols with least address and fixup for kernel start address.
Another method is to directly parse kallsyms in the function machine__get_kernel_start(), thus the change can be used for all archs;
Seems to me the second method is to address this issue as a common issue crossing all archs. But not sure if this is the requirement for all archs or just this is only required for arm/arm64. Please let me know what's your preference or other thoughts. Thanks a lot!
Leo.
endif +CFLAGS += -DARM_PRE_START_SIZE=$(ARM_PRE_START_SIZE)
ifeq ($(SRCARCH),csky) NO_PERF_REGS := 0 endif diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..e993f891bb82 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2687,13 +2687,26 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map);
if (err)return err;- /*
*/
- On x86_64, PTI entry trampolines are less than the
- start of kernel text, but still above 2^63. So leave
- kernel_start = 1ULL << 63 for x86_64.
if (!err && !machine__is(machine, "x86_64"))
if (!machine__is(machine, "x86_64")) machine->kernel_start = map->start;/** On arm/arm64, the kernel uses some memory regions which are* prior to '_stext' symbol; to reflect the complete kernel* address space, compensate these pre-defined regions for* kernel start address.*/if (!strcmp(perf_env__arch(machine->env), "arm") ||!strcmp(perf_env__arch(machine->env), "arm64"))machine->kernel_start -= ARM_PRE_START_SIZE;}
Hi Adrian,
On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
[...]
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
Based on your suggestion, I worked out below change and verified it can work well on arm64 for fixing up start address; please let me know if the change works for you?
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..51d78313dca1 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2671,9 +2671,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; }
+static int machine__fixup_kernel_start(void *arg, + const char *name __maybe_unused, + char type, + u64 start) +{ + struct machine *machine = arg; + + type = toupper(type); + + /* Fixup for text, weak, data and bss sections. */ + if (type == 'T' || type == 'W' || type == 'D' || type == 'B') + machine->kernel_start = min(machine->kernel_start, start); + + return 0; +} + int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine); + char filename[PATH_MAX]; int err = 0;
/* @@ -2687,6 +2704,7 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map); /* * On x86_64, PTI entry trampolines are less than the * start of kernel text, but still above 2^63. So leave @@ -2695,6 +2713,16 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; } + + machine__get_kallsyms_filename(machine, filename, PATH_MAX); + + if (symbol__restricted_filename(filename, "/proc/kallsyms")) + goto out; + + if (kallsyms__parse(filename, machine, machine__fixup_kernel_start)) + pr_warning("Fail to fixup kernel start address. skipping...\n"); + +out: return err; }
Thanks, Leo Yan
On 16/08/19 4:45 AM, Leo Yan wrote:
Hi Adrian,
On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
[...]
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
Based on your suggestion, I worked out below change and verified it can work well on arm64 for fixing up start address; please let me know if the change works for you?
How does that work if take a perf.data file to a machine with a different architecture?
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index f6ee7fbad3e4..51d78313dca1 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2671,9 +2671,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; } +static int machine__fixup_kernel_start(void *arg,
const char *name __maybe_unused,char type,u64 start)+{
- struct machine *machine = arg;
- type = toupper(type);
- /* Fixup for text, weak, data and bss sections. */
- if (type == 'T' || type == 'W' || type == 'D' || type == 'B')
machine->kernel_start = min(machine->kernel_start, start);- return 0;
+}
int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine);
- char filename[PATH_MAX]; int err = 0;
/* @@ -2687,6 +2704,7 @@ int machine__get_kernel_start(struct machine *machine) machine->kernel_start = 1ULL << 63; if (map) { err = map__load(map); /* * On x86_64, PTI entry trampolines are less than the * start of kernel text, but still above 2^63. So leave @@ -2695,6 +2713,16 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; }
- machine__get_kallsyms_filename(machine, filename, PATH_MAX);
- if (symbol__restricted_filename(filename, "/proc/kallsyms"))
goto out;- if (kallsyms__parse(filename, machine, machine__fixup_kernel_start))
pr_warning("Fail to fixup kernel start address. skipping...\n");+out: return err; }
Thanks, Leo Yan
Hi Adrian,
On Fri, Aug 16, 2019 at 04:00:02PM +0300, Adrian Hunter wrote:
On 16/08/19 4:45 AM, Leo Yan wrote:
Hi Adrian,
On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
[...]
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
Based on your suggestion, I worked out below change and verified it can work well on arm64 for fixing up start address; please let me know if the change works for you?
How does that work if take a perf.data file to a machine with a different architecture?
Sorry I delayed so long to respond to your question; I didn't have confidence to give out very reasonale answer and this is the main reason for delaying.
For your question for taking a perf.data file to a machine with a different architecture, we can firstly use command 'perf buildid-list' to print out the buildid for kallsyms, based on the dumped buildid we can find out the location for the saved kallsyms file; then we can use option '--kallsyms' to specify the offline kallsyms file and use the offline kallsyms to fixup kernel start address. The detailed commands are listed as below:
root@debian:~# perf buildid-list 7b36dfca8317ef74974ebd7ee5ec0a8b35c97640 [kernel.kallsyms] 56b84aa88a1bcfe222a97a53698b92723a3977ca /usr/lib/systemd/systemd 0956b952e9cd673d48ff2cfeb1a9dbd0c853e686 /usr/lib/aarch64-linux-gnu/libm-2.28.so [...]
root@debian:~# perf script --kallsyms ~/.debug/[kernel.kallsyms]/7b36dfca8317ef74974ebd7ee5ec0a8b35c97640/kallsyms
The amended patch is as below, please review and always welcome any suggestions or comments!
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 5734460fc89e..593f05cc453f 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2672,9 +2672,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; }
+static int machine__fixup_kernel_start(void *arg, + const char *name __maybe_unused, + char type, + u64 start) +{ + struct machine *machine = arg; + + type = toupper(type); + + /* Fixup for text, weak, data and bss sections. */ + if (type == 'T' || type == 'W' || type == 'D' || type == 'B') + machine->kernel_start = min(machine->kernel_start, start); + + return 0; +} + int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine); + char filename[PATH_MAX]; int err = 0;
/* @@ -2696,6 +2713,22 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; } + + if (symbol_conf.kallsyms_name != NULL) { + strncpy(filename, symbol_conf.kallsyms_name, PATH_MAX); + } else { + machine__get_kallsyms_filename(machine, filename, PATH_MAX); + + if (symbol__restricted_filename(filename, "/proc/kallsyms")) + goto out; + } + + if (kallsyms__parse(filename, machine, machine__fixup_kernel_start)) + pr_warning("Fail to fixup kernel start address. skipping...\n"); + +out: return err; }
Thanks, Leo Yan
Hi Adrian,
On Mon, Aug 26, 2019 at 08:51:05PM +0800, Leo Yan wrote:
Hi Adrian,
On Fri, Aug 16, 2019 at 04:00:02PM +0300, Adrian Hunter wrote:
On 16/08/19 4:45 AM, Leo Yan wrote:
Hi Adrian,
On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
[...]
How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
Based on your suggestion, I worked out below change and verified it can work well on arm64 for fixing up start address; please let me know if the change works for you?
How does that work if take a perf.data file to a machine with a different architecture?
Sorry I delayed so long to respond to your question; I didn't have confidence to give out very reasonale answer and this is the main reason for delaying.
Could you take chance to review my below replying? I'd like to get your confirmation before I send out offical patch.
Thanks, Leo Yan
For your question for taking a perf.data file to a machine with a different architecture, we can firstly use command 'perf buildid-list' to print out the buildid for kallsyms, based on the dumped buildid we can find out the location for the saved kallsyms file; then we can use option '--kallsyms' to specify the offline kallsyms file and use the offline kallsyms to fixup kernel start address. The detailed commands are listed as below:
root@debian:~# perf buildid-list 7b36dfca8317ef74974ebd7ee5ec0a8b35c97640 [kernel.kallsyms] 56b84aa88a1bcfe222a97a53698b92723a3977ca /usr/lib/systemd/systemd 0956b952e9cd673d48ff2cfeb1a9dbd0c853e686 /usr/lib/aarch64-linux-gnu/libm-2.28.so [...]
root@debian:~# perf script --kallsyms ~/.debug/[kernel.kallsyms]/7b36dfca8317ef74974ebd7ee5ec0a8b35c97640/kallsyms
The amended patch is as below, please review and always welcome any suggestions or comments!
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 5734460fc89e..593f05cc453f 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2672,9 +2672,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; } +static int machine__fixup_kernel_start(void *arg,
const char *name __maybe_unused,char type,u64 start)+{
- struct machine *machine = arg;
- type = toupper(type);
- /* Fixup for text, weak, data and bss sections. */
- if (type == 'T' || type == 'W' || type == 'D' || type == 'B')
machine->kernel_start = min(machine->kernel_start, start);- return 0;
+}
int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine);
- char filename[PATH_MAX]; int err = 0;
/* @@ -2696,6 +2713,22 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; }
- if (symbol_conf.kallsyms_name != NULL) {
strncpy(filename, symbol_conf.kallsyms_name, PATH_MAX);- } else {
machine__get_kallsyms_filename(machine, filename, PATH_MAX);if (symbol__restricted_filename(filename, "/proc/kallsyms"))goto out;- }
- if (kallsyms__parse(filename, machine, machine__fixup_kernel_start))
pr_warning("Fail to fixup kernel start address. skipping...\n");+out: return err; }
Thanks, Leo Yan
On 2/09/19 5:15 PM, Leo Yan wrote:
Hi Adrian,
On Mon, Aug 26, 2019 at 08:51:05PM +0800, Leo Yan wrote:
Hi Adrian,
On Fri, Aug 16, 2019 at 04:00:02PM +0300, Adrian Hunter wrote:
On 16/08/19 4:45 AM, Leo Yan wrote:
Hi Adrian,
On Thu, Aug 15, 2019 at 02:45:57PM +0300, Adrian Hunter wrote:
[...]
> How come you cannot use kallsyms to get the information?
Thanks for pointing out this. Sorry I skipped your comment "I don't know how you intend to calculate ARM_PRE_START_SIZE" when you reviewed the patch v3, I should use that chance to elaborate the detailed idea and so can get more feedback/guidance before procceed.
Actually, I have considered to use kallsyms when worked on the previous patch set.
As mentioned in patch set v4's cover letter, I tried to implement machine__create_extra_kernel_maps() for arm/arm64, the purpose is to parse kallsyms so can find more kernel maps and thus also can fixup the kernel start address. But I found the 'perf script' tool directly calls machine__get_kernel_start() instead of running into the flow for machine__create_extra_kernel_maps();
Doesn't it just need to loop through each kernel map to find the lowest start address?
Based on your suggestion, I worked out below change and verified it can work well on arm64 for fixing up start address; please let me know if the change works for you?
How does that work if take a perf.data file to a machine with a different architecture?
Sorry I delayed so long to respond to your question; I didn't have confidence to give out very reasonale answer and this is the main reason for delaying.
Could you take chance to review my below replying? I'd like to get your confirmation before I send out offical patch.
It is not necessary to do kallsyms__parse for x86_64, so it would be better to check the arch before calling that.
However in general, having to copy and use kallsyms with perf.data if on a different arch does not seem very user friendly. But really that is up to Arnaldo.
Thanks, Leo Yan
For your question for taking a perf.data file to a machine with a different architecture, we can firstly use command 'perf buildid-list' to print out the buildid for kallsyms, based on the dumped buildid we can find out the location for the saved kallsyms file; then we can use option '--kallsyms' to specify the offline kallsyms file and use the offline kallsyms to fixup kernel start address. The detailed commands are listed as below:
root@debian:~# perf buildid-list 7b36dfca8317ef74974ebd7ee5ec0a8b35c97640 [kernel.kallsyms] 56b84aa88a1bcfe222a97a53698b92723a3977ca /usr/lib/systemd/systemd 0956b952e9cd673d48ff2cfeb1a9dbd0c853e686 /usr/lib/aarch64-linux-gnu/libm-2.28.so [...]
root@debian:~# perf script --kallsyms ~/.debug/[kernel.kallsyms]/7b36dfca8317ef74974ebd7ee5ec0a8b35c97640/kallsyms
The amended patch is as below, please review and always welcome any suggestions or comments!
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 5734460fc89e..593f05cc453f 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2672,9 +2672,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; } +static int machine__fixup_kernel_start(void *arg,
const char *name __maybe_unused,char type,u64 start)+{
- struct machine *machine = arg;
- type = toupper(type);
- /* Fixup for text, weak, data and bss sections. */
- if (type == 'T' || type == 'W' || type == 'D' || type == 'B')
machine->kernel_start = min(machine->kernel_start, start);- return 0;
+}
int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine);
- char filename[PATH_MAX]; int err = 0;
/* @@ -2696,6 +2713,22 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; }
- if (symbol_conf.kallsyms_name != NULL) {
strncpy(filename, symbol_conf.kallsyms_name, PATH_MAX);- } else {
machine__get_kallsyms_filename(machine, filename, PATH_MAX);if (symbol__restricted_filename(filename, "/proc/kallsyms"))goto out;- }
- if (kallsyms__parse(filename, machine, machine__fixup_kernel_start))
pr_warning("Fail to fixup kernel start address. skipping...\n");+out: return err; }
Thanks, Leo Yan
Hi Adrian,
On Wed, Sep 04, 2019 at 10:26:13AM +0300, Adrian Hunter wrote:
[...]
Could you take chance to review my below replying? I'd like to get your confirmation before I send out offical patch.
It is not necessary to do kallsyms__parse for x86_64, so it would be better to check the arch before calling that.
Thanks for suggestion, will do it in the formal patch.
However in general, having to copy and use kallsyms with perf.data if on a different arch does not seem very user friendly.
Agree. Seems it's more reasonable to save related info in perf.data; TBH, I have no idea for how to do that.
But really that is up to Arnaldo.
@Arnaldo, if possible could you take a look for below change?
If you don't think below code is the right thing and it's not a common issue, then maybe it's more feasible to resolve this issue only for Arm CoreSight specific.
Please let me know how about you think for this?
Thanks, Leo Yan
For your question for taking a perf.data file to a machine with a different architecture, we can firstly use command 'perf buildid-list' to print out the buildid for kallsyms, based on the dumped buildid we can find out the location for the saved kallsyms file; then we can use option '--kallsyms' to specify the offline kallsyms file and use the offline kallsyms to fixup kernel start address. The detailed commands are listed as below:
root@debian:~# perf buildid-list 7b36dfca8317ef74974ebd7ee5ec0a8b35c97640 [kernel.kallsyms] 56b84aa88a1bcfe222a97a53698b92723a3977ca /usr/lib/systemd/systemd 0956b952e9cd673d48ff2cfeb1a9dbd0c853e686 /usr/lib/aarch64-linux-gnu/libm-2.28.so [...]
root@debian:~# perf script --kallsyms ~/.debug/[kernel.kallsyms]/7b36dfca8317ef74974ebd7ee5ec0a8b35c97640/kallsyms
The amended patch is as below, please review and always welcome any suggestions or comments!
diff --git a/tools/perf/util/machine.c b/tools/perf/util/machine.c index 5734460fc89e..593f05cc453f 100644 --- a/tools/perf/util/machine.c +++ b/tools/perf/util/machine.c @@ -2672,9 +2672,26 @@ int machine__nr_cpus_avail(struct machine *machine) return machine ? perf_env__nr_cpus_avail(machine->env) : 0; } +static int machine__fixup_kernel_start(void *arg,
const char *name __maybe_unused,char type,u64 start)+{
- struct machine *machine = arg;
- type = toupper(type);
- /* Fixup for text, weak, data and bss sections. */
- if (type == 'T' || type == 'W' || type == 'D' || type == 'B')
machine->kernel_start = min(machine->kernel_start, start);- return 0;
+}
int machine__get_kernel_start(struct machine *machine) { struct map *map = machine__kernel_map(machine);
- char filename[PATH_MAX]; int err = 0;
/* @@ -2696,6 +2713,22 @@ int machine__get_kernel_start(struct machine *machine) if (!err && !machine__is(machine, "x86_64")) machine->kernel_start = map->start; }
- if (symbol_conf.kallsyms_name != NULL) {
strncpy(filename, symbol_conf.kallsyms_name, PATH_MAX);- } else {
machine__get_kallsyms_filename(machine, filename, PATH_MAX);if (symbol__restricted_filename(filename, "/proc/kallsyms"))goto out;- }
- if (kallsyms__parse(filename, machine, machine__fixup_kernel_start))
pr_warning("Fail to fixup kernel start address. skipping...\n");+out: return err; }
Thanks, Leo Yan
-
Adrian Hunter -
Leo Yan