On Mon, Jun 25, 2018 at 11:02:48AM -0400, Steven Rostedt wrote: > On Mon, 25 Jun 2018 07:48:49 -0700 > "Paul E. McKenney" <paul...@linux.vnet.ibm.com> wrote: > > > > > @@ -923,7 +932,7 @@ void rcu_user_exit(void) > > > > #endif /* CONFIG_NO_HZ_FULL */ > > > > > > > > /** > > > > - * rcu_nmi_enter - inform RCU of entry to NMI context > > > > + * rcu_nmi_enter_common - inform RCU of entry to NMI context > > > > * > > > > * If the CPU was idle from RCU's viewpoint, update rdtp->dynticks and > > > > * rdtp->dynticks_nmi_nesting to let the RCU grace-period handling know > > > > @@ -931,10 +940,10 @@ void rcu_user_exit(void) > > > > * long as the nesting level does not overflow an int. (You will > > > > probably > > > > * run out of stack space first.) > > > > * > > > > - * If you add or remove a call to rcu_nmi_enter(), be sure to test > > > > + * If you add or remove a call to rcu_nmi_enter_common(), be sure to > > > > test > > > > * with CONFIG_RCU_EQS_DEBUG=y. > > > > */ > > > > -void rcu_nmi_enter(void) > > > > +static __always_inline void rcu_nmi_enter_common(bool irq) > > > > { > > > > struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks); > > > > long incby = 2; > > > > @@ -951,7 +960,15 @@ void rcu_nmi_enter(void) > > > > * period (observation due to Andy Lutomirski). > > > > */ > > > > if (rcu_dynticks_curr_cpu_in_eqs()) { > > > > + > > > > + if (irq) > > > > + rcu_dynticks_task_exit(); > > > > + > > > > rcu_dynticks_eqs_exit(); > > > > + > > > > + if (irq) > > > > + rcu_cleanup_after_idle(); > > > > + > > > > incby = 1; > > > > } > > > > trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="), > > > > > > > > > There is a slight change here, although I don't think it is an issue, > > > but I want to bring it up just in case. > > > > > > The old way had: > > > > > > rcu_dynticks_task_exit(); > > > rcu_dynticks_eqs_exit(); > > > trace_rcu_dyntick(); > > > rcu_cleanup_after_idle(); > > > > > > The new way has: > > > > > > rcu_dynticks_task_exit(); > > > rcu_dynticks_eqs_exit(); > > > rcu_cleanup_after_idle(); > > > trace_rcu_dyntick(); > > > > > > As that tracepoint will use RCU, will this cause any side effects? > > > > > > My thought is that the new way is actually more correct, as I'm not > > > sure we wanted RCU usage before the rcu_cleanup_after_idle(). > > > > I believe that this is OK because is is the position of the call to > > rcu_dynticks_eqs_exit() that really matters. Before this call, RCU > > is not yet watching, and after this call it is watching. Reversing > > the calls to rcu_cleanup_after_idle() and trace_rcu_dyntick() has them > > both being invoked while RCU is watching. > > > > All that rcu_cleanup_after_idle() does is to account for the time that > > passed while the CPU was idle, for example, advancing callbacks to allow > > for how ever many RCU grace periods completed during that idle period. > > > > Or am I missing something subtle. > > As I stated above, I actually think the new way is more correct. That's > because the trace event is the first user of RCU here and it probably > wont be the last. It makes more sense to do it after the call to > rcu_cleanup_after_idle(), just because it keeps all the RCU users after > the RCU internal code for coming out of idle. Sure, > rcu_cleanup_after_idle() doesn't do anything now that could affect > this, but maybe it will in the future?
If rcu_cleanup_after_idle() job changes, then yes, changes might be needed here and perhaps elsewhere as well. ;-) > > (At the very least, you would be quite right to ask that this be added > > to the commit log!) > > Yes, I agree. There should be a comment in the change log about this > simply because this is technically a functional change. Very good, will do! Thanx, Paul