[RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes (Google)]

When we're in hard interrupt context in rcu_read_unlock_special(), we
can still benefit from invoke_rcu_core() doing wake ups of rcuc
threads when the !use_softirq parameter is passed.  This is safe
to do so because:

1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
conditions in rcu_read_unlock_special()") by checking for the same in
this patch.

2. in_irq() implies in_interrupt() which implies raising softirq will
not do any wake ups.

The rcuc thread which is awakened will run when the interrupt returns.

We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
if expedited") thus doing the rcuc awakening only when none of the
following are true:
  1. Critical section is blocking an expedited GP.
  2. A nohz_full CPU.
If neither of these cases are true (exp == false), then the "else" block
will run to do the irq_work stuff.

This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
check added.

Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

---
v1->v2: Some minor character encoding issues in changelog corrected.

Note that I am still testing this patch, but I sent an early RFC for your
feedback. Thanks!

 kernel/rcu/tree_plugin.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 2defc7fe74c3..f4b3055026dc 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -621,6 +621,11 @@ static void rcu_read_unlock_special(struct task_struct *t)
 			// Using softirq, safe to awaken, and we get
 			// no help from enabling irqs, unlike bh/preempt.
 			raise_softirq_irqoff(RCU_SOFTIRQ);
+		} else if (exp && in_irq() && !use_softirq &&
+			   !t->rcu_read_unlock_special.b.deferred_qs) {
+			// Safe to awaken rcuc kthread which will be
+                       // scheduled in from the interrupt return path.
+			invoke_rcu_core();
 		} else {
 			// Enabling BH or preempt does reschedule, so...
 			// Also if no expediting or NO_HZ_FULL, slow is OK.
-- 
2.23.0.rc1.153.gdeed80330f-goog

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> When we're in hard interrupt context in rcu_read_unlock_special(), we
> can still benefit from invoke_rcu_core() doing wake ups of rcuc
> threads when the !use_softirq parameter is passed.  This is safe
> to do so because:
> 
> 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> conditions in rcu_read_unlock_special()") by checking for the same in
> this patch.
> 
> 2. in_irq() implies in_interrupt() which implies raising softirq will
> not do any wake ups.
> 
> The rcuc thread which is awakened will run when the interrupt returns.
> 
> We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> if expedited") thus doing the rcuc awakening only when none of the
> following are true:
>   1. Critical section is blocking an expedited GP.
>   2. A nohz_full CPU.
> If neither of these cases are true (exp == false), then the "else" block
> will run to do the irq_work stuff.
> 
> This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> check added.
> 
> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>

OK, I will bite...  If it is safe to wake up an rcuc kthread, why
is it not safe to do raise_softirq()?

And from the nit department, looks like some whitespace damage on the
comments.

							Thanx, Paul

> ---
> v1->v2: Some minor character encoding issues in changelog corrected.
> 
> Note that I am still testing this patch, but I sent an early RFC for your
> feedback. Thanks!
> 
>  kernel/rcu/tree_plugin.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 2defc7fe74c3..f4b3055026dc 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -621,6 +621,11 @@ static void rcu_read_unlock_special(struct task_struct *t)
>  			// Using softirq, safe to awaken, and we get
>  			// no help from enabling irqs, unlike bh/preempt.
>  			raise_softirq_irqoff(RCU_SOFTIRQ);
> +		} else if (exp && in_irq() && !use_softirq &&
> +			   !t->rcu_read_unlock_special.b.deferred_qs) {
> +			// Safe to awaken rcuc kthread which will be
> +                       // scheduled in from the interrupt return path.
> +			invoke_rcu_core();
>  		} else {
>  			// Enabling BH or preempt does reschedule, so...
>  			// Also if no expediting or NO_HZ_FULL, slow is OK.
> -- 
> 2.23.0.rc1.153.gdeed80330f-goog
> 

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > threads when the !use_softirq parameter is passed.  This is safe
> > to do so because:
> > 
> > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > conditions in rcu_read_unlock_special()") by checking for the same in
> > this patch.
> > 
> > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > not do any wake ups.
> > 
> > The rcuc thread which is awakened will run when the interrupt returns.
> > 
> > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > if expedited") thus doing the rcuc awakening only when none of the
> > following are true:
> >   1. Critical section is blocking an expedited GP.
> >   2. A nohz_full CPU.
> > If neither of these cases are true (exp == false), then the "else" block
> > will run to do the irq_work stuff.
> > 
> > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > check added.
> > 
> > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> 
> OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> is it not safe to do raise_softirq()?

Because raise_softirq should not be done and/or doesn't do anything
if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
use_softirq == false. The "else if" condition of this patch uses for
use_softirq.

Or, did I miss your point?

> And from the nit department, looks like some whitespace damage on the
> comments.

I will fix all of these in the change log, it was just a quick RFC I sent
with the idea, tagged as RFC and not yet for merging. I should also remove
the comment about " in_irq() implies in_interrupt() which implies raising
softirq" from the changelog since this patch is only concerned with the rcuc
kthread.

thanks!

- Joel


> 							Thanx, Paul
> 
> > ---
> > v1->v2: Some minor character encoding issues in changelog corrected.
> > 
> > Note that I am still testing this patch, but I sent an early RFC for your
> > feedback. Thanks!
> > 
> >  kernel/rcu/tree_plugin.h | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 2defc7fe74c3..f4b3055026dc 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -621,6 +621,11 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >  			// Using softirq, safe to awaken, and we get
> >  			// no help from enabling irqs, unlike bh/preempt.
> >  			raise_softirq_irqoff(RCU_SOFTIRQ);
> > +		} else if (exp && in_irq() && !use_softirq &&
> > +			   !t->rcu_read_unlock_special.b.deferred_qs) {
> > +			// Safe to awaken rcuc kthread which will be
> > +                       // scheduled in from the interrupt return path.
> > +			invoke_rcu_core();
> >  		} else {
> >  			// Enabling BH or preempt does reschedule, so...
> >  			// Also if no expediting or NO_HZ_FULL, slow is OK.
> > -- 
> > 2.23.0.rc1.153.gdeed80330f-goog
> > 
> 

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Sun, Aug 18, 2019 at 06:32:30PM -0400, Joel Fernandes wrote:
> On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> > On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > > threads when the !use_softirq parameter is passed.  This is safe
> > > to do so because:
> > > 
> > > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > > conditions in rcu_read_unlock_special()") by checking for the same in
> > > this patch.
> > > 
> > > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > > not do any wake ups.
> > > 
> > > The rcuc thread which is awakened will run when the interrupt returns.
> > > 
> > > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > > if expedited") thus doing the rcuc awakening only when none of the
> > > following are true:
> > >   1. Critical section is blocking an expedited GP.
> > >   2. A nohz_full CPU.
> > > If neither of these cases are true (exp == false), then the "else" block
> > > will run to do the irq_work stuff.
> > > 
> > > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > > check added.
> > > 
> > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > 
> > OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> > is it not safe to do raise_softirq()?
> 
> Because raise_softirq should not be done and/or doesn't do anything
> if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
> use_softirq == false. The "else if" condition of this patch uses for
> use_softirq.
> 
> Or, did I miss your point?
> 
> > And from the nit department, looks like some whitespace damage on the
> > comments.
> 
> I will fix all of these in the change log, it was just a quick RFC I sent
> with the idea, tagged as RFC and not yet for merging. I should also remove
> the comment about " in_irq() implies in_interrupt() which implies raising
> softirq" from the changelog since this patch is only concerned with the rcuc
> kthread.

Ah, I see you mean the comments on the code. Perhaps something went wrong
when I did 'git revert' on the original patch, or some such. Anyway, please
consider this as RFC-grade only. And hopefully I have been writing better
change logs (really trying!!).

thanks,

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Sun, Aug 18, 2019 at 06:35:11PM -0400, Joel Fernandes wrote:
> On Sun, Aug 18, 2019 at 06:32:30PM -0400, Joel Fernandes wrote:
> > On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> > > On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > > > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > > > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > > > threads when the !use_softirq parameter is passed.  This is safe
> > > > to do so because:
> > > > 
> > > > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > > > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > > > conditions in rcu_read_unlock_special()") by checking for the same in
> > > > this patch.
> > > > 
> > > > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > > > not do any wake ups.
> > > > 
> > > > The rcuc thread which is awakened will run when the interrupt returns.
> > > > 
> > > > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > > > if expedited") thus doing the rcuc awakening only when none of the
> > > > following are true:
> > > >   1. Critical section is blocking an expedited GP.
> > > >   2. A nohz_full CPU.
> > > > If neither of these cases are true (exp == false), then the "else" block
> > > > will run to do the irq_work stuff.
> > > > 
> > > > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > > > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > > > check added.
> > > > 
> > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > 
> > > OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> > > is it not safe to do raise_softirq()?
> > 
> > Because raise_softirq should not be done and/or doesn't do anything
> > if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
> > use_softirq == false. The "else if" condition of this patch uses for
> > use_softirq.
> > 
> > Or, did I miss your point?

I am concerned that added "else if" condition might not be sufficient
to eliminate all possible cases of the caller holding a scheduler lock,
which could result in deadlock in the ensuing wakeup.  Might be me missing
something, but such deadlocks have been a recurring problem in the past.

Also, your commit log's point #2 is "in_irq() implies in_interrupt()
which implies raising softirq will not do any wake ups."  This mention
of softirq seems a bit odd, given that we are going to wake up a rcuc
kthread.  Of course, this did nothing to quell my suspicions.  ;-)

							Thanx, Paul

> > > And from the nit department, looks like some whitespace damage on the
> > > comments.
> > 
> > I will fix all of these in the change log, it was just a quick RFC I sent
> > with the idea, tagged as RFC and not yet for merging. I should also remove
> > the comment about " in_irq() implies in_interrupt() which implies raising
> > softirq" from the changelog since this patch is only concerned with the rcuc
> > kthread.
> 
> Ah, I see you mean the comments on the code. Perhaps something went wrong
> when I did 'git revert' on the original patch, or some such. Anyway, please
> consider this as RFC-grade only. And hopefully I have been writing better
> change logs (really trying!!).
> 
> thanks,
> 
>  - Joel
> 

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Sun, Aug 18, 2019 at 04:31:35PM -0700, Paul E. McKenney wrote:
> On Sun, Aug 18, 2019 at 06:35:11PM -0400, Joel Fernandes wrote:
> > On Sun, Aug 18, 2019 at 06:32:30PM -0400, Joel Fernandes wrote:
> > > On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> > > > On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > > > > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > > > > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > > > > threads when the !use_softirq parameter is passed.  This is safe
> > > > > to do so because:
> > > > > 
> > > > > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > > > > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > > > > conditions in rcu_read_unlock_special()") by checking for the same in
> > > > > this patch.
> > > > > 
> > > > > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > > > > not do any wake ups.
> > > > > 
> > > > > The rcuc thread which is awakened will run when the interrupt returns.
> > > > > 
> > > > > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > > > > if expedited") thus doing the rcuc awakening only when none of the
> > > > > following are true:
> > > > >   1. Critical section is blocking an expedited GP.
> > > > >   2. A nohz_full CPU.
> > > > > If neither of these cases are true (exp == false), then the "else" block
> > > > > will run to do the irq_work stuff.
> > > > > 
> > > > > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > > > > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > > > > check added.
> > > > > 
> > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > 
> > > > OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> > > > is it not safe to do raise_softirq()?
> > > 
> > > Because raise_softirq should not be done and/or doesn't do anything
> > > if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
> > > use_softirq == false. The "else if" condition of this patch uses for
> > > use_softirq.
> > > 
> > > Or, did I miss your point?
> 
> I am concerned that added "else if" condition might not be sufficient
> to eliminate all possible cases of the caller holding a scheduler lock,
> which could result in deadlock in the ensuing wakeup.  Might be me missing
> something, but such deadlocks have been a recurring problem in the past.

I thought that was the whole point of the
rcu_read_unlock_special.b.deferred_qs bit that was introduced in
23634ebc1d94. We are checking that bit in the "else if" here as well. So this
should be no less immune to scheduler deadlocks any more than the preceding
"else if" where we are checking this bit.

> Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> which implies raising softirq will not do any wake ups."  This mention
> of softirq seems a bit odd, given that we are going to wake up a rcuc
> kthread.  Of course, this did nothing to quell my suspicions.  ;-)

Yes, I should delete this #2 from the changelog since it is not very relevant
(I feel now). My point with #2 was that even if were to raise a softirq
(which we are not), a scheduler wakeup of ksoftirqd is impossible in this
path anyway since in_irq() implies in_interrupt().

thanks,

 - Joel


> 							Thanx, Paul
> 
> > > > And from the nit department, looks like some whitespace damage on the
> > > > comments.
> > > 
> > > I will fix all of these in the change log, it was just a quick RFC I sent
> > > with the idea, tagged as RFC and not yet for merging. I should also remove
> > > the comment about " in_irq() implies in_interrupt() which implies raising
> > > softirq" from the changelog since this patch is only concerned with the rcuc
> > > kthread.
> > 
> > Ah, I see you mean the comments on the code. Perhaps something went wrong
> > when I did 'git revert' on the original patch, or some such. Anyway, please
> > consider this as RFC-grade only. And hopefully I have been writing better
> > change logs (really trying!!).
> > 
> > thanks,
> > 
> >  - Joel
> > 

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Sun, Aug 18, 2019 at 07:38:39PM -0400, Joel Fernandes wrote:
> On Sun, Aug 18, 2019 at 04:31:35PM -0700, Paul E. McKenney wrote:
> > On Sun, Aug 18, 2019 at 06:35:11PM -0400, Joel Fernandes wrote:
> > > On Sun, Aug 18, 2019 at 06:32:30PM -0400, Joel Fernandes wrote:
> > > > On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> > > > > On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > > > > > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > > > > > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > > > > > threads when the !use_softirq parameter is passed.  This is safe
> > > > > > to do so because:
> > > > > > 
> > > > > > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > > > > > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > > > > > conditions in rcu_read_unlock_special()") by checking for the same in
> > > > > > this patch.
> > > > > > 
> > > > > > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > > > > > not do any wake ups.
> > > > > > 
> > > > > > The rcuc thread which is awakened will run when the interrupt returns.
> > > > > > 
> > > > > > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > > > > > if expedited") thus doing the rcuc awakening only when none of the
> > > > > > following are true:
> > > > > >   1. Critical section is blocking an expedited GP.
> > > > > >   2. A nohz_full CPU.
> > > > > > If neither of these cases are true (exp == false), then the "else" block
> > > > > > will run to do the irq_work stuff.
> > > > > > 
> > > > > > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > > > > > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > > > > > check added.
> > > > > > 
> > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > 
> > > > > OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> > > > > is it not safe to do raise_softirq()?
> > > > 
> > > > Because raise_softirq should not be done and/or doesn't do anything
> > > > if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
> > > > use_softirq == false. The "else if" condition of this patch uses for
> > > > use_softirq.
> > > > 
> > > > Or, did I miss your point?
> > 
> > I am concerned that added "else if" condition might not be sufficient
> > to eliminate all possible cases of the caller holding a scheduler lock,
> > which could result in deadlock in the ensuing wakeup.  Might be me missing
> > something, but such deadlocks have been a recurring problem in the past.
> 
> I thought that was the whole point of the
> rcu_read_unlock_special.b.deferred_qs bit that was introduced in
> 23634ebc1d94. We are checking that bit in the "else if" here as well. So this
> should be no less immune to scheduler deadlocks any more than the preceding
> "else if" where we are checking this bit.

I would have much more confidence in a line of reasoning that led to
"immune to scheduler deadlocks" than one that led to "no less immune to
scheduler deadlocks".  ;-)

> > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > which implies raising softirq will not do any wake ups."  This mention
> > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> 
> Yes, I should delete this #2 from the changelog since it is not very relevant
> (I feel now). My point with #2 was that even if were to raise a softirq
> (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> path anyway since in_irq() implies in_interrupt().

Please!  Could you also add a first-principles explanation of why
the added condition is immune from scheduler deadlocks?

							Thanx, Paul

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> On Sun, Aug 18, 2019 at 07:38:39PM -0400, Joel Fernandes wrote:
> > On Sun, Aug 18, 2019 at 04:31:35PM -0700, Paul E. McKenney wrote:
> > > On Sun, Aug 18, 2019 at 06:35:11PM -0400, Joel Fernandes wrote:
> > > > On Sun, Aug 18, 2019 at 06:32:30PM -0400, Joel Fernandes wrote:
> > > > > On Sun, Aug 18, 2019 at 03:12:10PM -0700, Paul E. McKenney wrote:
> > > > > > On Sun, Aug 18, 2019 at 05:49:48PM -0400, Joel Fernandes (Google) wrote:
> > > > > > > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > > > > > > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > > > > > > threads when the !use_softirq parameter is passed.  This is safe
> > > > > > > to do so because:
> > > > > > > 
> > > > > > > 1. We avoid the scheduler deadlock issues thanks to the deferred_qs bit
> > > > > > > introduced in commit 23634ebc1d94 ("rcu: Check for wakeup-safe
> > > > > > > conditions in rcu_read_unlock_special()") by checking for the same in
> > > > > > > this patch.
> > > > > > > 
> > > > > > > 2. in_irq() implies in_interrupt() which implies raising softirq will
> > > > > > > not do any wake ups.
> > > > > > > 
> > > > > > > The rcuc thread which is awakened will run when the interrupt returns.
> > > > > > > 
> > > > > > > We also honor 25102de ("rcu: Only do rcu_read_unlock_special() wakeups
> > > > > > > if expedited") thus doing the rcuc awakening only when none of the
> > > > > > > following are true:
> > > > > > >   1. Critical section is blocking an expedited GP.
> > > > > > >   2. A nohz_full CPU.
> > > > > > > If neither of these cases are true (exp == false), then the "else" block
> > > > > > > will run to do the irq_work stuff.
> > > > > > > 
> > > > > > > This commit is based on a partial revert of d143b3d1cd89 ("rcu: Simplify
> > > > > > > rcu_read_unlock_special() deferred wakeups") with an additional in_irq()
> > > > > > > check added.
> > > > > > > 
> > > > > > > Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
> > > > > > 
> > > > > > OK, I will bite...  If it is safe to wake up an rcuc kthread, why
> > > > > > is it not safe to do raise_softirq()?
> > > > > 
> > > > > Because raise_softirq should not be done and/or doesn't do anything
> > > > > if use_softirq == false. In fact, RCU_SOFTIRQ doesn't even existing if
> > > > > use_softirq == false. The "else if" condition of this patch uses for
> > > > > use_softirq.
> > > > > 
> > > > > Or, did I miss your point?
> > > 
> > > I am concerned that added "else if" condition might not be sufficient
> > > to eliminate all possible cases of the caller holding a scheduler lock,
> > > which could result in deadlock in the ensuing wakeup.  Might be me missing
> > > something, but such deadlocks have been a recurring problem in the past.
> > 
> > I thought that was the whole point of the
> > rcu_read_unlock_special.b.deferred_qs bit that was introduced in
> > 23634ebc1d94. We are checking that bit in the "else if" here as well. So this
> > should be no less immune to scheduler deadlocks any more than the preceding
> > "else if" where we are checking this bit.
> 
> I would have much more confidence in a line of reasoning that led to
> "immune to scheduler deadlocks" than one that led to "no less immune to
> scheduler deadlocks".  ;-)

That is fair :-D But let me explain,

What I meant is, if we are saying that this solution has a scheduler
deadlock, then that would almost certainly imply that the existing code has
scheduler deadlock issue. Since the existing code uses the same technique
(using the deferred_qs bit in the union) to prevent the deadlock we were
discussing a few months back. If that is indeed the case, it is good to be
discussing this since we can discuss if the existing code needs any fixing as
well.

> > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > which implies raising softirq will not do any wake ups."  This mention
> > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > 
> > Yes, I should delete this #2 from the changelog since it is not very relevant
> > (I feel now). My point with #2 was that even if were to raise a softirq
> > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > path anyway since in_irq() implies in_interrupt().
> 
> Please!  Could you also add a first-principles explanation of why
> the added condition is immune from scheduler deadlocks?

Sure I can add an example in the change log, however I was thinking of this
example which you mentioned:
https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/

	previous_reader()
	{
		rcu_read_lock();
		do_something(); /* Preemption happened here. */
		local_irq_disable(); /* Cannot be the scheduler! */
		do_something_else();
		rcu_read_unlock();  /* Must defer QS, task still queued. */
		do_some_other_thing();
		local_irq_enable();
	}

	current_reader() /* QS from previous_reader() is still deferred. */
	{
		local_irq_disable();  /* Might be the scheduler. */
		do_whatever();
		rcu_read_lock();
		do_whatever_else();
		rcu_read_unlock();  /* Must still defer reporting QS. */
		do_whatever_comes_to_mind();
		local_irq_enable();
	}

One modification of the example could be, previous_reader() could also do:
	previous_reader()
	{
		rcu_read_lock();
		do_something_that_takes_really_long(); /* causes need_qs in
							  the unlock_special_union to be set */
		local_irq_disable(); /* Cannot be the scheduler! */
		do_something_else();
		rcu_read_unlock();  /* Must defer QS, task still queued. */
		do_some_other_thing();
		local_irq_enable();
	}


thanks!

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
[snip]
> > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > which implies raising softirq will not do any wake ups."  This mention
> > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > 
> > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > path anyway since in_irq() implies in_interrupt().
> > 
> > Please!  Could you also add a first-principles explanation of why
> > the added condition is immune from scheduler deadlocks?
> 
> Sure I can add an example in the change log, however I was thinking of this
> example which you mentioned:
> https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> 
> 	previous_reader()
> 	{
> 		rcu_read_lock();
> 		do_something(); /* Preemption happened here. */
> 		local_irq_disable(); /* Cannot be the scheduler! */
> 		do_something_else();
> 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> 		do_some_other_thing();
> 		local_irq_enable();
> 	}
> 
> 	current_reader() /* QS from previous_reader() is still deferred. */
> 	{
> 		local_irq_disable();  /* Might be the scheduler. */
> 		do_whatever();
> 		rcu_read_lock();
> 		do_whatever_else();
> 		rcu_read_unlock();  /* Must still defer reporting QS. */
> 		do_whatever_comes_to_mind();
> 		local_irq_enable();
> 	}
> 
> One modification of the example could be, previous_reader() could also do:
> 	previous_reader()
> 	{
> 		rcu_read_lock();
> 		do_something_that_takes_really_long(); /* causes need_qs in
> 							  the unlock_special_union to be set */
> 		local_irq_disable(); /* Cannot be the scheduler! */
> 		do_something_else();
> 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> 		do_some_other_thing();
> 		local_irq_enable();
> 	}

The point you were making in that thread being, current_reader() ->
rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
because previous_reader() sets the deferred_qs bit.

Anyway, I will add all of this into the changelog.

thanks,

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> [snip]
> > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > 
> > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > path anyway since in_irq() implies in_interrupt().
> > > 
> > > Please!  Could you also add a first-principles explanation of why
> > > the added condition is immune from scheduler deadlocks?
> > 
> > Sure I can add an example in the change log, however I was thinking of this
> > example which you mentioned:
> > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > 
> > 	previous_reader()
> > 	{
> > 		rcu_read_lock();
> > 		do_something(); /* Preemption happened here. */
> > 		local_irq_disable(); /* Cannot be the scheduler! */
> > 		do_something_else();
> > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > 		do_some_other_thing();
> > 		local_irq_enable();
> > 	}
> > 
> > 	current_reader() /* QS from previous_reader() is still deferred. */
> > 	{
> > 		local_irq_disable();  /* Might be the scheduler. */
> > 		do_whatever();
> > 		rcu_read_lock();
> > 		do_whatever_else();
> > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > 		do_whatever_comes_to_mind();
> > 		local_irq_enable();
> > 	}
> > 
> > One modification of the example could be, previous_reader() could also do:
> > 	previous_reader()
> > 	{
> > 		rcu_read_lock();
> > 		do_something_that_takes_really_long(); /* causes need_qs in
> > 							  the unlock_special_union to be set */
> > 		local_irq_disable(); /* Cannot be the scheduler! */
> > 		do_something_else();
> > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > 		do_some_other_thing();
> > 		local_irq_enable();
> > 	}
> 
> The point you were making in that thread being, current_reader() ->
> rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> because previous_reader() sets the deferred_qs bit.
> 
> Anyway, I will add all of this into the changelog.

Examples are good, but what makes it so that there are no examples of
its being unsafe?

And a few questions along the way, some quick quiz, some more serious.
Would it be safe if it checked in_interrupt() instead of in_irq()?
If not, should the in_interrupt() in the "if" condition preceding the
added "else if" be changed to in_irq()?  Would it make sense to add an
"|| !irqs_were_disabled" do your new "else if" condition?  Would the
body of the "else if" actually be executed in current mainline?

In an attempt to be at least a little constructive, I am doing some
testing of this patch overnight, along with a WARN_ON_ONCE() to see if
that invoke_rcu_core() is ever reached.

							Thanx, Paul

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > [snip]
> > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > 
> > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > path anyway since in_irq() implies in_interrupt().
> > > > 
> > > > Please!  Could you also add a first-principles explanation of why
> > > > the added condition is immune from scheduler deadlocks?
> > > 
> > > Sure I can add an example in the change log, however I was thinking of this
> > > example which you mentioned:
> > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > 
> > > 	previous_reader()
> > > 	{
> > > 		rcu_read_lock();
> > > 		do_something(); /* Preemption happened here. */
> > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > 		do_something_else();
> > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > 		do_some_other_thing();
> > > 		local_irq_enable();
> > > 	}
> > > 
> > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > 	{
> > > 		local_irq_disable();  /* Might be the scheduler. */
> > > 		do_whatever();
> > > 		rcu_read_lock();
> > > 		do_whatever_else();
> > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > 		do_whatever_comes_to_mind();
> > > 		local_irq_enable();
> > > 	}
> > > 
> > > One modification of the example could be, previous_reader() could also do:
> > > 	previous_reader()
> > > 	{
> > > 		rcu_read_lock();
> > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > 							  the unlock_special_union to be set */
> > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > 		do_something_else();
> > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > 		do_some_other_thing();
> > > 		local_irq_enable();
> > > 	}
> > 
> > The point you were making in that thread being, current_reader() ->
> > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > because previous_reader() sets the deferred_qs bit.
> > 
> > Anyway, I will add all of this into the changelog.
> 
> Examples are good, but what makes it so that there are no examples of
> its being unsafe?
> 
> And a few questions along the way, some quick quiz, some more serious.
> Would it be safe if it checked in_interrupt() instead of in_irq()?
> If not, should the in_interrupt() in the "if" condition preceding the
> added "else if" be changed to in_irq()?  Would it make sense to add an
> "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> body of the "else if" actually be executed in current mainline?
> 
> In an attempt to be at least a little constructive, I am doing some
> testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> that invoke_rcu_core() is ever reached.

And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
CFLIST that have CONFIG_PREEMPT=y.)

This of course raises other questions.  But first, do you see that code
executing in your testing?

							Thanx, Paul

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > [snip]
> > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > 
> > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > 
> > > > > Please!  Could you also add a first-principles explanation of why
> > > > > the added condition is immune from scheduler deadlocks?
> > > > 
> > > > Sure I can add an example in the change log, however I was thinking of this
> > > > example which you mentioned:
> > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > 
> > > > 	previous_reader()
> > > > 	{
> > > > 		rcu_read_lock();
> > > > 		do_something(); /* Preemption happened here. */
> > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > 		do_something_else();
> > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > 		do_some_other_thing();
> > > > 		local_irq_enable();
> > > > 	}
> > > > 
> > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > 	{
> > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > 		do_whatever();
> > > > 		rcu_read_lock();
> > > > 		do_whatever_else();
> > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > 		do_whatever_comes_to_mind();
> > > > 		local_irq_enable();
> > > > 	}
> > > > 
> > > > One modification of the example could be, previous_reader() could also do:
> > > > 	previous_reader()
> > > > 	{
> > > > 		rcu_read_lock();
> > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > 							  the unlock_special_union to be set */
> > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > 		do_something_else();
> > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > 		do_some_other_thing();
> > > > 		local_irq_enable();
> > > > 	}
> > > 
> > > The point you were making in that thread being, current_reader() ->
> > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > because previous_reader() sets the deferred_qs bit.
> > > 
> > > Anyway, I will add all of this into the changelog.
> > 
> > Examples are good, but what makes it so that there are no examples of
> > its being unsafe?
> > 
> > And a few questions along the way, some quick quiz, some more serious.
> > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > If not, should the in_interrupt() in the "if" condition preceding the
> > added "else if" be changed to in_irq()?  Would it make sense to add an
> > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > body of the "else if" actually be executed in current mainline?
> > 
> > In an attempt to be at least a little constructive, I am doing some
> > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > that invoke_rcu_core() is ever reached.
> 
> And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> CFLIST that have CONFIG_PREEMPT=y.)
> 
> This of course raises other questions.  But first, do you see that code
> executing in your testing?

Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...

							Thanx, Paul

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > [snip]
> > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > 
> > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > 
> > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > the added condition is immune from scheduler deadlocks?
> > > > > 
> > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > example which you mentioned:
> > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > 
> > > > > 	previous_reader()
> > > > > 	{
> > > > > 		rcu_read_lock();
> > > > > 		do_something(); /* Preemption happened here. */
> > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > 		do_something_else();
> > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > 		do_some_other_thing();
> > > > > 		local_irq_enable();
> > > > > 	}
> > > > > 
> > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > 	{
> > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > 		do_whatever();
> > > > > 		rcu_read_lock();
> > > > > 		do_whatever_else();
> > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > 		do_whatever_comes_to_mind();
> > > > > 		local_irq_enable();
> > > > > 	}
> > > > > 
> > > > > One modification of the example could be, previous_reader() could also do:
> > > > > 	previous_reader()
> > > > > 	{
> > > > > 		rcu_read_lock();
> > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > 							  the unlock_special_union to be set */
> > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > 		do_something_else();
> > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > 		do_some_other_thing();
> > > > > 		local_irq_enable();
> > > > > 	}
> > > > 
> > > > The point you were making in that thread being, current_reader() ->
> > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > because previous_reader() sets the deferred_qs bit.
> > > > 
> > > > Anyway, I will add all of this into the changelog.
> > > 
> > > Examples are good, but what makes it so that there are no examples of
> > > its being unsafe?
> > > 
> > > And a few questions along the way, some quick quiz, some more serious.
> > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > If not, should the in_interrupt() in the "if" condition preceding the
> > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > body of the "else if" actually be executed in current mainline?
> > > 
> > > In an attempt to be at least a little constructive, I am doing some
> > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > that invoke_rcu_core() is ever reached.
> > 
> > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > CFLIST that have CONFIG_PREEMPT=y.)
> > 
> > This of course raises other questions.  But first, do you see that code
> > executing in your testing?
> 
> Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...

So this time I ran the test this way:

tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"

Still no splats.  Though only 10-minute runs instead of the two-hour runs
I did last night.  (Got other stuff I need to do, sorry!)

My test version of your patch is shown below.  Please let me know if I messed
something up.

							Thanx, Paul

------------------------------------------------------------------------

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 2defc7fe74c3..abf2fbba2568 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -621,6 +621,10 @@ static void rcu_read_unlock_special(struct task_struct *t)
 			// Using softirq, safe to awaken, and we get
 			// no help from enabling irqs, unlike bh/preempt.
 			raise_softirq_irqoff(RCU_SOFTIRQ);
+		} else if (exp && in_irq() && !use_softirq &&
+			   !t->rcu_read_unlock_special.b.deferred_qs) {
+			WARN_ON_ONCE(1); // Live code?
+			invoke_rcu_core();
 		} else {
 			// Enabling BH or preempt does reschedule, so...
 			// Also if no expediting or NO_HZ_FULL, slow is OK.

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > [snip]
> > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > 
> > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > 
> > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > 
> > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > example which you mentioned:
> > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > 
> > > > > > 	previous_reader()
> > > > > > 	{
> > > > > > 		rcu_read_lock();
> > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > 		do_something_else();
> > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > 		do_some_other_thing();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > > 
> > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > 	{
> > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > 		do_whatever();
> > > > > > 		rcu_read_lock();
> > > > > > 		do_whatever_else();
> > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > 		do_whatever_comes_to_mind();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > > 
> > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > 	previous_reader()
> > > > > > 	{
> > > > > > 		rcu_read_lock();
> > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > 							  the unlock_special_union to be set */
> > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > 		do_something_else();
> > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > 		do_some_other_thing();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > 
> > > > > The point you were making in that thread being, current_reader() ->
> > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > because previous_reader() sets the deferred_qs bit.
> > > > > 
> > > > > Anyway, I will add all of this into the changelog.
> > > > 
> > > > Examples are good, but what makes it so that there are no examples of
> > > > its being unsafe?
> > > > 
> > > > And a few questions along the way, some quick quiz, some more serious.
> > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > body of the "else if" actually be executed in current mainline?
> > > > 
> > > > In an attempt to be at least a little constructive, I am doing some
> > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > that invoke_rcu_core() is ever reached.
> > > 
> > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > CFLIST that have CONFIG_PREEMPT=y.)
> > > 
> > > This of course raises other questions.  But first, do you see that code
> > > executing in your testing?
> > 
> > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> 
> So this time I ran the test this way:
> 
> tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> 
> Still no splats.  Though only 10-minute runs instead of the two-hour runs
> I did last night.  (Got other stuff I need to do, sorry!)
> 
> My test version of your patch is shown below.  Please let me know if I messed
> something up.
> 
> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 2defc7fe74c3..abf2fbba2568 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -621,6 +621,10 @@ static void rcu_read_unlock_special(struct task_struct *t)
>  			// Using softirq, safe to awaken, and we get
>  			// no help from enabling irqs, unlike bh/preempt.
>  			raise_softirq_irqoff(RCU_SOFTIRQ);
> +		} else if (exp && in_irq() && !use_softirq &&
> +			   !t->rcu_read_unlock_special.b.deferred_qs) {
> +			WARN_ON_ONCE(1); // Live code?
> +			invoke_rcu_core();

The change looks fine to me. I will test it out on my end today as well. It
could also be lack of code coverage for this case (?).

Will get back to you soon on this, thanks!

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Scott Wood]

On Sun, 2019-08-18 at 17:49 -0400, Joel Fernandes (Google) wrote:
> When we're in hard interrupt context in rcu_read_unlock_special(), we
> can still benefit from invoke_rcu_core() doing wake ups of rcuc
> threads when the !use_softirq parameter is passed.  This is safe
> to do so because:

What is the benefit, beyond skipping the irq work overhead?  Is there some
reason to specifically want the rcuc thread woken rather than just getting
into the scheduler (and thus rcu_note_context_switch) as soon as possible?

-Scott

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Mon, Aug 19, 2019 at 07:14:38PM -0500, Scott Wood wrote:
> On Sun, 2019-08-18 at 17:49 -0400, Joel Fernandes (Google) wrote:
> > When we're in hard interrupt context in rcu_read_unlock_special(), we
> > can still benefit from invoke_rcu_core() doing wake ups of rcuc
> > threads when the !use_softirq parameter is passed.  This is safe
> > to do so because:
> 
> What is the benefit, beyond skipping the irq work overhead?  Is there some
> reason to specifically want the rcuc thread woken rather than just getting
> into the scheduler (and thus rcu_note_context_switch) as soon as possible?

Isn't skipping irq work overhead enough of a benefit?

Anyway, I think it is useful in this scenario:
 Consider exp==true when the rcu_read_unlock() is done on a nohz_full CPU.

 If you simply 'get into the scheduler' as you pointed, that is not enough to
 end the grace period. The quiescent state has to be reported up the tree and
 propagated to the root node in the tree. This happens only in 2 places:
 	1. The scheduler tick raising softirq, the end of which will execute the
	   RCU core from the softirq or do the invoke_rcu_core().
	2. The FQS loop which needs to see a dyntick idle transition on the
	   CPU (usermode/idle to kernel or viceversa).

Case 1. is unlikely since the tick may be turned off but I worked last week
        with Paul on turning it on and is doing better.
Case 2. is not happening if we're looping in kernel mode.

In this scenario, calling invoke_rcu_core() directly is better than
scheduling the IRQ work. I don't think the IRQ work will do anything for
nohz_full CPU but I am not sure about that.

To give more background about why I arrived at this patch, I noticed that
this call to invoke_rcu_core() was already being done but it was removed
because the commit removing it said that it is pointless as it does not do
anything. But I think it does do something, that's why I introduced it back.
The rcu_read_unlock_special() is a slow path anyway so one more branch should
be harmless and actually could be beneficial. However, this is just RFC,
please treat it as such. I am running more tests on it based on Paul's
suggestions and looking more closely at it tomorrow.

Thanks!

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > [snip]
> > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > 
> > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > 
> > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > 
> > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > example which you mentioned:
> > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > 
> > > > > > 	previous_reader()
> > > > > > 	{
> > > > > > 		rcu_read_lock();
> > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > 		do_something_else();
> > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > 		do_some_other_thing();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > > 
> > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > 	{
> > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > 		do_whatever();
> > > > > > 		rcu_read_lock();
> > > > > > 		do_whatever_else();
> > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > 		do_whatever_comes_to_mind();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > > 
> > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > 	previous_reader()
> > > > > > 	{
> > > > > > 		rcu_read_lock();
> > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > 							  the unlock_special_union to be set */
> > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > 		do_something_else();
> > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > 		do_some_other_thing();
> > > > > > 		local_irq_enable();
> > > > > > 	}
> > > > > 
> > > > > The point you were making in that thread being, current_reader() ->
> > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > because previous_reader() sets the deferred_qs bit.
> > > > > 
> > > > > Anyway, I will add all of this into the changelog.
> > > > 
> > > > Examples are good, but what makes it so that there are no examples of
> > > > its being unsafe?
> > > > 
> > > > And a few questions along the way, some quick quiz, some more serious.
> > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > body of the "else if" actually be executed in current mainline?
> > > > 
> > > > In an attempt to be at least a little constructive, I am doing some
> > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > that invoke_rcu_core() is ever reached.
> > > 
> > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > CFLIST that have CONFIG_PREEMPT=y.)
> > > 
> > > This of course raises other questions.  But first, do you see that code
> > > executing in your testing?
> > 
> > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> 
> So this time I ran the test this way:
> 
> tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> 
> Still no splats.  Though only 10-minute runs instead of the two-hour runs
> I did last night.  (Got other stuff I need to do, sorry!)
> 
> My test version of your patch is shown below.  Please let me know if I messed
> something up.

I think you also need to pass rcutorture.irqreader=1 ?

Otherwise seems all readers happen in process context AFAICS.

thanks,

 - Joel


> 							Thanx, Paul
> 
> ------------------------------------------------------------------------
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index 2defc7fe74c3..abf2fbba2568 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -621,6 +621,10 @@ static void rcu_read_unlock_special(struct task_struct *t)
>  			// Using softirq, safe to awaken, and we get
>  			// no help from enabling irqs, unlike bh/preempt.
>  			raise_softirq_irqoff(RCU_SOFTIRQ);
> +		} else if (exp && in_irq() && !use_softirq &&
> +			   !t->rcu_read_unlock_special.b.deferred_qs) {
> +			WARN_ON_ONCE(1); // Live code?
> +			invoke_rcu_core();
>  		} else {
>  			// Enabling BH or preempt does reschedule, so...
>  			// Also if no expediting or NO_HZ_FULL, slow is OK.

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Wed, Aug 21, 2019 at 10:38:41AM -0400, Joel Fernandes wrote:
> On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > > [snip]
> > > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > > 
> > > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > > 
> > > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > > 
> > > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > > example which you mentioned:
> > > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > > 
> > > > > > > 	previous_reader()
> > > > > > > 	{
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > 		do_something_else();
> > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > 		do_some_other_thing();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > > 
> > > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > > 	{
> > > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > > 		do_whatever();
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_whatever_else();
> > > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > > 		do_whatever_comes_to_mind();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > > 
> > > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > > 	previous_reader()
> > > > > > > 	{
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > > 							  the unlock_special_union to be set */
> > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > 		do_something_else();
> > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > 		do_some_other_thing();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > 
> > > > > > The point you were making in that thread being, current_reader() ->
> > > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > > because previous_reader() sets the deferred_qs bit.
> > > > > > 
> > > > > > Anyway, I will add all of this into the changelog.
> > > > > 
> > > > > Examples are good, but what makes it so that there are no examples of
> > > > > its being unsafe?
> > > > > 
> > > > > And a few questions along the way, some quick quiz, some more serious.
> > > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > > body of the "else if" actually be executed in current mainline?
> > > > > 
> > > > > In an attempt to be at least a little constructive, I am doing some
> > > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > > that invoke_rcu_core() is ever reached.
> > > > 
> > > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > > CFLIST that have CONFIG_PREEMPT=y.)
> > > > 
> > > > This of course raises other questions.  But first, do you see that code
> > > > executing in your testing?
> > > 
> > > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> > 
> > So this time I ran the test this way:
> > 
> > tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> > 
> > Still no splats.  Though only 10-minute runs instead of the two-hour runs
> > I did last night.  (Got other stuff I need to do, sorry!)
> > 
> > My test version of your patch is shown below.  Please let me know if I messed
> > something up.
> 
> I think you also need to pass rcutorture.irqreader=1 ?
> 
> Otherwise seems all readers happen in process context AFAICS.

Which is the default setting for that, so that's not the issue.

I think one reason could be, in_irq() is false when the timer callback
executes, since the timer callback is executing after a grace-period. The
stack is as follows:

Any reason why we cannot both test for call_rcu() and execute the RCU
callback from the timer hardirq handler?

In fact, I guess on use_nosoftirq systems, the callback will not even run
in softirq context.

[   20.553361]  => rcu_torture_timer_cb
[   20.553361]  => rcu_do_batch
[   20.553361]  => rcu_core
[   20.553361]  => __do_softirq
[   20.553361]  => do_softirq_own_stack
[   20.553361]  => do_softirq.part.16
[   20.553361]  => __local_bh_enable_ip
[   20.553361]  => rcutorture_one_extend
[   20.553361]  => rcu_torture_one_read
[   20.553361]  => rcu_torture_reader
[   20.553361]  => kthread
[   20.553361]  => ret_from_fork


thanks,

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Wed, Aug 21, 2019 at 10:56:17AM -0400, Joel Fernandes wrote:
> On Wed, Aug 21, 2019 at 10:38:41AM -0400, Joel Fernandes wrote:
> > On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> > > On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > > > [snip]
> > > > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > > > 
> > > > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > > > 
> > > > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > > > 
> > > > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > > > example which you mentioned:
> > > > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > > > 
> > > > > > > > 	previous_reader()
> > > > > > > > 	{
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > 		do_something_else();
> > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > 		do_some_other_thing();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > > 
> > > > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > > > 	{
> > > > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > > > 		do_whatever();
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_whatever_else();
> > > > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > > > 		do_whatever_comes_to_mind();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > > 
> > > > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > > > 	previous_reader()
> > > > > > > > 	{
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > > > 							  the unlock_special_union to be set */
> > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > 		do_something_else();
> > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > 		do_some_other_thing();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > 
> > > > > > > The point you were making in that thread being, current_reader() ->
> > > > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > > > because previous_reader() sets the deferred_qs bit.
> > > > > > > 
> > > > > > > Anyway, I will add all of this into the changelog.
> > > > > > 
> > > > > > Examples are good, but what makes it so that there are no examples of
> > > > > > its being unsafe?
> > > > > > 
> > > > > > And a few questions along the way, some quick quiz, some more serious.
> > > > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > > > body of the "else if" actually be executed in current mainline?
> > > > > > 
> > > > > > In an attempt to be at least a little constructive, I am doing some
> > > > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > > > that invoke_rcu_core() is ever reached.
> > > > > 
> > > > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > > > CFLIST that have CONFIG_PREEMPT=y.)
> > > > > 
> > > > > This of course raises other questions.  But first, do you see that code
> > > > > executing in your testing?
> > > > 
> > > > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> > > 
> > > So this time I ran the test this way:
> > > 
> > > tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> > > 
> > > Still no splats.  Though only 10-minute runs instead of the two-hour runs
> > > I did last night.  (Got other stuff I need to do, sorry!)
> > > 
> > > My test version of your patch is shown below.  Please let me know if I messed
> > > something up.
> > 
> > I think you also need to pass rcutorture.irqreader=1 ?
> > 
> > Otherwise seems all readers happen in process context AFAICS.
> 
> Which is the default setting for that, so that's not the issue.
> 
> I think one reason could be, in_irq() is false when the timer callback
> executes, since the timer callback is executing after a grace-period. The
> stack is as follows:
> 
> Any reason why we cannot both test for call_rcu() and execute the RCU
> callback from the timer hardirq handler?
> 
> In fact, I guess on use_nosoftirq systems, the callback will not even run
> in softirq context.
> 
> [   20.553361]  => rcu_torture_timer_cb
> [   20.553361]  => rcu_do_batch
> [   20.553361]  => rcu_core
> [   20.553361]  => __do_softirq
> [   20.553361]  => do_softirq_own_stack
> [   20.553361]  => do_softirq.part.16
> [   20.553361]  => __local_bh_enable_ip
> [   20.553361]  => rcutorture_one_extend
> [   20.553361]  => rcu_torture_one_read
> [   20.553361]  => rcu_torture_reader
> [   20.553361]  => kthread
> [   20.553361]  => ret_from_fork

Oops! wrong stack trace, this is the one where it shows that the timer handler
is running from softirq, not hardirq. Both the rcu_torture_timer() and
rcu_torture_timer_cb() run from softirq context. ftrace confirms:

[   27.949671] rcu_tort-182     8..s1 7268705us : <stack trace>
[   27.949671]  => __ftrace_trace_stack
[   27.949671]  => rcu_torture_timer
[   27.949671]  => call_timer_fn
[   27.949671]  => run_timer_softirq
[   27.949671]  => __do_softirq
[   27.949671]  => irq_exit
[   27.949671]  => smp_apic_timer_interrupt
[   27.949671]  => apic_timer_interrupt
[   27.949671]  => rcutorture_one_extend
[   27.949671]  => rcu_torture_one_read
[   27.949671]  => rcu_torture_reader
[   27.949671]  => kthread
[   27.949671]  => ret_from_fork

So looks like torture testing modifications are called for, to run them in
hard interrupt context as well to provide this additional coverage.. Or am I
way off in the woods?

thanks,

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Wed, Aug 21, 2019 at 10:38:41AM -0400, Joel Fernandes wrote:
> On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> > On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > > [snip]
> > > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > > 
> > > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > > 
> > > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > > 
> > > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > > example which you mentioned:
> > > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > > 
> > > > > > > 	previous_reader()
> > > > > > > 	{
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > 		do_something_else();
> > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > 		do_some_other_thing();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > > 
> > > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > > 	{
> > > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > > 		do_whatever();
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_whatever_else();
> > > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > > 		do_whatever_comes_to_mind();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > > 
> > > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > > 	previous_reader()
> > > > > > > 	{
> > > > > > > 		rcu_read_lock();
> > > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > > 							  the unlock_special_union to be set */
> > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > 		do_something_else();
> > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > 		do_some_other_thing();
> > > > > > > 		local_irq_enable();
> > > > > > > 	}
> > > > > > 
> > > > > > The point you were making in that thread being, current_reader() ->
> > > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > > because previous_reader() sets the deferred_qs bit.
> > > > > > 
> > > > > > Anyway, I will add all of this into the changelog.
> > > > > 
> > > > > Examples are good, but what makes it so that there are no examples of
> > > > > its being unsafe?
> > > > > 
> > > > > And a few questions along the way, some quick quiz, some more serious.
> > > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > > body of the "else if" actually be executed in current mainline?
> > > > > 
> > > > > In an attempt to be at least a little constructive, I am doing some
> > > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > > that invoke_rcu_core() is ever reached.
> > > > 
> > > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > > CFLIST that have CONFIG_PREEMPT=y.)
> > > > 
> > > > This of course raises other questions.  But first, do you see that code
> > > > executing in your testing?
> > > 
> > > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> > 
> > So this time I ran the test this way:
> > 
> > tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> > 
> > Still no splats.  Though only 10-minute runs instead of the two-hour runs
> > I did last night.  (Got other stuff I need to do, sorry!)
> > 
> > My test version of your patch is shown below.  Please let me know if I messed
> > something up.
> 
> I think you also need to pass rcutorture.irqreader=1 ?

Yes, but rcutorture.irqreader=1 is the default:

rcu-torture:--- Start of test: nreaders=7 nfakewriters=4 stat_interval=15 verbose=1 test_no_idle_hz=1 shuffle_interval=3 stutter=5 irqreader=1 fqs_duration=0 fqs_holdoff=0 fqs_stutter=3 test_boost=1/0 test_boost_interval=7 test_boost_duration=4 shutdown_secs=28800 stall_cpu=0 stall_cpu_holdoff=10 stall_cpu_irqsoff=0 n_barrier_cbs=4 onoff_interval=1000 onoff_holdoff=30

Or from the source-code level:

torture_param(int, irqreader, 1, "Allow RCU readers from irq handlers");

> Otherwise seems all readers happen in process context AFAICS.

Yes, should irqreader=0, all readers would happen in process context.

							Thanx, Paul

> thanks,
> 
>  - Joel
> 
> 
> > 							Thanx, Paul
> > 
> > ------------------------------------------------------------------------
> > 
> > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > index 2defc7fe74c3..abf2fbba2568 100644
> > --- a/kernel/rcu/tree_plugin.h
> > +++ b/kernel/rcu/tree_plugin.h
> > @@ -621,6 +621,10 @@ static void rcu_read_unlock_special(struct task_struct *t)
> >  			// Using softirq, safe to awaken, and we get
> >  			// no help from enabling irqs, unlike bh/preempt.
> >  			raise_softirq_irqoff(RCU_SOFTIRQ);
> > +		} else if (exp && in_irq() && !use_softirq &&
> > +			   !t->rcu_read_unlock_special.b.deferred_qs) {
> > +			WARN_ON_ONCE(1); // Live code?
> > +			invoke_rcu_core();
> >  		} else {
> >  			// Enabling BH or preempt does reschedule, so...
> >  			// Also if no expediting or NO_HZ_FULL, slow is OK.

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Wed, Aug 21, 2019 at 10:56:17AM -0400, Joel Fernandes wrote:
> On Wed, Aug 21, 2019 at 10:38:41AM -0400, Joel Fernandes wrote:
> > On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> > > On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > > > [snip]
> > > > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > > > 
> > > > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > > > 
> > > > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > > > 
> > > > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > > > example which you mentioned:
> > > > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > > > 
> > > > > > > > 	previous_reader()
> > > > > > > > 	{
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > 		do_something_else();
> > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > 		do_some_other_thing();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > > 
> > > > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > > > 	{
> > > > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > > > 		do_whatever();
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_whatever_else();
> > > > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > > > 		do_whatever_comes_to_mind();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > > 
> > > > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > > > 	previous_reader()
> > > > > > > > 	{
> > > > > > > > 		rcu_read_lock();
> > > > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > > > 							  the unlock_special_union to be set */
> > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > 		do_something_else();
> > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > 		do_some_other_thing();
> > > > > > > > 		local_irq_enable();
> > > > > > > > 	}
> > > > > > > 
> > > > > > > The point you were making in that thread being, current_reader() ->
> > > > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > > > because previous_reader() sets the deferred_qs bit.
> > > > > > > 
> > > > > > > Anyway, I will add all of this into the changelog.
> > > > > > 
> > > > > > Examples are good, but what makes it so that there are no examples of
> > > > > > its being unsafe?
> > > > > > 
> > > > > > And a few questions along the way, some quick quiz, some more serious.
> > > > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > > > body of the "else if" actually be executed in current mainline?
> > > > > > 
> > > > > > In an attempt to be at least a little constructive, I am doing some
> > > > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > > > that invoke_rcu_core() is ever reached.
> > > > > 
> > > > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > > > CFLIST that have CONFIG_PREEMPT=y.)
> > > > > 
> > > > > This of course raises other questions.  But first, do you see that code
> > > > > executing in your testing?
> > > > 
> > > > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> > > 
> > > So this time I ran the test this way:
> > > 
> > > tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> > > 
> > > Still no splats.  Though only 10-minute runs instead of the two-hour runs
> > > I did last night.  (Got other stuff I need to do, sorry!)
> > > 
> > > My test version of your patch is shown below.  Please let me know if I messed
> > > something up.
> > 
> > I think you also need to pass rcutorture.irqreader=1 ?
> > 
> > Otherwise seems all readers happen in process context AFAICS.
> 
> Which is the default setting for that, so that's not the issue.

Yep!

> I think one reason could be, in_irq() is false when the timer callback
> executes, since the timer callback is executing after a grace-period. The
> stack is as follows:
> 
> Any reason why we cannot both test for call_rcu() and execute the RCU
> callback from the timer hardirq handler?
> 
> In fact, I guess on use_nosoftirq systems, the callback will not even run
> in softirq context.
> 
> [   20.553361]  => rcu_torture_timer_cb
> [   20.553361]  => rcu_do_batch
> [   20.553361]  => rcu_core
> [   20.553361]  => __do_softirq
> [   20.553361]  => do_softirq_own_stack
> [   20.553361]  => do_softirq.part.16
> [   20.553361]  => __local_bh_enable_ip
> [   20.553361]  => rcutorture_one_extend
> [   20.553361]  => rcu_torture_one_read
> [   20.553361]  => rcu_torture_reader
> [   20.553361]  => kthread
> [   20.553361]  => ret_from_fork

Well, it is rcu_read_lock() and rcu_read_unlock() that matters
for this case rather than the callback.  But yes, given in_irq(),
rcu_read_lock() and rcu_read_unlock() would need to have executed
from a hardware interrupt handler.  And would need to get one of the
->rcu_read_lock_special bits set somehow.

But you can use smp_call_function() to invoke a function that runs in
hardware interrupt handler context, and you can do this within either
rcuperf or rcutorture.

And yes, this line of reasoning did inform at least some of my skepticism
surrounding your initial patch, in case you were wondering about some
of my earlier questions.  ;-)

							Thanx, Paul

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Joel Fernandes]

On Wed, Aug 21, 2019 at 08:39:57AM -0700, Paul E. McKenney wrote:
> On Wed, Aug 21, 2019 at 10:56:17AM -0400, Joel Fernandes wrote:
[snip]
> > I think one reason could be, in_irq() is false when the timer callback
> > executes, since the timer callback is executing after a grace-period. The
> > stack is as follows:
> > 
> > Any reason why we cannot both test for call_rcu() and execute the RCU
> > callback from the timer hardirq handler?
> > 
> > In fact, I guess on use_nosoftirq systems, the callback will not even run
> > in softirq context.
> > 
> > [   20.553361]  => rcu_torture_timer_cb
> > [   20.553361]  => rcu_do_batch
> > [   20.553361]  => rcu_core
> > [   20.553361]  => __do_softirq
> > [   20.553361]  => do_softirq_own_stack
> > [   20.553361]  => do_softirq.part.16
> > [   20.553361]  => __local_bh_enable_ip
> > [   20.553361]  => rcutorture_one_extend
> > [   20.553361]  => rcu_torture_one_read
> > [   20.553361]  => rcu_torture_reader
> > [   20.553361]  => kthread
> > [   20.553361]  => ret_from_fork
> 
> Well, it is rcu_read_lock() and rcu_read_unlock() that matters

True!

> for this case rather than the callback.  But yes, given in_irq(),
> rcu_read_lock() and rcu_read_unlock() would need to have executed
> from a hardware interrupt handler.  And would need to get one of the
> ->rcu_read_lock_special bits set somehow.
> 
> But you can use smp_call_function() to invoke a function that runs in
> hardware interrupt handler context, and you can do this within either
> rcuperf or rcutorture.
> 
> And yes, this line of reasoning did inform at least some of my skepticism
> surrounding your initial patch, in case you were wondering about some
> of my earlier questions.  ;-)

Sounds great, I will try to modify the tests to trigger this case and also
look into your other questions. Thanks!!

 - Joel

Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock

[Paul E. McKenney]

On Wed, Aug 21, 2019 at 11:26:38AM -0400, Joel Fernandes wrote:
> On Wed, Aug 21, 2019 at 10:56:17AM -0400, Joel Fernandes wrote:
> > On Wed, Aug 21, 2019 at 10:38:41AM -0400, Joel Fernandes wrote:
> > > On Mon, Aug 19, 2019 at 08:41:43AM -0700, Paul E. McKenney wrote:
> > > > On Mon, Aug 19, 2019 at 07:33:14AM -0700, Paul E. McKenney wrote:
> > > > > On Mon, Aug 19, 2019 at 05:57:57AM -0700, Paul E. McKenney wrote:
> > > > > > On Sun, Aug 18, 2019 at 07:29:27PM -0700, Paul E. McKenney wrote:
> > > > > > > On Sun, Aug 18, 2019 at 09:46:23PM -0400, Joel Fernandes wrote:
> > > > > > > > On Sun, Aug 18, 2019 at 09:41:43PM -0400, Joel Fernandes wrote:
> > > > > > > > > On Sun, Aug 18, 2019 at 06:21:53PM -0700, Paul E. McKenney wrote:
> > > > > > > > [snip]
> > > > > > > > > > > > Also, your commit log's point #2 is "in_irq() implies in_interrupt()
> > > > > > > > > > > > which implies raising softirq will not do any wake ups."  This mention
> > > > > > > > > > > > of softirq seems a bit odd, given that we are going to wake up a rcuc
> > > > > > > > > > > > kthread.  Of course, this did nothing to quell my suspicions.  ;-)
> > > > > > > > > > > 
> > > > > > > > > > > Yes, I should delete this #2 from the changelog since it is not very relevant
> > > > > > > > > > > (I feel now). My point with #2 was that even if were to raise a softirq
> > > > > > > > > > > (which we are not), a scheduler wakeup of ksoftirqd is impossible in this
> > > > > > > > > > > path anyway since in_irq() implies in_interrupt().
> > > > > > > > > > 
> > > > > > > > > > Please!  Could you also add a first-principles explanation of why
> > > > > > > > > > the added condition is immune from scheduler deadlocks?
> > > > > > > > > 
> > > > > > > > > Sure I can add an example in the change log, however I was thinking of this
> > > > > > > > > example which you mentioned:
> > > > > > > > > https://lore.kernel.org/lkml/20190627173831.GW26519@linux.ibm.com/
> > > > > > > > > 
> > > > > > > > > 	previous_reader()
> > > > > > > > > 	{
> > > > > > > > > 		rcu_read_lock();
> > > > > > > > > 		do_something(); /* Preemption happened here. */
> > > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > > 		do_something_else();
> > > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > > 		do_some_other_thing();
> > > > > > > > > 		local_irq_enable();
> > > > > > > > > 	}
> > > > > > > > > 
> > > > > > > > > 	current_reader() /* QS from previous_reader() is still deferred. */
> > > > > > > > > 	{
> > > > > > > > > 		local_irq_disable();  /* Might be the scheduler. */
> > > > > > > > > 		do_whatever();
> > > > > > > > > 		rcu_read_lock();
> > > > > > > > > 		do_whatever_else();
> > > > > > > > > 		rcu_read_unlock();  /* Must still defer reporting QS. */
> > > > > > > > > 		do_whatever_comes_to_mind();
> > > > > > > > > 		local_irq_enable();
> > > > > > > > > 	}
> > > > > > > > > 
> > > > > > > > > One modification of the example could be, previous_reader() could also do:
> > > > > > > > > 	previous_reader()
> > > > > > > > > 	{
> > > > > > > > > 		rcu_read_lock();
> > > > > > > > > 		do_something_that_takes_really_long(); /* causes need_qs in
> > > > > > > > > 							  the unlock_special_union to be set */
> > > > > > > > > 		local_irq_disable(); /* Cannot be the scheduler! */
> > > > > > > > > 		do_something_else();
> > > > > > > > > 		rcu_read_unlock();  /* Must defer QS, task still queued. */
> > > > > > > > > 		do_some_other_thing();
> > > > > > > > > 		local_irq_enable();
> > > > > > > > > 	}
> > > > > > > > 
> > > > > > > > The point you were making in that thread being, current_reader() ->
> > > > > > > > rcu_read_unlock() -> rcu_read_unlock_special() would not do any wakeups
> > > > > > > > because previous_reader() sets the deferred_qs bit.
> > > > > > > > 
> > > > > > > > Anyway, I will add all of this into the changelog.
> > > > > > > 
> > > > > > > Examples are good, but what makes it so that there are no examples of
> > > > > > > its being unsafe?
> > > > > > > 
> > > > > > > And a few questions along the way, some quick quiz, some more serious.
> > > > > > > Would it be safe if it checked in_interrupt() instead of in_irq()?
> > > > > > > If not, should the in_interrupt() in the "if" condition preceding the
> > > > > > > added "else if" be changed to in_irq()?  Would it make sense to add an
> > > > > > > "|| !irqs_were_disabled" do your new "else if" condition?  Would the
> > > > > > > body of the "else if" actually be executed in current mainline?
> > > > > > > 
> > > > > > > In an attempt to be at least a little constructive, I am doing some
> > > > > > > testing of this patch overnight, along with a WARN_ON_ONCE() to see if
> > > > > > > that invoke_rcu_core() is ever reached.
> > > > > > 
> > > > > > And that WARN_ON_ONCE() never triggered in two-hour rcutorture runs of
> > > > > > TREE01, TREE02, TREE03, and TREE09.  (These are the TREE variants in
> > > > > > CFLIST that have CONFIG_PREEMPT=y.)
> > > > > > 
> > > > > > This of course raises other questions.  But first, do you see that code
> > > > > > executing in your testing?
> > > > > 
> > > > > Never mind!  Idiot here forgot the "--bootargs rcutree.use_softirq"...
> > > > 
> > > > So this time I ran the test this way:
> > > > 
> > > > tools/testing/selftests/rcutorture/bin/kvm.sh --cpus 8 --duration 10 --configs "TREE01 TREE02 TREE03 TREE09" --bootargs "rcutree.use_softirq=0"
> > > > 
> > > > Still no splats.  Though only 10-minute runs instead of the two-hour runs
> > > > I did last night.  (Got other stuff I need to do, sorry!)
> > > > 
> > > > My test version of your patch is shown below.  Please let me know if I messed
> > > > something up.
> > > 
> > > I think you also need to pass rcutorture.irqreader=1 ?
> > > 
> > > Otherwise seems all readers happen in process context AFAICS.
> > 
> > Which is the default setting for that, so that's not the issue.
> > 
> > I think one reason could be, in_irq() is false when the timer callback
> > executes, since the timer callback is executing after a grace-period. The
> > stack is as follows:
> > 
> > Any reason why we cannot both test for call_rcu() and execute the RCU
> > callback from the timer hardirq handler?
> > 
> > In fact, I guess on use_nosoftirq systems, the callback will not even run
> > in softirq context.
> > 
> > [   20.553361]  => rcu_torture_timer_cb
> > [   20.553361]  => rcu_do_batch
> > [   20.553361]  => rcu_core
> > [   20.553361]  => __do_softirq
> > [   20.553361]  => do_softirq_own_stack
> > [   20.553361]  => do_softirq.part.16
> > [   20.553361]  => __local_bh_enable_ip
> > [   20.553361]  => rcutorture_one_extend
> > [   20.553361]  => rcu_torture_one_read
> > [   20.553361]  => rcu_torture_reader
> > [   20.553361]  => kthread
> > [   20.553361]  => ret_from_fork
> 
> Oops! wrong stack trace, this is the one where it shows that the timer handler
> is running from softirq, not hardirq. Both the rcu_torture_timer() and
> rcu_torture_timer_cb() run from softirq context. ftrace confirms:
> 
> [   27.949671] rcu_tort-182     8..s1 7268705us : <stack trace>
> [   27.949671]  => __ftrace_trace_stack
> [   27.949671]  => rcu_torture_timer
> [   27.949671]  => call_timer_fn
> [   27.949671]  => run_timer_softirq
> [   27.949671]  => __do_softirq
> [   27.949671]  => irq_exit
> [   27.949671]  => smp_apic_timer_interrupt
> [   27.949671]  => apic_timer_interrupt
> [   27.949671]  => rcutorture_one_extend
> [   27.949671]  => rcu_torture_one_read
> [   27.949671]  => rcu_torture_reader
> [   27.949671]  => kthread
> [   27.949671]  => ret_from_fork
> 
> So looks like torture testing modifications are called for, to run them in
> hard interrupt context as well to provide this additional coverage.. Or am I
> way off in the woods?

That actually might be worth doing.

The reason I didn't bother is that in the common case, timer softirq
generates exactly the same race conditions as would a hard interrupt
handler.  You can see this in your stack trace, where the call is
coming from irq_exit(), that is, from the trailing edge of a hardware
interrupt.

							Thanx, Paul