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
