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@xxxxxxxxxxxxxxxxx>
> > > > 
> > > > 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
> >