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

["Paul E. McKenney" <paulmck@kernel.org>]

On Wed, Mar 06, 2024 at 10:37:19AM -0500, Steven Rostedt wrote:
> On Tue,  5 Mar 2024 14:24:20 +0800
> linke li <lilinke99@qq.com> wrote:
> 
> > > Anyway, a slightly related/different question:
> > > 
> > > Should that:
> > > WRITE_ONCE(rp->rtort_pipe_count, rp->rtort_pipe_count + 1);
> > > 
> > > Be:
> > > WRITE_ONCE(rp->rtort_pipe_count, READ_ONCE(rp->rtort_pipe_count) + 1);
> > > 
> > > ?  
> > 
> > Hi, Joel. Sorry, I am not very sure about this. I do a little research on
> > it.
> > 
> > I looked through all code in kernel that looks like this, both kinds are
> > used. I also try to compile this file with and without the READ_ONCE in
> > WRITE_ONCE using gcc-9. Their binary is just the same. 
> > 
> > Thus I think in the current compiler version, they do not have a big
> > difference, but if the compiler wants to optimize more, maybe the latter
> > one is better.
> 
> I'm sorry but all these READ_ONCE/WRITE_ONCE() additions that are being
> added because there's a fear of the compiler tearing long words, is going to
> make the code really ugly and hard to read.

There are quite a few other things to fear besides tearing.  The compiler
can and does invent and fuse normal loads, and it can and does fuse
normal stores.  And there really are compilers that tear stores of
certain constants on systems with short immediate fields.

I would argue that use of normal C-language loads and stores should be
accompanied by comments saying why the compiler cannot mess things up.
But maintainer's choice.  Those maintainers who keep a close personal
relationship with the ever-growing list of optimizations should have
no problem working out which use cases are safe for normal C-language
loads and stores.  Me, I would really rather play it safe, especially
in the innards of something like RCU.  ;-)

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

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

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!

							Thanx, Paul