Received-SPF: pass (google.com: domain of linux-kernel+bounces-94379-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Wed, 6 Mar 2024 10:09:48 -0800
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: linke li <lilinke99@qq.com>, joel@joelfernandes.org,
	boqun.feng@gmail.com, dave@stgolabs.net, frederic@kernel.org,
	jiangshanlai@gmail.com, josh@joshtriplett.org,
	linux-kernel@vger.kernel.org, mathieu.desnoyers@efficios.com,
	qiang.zhang1211@gmail.com, quic_neeraju@quicinc.com,
	rcu@vger.kernel.org, Linus Torvalds <torvalds@linux-foundation.org>
Subject: Re: [PATCH] rcutorture: Fix
 rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency
 bug
Message-ID: <140c2d21-1d52-4c46-bbdd-f7b4b7eabbff@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <f3624f39-bbb1-451d-8161-8518e4108d8e@joelfernandes.org>
 <tencent_9882B228F292088CDD68F10CF1C228742009@qq.com>
 <20240306103719.1d241b93@gandalf.local.home>
 <27665890-8314-4252-8622-1e019fee27e4@paulmck-laptop>
 <20240306130103.6da71ddf@gandalf.local.home>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240306130103.6da71ddf@gandalf.local.home>

On Wed, Mar 06, 2024 at 01:01:03PM -0500, Steven Rostedt wrote:
> On Wed, 6 Mar 2024 09:36:16 -0800
> "Paul E. McKenney" <paulmck@kernel.org> wrote:
> 
> > > If we take the policy of handling a compiler that can tear reads and writes
> > > of any size word, then we should have proper macros to handle it.  
> > 
> > Those are in fact READ_ONCE() and WRITE_ONCE() when given machine-word
> > sized/aligned variables.
> 
> IIRC, the original purpose of READ_ONCE() and WRITE_ONCE() was to make sure
> that the compiler only reads or writes the variable "once". Hence the name.
> That way after a load, you don't need to worry that the content of the
> variable you read isn't going to be read again from the original location
> because the compiler decided to save stack space and registers.
> 
> But that macro has now been extended for other purposes.

If I remember correctly, some 32-bit system had 64-bit PTEs that it
wanted to use WRITE_ONCE() on.  Does Linux still support that system?
If not, maybe it is time to remove that extension.

> > > Perhaps READ_SHARED(), WRITE_SHARED(), ADD_SHARED(), SUB_SHARED(). The ONCE
> > > has nothing to do with the reasons for these changes. But at least "SHARED"
> > > can be considered "this variable is shared between different contexts".
> > > Note, this is different than "atomic". It's just to document that this
> > > variable must be loaded or stored in one transaction.  
> > 
> > We already have READ_ONCE() and WRITE_ONCE().  An ADD_SHARED() might
> > be useful, though compilers are starting to learn how to emit good code
> > for things like WRITE_ONCE(a, READ_ONCE(a) + 1).
> 
> Well, if we keep the _ONCE() naming, it should be ADD_ONCE(). Because
> 
>   WRITE_ONCE(a, READ_ONCE(a) + 1)
> 
> is an abomination and should only be present in obfuscation contests.

I have no problem with replacing that sort of thing with ADD_ONCE().

> > But such things should also be documented and added to LKMM.
> > 
> > > I don't know if Linus even cares about fixing "read/write tearing" which is
> > > why I Cc'd him.  
> > 
> > I am sure that whatever his views, he will not suffer in silence.  ;-)
> > 
> > > But I'm not going to take any patches that add these macros to fix
> > > compilers that tear words on load and store until we have a set policy on
> > > what to do with them.  
> > 
> > Maintainer's choice!
> > 
> > For RCU, I want the code to just work with future compiler optimizations
> > as well as with current ones.  This stuff is fun enough without giving
> > the compiler opportunities for more mischief!
> 
> I'm not against the changes. I'm against the ugliness of the changes.
> Should we just create a ADD_ONCE() macro?

Works for me!  We should also update tools/memory-model/linux-kernel.defs
to allow it to be used in litmus tests.  (I can help with that.)

Plus of course documentation.

> If the approach is now to find all places that access a variable between
> different contexts, and create READ_ONCE()/WRITE_ONCE() around them, I'm
> fine with it.

I don't know that the entire kernel is going that far, but RCU has had
that philosophy for a very long time.  Yes, KCSAN sometimes finds places
where we slipped up, but those get fixed.

> Perhaps we need a way to annotate them, like we have with __rcu. "__shared"?
> 
> Then all accesses to that variable must be wrapped with a READ_ONCE() or
> WRITE_ONCE()? I mean, if this can cause legitimate bugs, we should probably
> address it like we do with locking and RCU.

If we want that, just mark the field "volatile", as in "jiffies".

And one of the strengths of READ_ONCE() and WRITE_ONCE() is that they
allow non-volatile access where it is safe.  For example, if you hold the
lock protecting all stores to that variable, you still need WRITE_ONCE()
but not READ_ONCE().  In initialization and cleanup code, you don't
need either.

							Thanx, Paul