From: Mathieu Desnoyers <mathieu.desnoyers@efficios.com> To: Linus Torvalds <torvalds@linux-foundation.org> Cc: Thomas Gleixner <tglx@linutronix.de>, "Joel Fernandes, Google" <joel@joelfernandes.org>, Alan Stern <stern@rowland.harvard.edu>, rostedt <rostedt@goodmis.org>, 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 21:36:49 -0400 (EDT) [thread overview] Message-ID: <1642847744.23403.1566005809759.JavaMail.zimbra@efficios.com> (raw) In-Reply-To: <CAHk-=wh9qDFfWJscAQw_w+obDmZvcE5jWJRdYPKYP6YhgoGgGA@mail.gmail.com> ----- 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. 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 ? Thanks, Mathieu > > But in the *absence* of locking, READ_ONCE/WRITE_ONCE is usually a > good thing. The READ_ONCE actually tends to matter, because even if > the value is used only once at a source level, the compiler *could* > decide to do something else. > > The WRITE_ONCE() may or may not matter (afaik, thanks to concurrency, > modern C standard does not allow optimistic writes anyway, and we > wouldn't really accept such a compiler option if it did). > > But if the write is done without locking, it's good practice just to > show you are aware of the whole "done without locking" part. > > Linus -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com
next prev parent reply other threads:[~2019-08-17 1:36 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 [this message] 2019-08-17 2:13 ` Steven Rostedt 2019-08-17 14:40 ` Mathieu Desnoyers 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=1642847744.23403.1566005809759.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.