Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> either an interrupt that invokes rcu_irq_enter() but never invokes the
> corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> on the other.  These things really did happen at one time, as evidenced
> by this ca-2011 LKML post:
> 
> http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> 
> The reason why RCU tolerates half-interrupts is that usermode helpers
> used exceptions to invoke a system call from within the kernel such that
> the system call did a normal return (not a return from exception) to
> the calling context.  This caused rcu_irq_enter() to be invoked without
> a matching rcu_irq_exit().  However, usermode helpers have since been
> rewritten to make much more housebroken use of workqueues, kernel threads,
> and do_execve(), and therefore should no longer produce half-interrupts.
> No one knows of any other source of half-interrupts, but then again,
> no one seems insane enough to go audit the entire kernel to verify that
> half-interrupts really are a relic of the past.
> 
> This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> trigger in the presence of half interrupts, which the code will continue
> to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> mid-2021, then perhaps RCU can stop handling half-interrupts, which
> would be a considerable simplification.

Hi Paul and everyone,
I was thinking some more about this patch and whether we can simplify this code
much in 2021. Since 2021 is a bit far away, I thought working on it in again to
keep it fresh in memory is a good idea ;-)

To me it seems we cannot easily combine the counters (dynticks_nesting and
dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
half-interrupt scenario (assuming simplication means counter combining like
Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
2 counters need to be tracked separately as they are used differently in the
following function:

static int rcu_is_cpu_rrupt_from_idle(void)
{
        return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
               __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
}

dynticks_nesting actually tracks if we entered/exited idle or user mode.

dynticks_nmi_nesting tracks if we entered/exited interrupts.

We have to do the "dynticks_nmi_nesting <= 1" check because
rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
(like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
check is because the CPU MUST be in user or idle for the check to return
true. We can't really combine these two into one counter then I think because
they both convey different messages.

The only simplication we can do, is probably the "crowbar" updates to
dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
no more half-interrupts are possible. Which might still be a worthwhile thing
to do (while still keeping both counters separate).

However, I think we could combine the counters and lead to simplying the code
in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
not need the counters but NOHZ_FULL may take issue with that since it needs
rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.

Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
will really get fired for half-interrupts. The vast majority of the systems I believe are
NOHZ_IDLE not NOHZ_FULL.
This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
rcu_user_enter()  [due to usermode upcall]
rcu_user_exit()
(no more rcu_irq_exit() - hence half an interrupt)

But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
rcu_eqs_e{xit,nter} really do anything?

Or was the idea with adding the new warnings, that they would fire the next
time rcu_idle_enter/exit is called? Like for example:

rcu_irq_enter()   [This is due to half-interrupt]
rcu_idle_enter()  [Eventually we enter the idle loop at some point
		   after the half-interrupt and the rcu_eqs_enter()
		   would "crowbar" the dynticks_nmi_nesting counter to 0].

thanks!

 - Joel

> 
> Reported-by: Steven Rostedt <rostedt@goodmis.org>
> Reported-by: Joel Fernandes <joel@joelfernandes.org>
> Reported-by: Andy Lutomirski <luto@kernel.org>
> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index dc041c2afbcc..d2b6ade692c9 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
>  	struct rcu_dynticks *rdtp;
>  
>  	rdtp = this_cpu_ptr(&rcu_dynticks);
> +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
>  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
>  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>  		     rdtp->dynticks_nesting == 0);
> @@ -895,6 +896,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);
> +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
>  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
>  }
>  
> -- 
> 2.17.1
> 

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Paul E. McKenney]

On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > on the other.  These things really did happen at one time, as evidenced
> > by this ca-2011 LKML post:
> > 
> > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > 
> > The reason why RCU tolerates half-interrupts is that usermode helpers
> > used exceptions to invoke a system call from within the kernel such that
> > the system call did a normal return (not a return from exception) to
> > the calling context.  This caused rcu_irq_enter() to be invoked without
> > a matching rcu_irq_exit().  However, usermode helpers have since been
> > rewritten to make much more housebroken use of workqueues, kernel threads,
> > and do_execve(), and therefore should no longer produce half-interrupts.
> > No one knows of any other source of half-interrupts, but then again,
> > no one seems insane enough to go audit the entire kernel to verify that
> > half-interrupts really are a relic of the past.
> > 
> > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > trigger in the presence of half interrupts, which the code will continue
> > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > would be a considerable simplification.
> 
> Hi Paul and everyone,
> I was thinking some more about this patch and whether we can simplify this code
> much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> keep it fresh in memory is a good idea ;-)

Indeed, easy to forget.  ;-)

> To me it seems we cannot easily combine the counters (dynticks_nesting and
> dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> half-interrupt scenario (assuming simplication means counter combining like
> Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> 2 counters need to be tracked separately as they are used differently in the
> following function:
> 
> static int rcu_is_cpu_rrupt_from_idle(void)
> {
>         return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
>                __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> }
> 
> dynticks_nesting actually tracks if we entered/exited idle or user mode.

True, though it tracks user mode only in CONFIG_NO_HZ_FULL kernels.

> dynticks_nmi_nesting tracks if we entered/exited interrupts.

Including NMIs, yes.

> We have to do the "dynticks_nmi_nesting <= 1" check because
> rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
> (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
> check is because the CPU MUST be in user or idle for the check to return
> true. We can't really combine these two into one counter then I think because
> they both convey different messages.
> 
> The only simplication we can do, is probably the "crowbar" updates to
> dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
> no more half-interrupts are possible. Which might still be a worthwhile thing
> to do (while still keeping both counters separate).
> 
> However, I think we could combine the counters and lead to simplying the code
> in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
> not need the counters but NOHZ_FULL may take issue with that since it needs
> rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.

I haven't gone through it in detail, but it seems like we should be able
to treat in-kernel process-level execution like an interrupt from idle
or userspace, as the case might be.  If we did that, shouldn't we be
able to just do this?

static int rcu_is_cpu_rrupt_from_idle(void)
{
        return __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
}

> Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
> In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
> will really get fired for half-interrupts. The vast majority of the systems I believe are
> NOHZ_IDLE not NOHZ_FULL.
> This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
> rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
> rcu_user_enter()  [due to usermode upcall]
> rcu_user_exit()
> (no more rcu_irq_exit() - hence half an interrupt)
> 
> But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
> rcu_eqs_e{xit,nter} really do anything?

Yes, because these are also called from rcu_idle_enter() and
rcu_idle_exit(), which is invoked even in !NO_HZ_FULL kernels.

> Or was the idea with adding the new warnings, that they would fire the next
> time rcu_idle_enter/exit is called? Like for example:
> 
> rcu_irq_enter()   [This is due to half-interrupt]
> rcu_idle_enter()  [Eventually we enter the idle loop at some point
> 		   after the half-interrupt and the rcu_eqs_enter()
> 		   would "crowbar" the dynticks_nmi_nesting counter to 0].

You got it!  ;-)

So yes, these warnings just detect the presence of misnesting.  Presumably
event tracing would then be used to track down the culprits.  Assuming that
the misnesting is reproducible and all that.

							Thanx, Paul

> thanks!
> 
>  - Joel
> 
> > 
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Reported-by: Joel Fernandes <joel@joelfernandes.org>
> > Reported-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index dc041c2afbcc..d2b6ade692c9 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
> >  	struct rcu_dynticks *rdtp;
> >  
> >  	rdtp = this_cpu_ptr(&rcu_dynticks);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
> >  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> >  		     rdtp->dynticks_nesting == 0);
> > @@ -895,6 +896,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);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> >  }
> >  
> > -- 
> > 2.17.1
> > 
> 

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Mon, Mar 11, 2019 at 03:29:03PM -0700, Paul E. McKenney wrote:
> On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> > On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > > on the other.  These things really did happen at one time, as evidenced
> > > by this ca-2011 LKML post:
> > > 
> > > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > > 
> > > The reason why RCU tolerates half-interrupts is that usermode helpers
> > > used exceptions to invoke a system call from within the kernel such that
> > > the system call did a normal return (not a return from exception) to
> > > the calling context.  This caused rcu_irq_enter() to be invoked without
> > > a matching rcu_irq_exit().  However, usermode helpers have since been
> > > rewritten to make much more housebroken use of workqueues, kernel threads,
> > > and do_execve(), and therefore should no longer produce half-interrupts.
> > > No one knows of any other source of half-interrupts, but then again,
> > > no one seems insane enough to go audit the entire kernel to verify that
> > > half-interrupts really are a relic of the past.
> > > 
> > > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > > trigger in the presence of half interrupts, which the code will continue
> > > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > > would be a considerable simplification.
> > 
> > Hi Paul and everyone,
> > I was thinking some more about this patch and whether we can simplify this code
> > much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> > keep it fresh in memory is a good idea ;-)
> 
> Indeed, easy to forget.  ;-)
> 
> > To me it seems we cannot easily combine the counters (dynticks_nesting and
> > dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> > half-interrupt scenario (assuming simplication means counter combining like
> > Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> > 2 counters need to be tracked separately as they are used differently in the
> > following function:
> > 
> > static int rcu_is_cpu_rrupt_from_idle(void)
> > {
> >         return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
> >                __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> > }
> > 
> > dynticks_nesting actually tracks if we entered/exited idle or user mode.
> 
> True, though it tracks user mode only in CONFIG_NO_HZ_FULL kernels.
> 
> > dynticks_nmi_nesting tracks if we entered/exited interrupts.
> 
> Including NMIs, yes.
> 
> > We have to do the "dynticks_nmi_nesting <= 1" check because
> > rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
> > (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
> > check is because the CPU MUST be in user or idle for the check to return
> > true. We can't really combine these two into one counter then I think because
> > they both convey different messages.
> > 
> > The only simplication we can do, is probably the "crowbar" updates to
> > dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
> > no more half-interrupts are possible. Which might still be a worthwhile thing
> > to do (while still keeping both counters separate).
> > 
> > However, I think we could combine the counters and lead to simplying the code
> > in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
> > not need the counters but NOHZ_FULL may take issue with that since it needs
> > rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.
> 
> I haven't gone through it in detail, but it seems like we should be able
> to treat in-kernel process-level execution like an interrupt from idle
> or userspace, as the case might be.  If we did that, shouldn't we be
> able to just do this?
> 
> static int rcu_is_cpu_rrupt_from_idle(void)
> {
>         return __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> }

I think that would work only if this function is always called from an
interrupt:

The comments on this function says:
 * If the current CPU is idle or running at a first-level (not nested)
 * interrupt from idle, return true.  The caller must have at least
 * disabled preemption.
 */

According to this comment rcu_is_cpu_rrupt_from_idle can be called from
somewhere other than an interrupt although from code-reading that does not
seem to be.

If it is ever possible to call rcu_is_cpu_rrupt_from_idle from anywhere but
an interrupt, then we would have a scenario like:
rcu_eqs_exit()   (Called say from rcu_idle_exit)
rcu_is_cpu_rrupt_from_idle  (now nesting counter is 1, so the <=1 check
                             returns true).

This would result in the function falsely claiming the CPU was idle from an
RCU standpoint I think.

However both from testing and from code reading, I don't see such a scenario
happening. Could we be more explicit in the code that this function can only
be called from an interrupt, and also we change the code comment to be more
clear about it (like the following diff)?

I made the following change and it didn't seem to blow up and also makes the
code more clear. The only change I would make if we were considering merging
this is to change the BUG_ON to WARN_ON_ONCE but I want to check first if you
think this change is useful to do. I think being explicit about the intention
is a good thing. I also specifically check for first-level of interrupt
nesting (==1).

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index a45c45a4a09b..531c63c40001 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -377,14 +377,19 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
 /**
  * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
  *
- * If the current CPU is idle or running at a first-level (not nested)
+ * If the current CPU is idle and running at a first-level (not nested)
  * interrupt from idle, return true.  The caller must have at least
  * disabled preemption.
  */
 static int rcu_is_cpu_rrupt_from_idle(void)
 {
-	return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
-	       __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
+	int dynticks_nesting = __this_cpu_read(rcu_data.dynticks_nesting);
+	int dynticks_nmi_nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
+
+	/* This function should only be called from an interrupt */
+	BUG_ON(dynticks_nmi_nesting < 1);
+
+	return dynticks_nmi_nesting == 1 && dynticks_nesting <= 0;
 }
 
 #define DEFAULT_RCU_BLIMIT 10     /* Maximum callbacks per rcu_do_batch. */


> > Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
> > In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
> > will really get fired for half-interrupts. The vast majority of the systems I believe are
> > NOHZ_IDLE not NOHZ_FULL.
> > This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
> > rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
> > rcu_user_enter()  [due to usermode upcall]
> > rcu_user_exit()
> > (no more rcu_irq_exit() - hence half an interrupt)
> > 
> > But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
> > rcu_eqs_e{xit,nter} really do anything?
> 
> Yes, because these are also called from rcu_idle_enter() and
> rcu_idle_exit(), which is invoked even in !NO_HZ_FULL kernels.

Got it.

> > Or was the idea with adding the new warnings, that they would fire the next
> > time rcu_idle_enter/exit is called? Like for example:
> > 
> > rcu_irq_enter()   [This is due to half-interrupt]
> > rcu_idle_enter()  [Eventually we enter the idle loop at some point
> > 		   after the half-interrupt and the rcu_eqs_enter()
> > 		   would "crowbar" the dynticks_nmi_nesting counter to 0].
> 
> You got it!  ;-)

Thanks a lot for confirming,

- Joel

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Paul E. McKenney]

On Tue, Mar 12, 2019 at 11:05:14AM -0400, Joel Fernandes wrote:
> On Mon, Mar 11, 2019 at 03:29:03PM -0700, Paul E. McKenney wrote:
> > On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > > > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > > > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > > > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > > > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > > > on the other.  These things really did happen at one time, as evidenced
> > > > by this ca-2011 LKML post:
> > > > 
> > > > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > > > 
> > > > The reason why RCU tolerates half-interrupts is that usermode helpers
> > > > used exceptions to invoke a system call from within the kernel such that
> > > > the system call did a normal return (not a return from exception) to
> > > > the calling context.  This caused rcu_irq_enter() to be invoked without
> > > > a matching rcu_irq_exit().  However, usermode helpers have since been
> > > > rewritten to make much more housebroken use of workqueues, kernel threads,
> > > > and do_execve(), and therefore should no longer produce half-interrupts.
> > > > No one knows of any other source of half-interrupts, but then again,
> > > > no one seems insane enough to go audit the entire kernel to verify that
> > > > half-interrupts really are a relic of the past.
> > > > 
> > > > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > > > trigger in the presence of half interrupts, which the code will continue
> > > > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > > > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > > > would be a considerable simplification.
> > > 
> > > Hi Paul and everyone,
> > > I was thinking some more about this patch and whether we can simplify this code
> > > much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> > > keep it fresh in memory is a good idea ;-)
> > 
> > Indeed, easy to forget.  ;-)
> > 
> > > To me it seems we cannot easily combine the counters (dynticks_nesting and
> > > dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> > > half-interrupt scenario (assuming simplication means counter combining like
> > > Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> > > 2 counters need to be tracked separately as they are used differently in the
> > > following function:
> > > 
> > > static int rcu_is_cpu_rrupt_from_idle(void)
> > > {
> > >         return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
> > >                __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> > > }
> > > 
> > > dynticks_nesting actually tracks if we entered/exited idle or user mode.
> > 
> > True, though it tracks user mode only in CONFIG_NO_HZ_FULL kernels.
> > 
> > > dynticks_nmi_nesting tracks if we entered/exited interrupts.
> > 
> > Including NMIs, yes.
> > 
> > > We have to do the "dynticks_nmi_nesting <= 1" check because
> > > rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
> > > (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
> > > check is because the CPU MUST be in user or idle for the check to return
> > > true. We can't really combine these two into one counter then I think because
> > > they both convey different messages.
> > > 
> > > The only simplication we can do, is probably the "crowbar" updates to
> > > dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
> > > no more half-interrupts are possible. Which might still be a worthwhile thing
> > > to do (while still keeping both counters separate).
> > > 
> > > However, I think we could combine the counters and lead to simplying the code
> > > in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
> > > not need the counters but NOHZ_FULL may take issue with that since it needs
> > > rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.
> > 
> > I haven't gone through it in detail, but it seems like we should be able
> > to treat in-kernel process-level execution like an interrupt from idle
> > or userspace, as the case might be.  If we did that, shouldn't we be
> > able to just do this?
> > 
> > static int rcu_is_cpu_rrupt_from_idle(void)
> > {
> >         return __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> > }
> 
> I think that would work only if this function is always called from an
> interrupt:
> 
> The comments on this function says:
>  * If the current CPU is idle or running at a first-level (not nested)
>  * interrupt from idle, return true.  The caller must have at least
>  * disabled preemption.
>  */
> 
> According to this comment rcu_is_cpu_rrupt_from_idle can be called from
> somewhere other than an interrupt although from code-reading that does not
> seem to be.
> 
> If it is ever possible to call rcu_is_cpu_rrupt_from_idle from anywhere but
> an interrupt, then we would have a scenario like:
> rcu_eqs_exit()   (Called say from rcu_idle_exit)
> rcu_is_cpu_rrupt_from_idle  (now nesting counter is 1, so the <=1 check
>                              returns true).
> 
> This would result in the function falsely claiming the CPU was idle from an
> RCU standpoint I think.
> 
> However both from testing and from code reading, I don't see such a scenario
> happening.

Agreed!

>            Could we be more explicit in the code that this function can only
> be called from an interrupt, and also we change the code comment to be more
> clear about it (like the following diff)?

That would be good!

Nice trick on using dyntick state to check for interrupt nesting, but
wouldn't consolidating the counters break that?  But is there a lockdep
check for being in a hardware interrupt handler?  If not, could one
be added?  This would have the benefit of not adding overhead to the
scheduling-clock interrupt in production builds of the Linux kernel,
while still finding this bug in testing.

(Another approach would be to use IS_ENABLED(CONFIG_PROVE_RCU), but
a lockdep check would be cleaner.)

> I made the following change and it didn't seem to blow up and also makes the
> code more clear. The only change I would make if we were considering merging
> this is to change the BUG_ON to WARN_ON_ONCE but I want to check first if you
> think this change is useful to do. I think being explicit about the intention
> is a good thing. I also specifically check for first-level of interrupt
> nesting (==1).
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index a45c45a4a09b..531c63c40001 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -377,14 +377,19 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
>  /**
>   * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
>   *
> - * If the current CPU is idle or running at a first-level (not nested)
> + * If the current CPU is idle and running at a first-level (not nested)
>   * interrupt from idle, return true.  The caller must have at least
>   * disabled preemption.
>   */
>  static int rcu_is_cpu_rrupt_from_idle(void)
>  {
> -	return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
> -	       __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> +	int dynticks_nesting = __this_cpu_read(rcu_data.dynticks_nesting);
> +	int dynticks_nmi_nesting = __this_cpu_read(rcu_data.dynticks_nmi_nesting);
> +
> +	/* This function should only be called from an interrupt */
> +	BUG_ON(dynticks_nmi_nesting < 1);
> +
> +	return dynticks_nmi_nesting == 1 && dynticks_nesting <= 0;
>  }
>  
>  #define DEFAULT_RCU_BLIMIT 10     /* Maximum callbacks per rcu_do_batch. */
> 
> 
> > > Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
> > > In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
> > > will really get fired for half-interrupts. The vast majority of the systems I believe are
> > > NOHZ_IDLE not NOHZ_FULL.
> > > This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
> > > rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
> > > rcu_user_enter()  [due to usermode upcall]
> > > rcu_user_exit()
> > > (no more rcu_irq_exit() - hence half an interrupt)
> > > 
> > > But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
> > > rcu_eqs_e{xit,nter} really do anything?
> > 
> > Yes, because these are also called from rcu_idle_enter() and
> > rcu_idle_exit(), which is invoked even in !NO_HZ_FULL kernels.
> 
> Got it.
> 
> > > Or was the idea with adding the new warnings, that they would fire the next
> > > time rcu_idle_enter/exit is called? Like for example:
> > > 
> > > rcu_irq_enter()   [This is due to half-interrupt]
> > > rcu_idle_enter()  [Eventually we enter the idle loop at some point
> > > 		   after the half-interrupt and the rcu_eqs_enter()
> > > 		   would "crowbar" the dynticks_nmi_nesting counter to 0].
> > 
> > You got it!  ;-)
> 
> Thanks a lot for confirming,

Thank you for looking into this!

								Thanx, Paul

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Tue, Mar 12, 2019 at 08:20:34AM -0700, Paul E. McKenney wrote:
[snip]
> 
> >            Could we be more explicit in the code that this function can only
> > be called from an interrupt, and also we change the code comment to be more
> > clear about it (like the following diff)?
> 
> That would be good!
> 
> Nice trick on using dyntick state to check for interrupt nesting, but
> wouldn't consolidating the counters break that?  But is there a lockdep
> check for being in a hardware interrupt handler?  If not, could one
> be added?  This would have the benefit of not adding overhead to the
> scheduling-clock interrupt in production builds of the Linux kernel,
> while still finding this bug in testing.
> 
> (Another approach would be to use IS_ENABLED(CONFIG_PROVE_RCU), but
> a lockdep check would be cleaner.)

AFAICS, lockdep does not specifically track when we enter an interrupt, but
rather only tracks when interrupts are enabled/disabled.

But we could use in_irq() to find if we are in an interrupt (which uses the
preempt_count to track in HARDIRQ_MASK section of the counter).

I will add an in_irq() check that does the check only when PROVE_RCU is
enabled, and send a patch.

Thanks and it is my pleasure to look into this, quite interesting!

 - Joel

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Steven Rostedt]

On Wed, 13 Mar 2019 11:09:48 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> AFAICS, lockdep does not specifically track when we enter an interrupt, but
> rather only tracks when interrupts are enabled/disabled.

It does:

#define __irq_enter()					\
	do {						\
		account_irq_enter_time(current);	\
		preempt_count_add(HARDIRQ_OFFSET);	\
		trace_hardirq_enter();			\
	} while (0)

# define trace_hardirq_enter()			\
do {						\
	current->hardirq_context++;		\
} while (0)


And if the hardirq_context ever does not match "in_irq()" lockdep will
complain loudly.

-- Steve

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Paul E. McKenney]

On Wed, Mar 13, 2019 at 11:27:26AM -0400, Steven Rostedt wrote:
> On Wed, 13 Mar 2019 11:09:48 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > AFAICS, lockdep does not specifically track when we enter an interrupt, but
> > rather only tracks when interrupts are enabled/disabled.
> 
> It does:
> 
> #define __irq_enter()					\
> 	do {						\
> 		account_irq_enter_time(current);	\
> 		preempt_count_add(HARDIRQ_OFFSET);	\
> 		trace_hardirq_enter();			\
> 	} while (0)
> 
> # define trace_hardirq_enter()			\
> do {						\
> 	current->hardirq_context++;		\
> } while (0)
> 
> 
> And if the hardirq_context ever does not match "in_irq()" lockdep will
> complain loudly.

Good to know, thank you!

Does this mean that there is a better approach that Joel's suggestion?
I believe he would end up with something like this:

	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && !in_irq());

It would be nice if there is something like this:

	lockdep_assert_in_irq_handler();

But I haven't seen this.  (Not that I have looked particularly hard for
such a thing, mind you!)

							Thanx, Paul

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Steven Rostedt]

On Wed, 13 Mar 2019 08:51:55 -0700
"Paul E. McKenney" <paulmck@linux.ibm.com> wrote:

> Does this mean that there is a better approach that Joel's suggestion?
> I believe he would end up with something like this:
> 
> 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && !in_irq());
> 
> It would be nice if there is something like this:
> 
> 	lockdep_assert_in_irq_handler();
> 
> But I haven't seen this.  (Not that I have looked particularly hard for
> such a thing, mind you!)

That would be trivial to implement:

#define lockdep_assert_in_irq() do {
		WARN_ON(debug_locks && !current->hardirq_context);
	} while (0)

-- Steve

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Paul E. McKenney]

On Wed, Mar 13, 2019 at 12:51:25PM -0400, Steven Rostedt wrote:
> On Wed, 13 Mar 2019 08:51:55 -0700
> "Paul E. McKenney" <paulmck@linux.ibm.com> wrote:
> 
> > Does this mean that there is a better approach that Joel's suggestion?
> > I believe he would end up with something like this:
> > 
> > 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && !in_irq());
> > 
> > It would be nice if there is something like this:
> > 
> > 	lockdep_assert_in_irq_handler();
> > 
> > But I haven't seen this.  (Not that I have looked particularly hard for
> > such a thing, mind you!)
> 
> That would be trivial to implement:
> 
> #define lockdep_assert_in_irq() do {
> 		WARN_ON(debug_locks && !current->hardirq_context);
> 	} while (0)

Looks good to me!

Joel, does this work for you?  I could be wrong, but I suspect that Steve
is suggesting that you incorporate the above into your eventual patch.  ;-)

							Thanx, Paul

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Wed, Mar 13, 2019 at 11:07:30AM -0700, Paul E. McKenney wrote:
> On Wed, Mar 13, 2019 at 12:51:25PM -0400, Steven Rostedt wrote:
> > On Wed, 13 Mar 2019 08:51:55 -0700
> > "Paul E. McKenney" <paulmck@linux.ibm.com> wrote:
> > 
> > > Does this mean that there is a better approach that Joel's suggestion?
> > > I believe he would end up with something like this:
> > > 
> > > 	WARN_ON_ONCE(IS_ENABLED(CONFIG_PROVE_RCU) && !in_irq());
> > > 
> > > It would be nice if there is something like this:
> > > 
> > > 	lockdep_assert_in_irq_handler();
> > > 
> > > But I haven't seen this.  (Not that I have looked particularly hard for
> > > such a thing, mind you!)
> > 
> > That would be trivial to implement:
> > 
> > #define lockdep_assert_in_irq() do {
> > 		WARN_ON(debug_locks && !current->hardirq_context);
> > 	} while (0)
> 
> Looks good to me!
> 
> Joel, does this work for you?  I could be wrong, but I suspect that Steve
> is suggesting that you incorporate the above into your eventual patch.  ;-)

Oh thanks for pointing that out. Yes it does work for me. I agree with the
lockdep API addition and others could benefit from it too. I will incorporate
the lockdep API addition into the RCU patch, but let me know if I should
rather split it.

thanks!

 - Joel

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Steven Rostedt]

On Thu, 14 Mar 2019 08:31:59 -0400
Joel Fernandes <joel@joelfernandes.org> wrote:

> Oh thanks for pointing that out. Yes it does work for me. I agree with the
> lockdep API addition and others could benefit from it too. I will incorporate
> the lockdep API addition into the RCU patch, but let me know if I should
> rather split it.

I'd recommend splitting it (adding the lockdep_assert in a patch by
itself), but make sure that patch Cc's the lockdep maintainers and
explains the reason for adding it. Might as well Cc the lockdep
maintainers on the entire series too.

-- Steve

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Steven Rostedt]

On Thu, 14 Mar 2019 09:36:57 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> On Thu, 14 Mar 2019 08:31:59 -0400
> Joel Fernandes <joel@joelfernandes.org> wrote:
> 
> > Oh thanks for pointing that out. Yes it does work for me. I agree with the
> > lockdep API addition and others could benefit from it too. I will incorporate
> > the lockdep API addition into the RCU patch, but let me know if I should
> > rather split it.  
> 
> I'd recommend splitting it (adding the lockdep_assert in a patch by
> itself), but make sure that patch Cc's the lockdep maintainers and
> explains the reason for adding it. Might as well Cc the lockdep
> maintainers on the entire series too.
>

Feel free to add "Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>"
on that patch too.

-- Steve

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Thu, Mar 14, 2019 at 09:37:46AM -0400, Steven Rostedt wrote:
> On Thu, 14 Mar 2019 09:36:57 -0400
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > On Thu, 14 Mar 2019 08:31:59 -0400
> > Joel Fernandes <joel@joelfernandes.org> wrote:
> > 
> > > Oh thanks for pointing that out. Yes it does work for me. I agree with the
> > > lockdep API addition and others could benefit from it too. I will incorporate
> > > the lockdep API addition into the RCU patch, but let me know if I should
> > > rather split it.  
> > 
> > I'd recommend splitting it (adding the lockdep_assert in a patch by
> > itself), but make sure that patch Cc's the lockdep maintainers and
> > explains the reason for adding it. Might as well Cc the lockdep
> > maintainers on the entire series too.
> >
> 
> Feel free to add "Suggested-by: Steven Rostedt (VMware) <rostedt@goodmis.org>"
> on that patch too.

Will definitely split it and add your tag. Thanks!

 - Joel

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Byungchul Park]

On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > on the other.  These things really did happen at one time, as evidenced
> > by this ca-2011 LKML post:
> > 
> > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > 
> > The reason why RCU tolerates half-interrupts is that usermode helpers
> > used exceptions to invoke a system call from within the kernel such that
> > the system call did a normal return (not a return from exception) to
> > the calling context.  This caused rcu_irq_enter() to be invoked without
> > a matching rcu_irq_exit().  However, usermode helpers have since been
> > rewritten to make much more housebroken use of workqueues, kernel threads,
> > and do_execve(), and therefore should no longer produce half-interrupts.
> > No one knows of any other source of half-interrupts, but then again,
> > no one seems insane enough to go audit the entire kernel to verify that
> > half-interrupts really are a relic of the past.
> > 
> > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > trigger in the presence of half interrupts, which the code will continue
> > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > would be a considerable simplification.
> 
> Hi Paul and everyone,
> I was thinking some more about this patch and whether we can simplify this code
> much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> keep it fresh in memory is a good idea ;-)
> 
> To me it seems we cannot easily combine the counters (dynticks_nesting and
> dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> half-interrupt scenario (assuming simplication means counter combining like
> Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> 2 counters need to be tracked separately as they are used differently in the
> following function:

Hi Joel and Paul,

I always love the way to logically approach problems so I'm a fan of
all your works :) But I'm JUST curious about something here. Why can't
we combine them the way I tried even if we confirm no possibility of
half-interrupt? IMHO, the only thing we want to know through calling
rcu_is_cpu_rrupt_from_idle() is whether the interrupt comes from
RCU-idle or not - of course assuming the caller context always be an
well-defined interrupt context like e.g. the tick handler.

So the function can return true if the caller is within a RCU-idle
region except a well-known single interrupt nested.

Of course, now that we cannot confirm it yet, the crowbar is necessary.
But does it still have a problem even after confirming it? Why? What am
I missing? Could you explain why for me? :(

Thanks,
Byungchul

> static int rcu_is_cpu_rrupt_from_idle(void)
> {
>         return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
>                __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
> }
> 
> dynticks_nesting actually tracks if we entered/exited idle or user mode.
> 
> dynticks_nmi_nesting tracks if we entered/exited interrupts.
> 
> We have to do the "dynticks_nmi_nesting <= 1" check because
> rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
> (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
> check is because the CPU MUST be in user or idle for the check to return
> true. We can't really combine these two into one counter then I think because
> they both convey different messages.
> 
> The only simplication we can do, is probably the "crowbar" updates to
> dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
> no more half-interrupts are possible. Which might still be a worthwhile thing
> to do (while still keeping both counters separate).
> 
> However, I think we could combine the counters and lead to simplying the code
> in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
> not need the counters but NOHZ_FULL may take issue with that since it needs
> rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.
> 
> Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
> In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
> will really get fired for half-interrupts. The vast majority of the systems I believe are
> NOHZ_IDLE not NOHZ_FULL.
> This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
> rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
> rcu_user_enter()  [due to usermode upcall]
> rcu_user_exit()
> (no more rcu_irq_exit() - hence half an interrupt)
> 
> But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
> rcu_eqs_e{xit,nter} really do anything?
> 
> Or was the idea with adding the new warnings, that they would fire the next
> time rcu_idle_enter/exit is called? Like for example:
> 
> rcu_irq_enter()   [This is due to half-interrupt]
> rcu_idle_enter()  [Eventually we enter the idle loop at some point
> 		   after the half-interrupt and the rcu_eqs_enter()
> 		   would "crowbar" the dynticks_nmi_nesting counter to 0].
> 
> thanks!
> 
>  - Joel
> 
> > 
> > Reported-by: Steven Rostedt <rostedt@goodmis.org>
> > Reported-by: Joel Fernandes <joel@joelfernandes.org>
> > Reported-by: Andy Lutomirski <luto@kernel.org>
> > Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
> > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index dc041c2afbcc..d2b6ade692c9 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
> >  	struct rcu_dynticks *rdtp;
> >  
> >  	rdtp = this_cpu_ptr(&rcu_dynticks);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
> >  	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
> >  		     rdtp->dynticks_nesting == 0);
> > @@ -895,6 +896,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);
> > +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
> >  	WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
> >  }
> >  
> > -- 
> > 2.17.1
> > 

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Byungchul Park]

On 03/15/2019 04:31 PM, Byungchul Park wrote:
> On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
>> On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
>>> RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
>>> either an interrupt that invokes rcu_irq_enter() but never invokes the
>>> corresponding rcu_irq_exit() on the one hand, or an interrupt that never
>>> invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
>>> on the other.  These things really did happen at one time, as evidenced
>>> by this ca-2011 LKML post:
>>>
>>> http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
>>>
>>> The reason why RCU tolerates half-interrupts is that usermode helpers
>>> used exceptions to invoke a system call from within the kernel such that
>>> the system call did a normal return (not a return from exception) to
>>> the calling context.  This caused rcu_irq_enter() to be invoked without
>>> a matching rcu_irq_exit().  However, usermode helpers have since been
>>> rewritten to make much more housebroken use of workqueues, kernel threads,
>>> and do_execve(), and therefore should no longer produce half-interrupts.
>>> No one knows of any other source of half-interrupts, but then again,
>>> no one seems insane enough to go audit the entire kernel to verify that
>>> half-interrupts really are a relic of the past.
>>>
>>> This commit therefore adds a pair of WARN_ON_ONCE() calls that will
>>> trigger in the presence of half interrupts, which the code will continue
>>> to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
>>> mid-2021, then perhaps RCU can stop handling half-interrupts, which
>>> would be a considerable simplification.
>> Hi Paul and everyone,
>> I was thinking some more about this patch and whether we can simplify this code
>> much in 2021. Since 2021 is a bit far away, I thought working on it in again to
>> keep it fresh in memory is a good idea ;-)
>>
>> To me it seems we cannot easily combine the counters (dynticks_nesting and
>> dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
>> half-interrupt scenario (assuming simplication means counter combining like
>> Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
>> 2 counters need to be tracked separately as they are used differently in the
>> following function:
> Hi Joel and Paul,
>
> I always love the way to logically approach problems so I'm a fan of
> all your works :) But I'm JUST curious about something here. Why can't
> we combine them the way I tried even if we confirm no possibility of
> half-interrupt? IMHO, the only thing we want to know through calling
> rcu_is_cpu_rrupt_from_idle() is whether the interrupt comes from
> RCU-idle or not - of course assuming the caller context always be an
> well-defined interrupt context like e.g. the tick handler.
>
> So the function can return true if the caller is within a RCU-idle
> region except a well-known single interrupt nested.
>
> Of course, now that we cannot confirm it yet, the crowbar is necessary.
> But does it still have a problem even after confirming it? Why? What am
> I missing? Could you explain why for me? :(


Did you also want to consider the case the function is called from 
others than
well-known interrupt contexts? If yes, then I agree with you, there doesn't
seem to be the kind of code and it's not a good idea to let the function be
called generally though.


> Thanks,
> Byungchul
>
>> static int rcu_is_cpu_rrupt_from_idle(void)
>> {
>>          return __this_cpu_read(rcu_data.dynticks_nesting) <= 0 &&
>>                 __this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 1;
>> }
>>
>> dynticks_nesting actually tracks if we entered/exited idle or user mode.
>>
>> dynticks_nmi_nesting tracks if we entered/exited interrupts.
>>
>> We have to do the "dynticks_nmi_nesting <= 1" check because
>> rcu_is_cpu_rrupt_from_idle() can possibly be called from an interrupt itself
>> (like timer) so we discount 1 interrupt, and, the "dynticks_nesting <= 0"
>> check is because the CPU MUST be in user or idle for the check to return
>> true. We can't really combine these two into one counter then I think because
>> they both convey different messages.
>>
>> The only simplication we can do, is probably the "crowbar" updates to
>> dynticks_nmi_nesting can be removed from rcu_eqs_enter/exit once we confirm
>> no more half-interrupts are possible. Which might still be a worthwhile thing
>> to do (while still keeping both counters separate).
>>
>> However, I think we could combine the counters and lead to simplying the code
>> in case we implement rcu_is_cpu_rrupt_from_idle differently such that it does
>> not need the counters but NOHZ_FULL may take issue with that since it needs
>> rcu_user_enter->rcu_eqs_enter to convey that the CPU is "RCU"-idle.
>>
>> Actually, I had another question... rcu_user_enter() is a NOOP in !NOHZ_FULL config.
>> In this case I was wondering if the the warning Paul added (in the patch I'm replying to)
>> will really get fired for half-interrupts. The vast majority of the systems I believe are
>> NOHZ_IDLE not NOHZ_FULL.
>> This is what a half-interrupt really looks like right? Please correct me if I'm wrong:
>> rcu_irq_enter()   [half interrupt causes an exception and thus rcu_irq_enter]
>> rcu_user_enter()  [due to usermode upcall]
>> rcu_user_exit()
>> (no more rcu_irq_exit() - hence half an interrupt)
>>
>> But the rcu_user_enter()/exit is a NOOP in some configs, so will the warning in
>> rcu_eqs_e{xit,nter} really do anything?
>>
>> Or was the idea with adding the new warnings, that they would fire the next
>> time rcu_idle_enter/exit is called? Like for example:
>>
>> rcu_irq_enter()   [This is due to half-interrupt]
>> rcu_idle_enter()  [Eventually we enter the idle loop at some point
>> 		   after the half-interrupt and the rcu_eqs_enter()
>> 		   would "crowbar" the dynticks_nmi_nesting counter to 0].
>>
>> thanks!
>>
>>   - Joel
>>
>>> Reported-by: Steven Rostedt <rostedt@goodmis.org>
>>> Reported-by: Joel Fernandes <joel@joelfernandes.org>
>>> Reported-by: Andy Lutomirski <luto@kernel.org>
>>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com>
>>> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>> ---
>>>   kernel/rcu/tree.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
>>> index dc041c2afbcc..d2b6ade692c9 100644
>>> --- a/kernel/rcu/tree.c
>>> +++ b/kernel/rcu/tree.c
>>> @@ -714,6 +714,7 @@ static void rcu_eqs_enter(bool user)
>>>   	struct rcu_dynticks *rdtp;
>>>   
>>>   	rdtp = this_cpu_ptr(&rcu_dynticks);
>>> +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting != DYNTICK_IRQ_NONIDLE);
>>>   	WRITE_ONCE(rdtp->dynticks_nmi_nesting, 0);
>>>   	WARN_ON_ONCE(IS_ENABLED(CONFIG_RCU_EQS_DEBUG) &&
>>>   		     rdtp->dynticks_nesting == 0);
>>> @@ -895,6 +896,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);
>>> +	WARN_ON_ONCE(rdtp->dynticks_nmi_nesting);
>>>   	WRITE_ONCE(rdtp->dynticks_nmi_nesting, DYNTICK_IRQ_NONIDLE);
>>>   }
>>>   
>>> -- 
>>> 2.17.1
>>>

Re: [PATCH tip/core/rcu 06/19] rcu: Add warning to detect half-interrupts

[Joel Fernandes]

On Fri, Mar 15, 2019 at 04:44:52PM +0900, Byungchul Park wrote:
> On 03/15/2019 04:31 PM, Byungchul Park wrote:
> > On Mon, Mar 11, 2019 at 09:39:39AM -0400, Joel Fernandes wrote:
> > > On Wed, Aug 29, 2018 at 03:20:34PM -0700, Paul E. McKenney wrote:
> > > > RCU's dyntick-idle code is written to tolerate half-interrupts, that it,
> > > > either an interrupt that invokes rcu_irq_enter() but never invokes the
> > > > corresponding rcu_irq_exit() on the one hand, or an interrupt that never
> > > > invokes rcu_irq_enter() but does invoke the "corresponding" rcu_irq_exit()
> > > > on the other.  These things really did happen at one time, as evidenced
> > > > by this ca-2011 LKML post:
> > > > 
> > > > http://lkml.kernel.org/r/20111014170019.GE2428@linux.vnet.ibm.com
> > > > 
> > > > The reason why RCU tolerates half-interrupts is that usermode helpers
> > > > used exceptions to invoke a system call from within the kernel such that
> > > > the system call did a normal return (not a return from exception) to
> > > > the calling context.  This caused rcu_irq_enter() to be invoked without
> > > > a matching rcu_irq_exit().  However, usermode helpers have since been
> > > > rewritten to make much more housebroken use of workqueues, kernel threads,
> > > > and do_execve(), and therefore should no longer produce half-interrupts.
> > > > No one knows of any other source of half-interrupts, but then again,
> > > > no one seems insane enough to go audit the entire kernel to verify that
> > > > half-interrupts really are a relic of the past.
> > > > 
> > > > This commit therefore adds a pair of WARN_ON_ONCE() calls that will
> > > > trigger in the presence of half interrupts, which the code will continue
> > > > to handle correctly.  If neither of these WARN_ON_ONCE() trigger by
> > > > mid-2021, then perhaps RCU can stop handling half-interrupts, which
> > > > would be a considerable simplification.
> > > Hi Paul and everyone,
> > > I was thinking some more about this patch and whether we can simplify this code
> > > much in 2021. Since 2021 is a bit far away, I thought working on it in again to
> > > keep it fresh in memory is a good idea ;-)
> > > 
> > > To me it seems we cannot easily combine the counters (dynticks_nesting and
> > > dynticks_nmi_nesting) even if we confirmed that there is no possibility of a
> > > half-interrupt scenario (assuming simplication means counter combining like
> > > Byungchul tried to do in https://goo.gl/X1U77X). The reason is because these
> > > 2 counters need to be tracked separately as they are used differently in the
> > > following function:
> > Hi Joel and Paul,
> > 
> > I always love the way to logically approach problems so I'm a fan of
> > all your works :) But I'm JUST curious about something here. Why can't
> > we combine them the way I tried even if we confirm no possibility of
> > half-interrupt? IMHO, the only thing we want to know through calling
> > rcu_is_cpu_rrupt_from_idle() is whether the interrupt comes from
> > RCU-idle or not - of course assuming the caller context always be an
> > well-defined interrupt context like e.g. the tick handler.
> > 
> > So the function can return true if the caller is within a RCU-idle
> > region except a well-known single interrupt nested.
> > 
> > Of course, now that we cannot confirm it yet, the crowbar is necessary.
> > But does it still have a problem even after confirming it? Why? What am
> > I missing? Could you explain why for me? :(
> 
> 
> Did you also want to consider the case the function is called from others
> than
> well-known interrupt contexts? If yes, then I agree with you, there doesn't
> seem to be the kind of code and it's not a good idea to let the function be
> called generally though.

We were discussing exactly this on the thread. I am going to be adding a
lockdep check to make sure the function isn't called from anywhere but an
interrupt context. Then once we can confirm that there are no more
half-interrupts in the future, we can apply your counter combining approach.

Based on the comments on the rrupt_from_idle() function, it wasn't clear to
me if the function was intended to be called from any context. That's why I
thought the counter combing approach would not work, but you are right - it
should work.

I will CC you on the lockdep check patch. Also hope you are subscribed to
the new shiny rcu@vger.kernel.org list as well :-)

thanks,

 - Joel