Skip to contentSkip to conversation

x86: entry_64 - introduce FTRACE_ frame macro

RFC In review Cyrill Gorcunov 4 participants 17 messages Dec 12, 2008 – Dec 13, 2008

Subsystems (2)
x86 ftrace
lore
Share & tools
lore

Cyrill Gorcunov

author
#0 lore raw

RFC 1/1 x86: entry_64 - introduce FTRACE_ frame macro

Impact: clean up

Itroduce FTRACE_SAVE/RESTORE_FRAME which allow us to save a number of lines on source level.

Also fix a comment in ftrace.h.

---
Actually I'm not that sure about bringing new macros there since we already have a lot so feel free to just drop it.

Maybe it would be good to comment args passed
to ftrace_call on ABI level :)

Ah, the patch is on top of -tip e3bdf48

 arch/x86/include/asm/ftrace.h |    2 -
 arch/x86/kernel/entry_64.S    |   70 +++++++++++++++---------------------------
 2 files changed, 26 insertions(+), 46 deletions(-)
arch/x86/include/asm/ftrace.h +1 -1
Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
===================================================================
@@ -46,7 +46,7 @@ struct ftrace_ret_stack {
 /*
  * Primary handler of a function return.
  * It relays on ftrace_return_to_handler.
- * Defined in entry32.S
+ * Defined in entry_32/64.S
  */
 extern void return_to_handler(void);
arch/x86/kernel/entry_64.S +25 -45

113 lines not on this page · raw message

Acked-by on v1 1/1
#1 lore raw
Quoted 5 lines · Cyrill Gorcunov

Impact: clean up

Itroduce FTRACE_SAVE/RESTORE_FRAME which allow us to save a number of lines on source level.

Quoted 8 lines

Also fix a comment in ftrace.h.

---
Actually I'm not that sure about bringing new macros there since we already have a lot so feel free to just drop it.

Maybe it would be good to comment args passed
to ftrace_call on ABI level :)

Actually that is pretty much the same as mcount call. But we may change it in the future. We probably should document (if it is not already) the ftrace call graph ABI.

Quoted 2 lines

Ah, the patch is on top of -tip e3bdf48

If you have tested this with and without dynamic ftrace, for both the function tracer and the function graph tracer, then you have my...

Acked-by Steven Rostedt <[email protected]>

-- Steve

Quoted 142 lines
 arch/x86/include/asm/ftrace.h |    2 -
 arch/x86/kernel/entry_64.S    |   70 +++++++++++++++---------------------------
 2 files changed, 26 insertions(+), 46 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
===================================================================

arch/x86/include/asm/ftrace.h +1 -1
@@ -46,7 +46,7 @@ struct ftrace_ret_stack {
 /*
  * Primary handler of a function return.
  * It relays on ftrace_return_to_handler.
- * Defined in entry32.S
+ * Defined in entry_32/64.S
  */
 extern void return_to_handler(void);

Index: linux-2.6.git/arch/x86/kernel/entry_64.S
===================================================================

arch/x86/kernel/entry_64.S +25 -45
@@ -66,10 +66,7 @@ ENTRY(mcount)
 	retq
 END(mcount)
 
-ENTRY(ftrace_caller)
-	cmpl $0, function_trace_stop
-	jne  ftrace_stub
-
+	.macro FTRACE_SAVE_FRAME
 	/* taken from glibc */
 	subq $0x38, %rsp
 	movq %rax, (%rsp)

@@ -79,15 +76,9 @@ ENTRY(ftrace_caller)
 	movq %rdi, 32(%rsp)
 	movq %r8, 40(%rsp)
 	movq %r9, 48(%rsp)
+	.endm
 
-	movq 0x38(%rsp), %rdi
-	movq 8(%rbp), %rsi
-	subq $MCOUNT_INSN_SIZE, %rdi
-
-.globl ftrace_call
-ftrace_call:
-	call ftrace_stub
-
+	.macro FTRACE_RESTORE_FRAME
 	movq 48(%rsp), %r9
 	movq 40(%rsp), %r8
 	movq 32(%rsp), %rdi

@@ -96,6 +87,23 @@ ftrace_call:
 	movq 8(%rsp), %rcx
 	movq (%rsp), %rax
 	addq $0x38, %rsp
+	.endm
+
+ENTRY(ftrace_caller)
+	cmpl $0, function_trace_stop
+	jne  ftrace_stub
+
+	FTRACE_SAVE_FRAME
+
+	movq 0x38(%rsp), %rdi
+	movq 8(%rbp), %rsi
+	subq $MCOUNT_INSN_SIZE, %rdi
+
+.globl ftrace_call
+ftrace_call:
+	call ftrace_stub
+
+	FTRACE_RESTORE_FRAME
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call

@@ -129,15 +137,7 @@ ftrace_stub:
 	retq
 
 trace:
-	/* taken from glibc */
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	FTRACE_SAVE_FRAME
 
 	movq 0x38(%rsp), %rdi
 	movq 8(%rbp), %rsi

@@ -145,14 +145,7 @@ trace:
 
 	call   *ftrace_trace_function
 
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	FTRACE_RESTORE_FRAME
 
 	jmp ftrace_stub
 END(mcount)

@@ -164,14 +157,7 @@ ENTRY(ftrace_graph_caller)
 	cmpl $0, function_trace_stop
 	jne ftrace_stub
 
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	FTRACE_SAVE_FRAME
 
 	leaq 8(%rbp), %rdi
 	movq 0x38(%rsp), %rsi

@@ -179,14 +165,8 @@ ENTRY(ftrace_graph_caller)
 
 	call	prepare_ftrace_return
 
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	FTRACE_RESTORE_FRAME
+
 	retq
 END(ftrace_graph_caller)
author
#2 lore raw

[Steven Rostedt - Fri, Dec 12, 2008 at 03:19:27PM -0500] |
...

| > 
| > Ah, the patch is on top of -tip e3bdf48
| 
| If you have tested this with and without dynamic ftrace, for both the
| function tracer and the function graph tracer, then you have my...
| 
| Acked-by: Steven Rostedt <[email protected]>
| 
| -- Steve

...

Thanks Steven! Actually just catched a bug :) I placed these macros under CONFIG_DYNAMIC_FTRACE but have them used even if it was undefined -- will fix shortly.

Well... I think the macros should be placed in differen location (maybe in header) to not overload the source.

- Cyrill -

maintainer
#3 lore raw
Quoted 12 lines · Cyrill Gorcunov

[Steven Rostedt - Fri, Dec 12, 2008 at 03:19:27PM -0500] |
...

| > 
| > Ah, the patch is on top of -tip e3bdf48
| 
| If you have tested this with and without dynamic ftrace, for both the
| function tracer and the function graph tracer, then you have my...
| 
| Acked-by: Steven Rostedt <[email protected]>
| 
| -- Steve
Quoted 8 lines

...

Thanks Steven! Actually just catched a bug :) I placed these macros under CONFIG_DYNAMIC_FTRACE but have them used even if it was undefined -- will fix shortly.

Well... I think the macros should be placed in differen location (maybe in header) to not overload the source.

You can put them in arch/x86/include/asm/ftrace.h That's assembly safe.

-- Steve

author
#4 lore raw

[Steven Rostedt - Fri, Dec 12, 2008 at 03:52:56PM -0500] |
...

| You can put them in arch/x86/include/asm/ftrace.h That's assembly safe.
| 
| -- Steve
|

Yep, while you were typing the message I was testing the change :) Do you like it?

- Cyrill -

---
 arch/x86/include/asm/ftrace.h |   29 ++++++++++++++++++++-
 arch/x86/kernel/entry_64.S    |   57 +++++-------------------------------------
 2 files changed, 35 insertions(+), 51 deletions(-)
arch/x86/include/asm/ftrace.h +28 -1
Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
===================================================================
@@ -1,6 +1,33 @@
 #ifndef _ASM_X86_FTRACE_H
 #define _ASM_X86_FTRACE_H
 
+#ifdef __ASSEMBLY__
+
+	.macro FTRACE_SAVE_FRAME
+	/* taken from glibc */
+	subq $0x38, %rsp
+	movq %rax, (%rsp)
+	movq %rcx, 8(%rsp)
+	movq %rdx, 16(%rsp)
+	movq %rsi, 24(%rsp)
+	movq %rdi, 32(%rsp)
+	movq %r8, 40(%rsp)
+	movq %r9, 48(%rsp)
+	.endm
+
+	.macro FTRACE_RESTORE_FRAME
+	movq 48(%rsp), %r9
+	movq 40(%rsp), %r8
+	movq 32(%rsp), %rdi
+	movq 24(%rsp), %rsi
+	movq 16(%rsp), %rdx
+	movq 8(%rsp), %rcx
+	movq (%rsp), %rax
+	addq $0x38, %rsp
+	.endm
+
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((long)(mcount))
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */

@@ -46,7 +73,7 @@ struct ftrace_ret_stack {
 /*
  * Primary handler of a function return.
  * It relays on ftrace_return_to_handler.
- * Defined in entry32.S
+ * Defined in entry_32/64.S
  */
 extern void return_to_handler(void);
arch/x86/kernel/entry_64.S +7 -50

93 lines not on this page · raw message

author
#5 lore raw

[Cyrill Gorcunov - Fri, Dec 12, 2008 at 11:57:29PM +0300]

| [Steven Rostedt - Fri, Dec 12, 2008 at 03:52:56PM -0500]
| | 
| ... 
| | You can put them in arch/x86/include/asm/ftrace.h That's assembly safe.
| | 
| | -- Steve
| | 
| 
| Yep, while you were typing the message I was testing the change :)
| Do you like it?
| 
| 		- Cyrill -

By "testing" I mean "to compare objdump output".

- Cyrill -

maintainer
#6 lore raw
In reply to Cyrill Gorcunov
Quoted 23 lines · Cyrill Gorcunov

[Steven Rostedt - Fri, Dec 12, 2008 at 03:52:56PM -0500] |
...

| You can put them in arch/x86/include/asm/ftrace.h That's assembly safe.
| 
| -- Steve
|

Yep, while you were typing the message I was testing the change :) Do you like it?

- Cyrill -

---
 arch/x86/include/asm/ftrace.h |   29 ++++++++++++++++++++-
 arch/x86/kernel/entry_64.S    |   57 +++++-------------------------------------
 2 files changed, 35 insertions(+), 51 deletions(-)

Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
===================================================================

arch/x86/include/asm/ftrace.h line 4

Quoted 4 lines
arch/x86/include/asm/ftrace.h +1 -0
@@ -1,6 +1,33 @@
 #ifndef _ASM_X86_FTRACE_H
 #define _ASM_X86_FTRACE_H
 
+#ifdef __ASSEMBLY__

Why add assembly condition? We only want this if CONFIG_FUNCTION_TRACER is enabled, right? We could combine it with the current #ifndef inside the CONFIG_FUNCTION_TRACER. But I would make do:

#ifdef __ASSEMBLY__
<your stuff>
#else
< C stuff >
#endif

-- Steve

Quoted 143 lines
arch/x86/include/asm/ftrace.h +27 -1
+
+	.macro FTRACE_SAVE_FRAME
+	/* taken from glibc */
+	subq $0x38, %rsp
+	movq %rax, (%rsp)
+	movq %rcx, 8(%rsp)
+	movq %rdx, 16(%rsp)
+	movq %rsi, 24(%rsp)
+	movq %rdi, 32(%rsp)
+	movq %r8, 40(%rsp)
+	movq %r9, 48(%rsp)
+	.endm
+
+	.macro FTRACE_RESTORE_FRAME
+	movq 48(%rsp), %r9
+	movq 40(%rsp), %r8
+	movq 32(%rsp), %rdi
+	movq 24(%rsp), %rsi
+	movq 16(%rsp), %rdx
+	movq 8(%rsp), %rcx
+	movq (%rsp), %rax
+	addq $0x38, %rsp
+	.endm
+
+#endif
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((long)(mcount))
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */

@@ -46,7 +73,7 @@ struct ftrace_ret_stack {
 /*
  * Primary handler of a function return.
  * It relays on ftrace_return_to_handler.
- * Defined in entry32.S
+ * Defined in entry_32/64.S
  */
 extern void return_to_handler(void);

Index: linux-2.6.git/arch/x86/kernel/entry_64.S
===================================================================

arch/x86/kernel/entry_64.S +7 -50
@@ -70,15 +70,7 @@ ENTRY(ftrace_caller)
 	cmpl $0, function_trace_stop
 	jne  ftrace_stub
 
-	/* taken from glibc */
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	FTRACE_SAVE_FRAME
 
 	movq 0x38(%rsp), %rdi
 	movq 8(%rbp), %rsi

@@ -88,14 +80,7 @@ ENTRY(ftrace_caller)
 ftrace_call:
 	call ftrace_stub
 
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	FTRACE_RESTORE_FRAME
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 .globl ftrace_graph_call

@@ -129,15 +114,7 @@ ftrace_stub:
 	retq
 
 trace:
-	/* taken from glibc */
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	FTRACE_SAVE_FRAME
 
 	movq 0x38(%rsp), %rdi
 	movq 8(%rbp), %rsi

@@ -145,14 +122,7 @@ trace:
 
 	call   *ftrace_trace_function
 
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	FTRACE_RESTORE_FRAME
 
 	jmp ftrace_stub
 END(mcount)

@@ -164,14 +134,7 @@ ENTRY(ftrace_graph_caller)
 	cmpl $0, function_trace_stop
 	jne ftrace_stub
 
-	subq $0x38, %rsp
-	movq %rax, (%rsp)
-	movq %rcx, 8(%rsp)
-	movq %rdx, 16(%rsp)
-	movq %rsi, 24(%rsp)
-	movq %rdi, 32(%rsp)
-	movq %r8, 40(%rsp)
-	movq %r9, 48(%rsp)
+	FTRACE_SAVE_FRAME
 
 	leaq 8(%rbp), %rdi
 	movq 0x38(%rsp), %rsi

@@ -179,14 +142,8 @@ ENTRY(ftrace_graph_caller)
 
 	call	prepare_ftrace_return
 
-	movq 48(%rsp), %r9
-	movq 40(%rsp), %r8
-	movq 32(%rsp), %rdi
-	movq 24(%rsp), %rsi
-	movq 16(%rsp), %rdx
-	movq 8(%rsp), %rcx
-	movq (%rsp), %rax
-	addq $0x38, %rsp
+	FTRACE_RESTORE_FRAME
+
 	retq
 END(ftrace_graph_caller)
author
#7 lore raw

[Steven Rostedt - Fri, Dec 12, 2008 at 04:22:49PM -0500]

| 
| On Fri, 12 Dec 2008, Cyrill Gorcunov wrote:
| 
| > [Steven Rostedt - Fri, Dec 12, 2008 at 03:52:56PM -0500]
| > | 
| > ... 
| > | You can put them in arch/x86/include/asm/ftrace.h That's assembly safe.
| > | 
| > | -- Steve
| > | 
| > 
| > Yep, while you were typing the message I was testing the change :)
| > Do you like it?
| > 
| > 		- Cyrill -
| > 
| > ---
| >  arch/x86/include/asm/ftrace.h |   29 ++++++++++++++++++++-
| >  arch/x86/kernel/entry_64.S    |   57 +++++-------------------------------------
| >  2 files changed, 35 insertions(+), 51 deletions(-)
| > 
| > Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
| > ===================================================================
| > --- linux-2.6.git.orig/arch/x86/include/asm/ftrace.h
| > +++ linux-2.6.git/arch/x86/include/asm/ftrace.h
| > @@ -1,6 +1,33 @@
| >  #ifndef _ASM_X86_FTRACE_H
| >  #define _ASM_X86_FTRACE_H
| >  
| > +#ifdef __ASSEMBLY__
| 
| Why add assembly condition? We only want this if CONFIG_FUNCTION_TRACER 
| is enabled, right?  We could combine it with the current #ifndef inside 
| the CONFIG_FUNCTION_TRACER. But I would make do:
| 
| #ifdef __ASSEMBLY__
| <your stuff>
| #else
| < C stuff >
| #endif
| 
| -- Steve

...

OK, will back shortly.

- Cyrill -

author
#8 lore raw
In reply to Steven Rostedt

[Steven Rostedt - Fri, Dec 12, 2008 at 04:22:49PM -0500]
...

| > Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
| > ===================================================================
| > --- linux-2.6.git.orig/arch/x86/include/asm/ftrace.h
| > +++ linux-2.6.git/arch/x86/include/asm/ftrace.h
| > @@ -1,6 +1,33 @@
| >  #ifndef _ASM_X86_FTRACE_H
| >  #define _ASM_X86_FTRACE_H
| >  
| > +#ifdef __ASSEMBLY__
| 
| Why add assembly condition? We only want this if CONFIG_FUNCTION_TRACER 
| is enabled, right?  We could combine it with the current #ifndef inside 
| the CONFIG_FUNCTION_TRACER. But I would make do:
| 
| #ifdef __ASSEMBLY__
| <your stuff>
| #else
| < C stuff >
| #endif
| 
| -- Steve

Steve, here is how it could look like:
(I liked first proposal more :)

---
 arch/x86/include/asm/ftrace.h |   36 ++++++++++++++++++++++----
 arch/x86/kernel/entry_64.S    |   57 +++++-------------------------------------
 2 files changed, 37 insertions(+), 56 deletions(-)
arch/x86/include/asm/ftrace.h +30 -6
Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
===================================================================
@@ -1,11 +1,37 @@
 #ifndef _ASM_X86_FTRACE_H
 #define _ASM_X86_FTRACE_H
 
+#ifdef __ASSEMBLY__
+
+	.macro FTRACE_SAVE_FRAME
+	/* taken from glibc */
+	subq $0x38, %rsp
+	movq %rax, (%rsp)
+	movq %rcx, 8(%rsp)
+	movq %rdx, 16(%rsp)
+	movq %rsi, 24(%rsp)
+	movq %rdi, 32(%rsp)
+	movq %r8, 40(%rsp)
+	movq %r9, 48(%rsp)
+	.endm
+
+	.macro FTRACE_RESTORE_FRAME
+	movq 48(%rsp), %r9
+	movq 40(%rsp), %r8
+	movq 32(%rsp), %rdi
+	movq 24(%rsp), %rsi
+	movq 16(%rsp), %rdx
+	movq 8(%rsp), %rcx
+	movq (%rsp), %rax
+	addq $0x38, %rsp
+	.endm
+
+#else /* !__ASSEMBLY__ */
+
 #ifdef CONFIG_FUNCTION_TRACER
 #define MCOUNT_ADDR		((long)(mcount))
 #define MCOUNT_INSN_SIZE	5 /* sizeof mcount call */
 
-#ifndef __ASSEMBLY__
 extern void mcount(void);
 
 static inline unsigned long ftrace_call_adjust(unsigned long addr)

@@ -25,13 +51,10 @@ struct dyn_arch_ftrace {
 };
 
 #endif /*  CONFIG_DYNAMIC_FTRACE */
-#endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_TRACER */
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
-#ifndef __ASSEMBLY__
-
 /*
  * Stack of return addresses for functions
  * of a thread.

@@ -46,11 +69,12 @@ struct ftrace_ret_stack {
 /*
  * Primary handler of a function return.
  * It relays on ftrace_return_to_handler.
- * Defined in entry32.S
+ * Defined in entry_32/64.S
  */
 extern void return_to_handler(void);
 
-#endif /* __ASSEMBLY__ */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
 
+#endif /* !__ASSEMBLY__ */
+
 #endif /* _ASM_X86_FTRACE_H */
arch/x86/kernel/entry_64.S +7 -50

93 lines not on this page · raw message

- Cyrill -

author
#9 lore raw

[Cyrill Gorcunov - Sat, Dec 13, 2008 at 12:35:38AM +0300] |
...

| | Why add assembly condition? We only want this if CONFIG_FUNCTION_TRACER 
| | is enabled, right?  We could combine it with the current #ifndef inside 
| | the CONFIG_FUNCTION_TRACER. But I would make do:
| | 
| | #ifdef __ASSEMBLY__
| | <your stuff>
| | #else
| | < C stuff >
| | #endif
| | 
| | -- Steve
| 
| Steve, here is how it could look like:
| (I liked first proposal more :)

If you meant to put them under CONFIG_FUNCTION_TRACER only it would lead to more hard to parse by human representation I believe :) That is why I put __ASSEMBLY__ in that way (I mean only two sections __ASSEMBLY__ and #else and all original stuff here) Btw there was an error in ftrace.h. Iirc gas know nothing about (long) so MCOUNT_ADDR should be under #ifndef actually. But that is not that serious since we dont use it now in *.S (don't we?).

- Cyrill -

maintainer
#10 lore raw
In reply to Cyrill Gorcunov
Quoted 24 lines · Cyrill Gorcunov

[Steven Rostedt - Fri, Dec 12, 2008 at 04:22:49PM -0500]
...

| > Index: linux-2.6.git/arch/x86/include/asm/ftrace.h
| > ===================================================================
| > --- linux-2.6.git.orig/arch/x86/include/asm/ftrace.h
| > +++ linux-2.6.git/arch/x86/include/asm/ftrace.h
| > @@ -1,6 +1,33 @@
| >  #ifndef _ASM_X86_FTRACE_H
| >  #define _ASM_X86_FTRACE_H
| >  
| > +#ifdef __ASSEMBLY__
| 
| Why add assembly condition? We only want this if CONFIG_FUNCTION_TRACER 
| is enabled, right?  We could combine it with the current #ifndef inside 
| the CONFIG_FUNCTION_TRACER. But I would make do:
| 
| #ifdef __ASSEMBLY__
| <your stuff>
| #else
| < C stuff >
| #endif
| 
| -- Steve
Quoted 2 lines

Steve, here is how it could look like:
(I liked first proposal more :)

I see the confusion. In ftrace.h the function graph tracer config is not inside CONFIG_FUNCTION_TRACER, even though it is dependent on it (or was). OK, just do your first proposal then.

-- Steve

author
#11 lore raw

[Steven Rostedt - Fri, Dec 12, 2008 at 04:56:47PM -0500]
...

| > Steve, here is how it could look like:
| > (I liked first proposal more :)
| 
| I see the confusion. In ftrace.h the function graph tracer config is not 
| inside CONFIG_FUNCTION_TRACER, even though it is dependent on it (or 
| was). OK, just do your first proposal then.
| 
| -- Steve
|

ok, I sent it yesterday as

	http://lkml.org/lkml/2008/12/12/301
	[PATCH] x86: entry_64 - introduce FTRACE_ frame macro v2

Ingo, will you pick it up?

- Cyrill -

#12 lore raw

2008/12/13 Cyrill Gorcunov <[email protected]>:

Quoted 12 lines

[Steven Rostedt - Fri, Dec 12, 2008 at 04:56:47PM -0500]
...

| > Steve, here is how it could look like:
| > (I liked first proposal more :)
|
| I see the confusion. In ftrace.h the function graph tracer config is not
| inside CONFIG_FUNCTION_TRACER, even though it is dependent on it (or
| was). OK, just do your first proposal then.
|
| -- Steve
|
Quoted 9 lines

ok, I sent it yesterday as

http://lkml.org/lkml/2008/12/12/301
[PATCH] x86: entry_64 - introduce FTRACE_ frame macro v2

Ingo, will you pick it up?

- Cyrill -

Hi,

Looks good and clarify the asm bits.

I wonder if the function_graph_tracer should continue to necessarily depend on the function tracer.
It was more simplier to do so but perhaps that could be avoided with small modifications.

maintainer
#13 lore raw
Quoted 9 lines · Fr?d?ric Weisbecker

2008/12/13 Cyrill Gorcunov <[email protected]>:
Hi,

Looks good and clarify the asm bits.

I wonder if the function_graph_tracer should continue to necessarily depend on the function tracer.
It was more simplier to do so but perhaps that could be avoided with small modifications.

It could be, but remember, the dynamic ftrace gives the ability to specify which function can be chosen.

But on the other hand, I guess it would be fine to have it separate. There is no reason that it can not be. I would still keep the dynamic part dependent on function tracer. At least for now.

-- Steve

author
#15 lore raw
In reply to Frédéric Weisbecker

[Frédéric Weisbecker - Sat, Dec 13, 2008 at 03:46:03PM +0100]

| 2008/12/13 Cyrill Gorcunov <[email protected]>:
| > [Steven Rostedt - Fri, Dec 12, 2008 at 04:56:47PM -0500]
| > ...
| > | > Steve, here is how it could look like:
| > | > (I liked first proposal more :)
| > |
| > | I see the confusion. In ftrace.h the function graph tracer config is not
| > | inside CONFIG_FUNCTION_TRACER, even though it is dependent on it (or
| > | was). OK, just do your first proposal then.
| > |
| > | -- Steve
| > |
| >
| > ok, I sent it yesterday as
| >
| >        http://lkml.org/lkml/2008/12/12/301
| >        [PATCH] x86: entry_64 - introduce FTRACE_ frame macro v2
| >
| > Ingo, will you pick it up?
| >
| >                - Cyrill -
| >
| 
| Hi,
| 
| Looks good and clarify the asm bits.
| 
| I wonder if the function_graph_tracer should continue to necessarily
| depend on the function tracer.
| It was more simplier to do so but perhaps that could be avoided with
| small modifications.
|

Hi Frédéric,

I didn't play with tracer internals since was more concerned about asm part of code. If you find that we better should introduce additional deps here or change a "deps graph" itself - I think it will be easy to place a new patch on top of this (or replace my patch) :)

The only thing I would change in mine patch is -- subq/addq $0x38, %rsp since the following code uses decimal system but not a big deal anyway.

- Cyrill -

#16 lore raw

2008/12/13 Cyrill Gorcunov <[email protected]>:

Quoted 8 lines

Hi Frédéric,

I didn't play with tracer internals since was more concerned about asm part of code. If you find that we better should introduce additional deps here or change a "deps graph" itself - I think it will be easy to place a new patch on top of this (or replace my patch) :)

Quoted 4 lines

The only thing I would change in mine patch is -- subq/addq $0x38, %rsp since the following code uses decimal system but not a big deal anyway.

- Cyrill -

Yes, don't worry, these comments weren't relied on your patch :-) If some work has to be done on deps, it will be on top of your patch if Ingo applies it.

#14 lore raw
In reply to Steven Rostedt

2008/12/13 Steven Rostedt <[email protected]>:

Quoted 8 lines

It could be, but remember, the dynamic ftrace gives the ability to specify which function can be chosen.

But on the other hand, I guess it would be fine to have it separate. There is no reason that it can not be. I would still keep the dynamic part dependent on function tracer. At least for now.

-- Steve

I agree. I don't think it will require much modifications. I think I will change it for 2.6.30....