linux-kernel - Re: [PATCH 2/3] rcu/exp: Remove confusing needless full barrier on task unblock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <63cbab19-a7d4-4daa-8b54-58665e159e23@paulmck-laptop>
Date: Wed, 26 Feb 2025 07:04:28 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: Joel Fernandes <joelagnelf@...dia.com>,
	LKML <linux-kernel@...r.kernel.org>,
	Boqun Feng <boqun.feng@...il.com>,
	Joel Fernandes <joel@...lfernandes.org>,
	Neeraj Upadhyay <neeraj.upadhyay@....com>,
	Uladzislau Rezki <urezki@...il.com>,
	Zqiang <qiang.zhang1211@...il.com>, rcu <rcu@...r.kernel.org>
Subject: Re: [PATCH 2/3] rcu/exp: Remove confusing needless full barrier on
 task unblock

On Wed, Feb 26, 2025 at 01:52:09PM +0100, Frederic Weisbecker wrote:
> Le Tue, Feb 25, 2025 at 04:59:08PM -0500, Joel Fernandes a écrit :
> > On Fri, Feb 14, 2025 at 12:25:58AM +0100, Frederic Weisbecker wrote:
> > > A full memory barrier in the RCU-PREEMPT task unblock path advertizes
> > > to order the context switch (or rather the accesses prior to
> > > rcu_read_unlock()) with the expedited grace period fastpath.
> > > 
> > > However the grace period can not complete without the rnp calling into
> > > rcu_report_exp_rnp() with the node locked. This reports the quiescent
> > > state in a fully ordered fashion against updater's accesses thanks to:
> > > 
> > > 1) The READ-SIDE smp_mb__after_unlock_lock() barrier accross nodes
> > >    locking while propagating QS up to the root.
> > > 
> > > 2) The UPDATE-SIDE smp_mb__after_unlock_lock() barrier while holding the
> > >    the root rnp to wait/check for the GP completion.
> > > 
> > > 3) The (perhaps redundant given step 1) and 2)) smp_mb() in rcu_seq_end()
> > >    before the grace period completes.
> > > 
> > > This makes the explicit barrier in this place superflous. Therefore
> > > remove it as it is confusing.
> > > 
> > > Signed-off-by: Frederic Weisbecker <frederic@...nel.org>
> > > ---
> > >  kernel/rcu/tree_plugin.h | 1 -
> > >  1 file changed, 1 deletion(-)
> > > 
> > > diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> > > index 3c0bbbbb686f..d51cc7a5dfc7 100644
> > > --- a/kernel/rcu/tree_plugin.h
> > > +++ b/kernel/rcu/tree_plugin.h
> > > @@ -534,7 +534,6 @@ rcu_preempt_deferred_qs_irqrestore(struct task_struct *t, unsigned long flags)
> > >  		WARN_ON_ONCE(rnp->completedqs == rnp->gp_seq &&
> > >  			     (!empty_norm || rnp->qsmask));
> > >  		empty_exp = sync_rcu_exp_done(rnp);
> > > -		smp_mb(); /* ensure expedited fastpath sees end of RCU c-s. */
> > 
> > I was wondering though, this is a slow path and the smp_mb() has been there
> > since 2009 or so. Not sure if it is super valuable to remove it at this
> > point. But we/I should definitely understand it.
> 
> The point is indeed not to improve performance because this is a slowpath
> (although...). The main goal is to maintain a clear picture of the ordering
> without needless barriers that leave a taste of doubt to reviewers.

Agreed!

> > I was wondering if you could also point to the fastpath that this is racing
> > with, it is not immediately clear (to me) what this smp_mb() is pairing with
> > :(
> 
> It is supposed to pair with the barrier in sync_exp_work_done() but then again
> this is already enforced by the smp_mb__after_unlock_lock() chained through
> rnp locking.

You could interpret that "Order GP completion with preceding accesses"
to include preceding readers, which to your point sounds plausible.
And in that case, again as you say, the raw_spin_lock_irqsave_rcu_node()
in rcu_report_exp_rnp() provides the needed ordering.

I think.  ;-)

							Thanx, Paul

> Thanks.
> 
> > 
> > thanks,
> > 
> >  - Joel
> > 
> > 
> > 
> > 
> > 
> > >  		np = rcu_next_node_entry(t, rnp);
> > >  		list_del_init(&t->rcu_node_entry);
> > >  		t->rcu_blocked_node = NULL;
> > > -- 
> > > 2.46.0
> > >