[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-Id: <1561970917.6b4f6qppo3.naveen@linux.ibm.com>
Date: Mon, 01 Jul 2019 14:21:17 +0530
From: "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linux-kernel@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
Masami Hiramatsu <mhiramat@...nel.org>,
Ingo Molnar <mingo@...nel.org>,
Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>
Subject: Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding
mflr with -mprofile-kernel
Steven Rostedt wrote:
> On Thu, 27 Jun 2019 20:58:20 +0530
> "Naveen N. Rao" <naveen.n.rao@...ux.vnet.ibm.com> wrote:
>
>>
>> > But interesting, I don't see a synchronize_rcu_tasks() call
>> > there.
>>
>> We felt we don't need it in this case. We patch the branch to ftrace
>> with a nop first. Other cpus should see that first. But, now that I
>> think about it, should we add a memory barrier to ensure the writes get
>> ordered properly?
>
> Do you send an ipi to the other CPUs. I would just to be safe.
>
<snip>
>>
>> We are handling this through ftrace_replace_code() and
>> __ftrace_make_call_prep() below. For FTRACE_UPDATE_MAKE_CALL, we patch
>> in the mflr, followed by smp_call_function(isync) and
>> synchronize_rcu_tasks() before we proceed to patch the branch to ftrace.
>>
>> I don't see any other scenario where we end up in
>> __ftrace_make_nop_kernel() without going through ftrace_replace_code().
>> For kernel modules, this can happen during module load/init and so, I
>> patch out both instructions in __ftrace_make_call() above without any
>> synchronization.
>>
>> Am I missing anything?
>>
>
> No, I think I got confused ;-), it's the patch out that I was worried
> about, but when I was going through the scenario, I somehow turned it
> into the patching in (which I already audited :-p). I was going to
> reply with just the top part of this email, but then the confusion
> started :-/
>
> OK, yes, patching out should be fine, and you already covered the
> patching in. Sorry for the noise.
>
> Just to confirm and totally remove the confusion, the patch does:
>
> <func>:
> mflr r0 <-- preempt here
> bl _mcount
>
> <func>:
> mflr r0
> nop
>
> And this is fine regardless.
>
> OK, Reviewed-by: Steven Rostedt (VMware) <rostedt@...dmis.org>
Thanks for confirming! We do need an IPI to be sure, as you pointed out
above. I will have the patching out take the same path to simplify
things.
- Naveen
Powered by blists - more mailing lists