RCU Archive on lore.kernel.org
 help / color / Atom feed
From: Joel Fernandes <joel@joelfernandes.org>
To: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: linux-kernel@vger.kernel.org,
	Josh Triplett <josh@joshtriplett.org>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	rcu@vger.kernel.org, Steven Rostedt <rostedt@goodmis.org>
Subject: Re: [RFC v2] rcu/tree: Try to invoke_rcu_core() if in_irq() during unlock
Date: Sun, 18 Aug 2019 19:38:39 -0400
Message-ID: <20190818233839.GA160903@google.com> (raw)
In-Reply-To: <20190818233135.GQ28441@linux.ibm.com>

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

  reply index

Thread overview: 23+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-18 21:49 Joel Fernandes (Google)
2019-08-18 22:12 ` Paul E. McKenney
2019-08-18 22:32   ` Joel Fernandes
2019-08-18 22:35     ` Joel Fernandes
2019-08-18 23:31       ` Paul E. McKenney
2019-08-18 23:38         ` Joel Fernandes [this message]
2019-08-19  1:21           ` Paul E. McKenney
2019-08-19  1:41             ` Joel Fernandes
2019-08-19  1:46               ` Joel Fernandes
2019-08-19  2:29                 ` Paul E. McKenney
2019-08-19 12:57                   ` Paul E. McKenney
2019-08-19 14:33                     ` Paul E. McKenney
2019-08-19 15:41                       ` Paul E. McKenney
2019-08-19 16:25                         ` Joel Fernandes
2019-08-21 14:38                         ` Joel Fernandes
2019-08-21 14:56                           ` Joel Fernandes
2019-08-21 15:26                             ` Joel Fernandes
2019-08-21 15:47                               ` Paul E. McKenney
2019-08-21 15:39                             ` Paul E. McKenney
2019-08-21 15:46                               ` Joel Fernandes
2019-08-21 15:26                           ` Paul E. McKenney
2019-08-20  0:14 ` Scott Wood
2019-08-20  1:40   ` Joel Fernandes

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190818233839.GA160903@google.com \
    --to=joel@joelfernandes.org \
    --cc=jiangshanlai@gmail.com \
    --cc=josh@joshtriplett.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=paulmck@linux.ibm.com \
    --cc=rcu@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

RCU Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/rcu/0 rcu/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 rcu rcu/ https://lore.kernel.org/rcu \
		rcu@vger.kernel.org
	public-inbox-index rcu

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.rcu


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git