Date:   Sat, 17 Aug 2019 15:28:34 -0700
From:   "Paul E. McKenney" <paulmck@...ux.ibm.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Valentin Schneider <valentin.schneider@....com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Alan Stern <stern@...land.harvard.edu>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        rostedt <rostedt@...dmis.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Peter Zijlstra <peterz@...radead.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Will Deacon <will.deacon@....com>,
        David Howells <dhowells@...hat.com>
Subject: Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates

On Sat, Aug 17, 2019 at 01:28:48AM -0700, Linus Torvalds wrote:

[ . . . ]

> Put another way: a WRITE_ONCE() without a paired READ_ONCE() is almost
> certainly pointless.

"Your honor, I have no further questions at this time, but I reserve
the right to recall this witness."

Outside of things like MMIO (where one could argue that the corresponding
READ_ONCE() is in the device firmware), the use cases I can imagine
for WRITE_ONCE() with no READ_ONCE() are quite strange.  For example,
doing the WRITE_ONCE()s while read-holding a given lock and doing plain
reads while write-holding that same lock.  While at the same time being
worried about store tearing and similar.

Perhaps I am suffering a failure of imagination, but I am not seeing
a reasonable use for such things at the moment.

> But the reverse is not really true. All a READ_ONCE() says is "I want
> either the old or the new value", and it can get that _without_ being
> paired with a WRITE_ONCE().
> 
> See? They just aren't equally important.
> 
> > > And yes, reads are different from writes. Reads don't have the same
> > > kind of "other threads of execution can see them" effects, so a
> > > compiler turning a single read into multiple reads is much more
> > > realistic and not the same kind of "we need to expect a certain kind
> > > of sanity from the compiler" issue.
> >
> > Though each of those compiler-generated multiple reads might return a
> > different value, right?
> 
> Right. See the examples I have in the email to Mathieu:
> 
>    unsigned int bits = some_global_value;
>    ...test different bits in in 'bits' ...
> 
> can easily cause multiple reads (particularly on a CPU that has a
> "test bits in memory" instruction and a lack of registers.
> 
> So then doing it as
> 
>    unsigned int bits = READ_ONCE(some_global_value);
>    .. test different bits in 'bits'...
> 
> makes a real and obvious semantic difference. In ways that changing a one-time
> 
>    ptr->flag = true;
> 
> to
> 
>    WRITE_ONCE(ptr->flag, true);
> 
> does _not_ make any obvious semantic difference what-so-ever.

Agreed, especially given that only one bit of ->flag is most likely
ever changing.

> Caching reads is also something that makes sense and is common, in
> ways that caching writes does not. So doing
> 
>     while (in_progress_global) /* twiddle your thumbs */;
> 
> obviously trivially translates to an infinite loop with a single
> conditional in front of it, in ways that
> 
>    while (READ_ONCE(in_progress_global)) /* twiddle */;
> 
> does not.
> 
> So there are often _obvious_ reasons to use READ_ONCE().
> 
> I really do not find the same to be true of WRITE_ONCE(). There are
> valid uses, but they are definitely much less common, and much less
> obvious.

Agreed, and I expect READ_ONCE() to continue to be used more heavily than
is WRITE_ONCE(), even including the documentation-only WRITE_ONCE() usage.

							Thanx, Paul

Powered by blists - more mailing lists