On Thu, Jun 21, 2018 at 03:39:49PM +0900, Byungchul Park wrote:
> On Wed, Jun 20, 2018 at 10:40:37AM -0700, Paul E. McKenney wrote:
> > On Thu, Jun 21, 2018 at 02:15:07AM +0900, Byungchul Park wrote:
>
> [...]
>
> > > Totally agree with you. Sorry bothering you.
> >
> > Absolutely not a problem, absolutely no need to apologize! I am
> > actually very happy that you are taking RCU seriously and looking at it
> > in such depth.
>
> Thanks a lot. :)
>
> > My problem is that when I see a patch like this, something in the back of
> > my head screams "WRONG!!!", and I sometimes get confused about exactly
> > what the back of my head is screaming about, which was the case here.
> > Hence my misguided initial complaint about NMI nesting instead of about
> > the possibility of unpaired rcu_irq_enter() calls.
> >
> > So apologies for that, but I unfortunately cannot promise that this
>
> It's ok. I also made a mistake.
>
> > won't happen again. I have learned the hard way to trust the back of
> > my head. It sometimes makes mistakes, but less often than the rest of
> > my head does. ;-)
>
> I believe it doesn't matter at all as everybody makes mistakes. You must
> be much more careful in everything than others though. I believe the
> only problem with regard to human's mistakes is the attitude never even
> trying to communicate with others, being convinced that they've never
> made mistakes.
>
> > In the meantime, is it possible to rearrange rcu_irq_enter() and
> > rcu_nmi_enter() (and similarly rcu_irq_exit() and rcu_nmi_exit())
> > to avoid the conditionals (via compiler inlining) while still keeping
> > function calls ordered properly? I bet that you could do it by splitting
> > rcu_nmi_enter() and rcu_nmi_exit() sort of like this:
> >
> > static void rcu_nmi_enter_common(bool irq)
> > {
> > /*
> > * You fill this in. Maybe __always_inline above. The
> > * rcu_dynticks_task_exit() and rcu_cleanup_after_idle()
> > * calls need to be on opposite sides of the
> > * rcu_dynticks_eqs_exit() call, just like they are now.
> > */
> > }
> >
> > void rcu_nmi_enter(void)
> > {
> > rcu_nmi_enter_common(false);
> > }
> >
> > void rcu_irq_enter(void)
> > {
> > lockdep_assert_irqs_disabled();
> > rcu_nmi_enter(true);
> > }
> >
> > Saving a couple of branches on the irq enter/exit paths seems like it
> > just might be worth something. ;-)
>
> What about the following patch?
>
> I applied what you suggested and re-named rcu_nmi_{enter,exit} to
> rcu_irq_{enter,exit} and applied the same re-naming to
> ->dynticks_nmi_nesting as well, since those are not things to do with
> nmi anymore but both irq and nmi.
>
> I think "irq" is better to represent both irq and nmi than "nmi".
> Please let me know if you don't think so. I can get rid of the re-
> naming from the patch.
Or I can make this patch into two, one of which is only for the re-naming
including ./Documentation/RCU/Design/Data-Structures/Data-Structures.html
as well.
Just let me know. I will follow your decision.
> I will re-send this with a change log after getting your opinion.
>
> ----->8-----
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index deb2508..413fef7 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -260,7 +260,7 @@ void rcu_bh_qs(void)
>
> static DEFINE_PER_CPU(struct rcu_dynticks, rcu_dynticks) = {
> .dynticks_nesting = 1,
> - .dynticks_nmi_nesting = DYNTICK_IRQ_NONIDLE,
> + .dynticks_irq_nesting = DYNTICK_IRQ_NONIDLE,
> .dynticks = ATOMIC_INIT(RCU_DYNTICK_CTRL_CTR),
> };
>
> @@ -695,7 +695,7 @@ static struct rcu_node *rcu_get_root(struct rcu_state
> *rsp)
> * Enter an RCU extended quiescent state, which can be either the
> * idle loop or adaptive-tickless usermode execution.
> *
> - * We crowbar the ->dynticks_nmi_nesting field to zero to allow for
> + * We crowbar the ->dynticks_irq_nesting field to zero to allow for
> * the possibility of usermode upcalls having messed up our count
> * of interrupt nesting level during the prior busy period.
> */
> @@ -706,7 +706,7 @@ static void rcu_eqs_enter(bool user)
> struct rcu_dynticks *rdtp;
>
> rdtp = this_cpu_ptr(&rcu_dynticks);
> - WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
> + WRITE_ONCE(rdtp->dynticks_irq_nesting, 0);
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> rdtp->dynticks_nesting == 0);
> if (rdtp->dynticks_nesting != 1) {
> @@ -764,43 +764,58 @@ void rcu_user_enter(void)
> #endif /* CONFIG_NO_HZ_FULL */
>
> /**
> - * rcu_nmi_exit - inform RCU of exit from NMI context
> + * rcu_irq_exit_common - inform RCU of exit from interrupt 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 we are returning from the outermost interrupt handler that
> + * interrupted an RCU-idle period, update rdtp->dynticks and
> + * rdtp->dynticks_irq_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
> - * with CONFIG_RCU_EQS_DEBUG=y.
> + * If you add or remove a call to rcu_irq_exit_common(), be sure to
> + * test with CONFIG_RCU_EQS_DEBUG=y.
> */
> -void rcu_nmi_exit(void)
> +static __always_inline void rcu_irq_exit_common(bool nmi)
> {
> struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
>
> /*
> - * Check for ->dynticks_nmi_nesting underflow and bad ->dynticks.
> + * Check for ->dynticks_irq_nesting underflow and bad ->dynticks.
> * (We are exiting an NMI handler, so RCU better be paying attention
> * to us!)
> */
> - WARN_ON_ONCE(rdtp->dynticks_nmi_nesting <= 0);
> + WARN_ON_ONCE(rdtp->dynticks_irq_nesting <= 0);
> WARN_ON_ONCE(rcu_dynticks_curr_cpu_in_eqs());
>
> /*
> * If the nesting level is not 1, the CPU wasn't RCU-idle, so
> * leave it in non-RCU-idle state.
> */
> - if (rdtp->dynticks_nmi_nesting != 1) {
> - trace_rcu_dyntick(TPS("--="), rdtp->dynticks_nmi_nesting,
> rdtp->dynticks_nmi_nesting - 2, rdtp->dynticks);
> - WRITE_ONCE(rdtp->dynticks_nmi_nesting, /* No store tearing. */
> - rdtp->dynticks_nmi_nesting - 2);
> + if (rdtp->dynticks_irq_nesting != 1) {
> + trace_rcu_dyntick(TPS("--="), rdtp->dynticks_irq_nesting,
> rdtp->dynticks_irq_nesting - 2, rdtp->dynticks);
> + WRITE_ONCE(rdtp->dynticks_irq_nesting, /* No store tearing. */
> + rdtp->dynticks_irq_nesting - 2);
> return;
> }
>
> /* 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. */
> + trace_rcu_dyntick(TPS("Startirq"), rdtp->dynticks_irq_nesting, 0,
> rdtp->dynticks);
> + WRITE_ONCE(rdtp->dynticks_irq_nesting, 0); /* Avoid store tearing. */
> +
> + if (!nmi)
> + rcu_prepare_for_idle();
> +
> rcu_dynticks_eqs_enter();
> +
> + if (!nmi)
> + rcu_dynticks_task_enter();
> +}
> +
> +/**
> + * rcu_nmi_exit - inform RCU of exit from NMI context
> + */
> +void rcu_nmi_exit(void)
> +{
> + rcu_irq_exit_common(true);
> }
>
> /**
> @@ -824,14 +839,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_irq_exit_common(false);
> }
>
> /*
> @@ -853,7 +862,7 @@ void rcu_irq_exit_irqson(void)
> * Exit an RCU extended quiescent state, which can be either the
> * idle loop or adaptive-tickless usermode execution.
> *
> - * We crowbar the ->dynticks_nmi_nesting field to DYNTICK_IRQ_NONIDLE to
> + * We crowbar the ->dynticks_irq_nesting field to DYNTICK_IRQ_NONIDLE to
> * allow for the possibility of usermode upcalls messing up our count of
> * interrupt nesting level during the busy period that is just now starting.
> */
> @@ -876,7 +885,7 @@ static void rcu_eqs_exit(bool user)
> trace_rcu_dyntick(TPS("End"), rdtp->dynticks_nesting, 1,
> rdtp->dynticks);
> WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) && !user &&
> !is_idle_task(current));
> WRITE_ONCE(rdtp->dynticks_nesting, 1);
> - WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> + WRITE_ONCE(rdtp->dynticks_irq_nesting, DYNTICK_IRQ_NONIDLE);
> }
>
> /**
> @@ -914,46 +923,62 @@ void rcu_user_exit(void)
> #endif /* CONFIG_NO_HZ_FULL */
>
> /**
> - * rcu_nmi_enter - inform RCU of entry to NMI context
> + * rcu_irq_enter_common - inform RCU of entry to interrupt 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
> - * that the CPU is active. This implementation permits nested NMIs, as
> - * long as the nesting level does not overflow an int. (You will probably
> - * run out of stack space first.)
> + * rdtp->dynticks_irq_nesting to let the RCU grace-period handling know
> + * that the CPU is active. This implementation permits nested
> + * interrupts including NMIs, as 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
> - * with CONFIG_RCU_EQS_DEBUG=y.
> + * If you add or remove a call to rcu_irq_enter_common(), be sure to
> + * test with CONFIG_RCU_EQS_DEBUG=y.
> */
> -void rcu_nmi_enter(void)
> +static __always_inline void rcu_irq_enter_common(bool nmi)
> {
> struct rcu_dynticks *rdtp = this_cpu_ptr(&rcu_dynticks);
> long incby = 2;
>
> /* Complain about underflow. */
> - WARN_ON_ONCE(rdtp->dynticks_nmi_nesting < 0);
> + WARN_ON_ONCE(rdtp->dynticks_irq_nesting < 0);
>
> /*
> * If idle from RCU viewpoint, atomically increment ->dynticks
> - * to mark non-idle and increment ->dynticks_nmi_nesting by one.
> - * Otherwise, increment ->dynticks_nmi_nesting by two. This means
> - * if ->dynticks_nmi_nesting is equal to one, we are guaranteed
> + * to mark non-idle and increment ->dynticks_irq_nesting by one.
> + * Otherwise, increment ->dynticks_irq_nesting by two. This means
> + * if ->dynticks_irq_nesting is equal to one, we are guaranteed
> * to be in the outermost NMI handler that interrupted an RCU-idle
> * period (observation due to Andy Lutomirski).
> */
> if (rcu_dynticks_curr_cpu_in_eqs()) {
> +
> + if (!nmi)
> + rcu_dynticks_task_exit();
> +
> rcu_dynticks_eqs_exit();
> +
> + if (!nmi)
> + rcu_cleanup_after_idle();
> +
> incby = 1;
> }
> trace_rcu_dyntick(incby == 1 ? TPS("Endirq") : TPS("++="),
> - rdtp->dynticks_nmi_nesting,
> - rdtp->dynticks_nmi_nesting + incby, rdtp->dynticks);
> - WRITE_ONCE(rdtp->dynticks_nmi_nesting, /* Prevent store tearing. */
> - rdtp->dynticks_nmi_nesting + incby);
> + rdtp->dynticks_irq_nesting,
> + rdtp->dynticks_irq_nesting + incby, rdtp->dynticks);
> + WRITE_ONCE(rdtp->dynticks_irq_nesting, /* Prevent store tearing. */
> + rdtp->dynticks_irq_nesting + incby);
> barrier();
> }
>
> /**
> + * rcu_nmi_enter - inform RCU of entry to NMI context
> + */
> +void rcu_nmi_enter(void)
> +{
> + rcu_irq_enter_common(true);
> +}
> +
> +/**
> * rcu_irq_enter - inform RCU that current CPU is entering irq away from idle
> *
> * Enter an interrupt handler, which might possibly result in exiting
> @@ -977,14 +1002,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_irq_enter_common(false);
> }
>
> /*
> @@ -1092,7 +1111,7 @@ bool rcu_lockdep_current_cpu_online(void)
> static int rcu_is_cpu_rrupt_from_idle(void)
> {
> return __this_cpu_read(rcu_dynticks.dynticks_nesting) <= 0 &&
> - __this_cpu_read(rcu_dynticks.dynticks_nmi_nesting) <= 1;
> + __this_cpu_read(rcu_dynticks.dynticks_irq_nesting) <= 1;
> }
>
> /*
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 4e74df7..80ba455 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -39,7 +39,7 @@
> */
> struct rcu_dynticks {
> long dynticks_nesting; /* Track process nesting level. */
> - long dynticks_nmi_nesting; /* Track irq/NMI nesting level. */
> + long dynticks_irq_nesting; /* Track irq/NMI nesting level. */
> atomic_t dynticks; /* Even value for idle, else odd. */
> bool rcu_need_heavy_qs; /* GP old, need heavy quiescent state. */
> unsigned long rcu_qs_ctr; /* Light universal quiescent state ctr. */
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index c1b17f5..2cd637d 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1811,7 +1811,7 @@ static void print_cpu_stall_info(struct rcu_state *rsp,
> int cpu)
> "!."[!delta],
> ticks_value, ticks_title,
> rcu_dynticks_snap(rdtp) & 0xfff,
> - rdtp->dynticks_nesting, rdtp->dynticks_nmi_nesting,
> + rdtp->dynticks_nesting, rdtp->dynticks_irq_nesting,
> rdp->softirq_snap, kstat_softirqs_cpu(RCU_SOFTIRQ, cpu),
> READ_ONCE(rsp->n_force_qs) - rsp->n_force_qs_gpstart,
> fast_no_hz);
