On Thu, 27 Jun 2019 20:58:20 +0530 "Naveen N. Rao" <naveen.n....@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. > >> - if (patch_instruction((unsigned int *)ip, pop)) { > >> + /* > >> + * Our original call site looks like: > >> + * > >> + * bl <tramp> > >> + * ld r2,XX(r1) > >> + * > >> + * Milton Miller pointed out that we can not simply nop the branch. > >> + * If a task was preempted when calling a trace function, the nops > >> + * will remove the way to restore the TOC in r2 and the r2 TOC will > >> + * get corrupted. > >> + * > >> + * Use a b +8 to jump over the load. > >> + */ > >> + if (patch_instruction((unsigned int *)ip, PPC_INST_BRANCH | 8)) { > >> pr_err("Patching NOP failed.\n"); > >> return -EPERM; > >> } > >> +#endif /* CONFIG_MPROFILE_KERNEL */ > >> > >> return 0; > >> } > >> @@ -421,6 +429,26 @@ static int __ftrace_make_nop_kernel(struct dyn_ftrace > >> *rec, unsigned long addr) > >> return -EPERM; > >> } > >> > >> +#ifdef CONFIG_MPROFILE_KERNEL > > > > I would think you need to break this up into two parts as well, with a > > synchronize_rcu_tasks() in between. > > > > Imagine this scenario: > > > > <func>: > > nop <-- interrupt comes here, and preempts the task > > nop > > > > First change. > > > > <func>: > > mflr r0 > > nop > > > > Second change. > > > > <func>: > > mflr r0 > > bl _mcount > > > > Task returns from interrupt > > > > <func>: > > mflr r0 > > bl _mcount <-- executes here > > > > It never did the mflr r0, because the last command that was executed > > was a nop before it was interrupted. And yes, it can be interrupted > > on a nop! > > 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) <rost...@goodmis.org> -- Steve