Re: [PATCH] rcu: Refactor rcu_{nmi,irq}_{enter,exit}()

Paul E. McKenney
2018-06-25T15:41:43Z

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" <paulmck@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