Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates
From: Paul E. McKenney
Date: Sat Aug 17 2019 - 18:29:28 EST
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;
> 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
> 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
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.