All of lore.kernel.org
 help / color / mirror / Atom feed
From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
To: rostedt <rostedt@goodmis.org>
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: Sat, 17 Aug 2019 10:40:31 -0400 (EDT)	[thread overview]
Message-ID: <39888715.23900.1566052831673.JavaMail.zimbra@efficios.com> (raw)
In-Reply-To: <20190816221313.4b05b876@oasis.local.home>

----- On Aug 16, 2019, at 10:13 PM, rostedt rostedt@goodmis.org wrote:

> 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.

Please enlighten me by providing some details on what the CPU could do to
this word-aligned, word-sized variable in the absence of lock and barrier
that is relevant to this specific use-case ?

I suspect most of the barriers you refer to here are taken care of by the
tracepoint code which uses RCU to synchronize probe registration wrt
probe callback execution.

> 
> 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.

No, that's not my intent to add overhead to guarantee trace data
availability near trace beginning and end. However, considering that
READ_ONCE() and WRITE_ONCE() can provide additional data availability
guarantees in the middle of traces at no runtime cost, it seems like a
good trade off.

It's easier for an analysis to disregard missing information at the
beginning and end of trace without generating false-positive reports
than when it happens spuriously in the middle of traces.

> 
> 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.

Indeed the tracepoint use of RCU to synchronize registration vs probes
should take care of those barriers.

> 
> 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 just sent that fix in a separate patch.

> I'm now even more against adding the READ_ONCE() or WRITE_ONCE().

I'm not convinced by your arguments.

Thanks,

Mathieu

> 
> 
> -- 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 ?
>> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

  reply	other threads:[~2019-08-17 14:40 UTC|newest]

Thread overview: 60+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-18 10:29 WARNING in tracepoint_probe_register_prio (3) syzbot
2019-08-16  0:11 ` syzbot
2019-08-16 14:26   ` [PATCH 1/1] Fix: trace sched switch start/stop racy updates Mathieu Desnoyers
2019-08-16 16:25     ` Steven Rostedt
2019-08-16 16:48       ` Valentin Schneider
2019-08-16 17:04         ` Steven Rostedt
2019-08-16 17:41           ` Mathieu Desnoyers
2019-08-16 19:18             ` Steven Rostedt
2019-08-16 19:19             ` Alan Stern
2019-08-16 20:44               ` Joel Fernandes
2019-08-16 20:49                 ` Thomas Gleixner
2019-08-16 20:57                   ` Joel Fernandes
2019-08-16 22:27                     ` Valentin Schneider
2019-08-16 22:57                       ` Linus Torvalds
2019-08-17  1:41                         ` Mathieu Desnoyers
2019-08-17  4:52                         ` Paul E. McKenney
2019-08-17  8:28                           ` Linus Torvalds
2019-08-17  8:44                             ` Linus Torvalds
2019-08-17 15:02                               ` Mathieu Desnoyers
2019-08-17 20:03                                 ` Valentin Schneider
2019-08-17 23:00                                   ` Paul E. McKenney
2019-08-19 10:34                                     ` Valentin Schneider
2019-08-17 22:28                             ` Paul E. McKenney
2019-08-20 14:01                           ` Peter Zijlstra
2019-08-20 20:31                             ` Paul E. McKenney
2019-08-20 20:39                               ` Peter Zijlstra
2019-08-20 20:52                                 ` Paul E. McKenney
2019-08-16 21:04                   ` Linus Torvalds
2019-08-17  1:36                     ` Mathieu Desnoyers
2019-08-17  2:13                       ` Steven Rostedt
2019-08-17 14:40                         ` Mathieu Desnoyers [this message]
2019-08-17 15:26                           ` Steven Rostedt
2019-08-17 15:55                             ` Mathieu Desnoyers
2019-08-17 16:40                               ` Steven Rostedt
2019-08-17 22:06                                 ` Paul E. McKenney
2019-08-17  8:08                       ` Linus Torvalds
2019-08-20 13:56                         ` Peter Zijlstra
2019-08-20 20:29                           ` Paul E. McKenney
2019-08-21 10:32                             ` Will Deacon
2019-08-21 13:23                               ` Paul E. McKenney
2019-08-21 13:32                                 ` Will Deacon
2019-08-21 13:56                                   ` Paul E. McKenney
2019-08-21 16:22                                     ` Will Deacon
2019-08-21 15:33                                 ` Peter Zijlstra
2019-08-21 15:48                                   ` Mathieu Desnoyers
2019-08-21 16:14                                     ` Paul E. McKenney
2019-08-21 19:03                                     ` Joel Fernandes
2019-09-09  6:21                           ` Herbert Xu
2019-08-16 20:49                 ` Steven Rostedt
2019-08-16 20:59                   ` Joel Fernandes
2019-08-17  1:25                   ` Mathieu Desnoyers
2019-08-18  9:15                   ` stable markup was " Pavel Machek
2019-08-16 17:19       ` Mathieu Desnoyers
2019-08-16 19:15         ` Steven Rostedt
2019-08-17 14:27           ` Mathieu Desnoyers
2019-08-17 15:42             ` Steven Rostedt
2019-08-17 15:53               ` Mathieu Desnoyers
2019-08-17 16:43                 ` Steven Rostedt
2019-08-16 12:32 ` WARNING in tracepoint_probe_register_prio (3) syzbot
2019-08-16 12:41   ` Sebastian Andrzej Siewior

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=39888715.23900.1566052831673.JavaMail.zimbra@efficios.com \
    --to=mathieu.desnoyers@efficios.com \
    --cc=boqun.feng@gmail.com \
    --cc=dhowells@redhat.com \
    --cc=joel@joelfernandes.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=paulmck@linux.ibm.com \
    --cc=peterz@infradead.org \
    --cc=rostedt@goodmis.org \
    --cc=stern@rowland.harvard.edu \
    --cc=tglx@linutronix.de \
    --cc=torvalds@linux-foundation.org \
    --cc=valentin.schneider@arm.com \
    --cc=will.deacon@arm.com \
    --subject='Re: [PATCH 1/1] Fix: trace sched switch start/stop racy updates' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.