Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 01 Jul 2019 14:21:17 +0530
From:   "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Subject: Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding
 mflr with -mprofile-kernel
To:     Steven Rostedt <rostedt@goodmis.org>
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org,
        Masami Hiramatsu <mhiramat@kernel.org>,
        Ingo Molnar <mingo@kernel.org>,
        Michael Ellerman <mpe@ellerman.id.au>,
        Nicholas Piggin <npiggin@gmail.com>
References: <cover.1561634177.git.naveen.n.rao@linux.vnet.ibm.com>
        <841386feda429a1f0d4b7442c3ede1ed91466f92.1561634177.git.naveen.n.rao@linux.vnet.ibm.com>
        <20190627110819.4892780f@gandalf.local.home>
        <1561648598.uvetvkj39x.naveen@linux.ibm.com>
        <20190627121344.25b5449a@gandalf.local.home>
In-Reply-To: <20190627121344.25b5449a@gandalf.local.home>
User-Agent: astroid/0.14.0 (https://github.com/astroidmail/astroid)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8BIT
Message-Id: <1561970917.6b4f6qppo3.naveen@linux.ibm.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Steven Rostedt wrote:
> On Thu, 27 Jun 2019 20:58:20 +0530
> "Naveen N. Rao" <naveen.n.rao@linux.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@goodmis.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