Messages in this thread | | | Date | Sun, 29 Nov 2020 16:29:18 -0800 | From | "Paul E. McKenney" <> | Subject | Re: [GIT pull] locking/urgent for v5.10-rc6 |
| |
On Sun, Nov 29, 2020 at 11:31:41AM -0800, Linus Torvalds wrote: > On Sun, Nov 29, 2020 at 5:38 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > > > Yet two more places which invoke tracing from RCU disabled regions in the > > idle path. Similar to the entry path the low level idle functions have to > > be non-instrumentable. > > This really seems less than optimal. > > In particular, lookie here: > > > @@ -94,9 +94,35 @@ void __cpuidle default_idle_call(void) > > > > trace_cpu_idle(1, smp_processor_id()); > > stop_critical_timings(); > > + > > + /* > > + * arch_cpu_idle() is supposed to enable IRQs, however > > + * we can't do that because of RCU and tracing. > > + * > > + * Trace IRQs enable here, then switch off RCU, and have > > + * arch_cpu_idle() use raw_local_irq_enable(). Note that > > + * rcu_idle_enter() relies on lockdep IRQ state, so switch that > > + * last -- this is very similar to the entry code. > > + */ > > + trace_hardirqs_on_prepare(); > > + lockdep_hardirqs_on_prepare(_THIS_IP_); > > rcu_idle_enter(); > > + lockdep_hardirqs_on(_THIS_IP_); > > + > > arch_cpu_idle(); > > + > > + /* > > + * OK, so IRQs are enabled here, but RCU needs them disabled to > > + * turn itself back on.. funny thing is that disabling IRQs > > + * will cause tracing, which needs RCU. Jump through hoops to > > + * make it 'work'. > > + */ > > + raw_local_irq_disable(); > > + lockdep_hardirqs_off(_THIS_IP_); > > rcu_idle_exit(); > > + lockdep_hardirqs_on(_THIS_IP_); > > + raw_local_irq_enable(); > > + > > start_critical_timings(); > > trace_cpu_idle(PWR_EVENT_EXIT, smp_processor_id()); > > } > > And look at what the code generation for the idle exit path is when > lockdep isn't even on. > > It's *literally* > > cli > call rcu_idle_exit > sti > > and guess what rcu_idle_exit does? > > Yeah, that one does "pushf; cli; call rcu_eqs_exit; popf". > > So here we are, in the somewhat critical "an interrupt woke us up" > section, and we're doing just ridiculously stupid things. > > I've pulled this, because it solves a problem, but there's a deeper > problem here in how all this is done. > > The idle path is actually quite important. I can point to real loads > where this is a big part of the CPU profile, because you end up having > lots of "go to sleep for very short times, because the thing we were > waiting for takes almost no time at all".
This is because of the noinline implied by the noinstr on rcu_eqs_exit(). If I replace that with inline, it does get inlined. Except that, if I remember correctly, making that change messes up the tooling that enforces the no-instrumentation regions.
I -think- that a combination of instrumentation_end() and s/noinstr/inline/ might work, but I will need to consult with the experts on this.
Thanx, Paul
| |