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:

quoted

On Wed, May 18, 2022 at 09:21:18AM -0700, Paul E. McKenney wrote:

quoted

On Tue, May 17, 2022 at 12:13:45PM +0200, Jiri Olsa wrote:

quoted

On Mon, May 16, 2022 at 01:49:22PM +0200, Frederic Weisbecker wrote:

quoted

On Sun, May 15, 2022 at 09:25:35PM -0700, Paul E. McKenney wrote:

quoted

On Sun, May 15, 2022 at 10:36:52PM +0200, Jiri Olsa wrote:

quoted

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@kernel.org>

From an RCU viewpoint:

Reviewed-by: Paul E. McKenney <paulmck@kernel.org>

[ 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@kernel.org/ (local)

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.  ;-)

quoted

quoted

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