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@xxxxxxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxx>
Signed-off-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
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);
}
/*
