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" 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. 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: : mflrr0 <-- preempt here bl _mcount : mflrr0 nop And this is fine regardless. OK, Reviewed-by: Steven Rostedt (VMware) 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
Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel
Naveen N. Rao's on June 27, 2019 9:23 pm: > 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, 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. 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 'smp_call_function(isync); > 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. I think this seems like a reasonable sequence that will work on our hardware, although technically outside the ISA as specified maybe we should add a feature bit or at least comment for it. It would be kind of nice to not put this stuff directly in the ftrace code, but rather in the function patching subsystem. I think it would be too expensive to just make a runtime variant of patch_instruction that always does the SMP isync, but possibly a patch_instruction_sync() or something that we say ensures no processor is running code that has been patched away. Thanks, Nick
Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel
On Thu, 27 Jun 2019 20:58:20 +0530
"Naveen N. Rao" 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
> >> + * 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:
> >
> > :
> > nop <-- interrupt comes here, and preempts the task
> > nop
> >
> > First change.
> >
> > :
> > mflrr0
> > nop
> >
> > Second change.
> >
> > :
> > mflrr0
> > bl _mcount
> >
> > Task returns from interrupt
> >
> > :
> > mflrr0
> > 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:
:
mflrr0 <-- preempt here
bl _mcount
:
mflrr0
nop
And this is fine regardless.
OK, Reviewed-by: Steven Rostedt (VMware)
-- Steve
Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel
Hi Steven,
Thanks for the review!
Steven Rostedt wrote:
On Thu, 27 Jun 2019 16:53:52 +0530
"Naveen N. Rao" wrote:
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, 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. 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 'smp_call_function(isync);
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.
You may want to explain that you do the reverse when patching it out.
That is, patch out the "bl _mcount" into a nop and then patch out the
"mflr r0".
Sure. I think we can add:
"When disabling function tracing, we can nop out the two instructions
without need for any synchronization in between, as long as we nop out
the branch to ftrace first. The lone 'mflr r0' is harmless. Finally,
with FTRACE_UPDATE_MODIFY_CALL, no changes are needed since we are
simply changing where the branch to ftrace goes."
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?
Suggested-by: Nicholas Piggin
Reviewed-by: Nicholas Piggin
Signed-off-by: Naveen N. Rao
---
arch/powerpc/kernel/trace/ftrace.c | 258 ++---
1 file changed, 236 insertions(+), 22 deletions(-)
diff --git a/arch/powerpc/kernel/trace/ftrace.c
b/arch/powerpc/kernel/trace/ftrace.c
index 517662a56bdc..86c2b50dcaa9 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -125,7 +125,7 @@ __ftrace_make_nop(struct module *mod,
{
unsigned long entry, ptr, tramp;
unsigned long ip = rec->ip;
- unsigned int op, pop;
+ unsigned int op;
/* read where this goes */
if (probe_kernel_read(, (void *)ip, sizeof(int))) {
@@ -160,8 +160,6 @@ __ftrace_make_nop(struct module *mod,
#ifdef CONFIG_MPROFILE_KERNEL
/* When using -mkernel_profile there is no load to jump over */
- pop = PPC_INST_NOP;
-
if (probe_kernel_read(, (void *)(ip - 4), 4)) {
pr_err("Fetching instruction at %lx failed.\n", ip - 4);
return -EFAULT;
@@ -169,26 +167,23 @@ __ftrace_make_nop(struct module *mod,
/* We expect either a mflr r0, or a std r0, LRSAVE(r1) */
if (op != PPC_INST_MFLR && op != PPC_INST_STD_LR) {
- pr_err("Unexpected instruction %08x around bl _mcount\n", op);
+ pr_err("Unexpected instruction %08x before bl _mcount\n", op);
return -EINVAL;
}
-#else
- /*
-* Our original call site looks like:
-*
-* bl
-* 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.
-*/
- pop = PPC_INST_BRANCH | 8; /* b +8 */
+ /* We should patch out the bl to _mcount first */
+ if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
+ pr_err("Patching NOP failed.\n");
+ return -EPERM;
+ }
+ /* then, nop out the preceding 'mflr r0' as an optimization */
+ if (op == PPC_INST_MFLR &&
+ patch_instruction((unsigned int *)(ip - 4), PPC_INST_NOP)) {
+ pr_err("Patching NOP failed.\n");
+ return -EPERM;
+ }
+#else
/*
* Check what is in the next instruction. We can see ld r2,40(r1), but
* on first pass after boot we will see mflr r0.
@@ -202,12 +197,25 @@ __ftrace_make_nop(struct module *mod,
pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, op);
return -EINVAL;
}
-#endif /* CONFIG_MPROFILE_KERNEL */
- if (patch_instruction((unsigned int *)ip, pop)) {
+ /*
+* Our original call site looks like:
+*
+* bl
+* ld r2,XX(r1)
+*
+* Milton Miller pointed out that we can not simply nop the branch.
+* If a task was preempted
Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel
On Thu, 27 Jun 2019 16:53:52 +0530
"Naveen N. Rao" wrote:
> 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, 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. 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 'smp_call_function(isync);
> 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.
You may want to explain that you do the reverse when patching it out.
That is, patch out the "bl _mcount" into a nop and then patch out the
"mflr r0". But interesting, I don't see a synchronize_rcu_tasks() call
there.
>
> Suggested-by: Nicholas Piggin
> Reviewed-by: Nicholas Piggin
> Signed-off-by: Naveen N. Rao
> ---
> arch/powerpc/kernel/trace/ftrace.c | 258 ++---
> 1 file changed, 236 insertions(+), 22 deletions(-)
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c
> b/arch/powerpc/kernel/trace/ftrace.c
> index 517662a56bdc..86c2b50dcaa9 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -125,7 +125,7 @@ __ftrace_make_nop(struct module *mod,
> {
> unsigned long entry, ptr, tramp;
> unsigned long ip = rec->ip;
> - unsigned int op, pop;
> + unsigned int op;
>
> /* read where this goes */
> if (probe_kernel_read(, (void *)ip, sizeof(int))) {
> @@ -160,8 +160,6 @@ __ftrace_make_nop(struct module *mod,
>
> #ifdef CONFIG_MPROFILE_KERNEL
> /* When using -mkernel_profile there is no load to jump over */
> - pop = PPC_INST_NOP;
> -
> if (probe_kernel_read(, (void *)(ip - 4), 4)) {
> pr_err("Fetching instruction at %lx failed.\n", ip - 4);
> return -EFAULT;
> @@ -169,26 +167,23 @@ __ftrace_make_nop(struct module *mod,
>
> /* We expect either a mflr r0, or a std r0, LRSAVE(r1) */
> if (op != PPC_INST_MFLR && op != PPC_INST_STD_LR) {
> - pr_err("Unexpected instruction %08x around bl _mcount\n", op);
> + pr_err("Unexpected instruction %08x before bl _mcount\n", op);
> return -EINVAL;
> }
> -#else
> - /*
> - * Our original call site looks like:
> - *
> - * bl
> - * 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.
> - */
>
> - pop = PPC_INST_BRANCH | 8; /* b +8 */
> + /* We should patch out the bl to _mcount first */
> + if (patch_instruction((unsigned int *)ip, PPC_INST_NOP)) {
> + pr_err("Patching NOP failed.\n");
> + return -EPERM;
> + }
>
> + /* then, nop out the preceding 'mflr r0' as an optimization */
> + if (op == PPC_INST_MFLR &&
> + patch_instruction((unsigned int *)(ip - 4), PPC_INST_NOP)) {
> + pr_err("Patching NOP failed.\n");
> + return -EPERM;
> + }
> +#else
> /*
>* Check what is in the next instruction. We can see ld r2,40(r1), but
>* on first pass after boot we will see mflr r0.
> @@ -202,12 +197,25 @@ __ftrace_make_nop(struct module *mod,
> pr_err("Expected %08x found %08x\n", PPC_INST_LD_TOC, op);
> return -EINVAL;
> }
> -#endif /* CONFIG_MPROFILE_KERNEL */
>
> - if (patch_instruction((unsigned int *)ip, pop)) {
> + /*
> + * Our original call site looks like:
> + *
> + * bl
> + * 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
Re: [PATCH v2 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel
Naveen N. Rao wrote: 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, 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. 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 'smp_call_function(isync); 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. Suggested-by: Nicholas Piggin Reviewed-by: Nicholas Piggin Signed-off-by: Naveen N. Rao --- arch/powerpc/kernel/trace/ftrace.c | 258 ++--- 1 file changed, 236 insertions(+), 22 deletions(-) I missed adding a comment here to explain the changes. As discussed in the previous series, I think we are ok with this patch from a CMODX perspective. For smp_call_function(), I decided to have it included in this patch since we know that we need it here for sure. I am not entirely sure we want to do that in patch_instruction() since ftrace doesn't seem to need it elsewhere. As Nick Piggin pointed out, we may want to have users of patch_instruction() (kprobes) add the necessary synchronization. Thanks, Naveen
