[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <140c2d21-1d52-4c46-bbdd-f7b4b7eabbff@paulmck-laptop>
Date: Wed, 6 Mar 2024 10:09:48 -0800
From: "Paul E. McKenney" <paulmck@...nel.org>
To: Steven Rostedt <rostedt@...dmis.org>
Cc: linke li <lilinke99@...com>, joel@...lfernandes.org,
boqun.feng@...il.com, dave@...olabs.net, frederic@...nel.org,
jiangshanlai@...il.com, josh@...htriplett.org,
linux-kernel@...r.kernel.org, mathieu.desnoyers@...icios.com,
qiang.zhang1211@...il.com, quic_neeraju@...cinc.com,
rcu@...r.kernel.org, Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH] rcutorture: Fix
rcu_torture_pipe_update_one()/rcu_torture_writer() data race and concurrency
bug
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@...nel.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
Powered by blists - more mailing lists