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

From: Paul E. McKenney
Date: Mon Jun 25 2018 - 10:46:54 EST

Next message: Rob Herring: "Re: [PATCH v8 2/6] mtd: rawnand: add an option to specify NAND chip as a boot device"
Previous message: Heikki Krogerus: "Re: [PATCH] usb: typec: tps6598x: Remove VLA usage"
In reply to: Steven Rostedt: "Re: [PATCH] rcu: Refactor rcu_{nmi,irq}_{enter,exit}()"
Next in thread: Steven Rostedt: "Re: [PATCH] rcu: Refactor rcu_{nmi,irq}_{enter,exit}()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Jun 25, 2018 at 10:07:08AM -0400, Steven Rostedt wrote:
> On Sat, 23 Jun 2018 10:49:54 -0700
> "Paul E. McKenney" <paulmck@xxxxxxxxxxxxxxxxxx> wrote:
>
> > commit 5e5ea52645b197fb7ae2f59f7927079b91e91aa0
> > Author: Byungchul Park <byungchul.park@xxxxxxx>
> > Date: Fri Jun 22 15:12:06 2018 +0900
> >
> > rcu: Refactor rcu_{nmi,irq}_{enter,exit}()
> >
> > When entering or exiting irq or NMI handlers, the current code uses
> > ->dynticks_nmi_nesting to detect if it is in the outermost handler,
> > that is, the one interrupting or returning to an RCU-idle context (the
> > idle loop or nohz_full usermode execution). When entering the outermost
> > handler via an interrupt (as opposed to NMI), it is necessary to invoke
> > rcu_dynticks_task_exit() just before the CPU is marked non-idle from an
> > RCU perspective and to invoke rcu_cleanup_after_idle() just after the
> > CPU is marked non-idle. Similarly, when exiting the outermost handler
> > via an interrupt, it is necessary to invoke rcu_prepare_for_idle() just
> > before marking the CPU idle and to invoke rcu_dynticks_task_enter()
> > just after marking the CPU idle.
> >
> > The decision to execute these four functions is currently taken in
> > rcu_irq_enter() and rcu_irq_exit() as follows:
> >
> > rcu_irq_enter()
> > /* A conditional branch with ->dynticks_nmi_nesting */
> > rcu_nmi_enter()
> > /* A conditional branch with ->dynticks */
> > /* A conditional branch with ->dynticks_nmi_nesting */
> >
> > rcu_irq_exit()
> > /* A conditional branch with ->dynticks_nmi_nesting */
> > rcu_nmi_exit()
> > /* A conditional branch with ->dynticks_nmi_nesting */
> > /* A conditional branch with ->dynticks_nmi_nesting */
> >
> > rcu_nmi_enter()
> > /* A conditional branch with ->dynticks */
> >
> > rcu_nmi_exit()
> > /* A conditional branch with ->dynticks_nmi_nesting */
> >
> > This works, but the conditional branches in rcu_irq_enter() and
> > rcu_irq_exit() are redundant with those in rcu_nmi_enter() and
> > rcu_nmi_exit(), respectively. Redundant branches are not something
> > we want in the to/from-idle fastpaths, so this commit refactors
> > rcu_{nmi,irq}_{enter,exit}() so they use a common inlined function passed
> > a constant argument as follows:
> >
> > rcu_irq_enter() inlining rcu_nmi_enter_common(irq=true)
> > /* A conditional branch with ->dynticks */
> >
> > rcu_irq_exit() inlining rcu_nmi_exit_common(irq=true)
> > /* A conditional branch with ->dynticks_nmi_nesting */
> >
> > rcu_nmi_enter() inlining rcu_nmi_enter_common(irq=false)
> > /* A conditional branch with ->dynticks */
> >
> > rcu_nmi_exit() inlining rcu_nmi_exit_common(irq=false)
> > /* A conditional branch with ->dynticks_nmi_nesting */
> >
> > The combination of the constant function argument and the inlining allows
> > the compiler to discard the conditionals that previously controlled
> > execution of rcu_dynticks_task_exit(), rcu_cleanup_after_idle(),
> > rcu_prepare_for_idle(), and rcu_dynticks_task_enter(). This reduces both
> > the to-idle and from-idle path lengths by two conditional branches each,
> > and improves readability as well.
>
> Nice write up. Makes a lot of sense.

On behalf of both Byungchul and myself, glad you like it!

> > Suggested-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> > Signed-off-by: Byungchul Park <byungchul.park@xxxxxxx>
> > Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxxxxxxxxxx>
> >
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 8788ddbc0d13..4ed74f10c852 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -773,17 +773,17 @@ void rcu_user_enter(void)
> > #endif /* CONFIG_NO_HZ_FULL */
> >
> > /**
> > - * rcu_nmi_exit - inform RCU of exit from NMI context
> > + * rcu_nmi_exit_common - inform RCU of exit from NMI context
> > *
> > * If we are returning from the outermost NMI handler that interrupted an
> > * RCU-idle period, update rdtp->dynticks and rdtp->dynticks_nmi_nesting
> > * to let the RCU grace-period handling know that the CPU is back to
> > * being RCU-idle.
> > *
> > - * If you add or remove a call to rcu_nmi_exit(), be sure to test
> > + * If you add or remove a call to rcu_nmi_exit_common(), be sure to test
> > * with CONFIG_RCU_EQS_DEBUG=y.
> > */
> > -void rcu_nmi_exit(void)
> > +static __always_inline void rcu_nmi_exit_common(bool irq)
> > {
> > struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> >
> > @@ -809,7 +809,22 @@ void rcu_nmi_exit(void)
> > /* This NMI interrupted an RCU-idle CPU, restore RCU-idleness. */
> > trace_rcu_dyntick(TPS("Startirq"), rdtp->dynticks_nmi_nesting, 0, rdtp->dynticks);
> > WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0); /* Avoid store tearing. */
> > +
> > + if (irq)
> > + rcu_prepare_for_idle();
> > +
> > rcu_dynticks_eqs_enter();
> > +
> > + if (irq)
> > + rcu_dynticks_task_enter();
> > +}
> > +
> > +/**
> > + * rcu_nmi_exit - inform RCU of exit from NMI context
> > + */
> > +void rcu_nmi_exit(void)
> > +{
> > + rcu_nmi_exit_common(false);
> > }
> >
> > /**
> > @@ -833,14 +848,8 @@ void rcu_nmi_exit(void)
> > */
> > void rcu_irq_exit(void)
> > {
> > - struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> > -
> > lockdep_assert_irqs_disabled();
> > - if (rdtp->dynticks_nmi_nesting == 1)
> > - rcu_prepare_for_idle();
> > - rcu_nmi_exit();
> > - if (rdtp->dynticks_nmi_nesting == 0)
> > - rcu_dynticks_task_enter();
> > + rcu_nmi_exit_common(true);
> > }
> >
> > /*
> > @@ -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.

(At the very least, you would be quite right to ask that this be added
to the commit log!)

Thanx, Paul

> -- Steve
>
>
> > @@ -962,6 +979,14 @@ void rcu_nmi_enter(void)
> > barrier();
> > }
> >
> > +/**
> > + * rcu_nmi_enter - inform RCU of entry to NMI context
> > + */
> > +void rcu_nmi_enter(void)
> > +{
> > + rcu_nmi_enter_common(false);
> > +}
> > +
> > /**
> > * rcu_irq_enter - inform RCU that current CPU is entering irq away from idle
> > *
> > @@ -986,14 +1011,8 @@ void rcu_nmi_enter(void)
> > */
> > void rcu_irq_enter(void)
> > {
> > - struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> > -
> > lockdep_assert_irqs_disabled();
> > - if (rdtp->dynticks_nmi_nesting == 0)
> > - rcu_dynticks_task_exit();
> > - rcu_nmi_enter();
> > - if (rdtp->dynticks_nmi_nesting == 1)
> > - rcu_cleanup_after_idle();
> > + rcu_nmi_enter_common(true);
> > }
> >
> > /*
>

Next message: Rob Herring: "Re: [PATCH v8 2/6] mtd: rawnand: add an option to specify NAND chip as a boot device"
Previous message: Heikki Krogerus: "Re: [PATCH] usb: typec: tps6598x: Remove VLA usage"
In reply to: Steven Rostedt: "Re: [PATCH] rcu: Refactor rcu_{nmi,irq}_{enter,exit}()"
Next in thread: Steven Rostedt: "Re: [PATCH] rcu: Refactor rcu_{nmi,irq}_{enter,exit}()"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]