Re: [PATCH bpf-next 1/2] cpuidle/rcu: Making arch_cpu_idle and rcu_idle_exit noinstr

[Paul E. McKenney]

On Mon, May 23, 2022 at 03:12:28PM +0200, Jiri Olsa wrote:
> On Thu, May 19, 2022 at 06:54:39AM -0700, Paul E. McKenney wrote:
> > On Thu, May 19, 2022 at 01:33:16PM +0200, Jiri Olsa wrote:
> > > On Wed, May 18, 2022 at 09:21:18AM -0700, Paul E. McKenney wrote:
> > > > On Tue, May 17, 2022 at 12:13:45PM +0200, Jiri Olsa wrote:
> > > > > On Mon, May 16, 2022 at 01:49:22PM +0200, Frederic Weisbecker wrote:
> > > > > > On Sun, May 15, 2022 at 09:25:35PM -0700, Paul E. McKenney wrote:
> > > > > > > On Sun, May 15, 2022 at 10:36:52PM +0200, Jiri Olsa wrote:
> > > > > > > > Making arch_cpu_idle and rcu_idle_exit noinstr. Both functions run
> > > > > > > > in rcu 'not watching' context and if there's tracer attached to
> > > > > > > > them, which uses rcu (e.g. kprobe multi interface) it will hit RCU
> > > > > > > > warning like:
> > > > > > > > 
> > > > > > > >   [    3.017540] WARNING: suspicious RCU usage
> > > > > > > >   ...
> > > > > > > >   [    3.018363]  kprobe_multi_link_handler+0x68/0x1c0
> > > > > > > >   [    3.018364]  ? kprobe_multi_link_handler+0x3e/0x1c0
> > > > > > > >   [    3.018366]  ? arch_cpu_idle_dead+0x10/0x10
> > > > > > > >   [    3.018367]  ? arch_cpu_idle_dead+0x10/0x10
> > > > > > > >   [    3.018371]  fprobe_handler.part.0+0xab/0x150
> > > > > > > >   [    3.018374]  0xffffffffa00080c8
> > > > > > > >   [    3.018393]  ? arch_cpu_idle+0x5/0x10
> > > > > > > >   [    3.018398]  arch_cpu_idle+0x5/0x10
> > > > > > > >   [    3.018399]  default_idle_call+0x59/0x90
> > > > > > > >   [    3.018401]  do_idle+0x1c3/0x1d0
> > > > > > > > 
> > > > > > > > The call path is following:
> > > > > > > > 
> > > > > > > > default_idle_call
> > > > > > > >   rcu_idle_enter
> > > > > > > >   arch_cpu_idle
> > > > > > > >   rcu_idle_exit
> > > > > > > > 
> > > > > > > > The arch_cpu_idle and rcu_idle_exit are the only ones from above
> > > > > > > > path that are traceble and cause this problem on my setup.
> > > > > > > > 
> > > > > > > > Signed-off-by: Jiri Olsa <jolsa@xxxxxxxxxx>
> > > > > > > 
> > > > > > > From an RCU viewpoint:
> > > > > > > 
> > > > > > > Reviewed-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > > > > > > 
> > > > > > > [ I considered asking for an instrumentation_on() in rcu_idle_exit(),
> > > > > > > but there is no point given that local_irq_restore() isn't something
> > > > > > > you instrument anyway. ]
> > > > > > 
> > > > > > So local_irq_save() in the beginning of rcu_idle_exit() is unsafe because
> > > > > > it is instrumentable by the function (graph)  tracers and the irqsoff tracer.
> > > > > > 
> > > > > > Also it calls into lockdep that might make use of RCU.
> > > > > > 
> > > > > > That's why rcu_idle_exit() is not noinstr yet. See this patch:
> > > > > > 
> > > > > > https://lore.kernel.org/lkml/20220503100051.2799723-4-frederic@xxxxxxxxxx/
> > > > > 
> > > > > I see, could we mark it at least with notrace meanwhile?
> > > > 
> > > > For the RCU part, how about as follows?
> > > > 
> > > > If this approach is reasonable, my guess would be that Frederic will pull
> > > > it into his context-tracking series, perhaps using a revert of this patch
> > > > to maintain sanity in the near term.
> > > > 
> > > > If this approach is unreasonable, well, that is Murphy for you!
> > > 
> > > I checked and it works in my test ;-)
> > 
> > Whew!!!  One piece of the problem might be solved, then.  ;-)
> > 
> > > > For the x86 idle part, my feeling is still that the rcu_idle_enter()
> > > > and rcu_idle_exit() need to be pushed deeper into the code.  Perhaps
> > > > an ongoing process as the idle loop continues to be dug deeper?
> > > 
> > > for arch_cpu_idle with noinstr I'm getting this W=1 warning:
> > > 
> > > vmlinux.o: warning: objtool: arch_cpu_idle()+0xb: call to {dynamic}() leaves .noinstr.text section
> > > 
> > > we could have it with notrace if that's a problem
> > 
> > I would be happy to queue the arch_cpu_idle() portion of your patch on
> > -rcu, if that would move things forward.  I suspect that additional
> > x86_idle() surgery is required, but maybe I am just getting confused
> > about what the x86_idle() function pointer can point to.  But it looks
> > to me like these need further help:
> > 
> > o	static void amd_e400_idle(void)
> > 	Plus things it calls, like tick_broadcast_enter() and
> > 	tick_broadcast_exit().
> > 
> > o	static __cpuidle void mwait_idle(void)
> > 
> > So it might not be all that much additional work, even if I have avoided
> > confusion about what the x86_idle() function pointer can point to.  But
> > I do not trust my ability to test this accurately.
> 
> same here ;-) you're right, there will be other places based
> on x86_idle function pointer.. I'll check it, but perhaps we
> could address that when someone reports that
> 
> jirka

Any thoughts on the correct approach?  One extreme would be to
mark all sorts of things noinstr.  Another extreme would be to
enclose all sorts of things in RCU_NONIDLE().  Yet another extreme
would be to push the rcu_idle_enter() and rcu_idle_exit() calls
still deeper into the idle loop.

Or does Peter's recent series somehow cover all of this?

https://lore.kernel.org/all/20220519212750.656413111@xxxxxxxxxxxxx/

							Thanx, Paul