Re: [RFC PATCH 4/4] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel

[Naveen N. Rao]

Nicholas Piggin wrote:
> Naveen N. Rao's on May 18, 2019 5:02 am:
> > With -mprofile-kernel, gcc emits 'mflr r0', followed by 'bl _mcount' to
> > enable function tracing and profiling. So far, with dynamic ftrace, we
> > used to only patch out the branch to _mcount(). However, Nick Piggin
> > points out that "mflr is executed by the branch unit that can only
> > execute one per cycle on POWER9 and shared with branches, so it would be
> > nice to avoid it where possible."
> >
> > We cannot simply nop out the mflr either. Michael Ellerman pointed out
> > that when enabling function tracing, there can be a race if tracing is
> > enabled when some thread was interrupted after executing a nop'ed out
> > mflr. In this case, the thread would execute the now-patched-in branch
> > to _mcount() without having executed the preceding mflr.
> >
> > To solve this, we now enable function tracing in 2 steps: patch in the
> > mflr instruction, use synchronize_rcu_tasks() to ensure all existing
> > threads make progress, and then patch in the branch to _mcount(). We
> > override ftrace_replace_code() with a powerpc64 variant for this
> > purpose.
> >
> > Signed-off-by: Nicholas Piggin <npiggin@xxxxxxxxx>
> > Signed-off-by: Naveen N. Rao <naveen.n.rao@xxxxxxxxxxxxxxxxxx>
>
> Nice! Thanks for doing a real patch. You needn't add my SOB there: my
> hack was obviously garbage :) Suggested-by if anything, then for
> clarity of changelog you can write the motivation directly rather than
> quote me.

Thanks, I meant to call out the fact that I had added your SOB before
sending the patch, but missed doing so. Your patch was perfectly fine ;)

> I don't know the ftrace subsystem well, but the powerpc instructions
> and patching sequence appears to match what we agreed is the right way
> to go.
>
> As a suggestion, I would perhaps add most of information from the
> second and third paragraphs of the changelog into comments
> (and also explain that the lone mflr r0 is harmless).
>
> But otherwise it looks good
>
> Reviewed-by: Nicholas Piggin <npiggin@xxxxxxxxx>

Thanks, I will incorporate those changes.


- Naveen