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

From: Steven Rostedt <rostedt@goodmis.org>
To: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Thomas Gleixner <tglx@linutronix.de>,
	"Joel Fernandes, Google" <joel@joelfernandes.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	Valentin Schneider <valentin.schneider@arm.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	paulmck <paulmck@linux.ibm.com>,
	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: Fri, 16 Aug 2019 22:13:13 -0400	[thread overview]
Message-ID: <20190816221313.4b05b876@oasis.local.home> (raw)
In-Reply-To: <1642847744.23403.1566005809759.JavaMail.zimbra@efficios.com>

On Fri, 16 Aug 2019 21:36:49 -0400 (EDT)
Mathieu Desnoyers <mathieu.desnoyers@efficios.com> wrote:

> ----- On Aug 16, 2019, at 5:04 PM, Linus Torvalds torvalds@linux-foundation.org wrote:
> 
> > On Fri, Aug 16, 2019 at 1:49 PM Thomas Gleixner <tglx@linutronix.de> wrote:  
> >>
> >> Can we finally put a foot down and tell compiler and standard committee
> >> people to stop this insanity?  
> > 
> > It's already effectively done.
> > 
> > Yes, values can be read from memory multiple times if they need
> > reloading. So "READ_ONCE()" when the value can change is a damn good
> > idea.
> > 
> > But it should only be used if the value *can* change. Inside a locked
> > region it is actively pointless and misleading.
> > 
> > Similarly, WRITE_ONCE() should only be used if you have a _reason_ for
> > using it (notably if you're not holding a lock).
> > 
> > If people use READ_ONCE/WRITE_ONCE when there are locks that prevent
> > the values from changing, they are only making the code illegible.
> > Don't do it.  
> 
> I agree with your argument in the case where both read-side and write-side
> are protected by the same lock: READ/WRITE_ONCE are useless then. However,
> in the scenario we have here, only the write-side is protected by the lock
> against concurrent updates, but reads don't take any lock.

And because reads are not protected by any lock or memory barrier,
using READ_ONCE() is pointless. The CPU could be doing a lot of hidden
manipulation of that variable too.

Again, this is just to enable caching of the comm. Nothing more. It's a
"best effort" algorithm. We get it, we then can map a pid to a name. If
not, we just have a pid and we map "<...>".

Next you'll be asking for the memory barriers to guarantee a real hit.
And honestly, this information is not worth any overhead.

And most often we enable this before we enable the tracepoint we want
this information from, which requires modification of the text area and
will do a bunch of syncs across CPUs. That alone will most likely keep
any race from happening.

The only real bug is the check to know if we should add the probe or
not was done outside the lock, and when we hit the race, we could add a
probe twice, causing the kernel to spit out a warning. You fixed that,
and that's all that needs to be done. I'm now even more against adding
the READ_ONCE() or WRITE_ONCE().

-- Steve

> 
> If WRITE_ONCE has any use at all (protecting against store tearing and
> invented stores), it should be used even with a lock held in this
> scenario, because the lock does not prevent READ_ONCE() from observing
> transient states caused by lack of WRITE_ONCE() for the update.
> 
> So why does WRITE_ONCE exist in the first place ? Is it for documentation
> purposes only or are there actual situations where omitting it can cause
> bugs with real-life compilers ?
> 
> In terms of code change, should we favor only introducing WRITE_ONCE
> in new code, or should existing code matching those conditions be
> moved to WRITE_ONCE as bug fixes ?
> 
>