Re: [PATCH] RISC-V: Take text_mutex in ftrace_init_nop()

[Steven Rostedt <rostedt@goodmis.org>]

On Mon, 24 Aug 2020 17:21:22 -0700
Palmer Dabbelt <palmerdabbelt@google.com> wrote:

> Without this we get lockdep failures.  They're spurious failures as SMP isn't
> up when ftrace_init_nop() is called.  As far as I can tell the easiest fix is
> to just take the lock, which also seems like the safest fix.
> 
> Signed-off-by: Palmer Dabbelt <palmerdabbelt@google.com>
> ---
> I haven't actually tested this, as I don't have a workload that exercises
> ftrace in a meaningful fashion, but this seems pretty safe to me.  It smells to
> me like we should handle this in the generic code (make the generic
> ftrace_init_nop() call brand new
> ftrace_arch_code_modify_{prepare,post_process}_init(), which default to calling
> ftrace_arch_code_modify_{prepare,post_process}() or just juggling the lock
> (depending on what most architectures are doing)), but this at least fixes the
> issue on our end so it seems reasonable for now.
> 
> Thinking about it: I guess if I just booted with ftrace and lockdep it'd catch
> this issue, so that seems like an obvious test case to add.  If someone has an
> easy way to exercise more ftrace stuff I'm happy to run that in addition.

I'm not able to test this either, but it looks reasonable to me.

Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>

-- Steve

> ---
>  arch/riscv/include/asm/ftrace.h |  7 +++++++
>  arch/riscv/kernel/ftrace.c      | 19 +++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/ftrace.h b/arch/riscv/include/asm/ftrace.h
> index ace8a6e2d11d..845002cc2e57 100644
> --- a/arch/riscv/include/asm/ftrace.h
> +++ b/arch/riscv/include/asm/ftrace.h
> @@ -66,6 +66,13 @@ do {									\
>   * Let auipc+jalr be the basic *mcount unit*, so we make it 8 bytes here.
>   */
>  #define MCOUNT_INSN_SIZE 8
> +
> +#ifndef __ASSEMBLY__
> +struct dyn_ftrace;
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec);
> +#define ftrace_init_nop ftrace_init_nop
> +#endif
> +
>  #endif
>  
>  #endif /* _ASM_RISCV_FTRACE_H */
> diff --git a/arch/riscv/kernel/ftrace.c b/arch/riscv/kernel/ftrace.c
> index 2ff63d0cbb50..99e12faa5498 100644
> --- a/arch/riscv/kernel/ftrace.c
> +++ b/arch/riscv/kernel/ftrace.c
> @@ -97,6 +97,25 @@ int ftrace_make_nop(struct module *mod, struct dyn_ftrace *rec,
>  	return __ftrace_modify_call(rec->ip, addr, false);
>  }
>  
> +
> +/*
> + * This is called early on, and isn't wrapped by
> + * ftrace_arch_code_modify_{prepare,post_process}() and therefor doesn't hold
> + * text_mutex, which triggers a lockdep failure.  SMP isn't running so we could
> + * just directly poke the text, but it's simpler to just take the lock
> + * ourselves.
> + */
> +int ftrace_init_nop(struct module *mod, struct dyn_ftrace *rec)
> +{
> +	int out;
> +
> +	ftrace_arch_code_modify_prepare();
> +	out = ftrace_make_nop(mod, rec, MCOUNT_ADDR);
> +	ftrace_arch_code_modify_post_process();
> +
> +	return out;
> +}
> +
>  int ftrace_update_ftrace_func(ftrace_func_t func)
>  {
>  	int ret = __ftrace_modify_call((unsigned long)&ftrace_call,


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