Re: [PATCH 4/7] powerpc/ftrace: Additionally nop out the preceding mflr with -mprofile-kernel

From: Naveen N. Rao
Date: Wed Jun 19 2019 - 13:20:19 EST

Nicholas Piggin wrote:
Naveen N. Rao's on June 19, 2019 7:53 pm:
Nicholas Piggin wrote:
Michael Ellerman's on June 19, 2019 3:14 pm:

I'm also not convinced the ordering between the two patches is
guaranteed by the ISA, given that there's possibly no isync on the other

Will they go through a context synchronizing event?

synchronize_rcu_tasks() should ensure a thread is scheduled away, but
I'm not actually sure it guarantees CSI if it's kernel->kernel. Could
do a smp_call_function to do the isync on each CPU to be sure.

Good point. Per Documentation/RCU/Design/Requirements/Requirements.html#Tasks RCU:
"The solution, in the form of Tasks RCU, is to have implicit read-side critical sections that are delimited by voluntary context switches, that is, calls to schedule(), cond_resched(), and synchronize_rcu_tasks(). In addition, transitions to and from userspace execution also delimit tasks-RCU read-side critical sections."

I suppose transitions to/from userspace, as well as calls to schedule() result in context synchronizing instruction being executed. But, if some tasks call cond_resched() and synchronize_rcu_tasks(), we probably won't have a CSI executed.

"In CONFIG_PREEMPT=n kernels, trampolines cannot be preempted, so these APIs map to call_rcu(), synchronize_rcu(), and rcu_barrier(), respectively."

In this scenario as well, I think we won't have a CSI executed in case of cond_resched().

Should we enhance patch_instruction() to handle that?

Well, not sure. Do we have many post-boot callers of it? Should
they take care of their own synchronization requirements?

Kprobes and ftrace are the two users (along with anything else that may use jump labels).

Looking at this from the CMODX perspective: the main example quoted of an erratic behavior is when any variant of the patched instruction causes an exception.

With ftrace, I think we are ok since we only ever patch a 'nop' or a 'bl' (and the 'mflr' now), none of which should cause an exception. As such, the existing patch_instruction() should suffice.

However, with kprobes, we patch a 'trap' (or a branch in case of optprobes) on most instructions. I wonder if we should be issuing an 'isync' on all cpus in this case. Or, even if that is sufficient or necessary.