[v1] riscv: ftrace: Fix the logic issue in DYNAMIC_FTRACE selection

[PATCH] riscv: ftrace: Fix the logic issue in DYNAMIC_FTRACE selection

Posted by ChenMiao 3 months ago

From: chenmiao <chenmiao.ku@gmail.com>

When I was reading the source code of ftrace, I learned that
ftrace has two types: static and dynamic. Initially, I planned
to prioritize reading the static source code, so I disabled
the enable dynamic option in RISCV.

[*]   Kernel Function Tracer
[ ]   Kernel Function Graph Tracer
[ ]   enable/disable function tracing dynamically (NEW)

However, when I tried to compile it, the build failed.

./include/linux/ftrace.h:190:16: error: implicit declaration of
function ‘arch_ftrace_get_regs’; did you mean ‘arch_ftrace_regs’?
[-Wimplicit-function-declaration]
  190 |         return arch_ftrace_get_regs(fregs);
      |                ^~~~~~~~~~~~~~~~~~~~
      |                arch_ftrace_regs

After comparing it with the ARM64 architecture, I found that
ARM64 automatically enables DYNAMIC_FTRACE by default once
FUNCTION_TRACER is turned on, and this cannot be set to "no".
Therefore, I believe the optional DYNAMIC_FTRACE setting in
RISC-V has a logic flaw—if FUNCTION_TRACER is enabled,
DYNAMIC_FTRACE should also be enabled, and vice versa. Moreover,
it's clear that RISC-V lacks the necessary support to successfully
compile the kernel when DYNAMIC_FTRACE is disabled.

[*]   Kernel Function Tracer
[ ]   Kernel Function Graph Tracer
-*-   enable/disable function tracing dynamically

Signed-off-by: chenmiao <chenmiao.ku@gmail.com>
---
 arch/riscv/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
index 36061f473..f7fc8b460 100644
--- a/arch/riscv/Kconfig
+++ b/arch/riscv/Kconfig
@@ -97,6 +97,7 @@ config RISCV
 	select CLONE_BACKWARDS
 	select COMMON_CLK
 	select CPU_PM if CPU_IDLE || HIBERNATION || SUSPEND
+	select DYNAMIC_FTRACE if FUNCTION_TRACER
 	select EDAC_SUPPORT
 	select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
 	select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE
-- 
2.45.2

Re: [PATCH] riscv: ftrace: Fix the logic issue in DYNAMIC_FTRACE selection

Posted by Alexandre Ghiti 3 months ago

Hi ChenMiao,

On 7/3/25 10:45, ChenMiao wrote:
> From: chenmiao <chenmiao.ku@gmail.com>
>
> When I was reading the source code of ftrace, I learned that
> ftrace has two types: static and dynamic. Initially, I planned
> to prioritize reading the static source code, so I disabled
> the enable dynamic option in RISCV.
>
> [*]   Kernel Function Tracer
> [ ]   Kernel Function Graph Tracer
> [ ]   enable/disable function tracing dynamically (NEW)
>
> However, when I tried to compile it, the build failed.
>
> ./include/linux/ftrace.h:190:16: error: implicit declaration of
> function ‘arch_ftrace_get_regs’; did you mean ‘arch_ftrace_regs’?
> [-Wimplicit-function-declaration]
>    190 |         return arch_ftrace_get_regs(fregs);
>        |                ^~~~~~~~~~~~~~~~~~~~
>        |                arch_ftrace_regs
>
> After comparing it with the ARM64 architecture, I found that
> ARM64 automatically enables DYNAMIC_FTRACE by default once
> FUNCTION_TRACER is turned on, and this cannot be set to "no".
> Therefore, I believe the optional DYNAMIC_FTRACE setting in
> RISC-V has a logic flaw—if FUNCTION_TRACER is enabled,
> DYNAMIC_FTRACE should also be enabled, and vice versa. Moreover,
> it's clear that RISC-V lacks the necessary support to successfully
> compile the kernel when DYNAMIC_FTRACE is disabled.


We could support static ftrace, but I don't think we should, so I agree 
with this patch. In fact I had just prepared a patch for this here 
https://github.com/linux-riscv/linux/pull/556/commits/0481092a5bec3818658981c11f629e06e66382b3 
which is a bit more complete since I have removed some dead code.

Let's see what other people think about supporting static ftrace, I have 
added Steven in cc if he has an opinion.

Thanks,

Alex


>
> [*]   Kernel Function Tracer
> [ ]   Kernel Function Graph Tracer
> -*-   enable/disable function tracing dynamically
>
> Signed-off-by: chenmiao <chenmiao.ku@gmail.com>
> ---
>   arch/riscv/Kconfig | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/arch/riscv/Kconfig b/arch/riscv/Kconfig
> index 36061f473..f7fc8b460 100644
> --- a/arch/riscv/Kconfig
> +++ b/arch/riscv/Kconfig
> @@ -97,6 +97,7 @@ config RISCV
>   	select CLONE_BACKWARDS
>   	select COMMON_CLK
>   	select CPU_PM if CPU_IDLE || HIBERNATION || SUSPEND
> +	select DYNAMIC_FTRACE if FUNCTION_TRACER
>   	select EDAC_SUPPORT
>   	select FRAME_POINTER if PERF_EVENTS || (FUNCTION_TRACER && !DYNAMIC_FTRACE)
>   	select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY if DYNAMIC_FTRACE

Re: [PATCH] riscv: ftrace: Fix the logic issue in DYNAMIC_FTRACE selection

Posted by Steven Rostedt 3 months ago

On Thu, 3 Jul 2025 15:00:02 +0200
Alexandre Ghiti <alex@ghiti.fr> wrote:

> 
> We could support static ftrace, but I don't think we should, so I agree 
> with this patch. In fact I had just prepared a patch for this here 
> https://github.com/linux-riscv/linux/pull/556/commits/0481092a5bec3818658981c11f629e06e66382b3 
> which is a bit more complete since I have removed some dead code.
> 
> Let's see what other people think about supporting static ftrace, I have 
> added Steven in cc if he has an opinion.

Yes, please only support the dynamic ftrace. The static is there only
to help archs to get ftrace up and running. Once dynamic is supported,
static should not be used.

Hmm, maybe I should just remove the prompt for DYNAMIC_FTRACE.

That is, once it is supported by an architecture, it should be the only
thing used.

-- Steve

Re: [PATCH] riscv: ftrace: Fix the logic issue in DYNAMIC_FTRACE selection

Posted by Alexandre Ghiti 3 months ago

Hi Steve,

On 7/3/25 17:40, Steven Rostedt wrote:
> On Thu, 3 Jul 2025 15:00:02 +0200
> Alexandre Ghiti <alex@ghiti.fr> wrote:
>
>
>> We could support static ftrace, but I don't think we should, so I agree
>> with this patch. In fact I had just prepared a patch for this here
>> https://github.com/linux-riscv/linux/pull/556/commits/0481092a5bec3818658981c11f629e06e66382b3
>> which is a bit more complete since I have removed some dead code.
>>
>> Let's see what other people think about supporting static ftrace, I have
>> added Steven in cc if he has an opinion.
> Yes, please only support the dynamic ftrace. The static is there only
> to help archs to get ftrace up and running. Once dynamic is supported,
> static should not be used.
>
> Hmm, maybe I should just remove the prompt for DYNAMIC_FTRACE.
>
> That is, once it is supported by an architecture, it should be the only
> thing used.
>
> -- Steve


Thanks for your input.

@ChenMiao: can you come up with a v2 that, in addition, deletes the dead 
code and with a commit log that explains what Steven said? If not 
possible for you, let me know and I'll do it.

Thanks,

Alex