[RFC 1/2] lockdep: Add assertion to check if in an interrupt

[RFC 1/2] lockdep: Add assertion to check if in an interrupt

[Joel Fernandes (Google)]

In rcu_rrupt_from_idle, we want to check if it is called from within an
interrupt, but want to do such checking only for debug builds. lockdep
already tracks when we enter an interrupt. Let us expose it as an
assertion macro so it can be used to assert this.

Suggested-by: Steven Rostedt <rostedt@goodmis.org>
Cc: kernel-team@android.com
Cc: rcu@vger.kernel.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 include/linux/lockdep.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/linux/lockdep.h b/include/linux/lockdep.h
index c5335df2372f..d24f564823d3 100644
--- a/include/linux/lockdep.h
+++ b/include/linux/lockdep.h
@@ -601,11 +601,18 @@ do {									\
 			  "IRQs not disabled as expected\n");		\
 	} while (0)
 
+#define lockdep_assert_in_irq() do {					\
+		WARN_ONCE(debug_locks && !current->lockdep_recursion &&	\
+			  !current->hardirq_context,			\
+			  "Not in hardirq as expected\n");		\
+	} while (0)
+
 #else
 # define might_lock(lock) do { } while (0)
 # define might_lock_read(lock) do { } while (0)
 # define lockdep_assert_irqs_enabled() do { } while (0)
 # define lockdep_assert_irqs_disabled() do { } while (0)
+# define lockdep_assert_in_irq() do { } while (0)
 #endif
 
 #ifdef CONFIG_LOCKDEP
-- 
2.21.0.392.gf8f6787159e-goog

[RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Joel Fernandes (Google)]

In the future we would like to combine the dynticks and dynticks_nesting
counters thus leading to simplifying the code. At the moment we cannot
do that due to concerns about usermode upcalls appearing to RCU as half
of an interrupt. Byungchul tried to do it in [1] but the
"half-interrupt" concern was raised. It is half because, what RCU
expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
exception happens. However, only rcu_irq_enter() is observed. This
concern may not be valid anymore, but at least it used to be the case.

Out of abundance of caution, Paul added warnings [2] in the RCU code
which if not fired by 2021 may allow us to assume that such
half-interrupt scenario cannot happen any more, which can lead to
simplification of this code.

Summary of the changes are the following:

(1) In preparation for this combination of counters in the future, we
first need to first be sure that rcu_rrupt_from_idle cannot be called
from anywhere but a hard-interrupt because previously, the comments
suggested otherwise so let us be sure. We discussed this here [3]. We
use the services of lockdep to accomplish this.

(2) Further rcu_rrupt_from_idle() is not explicit about how it is using
the counters which can lead to weird future bugs. This patch therefore
makes it more explicit about the specific counter values being tested

(3) Lastly, we check for counter underflows just to be sure these are
not happening, because the previous code in rcu_rrupt_from_idle() was
allowing the case where the counters can underflow, and the function
would still return true. Now we are checking for specific values so let
us be confident by additional checking, that such underflows don't
happen. Any case, if they do, we should fix them and the screaming
warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
and PROVE_LOCKING are disabled.

[1] https://lore.kernel.org/patchwork/patch/952349/
[2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
[3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/

Cc: byungchul.park@lge.com
Cc: kernel-team@android.com
Cc: rcu@vger.kernel.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9180158756d2..d94c8ed29f6b 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
 }
 
 /**
- * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
+ * rcu_is_cpu_rrupt_from_idle - see if 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;
+	/* Called only from within the scheduling-clock interrupt */
+	lockdep_assert_in_irq();
+
+	/* Check for counter underflows */
+	RCU_LOCKDEP_WARN(
+		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
+		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),
+		"RCU dynticks nesting counters underflow!");
+
+	/* Are we at first interrupt nesting level? */
+	if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
+		return false;
+
+	/* Does CPU appear to be idle from an RCU standpoint? */
+	return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
 }
 
 #define DEFAULT_RCU_BLIMIT 10     /* Maximum callbacks per rcu_do_batch. */
-- 
2.21.0.392.gf8f6787159e-goog

Re: [RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Joel Fernandes]

On Fri, Mar 22, 2019 at 09:29:39PM -0400, Joel Fernandes (Google) wrote:
> In the future we would like to combine the dynticks and dynticks_nesting
> counters thus leading to simplifying the code. At the moment we cannot
> do that due to concerns about usermode upcalls appearing to RCU as half
> of an interrupt. Byungchul tried to do it in [1] but the
> "half-interrupt" concern was raised. It is half because, what RCU
> expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> exception happens. However, only rcu_irq_enter() is observed. This
> concern may not be valid anymore, but at least it used to be the case.
> 
> Out of abundance of caution, Paul added warnings [2] in the RCU code
> which if not fired by 2021 may allow us to assume that such
> half-interrupt scenario cannot happen any more, which can lead to
> simplification of this code.
> 
> Summary of the changes are the following:
> 
> (1) In preparation for this combination of counters in the future, we
> first need to first be sure that rcu_rrupt_from_idle cannot be called
> from anywhere but a hard-interrupt because previously, the comments
> suggested otherwise so let us be sure. We discussed this here [3]. We
> use the services of lockdep to accomplish this.
> 
> (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> the counters which can lead to weird future bugs. This patch therefore
> makes it more explicit about the specific counter values being tested
> 
> (3) Lastly, we check for counter underflows just to be sure these are
> not happening, because the previous code in rcu_rrupt_from_idle() was
> allowing the case where the counters can underflow, and the function
> would still return true. Now we are checking for specific values so let
> us be confident by additional checking, that such underflows don't
> happen. Any case, if they do, we should fix them and the screaming
> warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> and PROVE_LOCKING are disabled.
> 
> [1] https://lore.kernel.org/patchwork/patch/952349/
> [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> 
> Cc: byungchul.park@lge.com
> Cc: kernel-team@android.com
> Cc: rcu@vger.kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> ---
>  kernel/rcu/tree.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 9180158756d2..d94c8ed29f6b 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
>  }
>  
>  /**
> - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> + * rcu_is_cpu_rrupt_from_idle - see if 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;
> +	/* Called only from within the scheduling-clock interrupt */
> +	lockdep_assert_in_irq();
> +
> +	/* Check for counter underflows */
> +	RCU_LOCKDEP_WARN(
> +		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
> +		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),

 
This condition for the warning is supposed to be || instead of &&. Sorry.

Or, I will just use 2 RCU_LOCKDEP_WARN(s) here, that's better.

thanks,

 - Joel

Re: [RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Paul E. McKenney]

On Fri, Mar 22, 2019 at 11:02:51PM -0400, Joel Fernandes wrote:
> On Fri, Mar 22, 2019 at 09:29:39PM -0400, Joel Fernandes (Google) wrote:
> > In the future we would like to combine the dynticks and dynticks_nesting
> > counters thus leading to simplifying the code. At the moment we cannot
> > do that due to concerns about usermode upcalls appearing to RCU as half
> > of an interrupt. Byungchul tried to do it in [1] but the
> > "half-interrupt" concern was raised. It is half because, what RCU
> > expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> > exception happens. However, only rcu_irq_enter() is observed. This
> > concern may not be valid anymore, but at least it used to be the case.
> > 
> > Out of abundance of caution, Paul added warnings [2] in the RCU code
> > which if not fired by 2021 may allow us to assume that such
> > half-interrupt scenario cannot happen any more, which can lead to
> > simplification of this code.
> > 
> > Summary of the changes are the following:
> > 
> > (1) In preparation for this combination of counters in the future, we
> > first need to first be sure that rcu_rrupt_from_idle cannot be called
> > from anywhere but a hard-interrupt because previously, the comments
> > suggested otherwise so let us be sure. We discussed this here [3]. We
> > use the services of lockdep to accomplish this.
> > 
> > (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> > the counters which can lead to weird future bugs. This patch therefore
> > makes it more explicit about the specific counter values being tested
> > 
> > (3) Lastly, we check for counter underflows just to be sure these are
> > not happening, because the previous code in rcu_rrupt_from_idle() was
> > allowing the case where the counters can underflow, and the function
> > would still return true. Now we are checking for specific values so let
> > us be confident by additional checking, that such underflows don't
> > happen. Any case, if they do, we should fix them and the screaming
> > warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> > and PROVE_LOCKING are disabled.
> > 
> > [1] https://lore.kernel.org/patchwork/patch/952349/
> > [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> > [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> > 
> > Cc: byungchul.park@lge.com
> > Cc: kernel-team@android.com
> > Cc: rcu@vger.kernel.org
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > ---
> >  kernel/rcu/tree.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > index 9180158756d2..d94c8ed29f6b 100644
> > --- a/kernel/rcu/tree.c
> > +++ b/kernel/rcu/tree.c
> > @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
> >  }
> >  
> >  /**
> > - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> > + * rcu_is_cpu_rrupt_from_idle - see if 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;
> > +	/* Called only from within the scheduling-clock interrupt */
> > +	lockdep_assert_in_irq();
> > +
> > +	/* Check for counter underflows */
> > +	RCU_LOCKDEP_WARN(
> > +		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
> > +		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),
> 
>  
> This condition for the warning is supposed to be || instead of &&. Sorry.
> 
> Or, I will just use 2 RCU_LOCKDEP_WARN(s) here, that's better.

Also, the dynticks_nmi_nesting being zero is a bug given that we know
we are in an interrupt handler, right?  Or am I off by one again?

							Thanx, Paul

Re: [RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Joel Fernandes]

On Sun, Mar 24, 2019 at 04:43:51PM -0700, Paul E. McKenney wrote:
> On Fri, Mar 22, 2019 at 11:02:51PM -0400, Joel Fernandes wrote:
> > On Fri, Mar 22, 2019 at 09:29:39PM -0400, Joel Fernandes (Google) wrote:
> > > In the future we would like to combine the dynticks and dynticks_nesting
> > > counters thus leading to simplifying the code. At the moment we cannot
> > > do that due to concerns about usermode upcalls appearing to RCU as half
> > > of an interrupt. Byungchul tried to do it in [1] but the
> > > "half-interrupt" concern was raised. It is half because, what RCU
> > > expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> > > exception happens. However, only rcu_irq_enter() is observed. This
> > > concern may not be valid anymore, but at least it used to be the case.
> > > 
> > > Out of abundance of caution, Paul added warnings [2] in the RCU code
> > > which if not fired by 2021 may allow us to assume that such
> > > half-interrupt scenario cannot happen any more, which can lead to
> > > simplification of this code.
> > > 
> > > Summary of the changes are the following:
> > > 
> > > (1) In preparation for this combination of counters in the future, we
> > > first need to first be sure that rcu_rrupt_from_idle cannot be called
> > > from anywhere but a hard-interrupt because previously, the comments
> > > suggested otherwise so let us be sure. We discussed this here [3]. We
> > > use the services of lockdep to accomplish this.
> > > 
> > > (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> > > the counters which can lead to weird future bugs. This patch therefore
> > > makes it more explicit about the specific counter values being tested
> > > 
> > > (3) Lastly, we check for counter underflows just to be sure these are
> > > not happening, because the previous code in rcu_rrupt_from_idle() was
> > > allowing the case where the counters can underflow, and the function
> > > would still return true. Now we are checking for specific values so let
> > > us be confident by additional checking, that such underflows don't
> > > happen. Any case, if they do, we should fix them and the screaming
> > > warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> > > and PROVE_LOCKING are disabled.
> > > 
> > > [1] https://lore.kernel.org/patchwork/patch/952349/
> > > [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> > > [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> > > 
> > > Cc: byungchul.park@lge.com
> > > Cc: kernel-team@android.com
> > > Cc: rcu@vger.kernel.org
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > ---
> > >  kernel/rcu/tree.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > index 9180158756d2..d94c8ed29f6b 100644
> > > --- a/kernel/rcu/tree.c
> > > +++ b/kernel/rcu/tree.c
> > > @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
> > >  }
> > >  
> > >  /**
> > > - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> > > + * rcu_is_cpu_rrupt_from_idle - see if 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;
> > > +	/* Called only from within the scheduling-clock interrupt */
> > > +	lockdep_assert_in_irq();
> > > +
> > > +	/* Check for counter underflows */
> > > +	RCU_LOCKDEP_WARN(
> > > +		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
> > > +		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),
> > 
> >  
> > This condition for the warning is supposed to be || instead of &&. Sorry.
> > 
> > Or, I will just use 2 RCU_LOCKDEP_WARN(s) here, that's better.
> 
> Also, the dynticks_nmi_nesting being zero is a bug given that we know
> we are in an interrupt handler, right?  Or am I off by one again?

You are right, we can do additional checking for making sure its never zero.
I refreshed the patch as below, does this look Ok?

---8<-----------------------

From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
Subject: [RFC v2] rcutree: Add checks for dynticks counters in

In the future we would like to combine the dynticks and dynticks_nesting
counters thus leading to simplifying the code. At the moment we cannot
do that due to concerns about usermode upcalls appearing to RCU as half
of an interrupt. Byungchul tried to do it in [1] but the
"half-interrupt" concern was raised. It is half because, what RCU
expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
exception happens. However, only rcu_irq_enter() is observed. This
concern may not be valid anymore, but at least it used to be the case.

Out of abundance of caution, Paul added warnings [2] in the RCU code
which if not fired by 2021 may allow us to assume that such
half-interrupt scenario cannot happen any more, which can lead to
simplification of this code.

Summary of the changes are the following:

(1) In preparation for this combination of counters in the future, we
first need to first be sure that rcu_rrupt_from_idle cannot be called
from anywhere but a hard-interrupt because previously, the comments
suggested otherwise so let us be sure. We discussed this here [3]. We
use the services of lockdep to accomplish this.

(2) Further rcu_rrupt_from_idle() is not explicit about how it is using
the counters which can lead to weird future bugs. This patch therefore
makes it more explicit about the specific counter values being tested

(3) Lastly, we check for counter underflows just to be sure these are
not happening, because the previous code in rcu_rrupt_from_idle() was
allowing the case where the counters can underflow, and the function
would still return true. Now we are checking for specific values so let
us be confident by additional checking, that such underflows don't
happen. Any case, if they do, we should fix them and the screaming
warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
and PROVE_LOCKING are disabled.

[1] https://lore.kernel.org/patchwork/patch/952349/
[2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
[3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/

Cc: byungchul.park@lge.com
Cc: kernel-team@android.com
Cc: rcu@vger.kernel.org
Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
---
 kernel/rcu/tree.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
index 9180158756d2..c2a56de098da 100644
--- a/kernel/rcu/tree.c
+++ b/kernel/rcu/tree.c
@@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
 }
 
 /**
- * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
+ * rcu_is_cpu_rrupt_from_idle - see if 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;
+	/* Called only from within the scheduling-clock interrupt */
+	lockdep_assert_in_irq();
+
+	/* Check for counter underflows */
+	RCU_LOCKDEP_WARN(_this_cpu_read(rcu_data.dynticks_nesting) < 0,
+			 "RCU dynticks_nesting counter underflow!");
+	RCU_LOCKDEP_WARN(_this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
+			 "RCU dynticks_nmi_nesting counter underflow/zero!");
+
+	/* Are we at first interrupt nesting level? */
+	if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
+		return false;
+
+	/* Does CPU appear to be idle from an RCU standpoint? */
+	return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
 }
 
 #define DEFAULT_RCU_BLIMIT 10     /* Maximum callbacks per rcu_do_batch. */
-- 
2.21.0.392.gf8f6787159e-goog

Re: [RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Paul E. McKenney]

On Mon, Mar 25, 2019 at 09:36:46AM -0400, Joel Fernandes wrote:
> On Sun, Mar 24, 2019 at 04:43:51PM -0700, Paul E. McKenney wrote:
> > On Fri, Mar 22, 2019 at 11:02:51PM -0400, Joel Fernandes wrote:
> > > On Fri, Mar 22, 2019 at 09:29:39PM -0400, Joel Fernandes (Google) wrote:
> > > > In the future we would like to combine the dynticks and dynticks_nesting
> > > > counters thus leading to simplifying the code. At the moment we cannot
> > > > do that due to concerns about usermode upcalls appearing to RCU as half
> > > > of an interrupt. Byungchul tried to do it in [1] but the
> > > > "half-interrupt" concern was raised. It is half because, what RCU
> > > > expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> > > > exception happens. However, only rcu_irq_enter() is observed. This
> > > > concern may not be valid anymore, but at least it used to be the case.
> > > > 
> > > > Out of abundance of caution, Paul added warnings [2] in the RCU code
> > > > which if not fired by 2021 may allow us to assume that such
> > > > half-interrupt scenario cannot happen any more, which can lead to
> > > > simplification of this code.
> > > > 
> > > > Summary of the changes are the following:
> > > > 
> > > > (1) In preparation for this combination of counters in the future, we
> > > > first need to first be sure that rcu_rrupt_from_idle cannot be called
> > > > from anywhere but a hard-interrupt because previously, the comments
> > > > suggested otherwise so let us be sure. We discussed this here [3]. We
> > > > use the services of lockdep to accomplish this.
> > > > 
> > > > (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> > > > the counters which can lead to weird future bugs. This patch therefore
> > > > makes it more explicit about the specific counter values being tested
> > > > 
> > > > (3) Lastly, we check for counter underflows just to be sure these are
> > > > not happening, because the previous code in rcu_rrupt_from_idle() was
> > > > allowing the case where the counters can underflow, and the function
> > > > would still return true. Now we are checking for specific values so let
> > > > us be confident by additional checking, that such underflows don't
> > > > happen. Any case, if they do, we should fix them and the screaming
> > > > warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> > > > and PROVE_LOCKING are disabled.
> > > > 
> > > > [1] https://lore.kernel.org/patchwork/patch/952349/
> > > > [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> > > > [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> > > > 
> > > > Cc: byungchul.park@lge.com
> > > > Cc: kernel-team@android.com
> > > > Cc: rcu@vger.kernel.org
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > ---
> > > >  kernel/rcu/tree.c | 21 +++++++++++++++++----
> > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > index 9180158756d2..d94c8ed29f6b 100644
> > > > --- a/kernel/rcu/tree.c
> > > > +++ b/kernel/rcu/tree.c
> > > > @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
> > > >  }
> > > >  
> > > >  /**
> > > > - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> > > > + * rcu_is_cpu_rrupt_from_idle - see if 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;
> > > > +	/* Called only from within the scheduling-clock interrupt */
> > > > +	lockdep_assert_in_irq();
> > > > +
> > > > +	/* Check for counter underflows */
> > > > +	RCU_LOCKDEP_WARN(
> > > > +		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
> > > > +		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),
> > > 
> > >  
> > > This condition for the warning is supposed to be || instead of &&. Sorry.
> > > 
> > > Or, I will just use 2 RCU_LOCKDEP_WARN(s) here, that's better.
> > 
> > Also, the dynticks_nmi_nesting being zero is a bug given that we know
> > we are in an interrupt handler, right?  Or am I off by one again?
> 
> You are right, we can do additional checking for making sure its never zero.
> I refreshed the patch as below, does this look Ok?
> 
> ---8<-----------------------
> 
> From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> Subject: [RFC v2] rcutree: Add checks for dynticks counters in
> 
> In the future we would like to combine the dynticks and dynticks_nesting
> counters thus leading to simplifying the code. At the moment we cannot
> do that due to concerns about usermode upcalls appearing to RCU as half
> of an interrupt. Byungchul tried to do it in [1] but the
> "half-interrupt" concern was raised. It is half because, what RCU
> expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> exception happens. However, only rcu_irq_enter() is observed. This
> concern may not be valid anymore, but at least it used to be the case.
> 
> Out of abundance of caution, Paul added warnings [2] in the RCU code
> which if not fired by 2021 may allow us to assume that such
> half-interrupt scenario cannot happen any more, which can lead to
> simplification of this code.
> 
> Summary of the changes are the following:
> 
> (1) In preparation for this combination of counters in the future, we
> first need to first be sure that rcu_rrupt_from_idle cannot be called
> from anywhere but a hard-interrupt because previously, the comments
> suggested otherwise so let us be sure. We discussed this here [3]. We
> use the services of lockdep to accomplish this.
> 
> (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> the counters which can lead to weird future bugs. This patch therefore
> makes it more explicit about the specific counter values being tested
> 
> (3) Lastly, we check for counter underflows just to be sure these are
> not happening, because the previous code in rcu_rrupt_from_idle() was
> allowing the case where the counters can underflow, and the function
> would still return true. Now we are checking for specific values so let
> us be confident by additional checking, that such underflows don't
> happen. Any case, if they do, we should fix them and the screaming
> warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> and PROVE_LOCKING are disabled.
> 
> [1] https://lore.kernel.org/patchwork/patch/952349/
> [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> 
> Cc: byungchul.park@lge.com
> Cc: kernel-team@android.com
> Cc: rcu@vger.kernel.org
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Looks better!  I have applied this and its predecessor, if in backwards
order.  (Will fix, rebase coming up anyway.)

I do like your Cc-ing kernel-team@android.com -- one less thing for me
to remember!  ;-)

							Thanx, Paul

> ---
>  kernel/rcu/tree.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 9180158756d2..c2a56de098da 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
>  }
>  
>  /**
> - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> + * rcu_is_cpu_rrupt_from_idle - see if 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;
> +	/* Called only from within the scheduling-clock interrupt */
> +	lockdep_assert_in_irq();
> +
> +	/* Check for counter underflows */
> +	RCU_LOCKDEP_WARN(_this_cpu_read(rcu_data.dynticks_nesting) < 0,
> +			 "RCU dynticks_nesting counter underflow!");
> +	RCU_LOCKDEP_WARN(_this_cpu_read(rcu_data.dynticks_nmi_nesting) <= 0,
> +			 "RCU dynticks_nmi_nesting counter underflow/zero!");
> +
> +	/* Are we at first interrupt nesting level? */
> +	if (__this_cpu_read(rcu_data.dynticks_nmi_nesting) != 1)
> +		return false;
> +
> +	/* Does CPU appear to be idle from an RCU standpoint? */
> +	return __this_cpu_read(rcu_data.dynticks_nesting) == 0;
>  }
>  
>  #define DEFAULT_RCU_BLIMIT 10     /* Maximum callbacks per rcu_do_batch. */
> -- 
> 2.21.0.392.gf8f6787159e-goog
> 

Re: [RFC 2/2] rcutree: Add checks for dynticks counters in rcu_is_cpu_rrupt_from_idle

[Joel Fernandes]

On Mon, Mar 25, 2019 at 08:53:41AM -0700, Paul E. McKenney wrote:
> On Mon, Mar 25, 2019 at 09:36:46AM -0400, Joel Fernandes wrote:
> > On Sun, Mar 24, 2019 at 04:43:51PM -0700, Paul E. McKenney wrote:
> > > On Fri, Mar 22, 2019 at 11:02:51PM -0400, Joel Fernandes wrote:
> > > > On Fri, Mar 22, 2019 at 09:29:39PM -0400, Joel Fernandes (Google) wrote:
> > > > > In the future we would like to combine the dynticks and dynticks_nesting
> > > > > counters thus leading to simplifying the code. At the moment we cannot
> > > > > do that due to concerns about usermode upcalls appearing to RCU as half
> > > > > of an interrupt. Byungchul tried to do it in [1] but the
> > > > > "half-interrupt" concern was raised. It is half because, what RCU
> > > > > expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> > > > > exception happens. However, only rcu_irq_enter() is observed. This
> > > > > concern may not be valid anymore, but at least it used to be the case.
> > > > > 
> > > > > Out of abundance of caution, Paul added warnings [2] in the RCU code
> > > > > which if not fired by 2021 may allow us to assume that such
> > > > > half-interrupt scenario cannot happen any more, which can lead to
> > > > > simplification of this code.
> > > > > 
> > > > > Summary of the changes are the following:
> > > > > 
> > > > > (1) In preparation for this combination of counters in the future, we
> > > > > first need to first be sure that rcu_rrupt_from_idle cannot be called
> > > > > from anywhere but a hard-interrupt because previously, the comments
> > > > > suggested otherwise so let us be sure. We discussed this here [3]. We
> > > > > use the services of lockdep to accomplish this.
> > > > > 
> > > > > (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> > > > > the counters which can lead to weird future bugs. This patch therefore
> > > > > makes it more explicit about the specific counter values being tested
> > > > > 
> > > > > (3) Lastly, we check for counter underflows just to be sure these are
> > > > > not happening, because the previous code in rcu_rrupt_from_idle() was
> > > > > allowing the case where the counters can underflow, and the function
> > > > > would still return true. Now we are checking for specific values so let
> > > > > us be confident by additional checking, that such underflows don't
> > > > > happen. Any case, if they do, we should fix them and the screaming
> > > > > warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> > > > > and PROVE_LOCKING are disabled.
> > > > > 
> > > > > [1] https://lore.kernel.org/patchwork/patch/952349/
> > > > > [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> > > > > [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> > > > > 
> > > > > Cc: byungchul.park@lge.com
> > > > > Cc: kernel-team@android.com
> > > > > Cc: rcu@vger.kernel.org
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > ---
> > > > >  kernel/rcu/tree.c | 21 +++++++++++++++++----
> > > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> > > > > index 9180158756d2..d94c8ed29f6b 100644
> > > > > --- a/kernel/rcu/tree.c
> > > > > +++ b/kernel/rcu/tree.c
> > > > > @@ -381,16 +381,29 @@ static void __maybe_unused rcu_momentary_dyntick_idle(void)
> > > > >  }
> > > > >  
> > > > >  /**
> > > > > - * rcu_is_cpu_rrupt_from_idle - see if idle or immediately interrupted from idle
> > > > > + * rcu_is_cpu_rrupt_from_idle - see if 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;
> > > > > +	/* Called only from within the scheduling-clock interrupt */
> > > > > +	lockdep_assert_in_irq();
> > > > > +
> > > > > +	/* Check for counter underflows */
> > > > > +	RCU_LOCKDEP_WARN(
> > > > > +		(__this_cpu_read(rcu_data.dynticks_nesting) < 0) &&
> > > > > +		(__this_cpu_read(rcu_data.dynticks_nmi_nesting) < 0),
> > > > 
> > > >  
> > > > This condition for the warning is supposed to be || instead of &&. Sorry.
> > > > 
> > > > Or, I will just use 2 RCU_LOCKDEP_WARN(s) here, that's better.
> > > 
> > > Also, the dynticks_nmi_nesting being zero is a bug given that we know
> > > we are in an interrupt handler, right?  Or am I off by one again?
> > 
> > You are right, we can do additional checking for making sure its never zero.
> > I refreshed the patch as below, does this look Ok?
> > 
> > ---8<-----------------------
> > 
> > From: "Joel Fernandes (Google)" <joel@joelfernandes.org>
> > Subject: [RFC v2] rcutree: Add checks for dynticks counters in
> > 
> > In the future we would like to combine the dynticks and dynticks_nesting
> > counters thus leading to simplifying the code. At the moment we cannot
> > do that due to concerns about usermode upcalls appearing to RCU as half
> > of an interrupt. Byungchul tried to do it in [1] but the
> > "half-interrupt" concern was raised. It is half because, what RCU
> > expects is rcu_irq_enter() and rcu_irq_exit() pairs when the usermode
> > exception happens. However, only rcu_irq_enter() is observed. This
> > concern may not be valid anymore, but at least it used to be the case.
> > 
> > Out of abundance of caution, Paul added warnings [2] in the RCU code
> > which if not fired by 2021 may allow us to assume that such
> > half-interrupt scenario cannot happen any more, which can lead to
> > simplification of this code.
> > 
> > Summary of the changes are the following:
> > 
> > (1) In preparation for this combination of counters in the future, we
> > first need to first be sure that rcu_rrupt_from_idle cannot be called
> > from anywhere but a hard-interrupt because previously, the comments
> > suggested otherwise so let us be sure. We discussed this here [3]. We
> > use the services of lockdep to accomplish this.
> > 
> > (2) Further rcu_rrupt_from_idle() is not explicit about how it is using
> > the counters which can lead to weird future bugs. This patch therefore
> > makes it more explicit about the specific counter values being tested
> > 
> > (3) Lastly, we check for counter underflows just to be sure these are
> > not happening, because the previous code in rcu_rrupt_from_idle() was
> > allowing the case where the counters can underflow, and the function
> > would still return true. Now we are checking for specific values so let
> > us be confident by additional checking, that such underflows don't
> > happen. Any case, if they do, we should fix them and the screaming
> > warning is appropriate. All these checks checks are NOOPs if PROVE_RCU
> > and PROVE_LOCKING are disabled.
> > 
> > [1] https://lore.kernel.org/patchwork/patch/952349/
> > [2] Commit e11ec65cc8d6 ("rcu: Add warning to detect half-interrupts")
> > [3] https://lore.kernel.org/lkml/20190312150514.GB249405@google.com/
> > 
> > Cc: byungchul.park@lge.com
> > Cc: kernel-team@android.com
> > Cc: rcu@vger.kernel.org
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> Looks better!  I have applied this and its predecessor, if in backwards
> order.  (Will fix, rebase coming up anyway.)
> 
> I do like your Cc-ing kernel-team@android.com -- one less thing for me
> to remember!  ;-)
 
Thanks a lot Paul! :)

 - Joel