Re: [PATCH V2 2/7] rcu: cleanup rcu_preempt_deferred_qs()

From: "Paul E. McKenney" <paulmck@kernel.org>
To: Lai Jiangshan <laijs@linux.alibaba.com>
Cc: Boqun Feng <boqun.feng@gmail.com>,
	linux-kernel@vger.kernel.org,
	Josh Triplett <josh@joshtriplett.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	rcu@vger.kernel.org
Subject: Re: [PATCH V2 2/7] rcu: cleanup rcu_preempt_deferred_qs()
Date: Mon, 11 Nov 2019 06:32:38 -0800	[thread overview]
Message-ID: <20191111143238.GA13306@paulmck-ThinkPad-P72> (raw)
In-Reply-To: <20191105071911.GL20975@paulmck-ThinkPad-P72>

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.

							Thanx, Paul