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

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

[Palmer Dabbelt]

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.
---
 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,
-- 
2.28.0.297.g1956fa8f8d-goog


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

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

[Guo Ren]

Thx Palmer,

The patch is what I need.

Acked-by: Guo Ren <guoren@kernel.org>

On Tue, Aug 25, 2020 at 8:29 AM 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.
> ---
>  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,
> --
> 2.28.0.297.g1956fa8f8d-goog
>


-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

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

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

[Guo Ren]

I'll test it later, and include it in my patchset series.

On Tue, Aug 25, 2020 at 4:43 PM Guo Ren <guoren@kernel.org> wrote:
>
> Thx Palmer,
>
> The patch is what I need.
>
> Acked-by: Guo Ren <guoren@kernel.org>
>
> On Tue, Aug 25, 2020 at 8:29 AM 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.
> > ---
> >  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,
> > --
> > 2.28.0.297.g1956fa8f8d-goog
> >
>
>
> --
> Best Regards
>  Guo Ren
>
> ML: https://lore.kernel.org/linux-csky/



-- 
Best Regards
 Guo Ren

ML: https://lore.kernel.org/linux-csky/

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

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

[Steven Rostedt]

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