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

[Naveen N. Rao]

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
> > > > CPU.
> > >
> > > 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.
> >
> > Also:
> > "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.


Thanks,
Naveen