Message-ID: <20200217232307.GA17570@paulmck-ThinkPad-P72>
Date:   Mon, 17 Feb 2020 15:23:07 -0800
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Lai Jiangshan <laijs@...ux.alibaba.com>
Cc:     Boqun Feng <boqun.feng@...il.com>, linux-kernel@...r.kernel.org,
        Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>, rcu@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Borislav Petkov <bp@...en8.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [PATCH V2 2/7] rcu: cleanup rcu_preempt_deferred_qs()

On Tue, Nov 12, 2019 at 09:28:37AM +0800, Lai Jiangshan wrote:
> 
> 
> On 2019/11/11 10:32 下午, Paul E. McKenney wrote:
> > On Mon, Nov 04, 2019 at 11:19:11PM -0800, Paul E. McKenney wrote:
> > > On Tue, Nov 05, 2019 at 10:09:15AM +0800, Lai Jiangshan wrote:
> > > > On 2019/11/4 10:55 下午, Paul E. McKenney wrote:
> > > > > On Sun, Nov 03, 2019 at 01:01:21PM +0800, Lai Jiangshan wrote:
> > > > > > 
> > > > > > 
> > > > > > On 2019/11/3 10:01 上午, Boqun Feng wrote:
> > > > > > > Hi Jiangshan,
> > > > > > > 
> > > > > > > 
> > > > > > > I haven't checked the correctness of this patch carefully, but..
> > > > > > > 
> > > > > > > 
> > > > > > > On Sat, Nov 02, 2019 at 12:45:54PM +0000, Lai Jiangshan wrote:
> > > > > > > > Don't need to set ->rcu_read_lock_nesting negative, irq-protected
> > > > > > > > rcu_preempt_deferred_qs_irqrestore() doesn't expect
> > > > > > > > ->rcu_read_lock_nesting to be negative to work, it even
> > > > > > > > doesn't access to ->rcu_read_lock_nesting any more.
> > > > > > > 
> > > > > > > rcu_preempt_deferred_qs_irqrestore() will report RCU qs, and may
> > > > > > > eventually call swake_up() or its friends to wake up, say, the gp
> > > > > > > kthread, and the wake up functions could go into the scheduler code
> > > > > > > path which might have RCU read-side critical section in it, IOW,
> > > > > > > accessing ->rcu_read_lock_nesting.
> > > > > > 
> > > > > > Sure, thank you for pointing it out.
> > > > > > 
> > > > > > I should rewrite the changelog in next round. Like this:
> > > > > > 
> > > > > > rcu: cleanup rcu_preempt_deferred_qs()
> > > > > > 
> > > > > > IRQ-protected rcu_preempt_deferred_qs_irqrestore() itself doesn't
> > > > > > expect ->rcu_read_lock_nesting to be negative to work.
> > > > > > 
> > > > > > There might be RCU read-side critical section in it (from wakeup()
> > > > > > or so), 1711d15bf5ef(rcu: Clear ->rcu_read_unlock_special only once)
> > > > > > will ensure that ->rcu_read_unlock_special is zero and these RCU
> > > > > > read-side critical sections will not call rcu_read_unlock_special().
> > > > > > 
> > > > > > Thanks
> > > > > > Lai
> > > > > > 
> > > > > > ===
> > > > > > PS: Were 1711d15bf5ef(rcu: Clear ->rcu_read_unlock_special only once)
> > > > > > not applied earlier, it will be protected by previous patch (patch1)
> > > > > > in this series
> > > > > > "rcu: use preempt_count to test whether scheduler locks is held"
> > > > > > when rcu_read_unlock_special() is called.
> > > > > 
> > > > > This one in -rcu, you mean?
> > > > > 
> > > > > 5c5d9065e4eb ("rcu: Clear ->rcu_read_unlock_special only once")
> > > > 
> > > > Yes, but the commit ID is floating in the tree.
> > > 
> > > Indeed, that part of -rcu is subject to rebase, and will continue
> > > to be until about v5.5-rc5 or thereabouts.
> > > 
> > > https://mirrors.edge.kernel.org/pub/linux/kernel/people/paulmck/rcutodo.html
> > > 
> > > My testing of your full stack should be complete by this coming Sunday
> > > morning, Pacific Time.
> > 
> > And you will be happy to hear that it ran the full time without errors.
> > 
> > Good show!!!
> > 
> > My next step is to look much more carefully at the remaining patches,
> > checking my first impressions.  This will take a few days.
> > 
> 
> Hi, All
> 
> I'm still asking for more comments.
> 
> By now, I have received some precious comments, mainly due to my
> stupid naming mistakes and a misleading changelog. I should have
> updated all these with a new series patches. But I hope I
> can polish more in the new patchset with more suggestions from
> valuable comments, especially in x86,scheduler,percpu and rcu
> areas.
> 
> I'm very obliged to hear anything.

Hearing no objections, and having more confidence in other commits that
(allegedly) guarantee forward progress for nohz_full CPUs, i have pulled
this into -rcu for testing and further review.  Please see below for the
updated patch.

							Thanx, Paul

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

commit 23a58acde0eea57ac77377e5d50d9562b2dbdfaa
Author: Lai Jiangshan <laijs@...ux.alibaba.com>
Date:   Sat Feb 15 14:37:26 2020 -0800

    rcu: Don't set nesting depth negative in rcu_preempt_deferred_qs()
    
    Now that RCU flavors have been consolidated, an RCU-preempt
    rcu_rea_unlock() in an interrupt or softirq handler cannot possibly
    end the RCU read-side critical section.  Consider the old vulnerability
    involving rcu_preempt_deferred_qs() being invoked within such a handler
    that interrupted an extended RCU read-side critical section, in which
    a wakeup might be invoked with a scheduler lock held.  Because
    rcu_read_unlock_special() no longer does wakeups in such situations,
    it is no longer necessary for rcu_preempt_deferred_qs() to set the
    nesting level negative.
    
    This commit therfore removes this recursion-protection code from
    rcu_preempt_deferred_qs().
    
    Signed-off-by: Lai Jiangshan <laijs@...ux.alibaba.com>
    Signed-off-by: Paul E. McKenney <paulmck@...nel.org>

diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 4bd062a..118407f8 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -569,16 +569,11 @@ static bool rcu_preempt_need_deferred_qs(struct task_struct *t)
 static void rcu_preempt_deferred_qs(struct task_struct *t)
 {
 	unsigned long flags;
-	bool couldrecurse = rcu_preempt_depth() >= 0;
 
 	if (!rcu_preempt_need_deferred_qs(t))
 		return;
-	if (couldrecurse)
-		rcu_preempt_depth_set(rcu_preempt_depth() - RCU_NEST_BIAS);
 	local_irq_save(flags);
 	rcu_preempt_deferred_qs_irqrestore(t, flags);
-	if (couldrecurse)
-		rcu_preempt_depth_set(rcu_preempt_depth() + RCU_NEST_BIAS);
 }
 
 /*

Powered by blists - more mailing lists