Re: [PATCH] rcutorture: Fix rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency bug

From: Steven Rostedt <rostedt@goodmis.org>
To: "Paul E. McKenney" <paulmck@kernel.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
Date: Wed, 6 Mar 2024 13:01:03 -0500	[thread overview]
Message-ID: <20240306130103.6da71ddf@gandalf.local.home> (raw)
In-Reply-To: <27665890-8314-4252-8622-1e019fee27e4@paulmck-laptop>

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.

> 
> > 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.

> 
> 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?

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.

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.

-- Steve