All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
@ 2022-04-09 15:35 ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, zhouchengming,
	linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
special hooks for graph tracer, which are not needed on some ARCHs
that use graph_ops:func function to install return_hooker.

So introduce the weak version in ftrace core code to cleanup
in x86.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
v4:
 - put weak ftrace_enable,disable_ftrace_graph_caller() in
   fgraph.c instead of ftrace.c as suggested by Steve.

v3:
 - consolidate two #if into a single #if, suggested by Steve. Thanks.
---
 arch/x86/kernel/ftrace.c | 17 ++---------------
 kernel/trace/fgraph.c    | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1e31c7d21597..b09d73c2ba89 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
-#ifdef CONFIG_DYNAMIC_FTRACE
-
-#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
 extern void ftrace_graph_call(void);
 static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
 {
@@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
 
 	return ftrace_mod_jmp(ip, &ftrace_stub);
 }
-#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
-int ftrace_enable_ftrace_graph_caller(void)
-{
-	return 0;
-}
-
-int ftrace_disable_ftrace_graph_caller(void)
-{
-	return 0;
-}
-#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
-#endif /* !CONFIG_DYNAMIC_FTRACE */
+#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 /*
  * Hook the return address and push it in the stack of return addrs
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 8f4fb328133a..289311680c29 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -30,6 +30,24 @@ int ftrace_graph_active;
 /* Both enabled by default (can be cleared by function_graph tracer flags */
 static bool fgraph_sleep_time = true;
 
+/*
+ * archs can override this function if they must do something
+ * to enable hook for graph tracer.
+ */
+int __weak ftrace_enable_ftrace_graph_caller(void)
+{
+	return 0;
+}
+
+/*
+ * archs can override this function if they must do something
+ * to disable hook for graph tracer.
+ */
+int __weak ftrace_disable_ftrace_graph_caller(void)
+{
+	return 0;
+}
+
 /**
  * ftrace_graph_stop - set to permanently disable function graph tracing
  *
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
@ 2022-04-09 15:35 ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, zhouchengming,
	linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
special hooks for graph tracer, which are not needed on some ARCHs
that use graph_ops:func function to install return_hooker.

So introduce the weak version in ftrace core code to cleanup
in x86.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
v4:
 - put weak ftrace_enable,disable_ftrace_graph_caller() in
   fgraph.c instead of ftrace.c as suggested by Steve.

v3:
 - consolidate two #if into a single #if, suggested by Steve. Thanks.
---
 arch/x86/kernel/ftrace.c | 17 ++---------------
 kernel/trace/fgraph.c    | 18 ++++++++++++++++++
 2 files changed, 20 insertions(+), 15 deletions(-)

diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
index 1e31c7d21597..b09d73c2ba89 100644
--- a/arch/x86/kernel/ftrace.c
+++ b/arch/x86/kernel/ftrace.c
@@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
 
 #ifdef CONFIG_FUNCTION_GRAPH_TRACER
 
-#ifdef CONFIG_DYNAMIC_FTRACE
-
-#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
+#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
 extern void ftrace_graph_call(void);
 static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
 {
@@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
 
 	return ftrace_mod_jmp(ip, &ftrace_stub);
 }
-#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
-int ftrace_enable_ftrace_graph_caller(void)
-{
-	return 0;
-}
-
-int ftrace_disable_ftrace_graph_caller(void)
-{
-	return 0;
-}
-#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
-#endif /* !CONFIG_DYNAMIC_FTRACE */
+#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
 
 /*
  * Hook the return address and push it in the stack of return addrs
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
index 8f4fb328133a..289311680c29 100644
--- a/kernel/trace/fgraph.c
+++ b/kernel/trace/fgraph.c
@@ -30,6 +30,24 @@ int ftrace_graph_active;
 /* Both enabled by default (can be cleared by function_graph tracer flags */
 static bool fgraph_sleep_time = true;
 
+/*
+ * archs can override this function if they must do something
+ * to enable hook for graph tracer.
+ */
+int __weak ftrace_enable_ftrace_graph_caller(void)
+{
+	return 0;
+}
+
+/*
+ * archs can override this function if they must do something
+ * to disable hook for graph tracer.
+ */
+int __weak ftrace_disable_ftrace_graph_caller(void)
+{
+	return 0;
+}
+
 /**
  * ftrace_graph_stop - set to permanently disable function graph tracing
  *
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
  2022-04-09 15:35 ` Chengming Zhou
@ 2022-04-09 15:35   ` Chengming Zhou
  -1 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, zhouchengming,
	linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
use ftrace directly"), we don't need special hook for graph tracer,
but instead we use graph_ops:func function to install return_hooker.

Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
the same cleanup on arm64.

And this cleanup only changes the FTRACE_WITH_REGS implementation,
so the mcount-based implementation is unaffected.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
v3:
 - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.

v2:
 - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
---
 arch/arm64/include/asm/ftrace.h  |  7 +++++++
 arch/arm64/kernel/entry-ftrace.S | 17 -----------------
 arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 1494cfa8639b..dbc45a4157fa 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 struct dyn_ftrace;
+struct ftrace_ops;
+struct ftrace_regs;
+
 int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
 #define ftrace_init_nop ftrace_init_nop
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
+#define ftrace_graph_func ftrace_graph_func
 #endif
 
 #define ftrace_return_address(n) return_address(n)
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index e535480a4069..d42a205ef625 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	bl	ftrace_stub
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
-	nop				// If enabled, this will be replaced
-					// "b ftrace_graph_caller"
-#endif
-
 /*
  * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
  * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
@@ -127,17 +121,6 @@ ftrace_common_return:
 	ret	x9
 SYM_CODE_END(ftrace_common)
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_CODE_START(ftrace_graph_caller)
-	ldr	x0, [sp, #S_PC]
-	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
-	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
-	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
-	bl	prepare_ftrace_return
-	b	ftrace_common_return
-SYM_CODE_END(ftrace_graph_caller)
-#endif
-
 #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 
 /*
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 4506c4a90ac1..35eb7c9b5e53 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	/*
+	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
+	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
+	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
+	 */
+	struct pt_regs *regs = arch_ftrace_get_regs(fregs);
+	unsigned long *parent = (unsigned long *)&procedure_link_pointer(regs);
+
+	prepare_ftrace_return(ip, parent, frame_pointer(regs));
+}
+#else
 /*
  * Turn on/off the call to ftrace_graph_caller() in ftrace_caller()
  * depending on @enable.
@@ -297,5 +313,6 @@ int ftrace_disable_ftrace_graph_caller(void)
 {
 	return ftrace_modify_graph_caller(false);
 }
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.35.1


^ permalink raw reply related	[flat|nested] 14+ messages in thread
* [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
@ 2022-04-09 15:35   ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-09 15:35 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, zhouchengming,
	linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
use ftrace directly"), we don't need special hook for graph tracer,
but instead we use graph_ops:func function to install return_hooker.

Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
the same cleanup on arm64.

And this cleanup only changes the FTRACE_WITH_REGS implementation,
so the mcount-based implementation is unaffected.

Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
---
v3:
 - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.

v2:
 - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
---
 arch/arm64/include/asm/ftrace.h  |  7 +++++++
 arch/arm64/kernel/entry-ftrace.S | 17 -----------------
 arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
 3 files changed, 24 insertions(+), 17 deletions(-)

diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
index 1494cfa8639b..dbc45a4157fa 100644
--- a/arch/arm64/include/asm/ftrace.h
+++ b/arch/arm64/include/asm/ftrace.h
@@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
 
 #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
 struct dyn_ftrace;
+struct ftrace_ops;
+struct ftrace_regs;
+
 int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
 #define ftrace_init_nop ftrace_init_nop
+
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs);
+#define ftrace_graph_func ftrace_graph_func
 #endif
 
 #define ftrace_return_address(n) return_address(n)
diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
index e535480a4069..d42a205ef625 100644
--- a/arch/arm64/kernel/entry-ftrace.S
+++ b/arch/arm64/kernel/entry-ftrace.S
@@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
 SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
 	bl	ftrace_stub
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
-	nop				// If enabled, this will be replaced
-					// "b ftrace_graph_caller"
-#endif
-
 /*
  * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
  * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
@@ -127,17 +121,6 @@ ftrace_common_return:
 	ret	x9
 SYM_CODE_END(ftrace_common)
 
-#ifdef CONFIG_FUNCTION_GRAPH_TRACER
-SYM_CODE_START(ftrace_graph_caller)
-	ldr	x0, [sp, #S_PC]
-	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
-	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
-	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
-	bl	prepare_ftrace_return
-	b	ftrace_common_return
-SYM_CODE_END(ftrace_graph_caller)
-#endif
-
 #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 
 /*
diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
index 4506c4a90ac1..35eb7c9b5e53 100644
--- a/arch/arm64/kernel/ftrace.c
+++ b/arch/arm64/kernel/ftrace.c
@@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
 }
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+
+#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
+void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
+		       struct ftrace_ops *op, struct ftrace_regs *fregs)
+{
+	/*
+	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
+	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
+	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
+	 */
+	struct pt_regs *regs = arch_ftrace_get_regs(fregs);
+	unsigned long *parent = (unsigned long *)&procedure_link_pointer(regs);
+
+	prepare_ftrace_return(ip, parent, frame_pointer(regs));
+}
+#else
 /*
  * Turn on/off the call to ftrace_graph_caller() in ftrace_caller()
  * depending on @enable.
@@ -297,5 +313,6 @@ int ftrace_disable_ftrace_graph_caller(void)
 {
 	return ftrace_modify_graph_caller(false);
 }
+#endif /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
 #endif /* CONFIG_DYNAMIC_FTRACE */
 #endif /* CONFIG_FUNCTION_GRAPH_TRACER */
-- 
2.35.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
  2022-04-09 15:35 ` Chengming Zhou
@ 2022-04-18 13:24   ` Chengming Zhou
  -1 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-18 13:24 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

Hello, friendly ping :-)


On 2022/4/9 23:35, Chengming Zhou wrote:
> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
> special hooks for graph tracer, which are not needed on some ARCHs
> that use graph_ops:func function to install return_hooker.
> 
> So introduce the weak version in ftrace core code to cleanup
> in x86.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v4:
>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>    fgraph.c instead of ftrace.c as suggested by Steve.
> 
> v3:
>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
> ---
>  arch/x86/kernel/ftrace.c | 17 ++---------------
>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1e31c7d21597..b09d73c2ba89 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE
> -
> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>  extern void ftrace_graph_call(void);
>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>  {
> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>  
>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>  }
> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -int ftrace_enable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -
> -int ftrace_disable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -#endif /* !CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>  
>  /*
>   * Hook the return address and push it in the stack of return addrs
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 8f4fb328133a..289311680c29 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>  static bool fgraph_sleep_time = true;
>  
> +/*
> + * archs can override this function if they must do something
> + * to enable hook for graph tracer.
> + */
> +int __weak ftrace_enable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
> +/*
> + * archs can override this function if they must do something
> + * to disable hook for graph tracer.
> + */
> +int __weak ftrace_disable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
>  /**
>   * ftrace_graph_stop - set to permanently disable function graph tracing
>   *

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
@ 2022-04-18 13:24   ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-18 13:24 UTC (permalink / raw)
  To: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, mark.rutland, ardb, linux-arm-kernel
  Cc: linux-kernel, duanxiongchun, songmuchun

Hello, friendly ping :-)


On 2022/4/9 23:35, Chengming Zhou wrote:
> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
> special hooks for graph tracer, which are not needed on some ARCHs
> that use graph_ops:func function to install return_hooker.
> 
> So introduce the weak version in ftrace core code to cleanup
> in x86.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v4:
>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>    fgraph.c instead of ftrace.c as suggested by Steve.
> 
> v3:
>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
> ---
>  arch/x86/kernel/ftrace.c | 17 ++---------------
>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1e31c7d21597..b09d73c2ba89 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE
> -
> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>  extern void ftrace_graph_call(void);
>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>  {
> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>  
>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>  }
> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -int ftrace_enable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -
> -int ftrace_disable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -#endif /* !CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>  
>  /*
>   * Hook the return address and push it in the stack of return addrs
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 8f4fb328133a..289311680c29 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>  static bool fgraph_sleep_time = true;
>  
> +/*
> + * archs can override this function if they must do something
> + * to enable hook for graph tracer.
> + */
> +int __weak ftrace_enable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
> +/*
> + * archs can override this function if they must do something
> + * to disable hook for graph tracer.
> + */
> +int __weak ftrace_disable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
>  /**
>   * ftrace_graph_stop - set to permanently disable function graph tracing
>   *

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
  2022-04-09 15:35 ` Chengming Zhou
@ 2022-04-19 11:41   ` Mark Rutland
  -1 siblings, 0 replies; 14+ messages in thread
From: Mark Rutland @ 2022-04-19 11:41 UTC (permalink / raw)
  To: Chengming Zhou
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
> special hooks for graph tracer, which are not needed on some ARCHs
> that use graph_ops:func function to install return_hooker.
> 
> So introduce the weak version in ftrace core code to cleanup
> in x86.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v4:
>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>    fgraph.c instead of ftrace.c as suggested by Steve.
> 
> v3:
>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
> ---
>  arch/x86/kernel/ftrace.c | 17 ++---------------
>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1e31c7d21597..b09d73c2ba89 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE
> -
> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>  extern void ftrace_graph_call(void);
>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>  {
> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>  
>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>  }
> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -int ftrace_enable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -
> -int ftrace_disable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -#endif /* !CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>  
>  /*
>   * Hook the return address and push it in the stack of return addrs
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 8f4fb328133a..289311680c29 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>  static bool fgraph_sleep_time = true;
>  
> +/*
> + * archs can override this function if they must do something
> + * to enable hook for graph tracer.
> + */
> +int __weak ftrace_enable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
> +/*
> + * archs can override this function if they must do something
> + * to disable hook for graph tracer.
> + */
> +int __weak ftrace_disable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}

IIUC an arch should either:

* Have ftrace_graph_call()

* Have both ftrace_enable_ftrace_graph_caller() and
  ftrace_disable_ftrace_graph_caller()

... and I can't think of a reason an arch would need both ftrace_graph_call()
*and* the enable/disable functions.

Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
make the above:

| #ifndef ftrace_graph_call
| int ftrace_enable_ftrace_graph_caller(void) { return 0; }
| int ftrace_disable_ftrace_graph_caller(void) { return 0; }
| #endif /* ftrace_graph_call *. 

That way we'd catch cases when:

* An architecture meant to provide one of these functions, but forgot (e.g. the
  name got typo'd)

* An architecture provides an unnecessary implementation of either of these
  functions.

Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
need to take this via the arm64 tree with the next patch, so we'd need your Ack.

Thanks,
Mark.

> +
>  /**
>   * ftrace_graph_stop - set to permanently disable function graph tracing
>   *
> -- 
> 2.35.1
> 

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
@ 2022-04-19 11:41   ` Mark Rutland
  0 siblings, 0 replies; 14+ messages in thread
From: Mark Rutland @ 2022-04-19 11:41 UTC (permalink / raw)
  To: Chengming Zhou
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
> special hooks for graph tracer, which are not needed on some ARCHs
> that use graph_ops:func function to install return_hooker.
> 
> So introduce the weak version in ftrace core code to cleanup
> in x86.
> 
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v4:
>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>    fgraph.c instead of ftrace.c as suggested by Steve.
> 
> v3:
>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
> ---
>  arch/x86/kernel/ftrace.c | 17 ++---------------
>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>  2 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
> index 1e31c7d21597..b09d73c2ba89 100644
> --- a/arch/x86/kernel/ftrace.c
> +++ b/arch/x86/kernel/ftrace.c
> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>  
>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>  
> -#ifdef CONFIG_DYNAMIC_FTRACE
> -
> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>  extern void ftrace_graph_call(void);
>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>  {
> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>  
>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>  }
> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -int ftrace_enable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -
> -int ftrace_disable_ftrace_graph_caller(void)
> -{
> -	return 0;
> -}
> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
> -#endif /* !CONFIG_DYNAMIC_FTRACE */
> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>  
>  /*
>   * Hook the return address and push it in the stack of return addrs
> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
> index 8f4fb328133a..289311680c29 100644
> --- a/kernel/trace/fgraph.c
> +++ b/kernel/trace/fgraph.c
> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>  static bool fgraph_sleep_time = true;
>  
> +/*
> + * archs can override this function if they must do something
> + * to enable hook for graph tracer.
> + */
> +int __weak ftrace_enable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}
> +
> +/*
> + * archs can override this function if they must do something
> + * to disable hook for graph tracer.
> + */
> +int __weak ftrace_disable_ftrace_graph_caller(void)
> +{
> +	return 0;
> +}

IIUC an arch should either:

* Have ftrace_graph_call()

* Have both ftrace_enable_ftrace_graph_caller() and
  ftrace_disable_ftrace_graph_caller()

... and I can't think of a reason an arch would need both ftrace_graph_call()
*and* the enable/disable functions.

Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
make the above:

| #ifndef ftrace_graph_call
| int ftrace_enable_ftrace_graph_caller(void) { return 0; }
| int ftrace_disable_ftrace_graph_caller(void) { return 0; }
| #endif /* ftrace_graph_call *. 

That way we'd catch cases when:

* An architecture meant to provide one of these functions, but forgot (e.g. the
  name got typo'd)

* An architecture provides an unnecessary implementation of either of these
  functions.

Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
need to take this via the arm64 tree with the next patch, so we'd need your Ack.

Thanks,
Mark.

> +
>  /**
>   * ftrace_graph_stop - set to permanently disable function graph tracing
>   *
> -- 
> 2.35.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
  2022-04-09 15:35   ` Chengming Zhou
@ 2022-04-19 12:55     ` Mark Rutland
  -1 siblings, 0 replies; 14+ messages in thread
From: Mark Rutland @ 2022-04-19 12:55 UTC (permalink / raw)
  To: Chengming Zhou
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote:
> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
> use ftrace directly"), we don't need special hook for graph tracer,
> but instead we use graph_ops:func function to install return_hooker.
> 
> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
> the same cleanup on arm64.
> 
> And this cleanup only changes the FTRACE_WITH_REGS implementation,
> so the mcount-based implementation is unaffected.

Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's
possible, but would require more invasive refactoring of the core code; have I
understood correctly?

If so, could we please make this:

| While in theory it would be possible to make a similar cleanup for
| !FTRACE_WITH_REGS, this will require rework of the core code, and so for now
| we only change the FTRACE_WITH_REGS implementation.

It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly,
but as it appeass that would require far more invasive changes, I'm happy to
leave that for future work.

>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v3:
>  - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.
> 
> v2:
>  - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
> ---
>  arch/arm64/include/asm/ftrace.h  |  7 +++++++
>  arch/arm64/kernel/entry-ftrace.S | 17 -----------------
>  arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 1494cfa8639b..dbc45a4157fa 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  struct dyn_ftrace;
> +struct ftrace_ops;
> +struct ftrace_regs;
> +
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
>  #define ftrace_init_nop ftrace_init_nop
> +
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
> +#define ftrace_graph_func ftrace_graph_func
>  #endif
>  
>  #define ftrace_return_address(n) return_address(n)
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index e535480a4069..d42a205ef625 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
>  SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>  	bl	ftrace_stub
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
> -	nop				// If enabled, this will be replaced
> -					// "b ftrace_graph_caller"
> -#endif
> -
>  /*
>   * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
>   * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
> @@ -127,17 +121,6 @@ ftrace_common_return:
>  	ret	x9
>  SYM_CODE_END(ftrace_common)
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_CODE_START(ftrace_graph_caller)
> -	ldr	x0, [sp, #S_PC]
> -	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
> -	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
> -	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
> -	bl	prepare_ftrace_return
> -	b	ftrace_common_return
> -SYM_CODE_END(ftrace_graph_caller)
> -#endif
> -
>  #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>  
>  /*
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 4506c4a90ac1..35eb7c9b5e53 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  }
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> +{
> +	/*
> +	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
> +	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
> +	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
> +	 */

This is a bit confusing, since it makes it sound like there's an bug in the
current implementation, rather than something that would need to change if
support for DYNAMIC_FTRACE_WITH_ARGS is added.

Could we please make this:

	/*
	 * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
	 * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
	 * in which we can safely modify the LR.
	 */

Other than that, this looks good to me. I gave it a spin under QEMU atop
v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the
graph tracer with:

| # echo do_el0_svc > /sys/kernel/tracing/set_graph_function 
| # echo function_graph > /sys/kernel/tracing/current_tracer

... for which the resutls looks sane.

To make sure this didn't adversely affect the return address rewriting, I also
concurrently ran perf with:

| # perf record -g -e raw_syscalls:sys_enter:k /bin/true
| # perf report

... for which the results also looked fine.

I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older
compiler and also building with !DYNAMIC_FTRACE, which all looked good.

So FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

... and if you make the changes I requested above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

If you could spin a v5 with that folded in, that would be great.

Thanks,
Mark.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
@ 2022-04-19 12:55     ` Mark Rutland
  0 siblings, 0 replies; 14+ messages in thread
From: Mark Rutland @ 2022-04-19 12:55 UTC (permalink / raw)
  To: Chengming Zhou
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote:
> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
> use ftrace directly"), we don't need special hook for graph tracer,
> but instead we use graph_ops:func function to install return_hooker.
> 
> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
> the same cleanup on arm64.
> 
> And this cleanup only changes the FTRACE_WITH_REGS implementation,
> so the mcount-based implementation is unaffected.

Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's
possible, but would require more invasive refactoring of the core code; have I
understood correctly?

If so, could we please make this:

| While in theory it would be possible to make a similar cleanup for
| !FTRACE_WITH_REGS, this will require rework of the core code, and so for now
| we only change the FTRACE_WITH_REGS implementation.

It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly,
but as it appeass that would require far more invasive changes, I'm happy to
leave that for future work.

>
> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
> ---
> v3:
>  - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.
> 
> v2:
>  - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
> ---
>  arch/arm64/include/asm/ftrace.h  |  7 +++++++
>  arch/arm64/kernel/entry-ftrace.S | 17 -----------------
>  arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
>  3 files changed, 24 insertions(+), 17 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
> index 1494cfa8639b..dbc45a4157fa 100644
> --- a/arch/arm64/include/asm/ftrace.h
> +++ b/arch/arm64/include/asm/ftrace.h
> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>  struct dyn_ftrace;
> +struct ftrace_ops;
> +struct ftrace_regs;
> +
>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
>  #define ftrace_init_nop ftrace_init_nop
> +
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
> +#define ftrace_graph_func ftrace_graph_func
>  #endif
>  
>  #define ftrace_return_address(n) return_address(n)
> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
> index e535480a4069..d42a205ef625 100644
> --- a/arch/arm64/kernel/entry-ftrace.S
> +++ b/arch/arm64/kernel/entry-ftrace.S
> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
>  SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>  	bl	ftrace_stub
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
> -	nop				// If enabled, this will be replaced
> -					// "b ftrace_graph_caller"
> -#endif
> -
>  /*
>   * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
>   * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
> @@ -127,17 +121,6 @@ ftrace_common_return:
>  	ret	x9
>  SYM_CODE_END(ftrace_common)
>  
> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
> -SYM_CODE_START(ftrace_graph_caller)
> -	ldr	x0, [sp, #S_PC]
> -	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
> -	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
> -	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
> -	bl	prepare_ftrace_return
> -	b	ftrace_common_return
> -SYM_CODE_END(ftrace_graph_caller)
> -#endif
> -
>  #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>  
>  /*
> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
> index 4506c4a90ac1..35eb7c9b5e53 100644
> --- a/arch/arm64/kernel/ftrace.c
> +++ b/arch/arm64/kernel/ftrace.c
> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>  }
>  
>  #ifdef CONFIG_DYNAMIC_FTRACE
> +
> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
> +{
> +	/*
> +	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
> +	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
> +	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
> +	 */

This is a bit confusing, since it makes it sound like there's an bug in the
current implementation, rather than something that would need to change if
support for DYNAMIC_FTRACE_WITH_ARGS is added.

Could we please make this:

	/*
	 * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
	 * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
	 * in which we can safely modify the LR.
	 */

Other than that, this looks good to me. I gave it a spin under QEMU atop
v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the
graph tracer with:

| # echo do_el0_svc > /sys/kernel/tracing/set_graph_function 
| # echo function_graph > /sys/kernel/tracing/current_tracer

... for which the resutls looks sane.

To make sure this didn't adversely affect the return address rewriting, I also
concurrently ran perf with:

| # perf record -g -e raw_syscalls:sys_enter:k /bin/true
| # perf report

... for which the results also looked fine.

I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older
compiler and also building with !DYNAMIC_FTRACE, which all looked good.

So FWIW:

Tested-by: Mark Rutland <mark.rutland@arm.com>

... and if you make the changes I requested above:

Reviewed-by: Mark Rutland <mark.rutland@arm.com>

If you could spin a v5 with that folded in, that would be great.

Thanks,
Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
  2022-04-19 12:55     ` Mark Rutland
@ 2022-04-19 13:27       ` Chengming Zhou
  -1 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-19 13:27 UTC (permalink / raw)
  To: Mark Rutland
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On 2022/4/19 20:55, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote:
>> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
>> use ftrace directly"), we don't need special hook for graph tracer,
>> but instead we use graph_ops:func function to install return_hooker.
>>
>> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
>> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
>> the same cleanup on arm64.
>>
>> And this cleanup only changes the FTRACE_WITH_REGS implementation,
>> so the mcount-based implementation is unaffected.
> 
> Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's
> possible, but would require more invasive refactoring of the core code; have I
> understood correctly?

Yes, I think so. The static mcount-based implementation should also be changed
in this way, but I haven't look too deep into that asm implementation yet.

> 
> If so, could we please make this:
> 
> | While in theory it would be possible to make a similar cleanup for
> | !FTRACE_WITH_REGS, this will require rework of the core code, and so for now
> | we only change the FTRACE_WITH_REGS implementation.
> 
> It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly,
> but as it appeass that would require far more invasive changes, I'm happy to
> leave that for future work.

Ok, will add it in the commit message. And leave this for future work.

> 
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v3:
>>  - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.
>>
>> v2:
>>  - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
>> ---
>>  arch/arm64/include/asm/ftrace.h  |  7 +++++++
>>  arch/arm64/kernel/entry-ftrace.S | 17 -----------------
>>  arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>> index 1494cfa8639b..dbc45a4157fa 100644
>> --- a/arch/arm64/include/asm/ftrace.h
>> +++ b/arch/arm64/include/asm/ftrace.h
>> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>  
>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>  struct dyn_ftrace;
>> +struct ftrace_ops;
>> +struct ftrace_regs;
>> +
>>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
>>  #define ftrace_init_nop ftrace_init_nop
>> +
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
>> +#define ftrace_graph_func ftrace_graph_func
>>  #endif
>>  
>>  #define ftrace_return_address(n) return_address(n)
>> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
>> index e535480a4069..d42a205ef625 100644
>> --- a/arch/arm64/kernel/entry-ftrace.S
>> +++ b/arch/arm64/kernel/entry-ftrace.S
>> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
>>  SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>>  	bl	ftrace_stub
>>  
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
>> -	nop				// If enabled, this will be replaced
>> -					// "b ftrace_graph_caller"
>> -#endif
>> -
>>  /*
>>   * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
>>   * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
>> @@ -127,17 +121,6 @@ ftrace_common_return:
>>  	ret	x9
>>  SYM_CODE_END(ftrace_common)
>>  
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -SYM_CODE_START(ftrace_graph_caller)
>> -	ldr	x0, [sp, #S_PC]
>> -	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
>> -	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
>> -	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
>> -	bl	prepare_ftrace_return
>> -	b	ftrace_common_return
>> -SYM_CODE_END(ftrace_graph_caller)
>> -#endif
>> -
>>  #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>>  
>>  /*
>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
>> index 4506c4a90ac1..35eb7c9b5e53 100644
>> --- a/arch/arm64/kernel/ftrace.c
>> +++ b/arch/arm64/kernel/ftrace.c
>> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>>  }
>>  
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>> +{
>> +	/*
>> +	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
>> +	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
>> +	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
>> +	 */
> 
> This is a bit confusing, since it makes it sound like there's an bug in the
> current implementation, rather than something that would need to change if
> support for DYNAMIC_FTRACE_WITH_ARGS is added.
> 
> Could we please make this:
> 
> 	/*
> 	 * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
> 	 * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
> 	 * in which we can safely modify the LR.
> 	 */
> 

Ok, will do. This expression is nicer, the previous comment maybe make people
think it's an bug to be fixed.

> Other than that, this looks good to me. I gave it a spin under QEMU atop
> v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the
> graph tracer with:
> 
> | # echo do_el0_svc > /sys/kernel/tracing/set_graph_function 
> | # echo function_graph > /sys/kernel/tracing/current_tracer
> 
> ... for which the resutls looks sane.
> 
> To make sure this didn't adversely affect the return address rewriting, I also
> concurrently ran perf with:
> 
> | # perf record -g -e raw_syscalls:sys_enter:k /bin/true
> | # perf report
> 
> ... for which the results also looked fine.
> 
> I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older
> compiler and also building with !DYNAMIC_FTRACE, which all looked good.
> 
> So FWIW:
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... and if you make the changes I requested above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> If you could spin a v5 with that folded in, that would be great.

Of course, will do in v5.

Thanks.

> 
> Thanks,
> Mark.

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly
@ 2022-04-19 13:27       ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-19 13:27 UTC (permalink / raw)
  To: Mark Rutland
  Cc: rostedt, mingo, catalin.marinas, will, tglx, bp, dave.hansen,
	x86, hpa, broonie, ardb, linux-arm-kernel, linux-kernel,
	duanxiongchun, songmuchun

On 2022/4/19 20:55, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:54PM +0800, Chengming Zhou wrote:
>> As we do in commit 0c0593b45c9b ("x86/ftrace: Make function graph
>> use ftrace directly"), we don't need special hook for graph tracer,
>> but instead we use graph_ops:func function to install return_hooker.
>>
>> Since commit 3b23e4991fb6 ("arm64: implement ftrace with regs") add
>> implementation for FTRACE_WITH_REGS on arm64, we can easily adopt
>> the same cleanup on arm64.
>>
>> And this cleanup only changes the FTRACE_WITH_REGS implementation,
>> so the mcount-based implementation is unaffected.
> 
> Could you please say *why* we only do this for FTRACE_WITH_REGS? IIUC that's
> possible, but would require more invasive refactoring of the core code; have I
> understood correctly?

Yes, I think so. The static mcount-based implementation should also be changed
in this way, but I haven't look too deep into that asm implementation yet.

> 
> If so, could we please make this:
> 
> | While in theory it would be possible to make a similar cleanup for
> | !FTRACE_WITH_REGS, this will require rework of the core code, and so for now
> | we only change the FTRACE_WITH_REGS implementation.
> 
> It'd be quite nice if we could clean up the !FTRACE_WITH_REGS case similarly,
> but as it appeass that would require far more invasive changes, I'm happy to
> leave that for future work.

Ok, will add it in the commit message. And leave this for future work.

> 
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v3:
>>  - Add comments in ftrace_graph_func() as suggested by Steve. Thanks.
>>
>> v2:
>>  - Remove FTRACE_WITH_REGS ftrace_graph_caller asm, thanks Mark.
>> ---
>>  arch/arm64/include/asm/ftrace.h  |  7 +++++++
>>  arch/arm64/kernel/entry-ftrace.S | 17 -----------------
>>  arch/arm64/kernel/ftrace.c       | 17 +++++++++++++++++
>>  3 files changed, 24 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/ftrace.h b/arch/arm64/include/asm/ftrace.h
>> index 1494cfa8639b..dbc45a4157fa 100644
>> --- a/arch/arm64/include/asm/ftrace.h
>> +++ b/arch/arm64/include/asm/ftrace.h
>> @@ -80,8 +80,15 @@ static inline unsigned long ftrace_call_adjust(unsigned long addr)
>>  
>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>  struct dyn_ftrace;
>> +struct ftrace_ops;
>> +struct ftrace_regs;
>> +
>>  int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
>>  #define ftrace_init_nop ftrace_init_nop
>> +
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs);
>> +#define ftrace_graph_func ftrace_graph_func
>>  #endif
>>  
>>  #define ftrace_return_address(n) return_address(n)
>> diff --git a/arch/arm64/kernel/entry-ftrace.S b/arch/arm64/kernel/entry-ftrace.S
>> index e535480a4069..d42a205ef625 100644
>> --- a/arch/arm64/kernel/entry-ftrace.S
>> +++ b/arch/arm64/kernel/entry-ftrace.S
>> @@ -97,12 +97,6 @@ SYM_CODE_START(ftrace_common)
>>  SYM_INNER_LABEL(ftrace_call, SYM_L_GLOBAL)
>>  	bl	ftrace_stub
>>  
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -SYM_INNER_LABEL(ftrace_graph_call, SYM_L_GLOBAL) // ftrace_graph_caller();
>> -	nop				// If enabled, this will be replaced
>> -					// "b ftrace_graph_caller"
>> -#endif
>> -
>>  /*
>>   * At the callsite x0-x8 and x19-x30 were live. Any C code will have preserved
>>   * x19-x29 per the AAPCS, and we created frame records upon entry, so we need
>> @@ -127,17 +121,6 @@ ftrace_common_return:
>>  	ret	x9
>>  SYM_CODE_END(ftrace_common)
>>  
>> -#ifdef CONFIG_FUNCTION_GRAPH_TRACER
>> -SYM_CODE_START(ftrace_graph_caller)
>> -	ldr	x0, [sp, #S_PC]
>> -	sub	x0, x0, #AARCH64_INSN_SIZE	// ip (callsite's BL insn)
>> -	add	x1, sp, #S_LR			// parent_ip (callsite's LR)
>> -	ldr	x2, [sp, #PT_REGS_SIZE]	   	// parent fp (callsite's FP)
>> -	bl	prepare_ftrace_return
>> -	b	ftrace_common_return
>> -SYM_CODE_END(ftrace_graph_caller)
>> -#endif
>> -
>>  #else /* CONFIG_DYNAMIC_FTRACE_WITH_REGS */
>>  
>>  /*
>> diff --git a/arch/arm64/kernel/ftrace.c b/arch/arm64/kernel/ftrace.c
>> index 4506c4a90ac1..35eb7c9b5e53 100644
>> --- a/arch/arm64/kernel/ftrace.c
>> +++ b/arch/arm64/kernel/ftrace.c
>> @@ -268,6 +268,22 @@ void prepare_ftrace_return(unsigned long self_addr, unsigned long *parent,
>>  }
>>  
>>  #ifdef CONFIG_DYNAMIC_FTRACE
>> +
>> +#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> +void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
>> +		       struct ftrace_ops *op, struct ftrace_regs *fregs)
>> +{
>> +	/*
>> +	 * Athough graph_ops doesn't have FTRACE_OPS_FL_SAVE_REGS set in flags,
>> +	 * regs can't be NULL in DYNAMIC_FTRACE_WITH_REGS. By design, it should
>> +	 * be fixed when DYNAMIC_FTRACE_WITH_ARGS is implemented.
>> +	 */
> 
> This is a bit confusing, since it makes it sound like there's an bug in the
> current implementation, rather than something that would need to change if
> support for DYNAMIC_FTRACE_WITH_ARGS is added.
> 
> Could we please make this:
> 
> 	/*
> 	 * When DYNAMIC_FTRACE_WITH_REGS is selected, `fregs` can never be NULL
> 	 * and arch_ftrace_get_regs(fregs) will always give a non-NULL pt_regs
> 	 * in which we can safely modify the LR.
> 	 */
> 

Ok, will do. This expression is nicer, the previous comment maybe make people
think it's an bug to be fixed.

> Other than that, this looks good to me. I gave it a spin under QEMU atop
> v5.18-rc3. The CONFIG_FTRACE_STARTUP_TEST tests all pass, and I played with the
> graph tracer with:
> 
> | # echo do_el0_svc > /sys/kernel/tracing/set_graph_function 
> | # echo function_graph > /sys/kernel/tracing/current_tracer
> 
> ... for which the resutls looks sane.
> 
> To make sure this didn't adversely affect the return address rewriting, I also
> concurrently ran perf with:
> 
> | # perf record -g -e raw_syscalls:sys_enter:k /bin/true
> | # perf report
> 
> ... for which the results also looked fine.
> 
> I also tested the !DYNAMIC_FTRACE_WITH_REGS modes by building with an older
> compiler and also building with !DYNAMIC_FTRACE, which all looked good.
> 
> So FWIW:
> 
> Tested-by: Mark Rutland <mark.rutland@arm.com>
> 
> ... and if you make the changes I requested above:
> 
> Reviewed-by: Mark Rutland <mark.rutland@arm.com>
> 
> If you could spin a v5 with that folded in, that would be great.

Of course, will do in v5.

Thanks.

> 
> Thanks,
> Mark.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
  2022-04-19 11:41   ` Mark Rutland
@ 2022-04-20 15:30     ` Chengming Zhou
  -1 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-20 15:30 UTC (permalink / raw)
  To: Mark Rutland, rostedt
  Cc: mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa,
	broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun,
	songmuchun

Hi,

On 2022/4/19 19:41, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
>> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
>> special hooks for graph tracer, which are not needed on some ARCHs
>> that use graph_ops:func function to install return_hooker.
>>
>> So introduce the weak version in ftrace core code to cleanup
>> in x86.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v4:
>>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>>    fgraph.c instead of ftrace.c as suggested by Steve.
>>
>> v3:
>>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
>> ---
>>  arch/x86/kernel/ftrace.c | 17 ++---------------
>>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1e31c7d21597..b09d73c2ba89 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>>  
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -
>> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
>> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>>  extern void ftrace_graph_call(void);
>>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>>  {
>> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>>  
>>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>>  }
>> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -int ftrace_enable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -
>> -int ftrace_disable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -#endif /* !CONFIG_DYNAMIC_FTRACE */
>> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>>  
>>  /*
>>   * Hook the return address and push it in the stack of return addrs
>> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
>> index 8f4fb328133a..289311680c29 100644
>> --- a/kernel/trace/fgraph.c
>> +++ b/kernel/trace/fgraph.c
>> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>>  static bool fgraph_sleep_time = true;
>>  
>> +/*
>> + * archs can override this function if they must do something
>> + * to enable hook for graph tracer.
>> + */
>> +int __weak ftrace_enable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +/*
>> + * archs can override this function if they must do something
>> + * to disable hook for graph tracer.
>> + */
>> +int __weak ftrace_disable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
> 
> IIUC an arch should either:
> 
> * Have ftrace_graph_call()
> 
> * Have both ftrace_enable_ftrace_graph_caller() and
>   ftrace_disable_ftrace_graph_caller()
> 
> ... and I can't think of a reason an arch would need both ftrace_graph_call()
> *and* the enable/disable functions.

This way is more precise, but we have to add "#define ftrace_graph_call ftrace_graph_call"
in all ARCH's asm/ftrace.h correctly.

Previously we only need to define override ftrace_[enable,disable]_ftrace_graph_caller()
in ARCH's ftrace.c, looks like a little more complexity?

> 
> Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
> make the above:
> 
> | #ifndef ftrace_graph_call
> | int ftrace_enable_ftrace_graph_caller(void) { return 0; }
> | int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> | #endif /* ftrace_graph_call *. 
> 
> That way we'd catch cases when:
> 
> * An architecture meant to provide one of these functions, but forgot (e.g. the
>   name got typo'd)
> 
> * An architecture provides an unnecessary implementation of either of these
>   functions.

I tried this way today, and it works too. I'm ok with both way. Maybe I should
send v5 with this patch unchanged first. Looking forward to Steve's opinion.

Thanks.

> 
> Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
> need to take this via the arm64 tree with the next patch, so we'd need your Ack.
> 
> Thanks,
> Mark.
> 
>> +
>>  /**
>>   * ftrace_graph_stop - set to permanently disable function graph tracing
>>   *
>> -- 
>> 2.35.1
>>

^ permalink raw reply	[flat|nested] 14+ messages in thread
* Re: [External] Re: [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable
@ 2022-04-20 15:30     ` Chengming Zhou
  0 siblings, 0 replies; 14+ messages in thread
From: Chengming Zhou @ 2022-04-20 15:30 UTC (permalink / raw)
  To: Mark Rutland, rostedt
  Cc: mingo, catalin.marinas, will, tglx, bp, dave.hansen, x86, hpa,
	broonie, ardb, linux-arm-kernel, linux-kernel, duanxiongchun,
	songmuchun

Hi,

On 2022/4/19 19:41, Mark Rutland wrote:
> On Sat, Apr 09, 2022 at 11:35:53PM +0800, Chengming Zhou wrote:
>> The ftrace_[enable,disable]_ftrace_graph_caller() are used to do
>> special hooks for graph tracer, which are not needed on some ARCHs
>> that use graph_ops:func function to install return_hooker.
>>
>> So introduce the weak version in ftrace core code to cleanup
>> in x86.
>>
>> Signed-off-by: Chengming Zhou <zhouchengming@bytedance.com>
>> ---
>> v4:
>>  - put weak ftrace_enable,disable_ftrace_graph_caller() in
>>    fgraph.c instead of ftrace.c as suggested by Steve.
>>
>> v3:
>>  - consolidate two #if into a single #if, suggested by Steve. Thanks.
>> ---
>>  arch/x86/kernel/ftrace.c | 17 ++---------------
>>  kernel/trace/fgraph.c    | 18 ++++++++++++++++++
>>  2 files changed, 20 insertions(+), 15 deletions(-)
>>
>> diff --git a/arch/x86/kernel/ftrace.c b/arch/x86/kernel/ftrace.c
>> index 1e31c7d21597..b09d73c2ba89 100644
>> --- a/arch/x86/kernel/ftrace.c
>> +++ b/arch/x86/kernel/ftrace.c
>> @@ -579,9 +579,7 @@ void arch_ftrace_trampoline_free(struct ftrace_ops *ops)
>>  
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>  
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -
>> -#ifndef CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS
>> +#if defined(CONFIG_DYNAMIC_FTRACE) && !defined(CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS)
>>  extern void ftrace_graph_call(void);
>>  static const char *ftrace_jmp_replace(unsigned long ip, unsigned long addr)
>>  {
>> @@ -610,18 +608,7 @@ int ftrace_disable_ftrace_graph_caller(void)
>>  
>>  	return ftrace_mod_jmp(ip, &ftrace_stub);
>>  }
>> -#else /* !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -int ftrace_enable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -
>> -int ftrace_disable_ftrace_graph_caller(void)
>> -{
>> -	return 0;
>> -}
>> -#endif /* CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>> -#endif /* !CONFIG_DYNAMIC_FTRACE */
>> +#endif /* CONFIG_DYNAMIC_FTRACE && !CONFIG_HAVE_DYNAMIC_FTRACE_WITH_ARGS */
>>  
>>  /*
>>   * Hook the return address and push it in the stack of return addrs
>> diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c
>> index 8f4fb328133a..289311680c29 100644
>> --- a/kernel/trace/fgraph.c
>> +++ b/kernel/trace/fgraph.c
>> @@ -30,6 +30,24 @@ int ftrace_graph_active;
>>  /* Both enabled by default (can be cleared by function_graph tracer flags */
>>  static bool fgraph_sleep_time = true;
>>  
>> +/*
>> + * archs can override this function if they must do something
>> + * to enable hook for graph tracer.
>> + */
>> +int __weak ftrace_enable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
>> +
>> +/*
>> + * archs can override this function if they must do something
>> + * to disable hook for graph tracer.
>> + */
>> +int __weak ftrace_disable_ftrace_graph_caller(void)
>> +{
>> +	return 0;
>> +}
> 
> IIUC an arch should either:
> 
> * Have ftrace_graph_call()
> 
> * Have both ftrace_enable_ftrace_graph_caller() and
>   ftrace_disable_ftrace_graph_caller()
> 
> ... and I can't think of a reason an arch would need both ftrace_graph_call()
> *and* the enable/disable functions.

This way is more precise, but we have to add "#define ftrace_graph_call ftrace_graph_call"
in all ARCH's asm/ftrace.h correctly.

Previously we only need to define override ftrace_[enable,disable]_ftrace_graph_caller()
in ARCH's ftrace.c, looks like a little more complexity?

> 
> Given that, could we drop the `__weak` and place these within ifdeffery, i.e.
> make the above:
> 
> | #ifndef ftrace_graph_call
> | int ftrace_enable_ftrace_graph_caller(void) { return 0; }
> | int ftrace_disable_ftrace_graph_caller(void) { return 0; }
> | #endif /* ftrace_graph_call *. 
> 
> That way we'd catch cases when:
> 
> * An architecture meant to provide one of these functions, but forgot (e.g. the
>   name got typo'd)
> 
> * An architecture provides an unnecessary implementation of either of these
>   functions.

I tried this way today, and it works too. I'm ok with both way. Maybe I should
send v5 with this patch unchanged first. Looking forward to Steve's opinion.

Thanks.

> 
> Regardless, this looks ok to me. Steve, are you happy with this? I suspect we'd
> need to take this via the arm64 tree with the next patch, so we'd need your Ack.
> 
> Thanks,
> Mark.
> 
>> +
>>  /**
>>   * ftrace_graph_stop - set to permanently disable function graph tracing
>>   *
>> -- 
>> 2.35.1
>>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-04-20 15:31 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-09 15:35 [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou
2022-04-09 15:35 ` Chengming Zhou
2022-04-09 15:35 ` [PATCH v4 2/2] arm64/ftrace: Make function graph use ftrace directly Chengming Zhou
2022-04-09 15:35   ` Chengming Zhou
2022-04-19 12:55   ` Mark Rutland
2022-04-19 12:55     ` Mark Rutland
2022-04-19 13:27     ` [External] " Chengming Zhou
2022-04-19 13:27       ` Chengming Zhou
2022-04-18 13:24 ` [PATCH v4 1/2] ftrace: cleanup ftrace_graph_caller enable and disable Chengming Zhou
2022-04-18 13:24   ` Chengming Zhou
2022-04-19 11:41 ` Mark Rutland
2022-04-19 11:41   ` Mark Rutland
2022-04-20 15:30   ` [External] " Chengming Zhou
2022-04-20 15:30     ` Chengming Zhou

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.