Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates

From: Linus Torvalds <torvalds@linux-foundation.org>
To: "Paul E. McKenney" <paulmck@linux.ibm.com>
Cc: Valentin Schneider <valentin.schneider@arm.com>,
	Joel Fernandes <joel@joelfernandes.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	Alan Stern <stern@rowland.harvard.edu>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	rostedt <rostedt@goodmis.org>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Will Deacon <will.deacon@arm.com>,
	David Howells <dhowells@redhat.com>
Subject: Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates
Date: Sat, 17 Aug 2019 01:28:48 -0700	[thread overview]
Message-ID: <CAHk-=wiOhiAJVU71968tAND6rrEJSaYPg7DXK6Y6iiz7_RJACw@mail.gmail.com> (raw)
In-Reply-To: <20190817045217.GZ28441@linux.ibm.com>

On Fri, Aug 16, 2019 at 9:52 PM Paul E. McKenney <paulmck@linux.ibm.com> wrote:
>
> > I'd love to have a flag that says "all undefined behavior is treated
> > as implementation-defined". There's a somewhat subtle - but very
> > important - difference there.
>
> It would also be nice to downgrade some of the undefined behavior in
> the standard itself.  Compiler writers usually hate this because it
> would require them to document what their compiler does in each case.
> So they would prefer "unspecified" if the could not have "undefined".

That certainly would sound very good to me.

It's not the "documented behavior" that is important to me (since it
is still going to be potentially different across different
platforms), it's the "at least it's *some* consistent behavior for
that platform".

That's just _so_ much better than "undefined behavior" which basically
says the compiler writer can do absolutely anything, whether it makes
sense or not, and whether it might open a small bug up to absolutely
catastrophic end results.

> >    if (a)
> >       global_var = 1
> >    else
> >       global_var = 0
>
> Me, I would still want to use WRITE_ONCE() in this case.

I actually suspect that if we had this kind of code, and a real reason
why readers would see it locklessly, then yes, WRITE_ONCE() makes
sense.

But most of the cases where people add WRITE_ONCE() aren't even the
above kind of half-questionable cases. They are just literally

        WRITE_ONCE(flag, value);

and since there is no real memory ordering implied by this, what's the
actual value of that statement? What problem does it really solve wrt
just writing it as

        flag = value;

which is generally a lot easier to read.

If the flag has semantic behavior wrt other things, and the write can
race with a read (whether it's the read or the write that is unlocked
is irrelevant), is still doesn't tend to make a lot of real
difference.

In the end, the most common reason for a WRITE_ONCE() is mostly just
"to visually pair up with the non-synchronized read that uses
READ_ONCE()"

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

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.

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.

                Linus