linux-kernel - Re: [PATCH 5/6] rcu: Remove full memory barrier on RCU stall printout

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: Windows password security audit tool. GUI, reports in PDF.

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

Date: Tue, 4 Jun 2024 07:00:53 -0700
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Frederic Weisbecker <frederic@...nel.org>
Cc: LKML <linux-kernel@...r.kernel.org>,
	Valentin Schneider <vschneid@...hat.com>,
	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 5/6] rcu: Remove full memory barrier on RCU stall printout

On Tue, Jun 04, 2024 at 01:13:25PM +0200, Frederic Weisbecker wrote:
> Le Mon, Jun 03, 2024 at 05:10:54PM -0700, Paul E. McKenney a écrit :
> > On Wed, May 15, 2024 at 02:53:31PM +0200, Frederic Weisbecker wrote:
> > > RCU stall printout fetches the EQS state of a CPU with a preceding full
> > > memory barrier. However there is nothing to order this read against at
> > > this debugging stage. It is inherently racy when performed remotely.
> > > 
> > > Do a plain read instead.
> > > 
> > > This was the last user of rcu_dynticks_snap().
> > > 
> > > Signed-off-by: Frederic Weisbecker <frederic@...nel.org>
> > 
> > I went through all of these, and the look good.  Though I am a bit
> > nervous about this one.  The RCU CPU stall warning code used to be
> > completely unordered, but the hardware taught me better.  I did not
> > add these in response to a problem (just lazily used the existing fully
> > ordered primitive), but you never know.
> 
> At least I haven't found against what it is ordering the dynticks counter here.
> 
> > Me, I would have kept the extra
> > memory barriers in all six patches because they are not on a fastpath,
> 
> It is still time to discard the patches :-)

And there is also still time for you to add comments.  ;-)

> > but you are quite correct that they are redundant.
> 
> Yes and it's not so much for optimization purpose, like you said it's
> not a fast-path, although in the case of fqs round scan it _might_ be
> debatable in the presence of hurry callbacks, but I use those changes
> more for documentation purpose. My opinion on that being that having
> memory barriers when they are not necessary doesn't help reviewers and
> doesn't bring the incentive to actually verify that the ordering is
> correct when it is really required, since there is so much of it
> everywhere anyway. I'd rather have a clear, well visible and precise
> picture. But that's just personal belief.

Redundant memory barriers can be OK, but only if they make the algorithm
easier to understand, as we found in SRCU.  It is not clear that these
fit that bill, or, alternatively, that appropriate comments wouldn't be
an improvement over the redundant memory barrier.

> > So I have queued these, and intend to send them into the next merge
> > window.  However, you now own vanilla RCU grace-period memory ordering,
> > both normal and expedited.  As in if someone else breaks it, you already
> > bought it.  ;-)
> 
> Sure, but it's a bet. That one day a younger person will buy it from me
> double the price ;-)

;-) ;-) ;-)

							Thanx, Paul