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

[Paul E. McKenney]

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@xxxxxxxxxx> 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