Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On 2017/03/15 09:11AM, Steven Rostedt wrote: > On Wed, 15 Mar 2017 14:35:16 +0530 > "Naveen N. Rao" wrote: > > > I don't have a strong opinion about this, but I feel that x86 can simply > > use ftrace_64.S, seeing as the current name is mcount_64.S. > > > > Other architectures could do something similar too, or fall back to > > ftrace_hook.S. That way, all ftrace low-level code can simply be > > referred to as arch/*/ftrace_*.S > > Just to clarify, I'm currently working on patches to clean up the > ftrace.S code in x86, and I renamed mcount_64.S to ftrace_64.S. I'm > also moving the ftrace code out of entry_32.S into a ftrace_32.S file > as well. Patches will hopefully be posted soon. Good to know, thanks for clarifying. In light of that, I hope that you're ok with the changes proposed to the ftrace bits under arch/powerpc in this patchset? - Naveen
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On Wed, 15 Mar 2017 14:35:16 +0530 "Naveen N. Rao" wrote: > I don't have a strong opinion about this, but I feel that x86 can simply > use ftrace_64.S, seeing as the current name is mcount_64.S. > > Other architectures could do something similar too, or fall back to > ftrace_hook.S. That way, all ftrace low-level code can simply be > referred to as arch/*/ftrace_*.S Just to clarify, I'm currently working on patches to clean up the ftrace.S code in x86, and I renamed mcount_64.S to ftrace_64.S. I'm also moving the ftrace code out of entry_32.S into a ftrace_32.S file as well. Patches will hopefully be posted soon. -- Steve
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On 2017/03/10 11:54AM, Steven Rostedt wrote: > On Fri, 10 Mar 2017 21:38:53 +0530 > "Naveen N. Rao" wrote: > > > On 2017/03/10 10:45AM, Steven Rostedt wrote: > > > On Thu, 02 Mar 2017 20:38:53 +1100 > > > Michael Ellerman wrote: > > > > > So if we drop that we're left with ftrace.S - which seems perfect to > > > > me. > > > > > > Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S > > > will get the same ftrace.o. Maybe make it ftrace-hook.S ? > > > > I've avoided that issue by naming the files ftrace_32.S and ftrace_64.S > > (which gets further split up). > > That's what I looked at doing for x86 as well. But not all archs have > 32 / 64 splits. Should we look to have something that all archs can be > consistent with? I don't have a strong opinion about this, but I feel that x86 can simply use ftrace_64.S, seeing as the current name is mcount_64.S. Other architectures could do something similar too, or fall back to ftrace_hook.S. That way, all ftrace low-level code can simply be referred to as arch/*/ftrace_*.S - Naveen
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On Fri, 10 Mar 2017 21:38:53 +0530 "Naveen N. Rao" wrote: > On 2017/03/10 10:45AM, Steven Rostedt wrote: > > On Thu, 02 Mar 2017 20:38:53 +1100 > > Michael Ellerman wrote: > > > So if we drop that we're left with ftrace.S - which seems perfect to me. > > > > Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S > > will get the same ftrace.o. Maybe make it ftrace-hook.S ? > > I've avoided that issue by naming the files ftrace_32.S and ftrace_64.S > (which gets further split up). That's what I looked at doing for x86 as well. But not all archs have 32 / 64 splits. Should we look to have something that all archs can be consistent with? -- Steve
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On 2017/03/10 10:45AM, Steven Rostedt wrote: > On Thu, 02 Mar 2017 20:38:53 +1100 > Michael Ellerman wrote: > > > Steven Rostedt writes: > > > > > On Tue, 28 Feb 2017 15:04:15 +1100 > > > Michael Ellerman wrote: > > > > > > kernel/trace/ftrace.c more obvious. > > >> > > >> I don't know if it's really worth keeping the names the same across > > >> arches, especially as we already have: > > >> > > >> arch/arm64/kernel/entry-ftrace.S > > >> arch/arm/kernel/entry-ftrace.S > > >> arch/blackfin/kernel/ftrace-entry.S > > >> arch/metag/kernel/ftrace_stub.S > > >> > > >> But we can rename it if you feel strongly about it. > > > > > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked > > > the "mcount.S" name. > > > > Except what does the "entry" part mean? > > > > Traditionally entry.S has been for the code that "enters" the kernel, > > ie. from userspace or elsewhere. But that's not the case with any of the > > ftrace code, it's kernel code called from the kernel. So using "entry" > > is a bit wrong IMHO. > > > > So if we drop that we're left with ftrace.S - which seems perfect to me. > > Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S > will get the same ftrace.o. Maybe make it ftrace-hook.S ? I've avoided that issue by naming the files ftrace_32.S and ftrace_64.S (which gets further split up). Thanks, Naveen
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On Thu, 02 Mar 2017 20:38:53 +1100 Michael Ellerman wrote: > Steven Rostedt writes: > > > On Tue, 28 Feb 2017 15:04:15 +1100 > > Michael Ellerman wrote: > > > > kernel/trace/ftrace.c more obvious. > >> > >> I don't know if it's really worth keeping the names the same across > >> arches, especially as we already have: > >> > >> arch/arm64/kernel/entry-ftrace.S > >> arch/arm/kernel/entry-ftrace.S > >> arch/blackfin/kernel/ftrace-entry.S > >> arch/metag/kernel/ftrace_stub.S > >> > >> But we can rename it if you feel strongly about it. > > > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked > > the "mcount.S" name. > > Except what does the "entry" part mean? > > Traditionally entry.S has been for the code that "enters" the kernel, > ie. from userspace or elsewhere. But that's not the case with any of the > ftrace code, it's kernel code called from the kernel. So using "entry" > is a bit wrong IMHO. > > So if we drop that we're left with ftrace.S - which seems perfect to me. Yeah, I agree. But then there's the problem that ftrace.c and ftrace.S will get the same ftrace.o. Maybe make it ftrace-hook.S ? -- Steve
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On 2017/03/02 08:38PM, Michael Ellerman wrote: > Steven Rostedt writes: > > > On Tue, 28 Feb 2017 15:04:15 +1100 > > Michael Ellerman wrote: > > > > kernel/trace/ftrace.c more obvious. > >> > >> I don't know if it's really worth keeping the names the same across > >> arches, especially as we already have: > >> > >> arch/arm64/kernel/entry-ftrace.S > >> arch/arm/kernel/entry-ftrace.S > >> arch/blackfin/kernel/ftrace-entry.S > >> arch/metag/kernel/ftrace_stub.S > >> > >> But we can rename it if you feel strongly about it. > > > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked > > the "mcount.S" name. > > Except what does the "entry" part mean? > > Traditionally entry.S has been for the code that "enters" the kernel, > ie. from userspace or elsewhere. But that's not the case with any of the > ftrace code, it's kernel code called from the kernel. So using "entry" > is a bit wrong IMHO. > > So if we drop that we're left with ftrace.S - which seems perfect to me. Hi Steve, Are you ok with this? I'd prefer to not add the 'entry-' prefix too, seeing as it will make the file names quite long without necessarily adding much. Thanks, Naveen
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
Steven Rostedt writes: > On Tue, 28 Feb 2017 15:04:15 +1100 > Michael Ellerman wrote: > > kernel/trace/ftrace.c more obvious. >> >> I don't know if it's really worth keeping the names the same across >> arches, especially as we already have: >> >> arch/arm64/kernel/entry-ftrace.S >> arch/arm/kernel/entry-ftrace.S >> arch/blackfin/kernel/ftrace-entry.S >> arch/metag/kernel/ftrace_stub.S >> >> But we can rename it if you feel strongly about it. > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked > the "mcount.S" name. Except what does the "entry" part mean? Traditionally entry.S has been for the code that "enters" the kernel, ie. from userspace or elsewhere. But that's not the case with any of the ftrace code, it's kernel code called from the kernel. So using "entry" is a bit wrong IMHO. So if we drop that we're left with ftrace.S - which seems perfect to me. cheers
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On 2017/02/28 09:11AM, Steven Rostedt wrote: > On Tue, 28 Feb 2017 15:04:15 +1100 > Michael Ellerman wrote: > > kernel/trace/ftrace.c more obvious. > > > > I don't know if it's really worth keeping the names the same across > > arches, especially as we already have: > > > > arch/arm64/kernel/entry-ftrace.S > > arch/arm/kernel/entry-ftrace.S > > arch/blackfin/kernel/ftrace-entry.S > > arch/metag/kernel/ftrace_stub.S > > > > But we can rename it if you feel strongly about it. > > Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked > the "mcount.S" name. Ok, just to be sure I get this right -- do you want me to name the powerpc files entry_ftrace_64.S (and subsequently to a slightly long entry_ftrace_64_mprofile.S), or rename the x86 mcount* files? Or both? Thanks for the review, - Naveen
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On Tue, 28 Feb 2017 15:04:15 +1100 Michael Ellerman wrote: kernel/trace/ftrace.c more obvious. > > I don't know if it's really worth keeping the names the same across > arches, especially as we already have: > > arch/arm64/kernel/entry-ftrace.S > arch/arm/kernel/entry-ftrace.S > arch/blackfin/kernel/ftrace-entry.S > arch/metag/kernel/ftrace_stub.S > > But we can rename it if you feel strongly about it. Hmm, perhaps "entry-ftrace.S" would be the better name. I never liked the "mcount.S" name. -- Steve
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
Steven Rostedt writes: > On Wed, 22 Feb 2017 00:31:01 +0530 > "Naveen N. Rao" wrote: > >> entry_*.S now includes a lot more than just kernel entry/exit code. As a >> first step at cleaning this up, let's split out the ftrace bits into >> separate files. Also move all related tracing code into a new trace/ >> subdirectory. >> >> No functional changes. > > I wonder if we should stay consistent among archs, and call these files > "mcount_*.S". Or perhaps we should change x86 from mcount_64.S to > ftrace_64.S? I prefer ftrace_64.S, there's a lot more in those files than just the mcount() implementation, and it also makes the link to kernel/trace/ftrace.c more obvious. I don't know if it's really worth keeping the names the same across arches, especially as we already have: arch/arm64/kernel/entry-ftrace.S arch/arm/kernel/entry-ftrace.S arch/blackfin/kernel/ftrace-entry.S arch/metag/kernel/ftrace_stub.S But we can rename it if you feel strongly about it. cheers
Re: [PATCH v3 1/2] powerpc: split ftrace bits into a separate file
On Wed, 22 Feb 2017 00:31:01 +0530 "Naveen N. Rao" wrote: > entry_*.S now includes a lot more than just kernel entry/exit code. As a > first step at cleaning this up, let's split out the ftrace bits into > separate files. Also move all related tracing code into a new trace/ > subdirectory. > > No functional changes. I wonder if we should stay consistent among archs, and call these files "mcount_*.S". Or perhaps we should change x86 from mcount_64.S to ftrace_64.S? -- Steve
[PATCH v3 1/2] powerpc: split ftrace bits into a separate file
entry_*.S now includes a lot more than just kernel entry/exit code. As a
first step at cleaning this up, let's split out the ftrace bits into
separate files. Also move all related tracing code into a new trace/
subdirectory.
No functional changes.
Suggested-by: Michael Ellerman
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/Makefile | 9 +-
arch/powerpc/kernel/entry_32.S| 107 ---
arch/powerpc/kernel/entry_64.S| 380 -
arch/powerpc/kernel/trace/Makefile| 24 ++
arch/powerpc/kernel/{ => trace}/ftrace.c | 0
arch/powerpc/kernel/trace/ftrace_32.S | 118
arch/powerpc/kernel/trace/ftrace_64.S | 391 ++
arch/powerpc/kernel/{ => trace}/trace_clock.c | 0
8 files changed, 534 insertions(+), 495 deletions(-)
create mode 100644 arch/powerpc/kernel/trace/Makefile
rename arch/powerpc/kernel/{ => trace}/ftrace.c (100%)
create mode 100644 arch/powerpc/kernel/trace/ftrace_32.S
create mode 100644 arch/powerpc/kernel/trace/ftrace_64.S
rename arch/powerpc/kernel/{ => trace}/trace_clock.c (100%)
diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index a048b37b9b27..e1a04237f09d 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -29,8 +29,6 @@ CFLAGS_REMOVE_cputable.o = -mno-sched-epilog
$(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom_init.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_btext.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
CFLAGS_REMOVE_prom.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
-# do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
# timers used by tracing
CFLAGS_REMOVE_time.o = -mno-sched-epilog $(CC_FLAGS_FTRACE)
endif
@@ -122,10 +120,7 @@ obj64-$(CONFIG_AUDIT) += compat_audit.o
obj-$(CONFIG_PPC_IO_WORKAROUNDS) += io-workarounds.o
-obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER)+= ftrace.o
-obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
-obj-$(CONFIG_TRACING) += trace_clock.o
+obj-y += trace/
ifneq ($(CONFIG_PPC_INDIRECT_PIO),y)
obj-y += iomap.o
@@ -146,8 +141,6 @@ obj-$(CONFIG_KVM_GUEST) += kvm.o kvm_emul.o
# Disable GCOV & sanitizers in odd or sensitive code
GCOV_PROFILE_prom_init.o := n
UBSAN_SANITIZE_prom_init.o := n
-GCOV_PROFILE_ftrace.o := n
-UBSAN_SANITIZE_ftrace.o := n
GCOV_PROFILE_machine_kexec_64.o := n
UBSAN_SANITIZE_machine_kexec_64.o := n
GCOV_PROFILE_machine_kexec_32.o := n
diff --git a/arch/powerpc/kernel/entry_32.S b/arch/powerpc/kernel/entry_32.S
index f3e4fc1c1b4d..5a2a13bd2193 100644
--- a/arch/powerpc/kernel/entry_32.S
+++ b/arch/powerpc/kernel/entry_32.S
@@ -31,7 +31,6 @@
#include
#include
#include
-#include
#include
#include
@@ -1319,109 +1318,3 @@ machine_check_in_rtas:
/* XXX load up BATs and panic */
#endif /* CONFIG_PPC_RTAS */
-
-#ifdef CONFIG_FUNCTION_TRACER
-#ifdef CONFIG_DYNAMIC_FTRACE
-_GLOBAL(mcount)
-_GLOBAL(_mcount)
- /*
-* It is required that _mcount on PPC32 must preserve the
-* link register. But we have r0 to play with. We use r0
-* to push the return address back to the caller of mcount
-* into the ctr register, restore the link register and
-* then jump back using the ctr register.
-*/
- mflrr0
- mtctr r0
- lwz r0, 4(r1)
- mtlrr0
- bctr
-
-_GLOBAL(ftrace_caller)
- MCOUNT_SAVE_FRAME
- /* r3 ends up with link register */
- subir3, r3, MCOUNT_INSN_SIZE
-.globl ftrace_call
-ftrace_call:
- bl ftrace_stub
- nop
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-.globl ftrace_graph_call
-ftrace_graph_call:
- b ftrace_graph_stub
-_GLOBAL(ftrace_graph_stub)
-#endif
- MCOUNT_RESTORE_FRAME
- /* old link register ends up in ctr reg */
- bctr
-#else
-_GLOBAL(mcount)
-_GLOBAL(_mcount)
-
- MCOUNT_SAVE_FRAME
-
- subir3, r3, MCOUNT_INSN_SIZE
- LOAD_REG_ADDR(r5, ftrace_trace_function)
- lwz r5,0(r5)
-
- mtctr r5
- bctrl
- nop
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
- b ftrace_graph_caller
-#endif
- MCOUNT_RESTORE_FRAME
- bctr
-#endif
-EXPORT_SYMBOL(_mcount)
-
-_GLOBAL(ftrace_stub)
- blr
-
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-_GLOBAL(ftrace_graph_caller)
- /* load r4 with local address */
- lwz r4, 44(r1)
- subir4, r4, MCOUNT_INSN_SIZE
-
- /* Grab the LR out of the caller stack frame */
- lwz r3,52(r1)
-
- bl prepare_ftrace_return
- nop
-
-/*
- * prepare_ftrace_return gives us the address we divert to.
- * Change the LR in the callers stack frame to this.
- */
- stw r3,52(r1)
-
- MCOUNT_RESTORE_FRAME
- /* old link register ends up in c
