From: Mathieu Desnoyers <firstname.lastname@example.org> To: rostedt <email@example.com> Cc: Linus Torvalds <firstname.lastname@example.org>, Thomas Gleixner <email@example.com>, "Joel Fernandes, Google" <firstname.lastname@example.org>, Alan Stern <email@example.com>, Valentin Schneider <firstname.lastname@example.org>, linux-kernel <email@example.com>, Peter Zijlstra <firstname.lastname@example.org>, paulmck <email@example.com>, Boqun Feng <firstname.lastname@example.org>, Will Deacon <email@example.com>, David Howells <firstname.lastname@example.org> 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.email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> ----- On Aug 16, 2019, at 10:13 PM, rostedt email@example.com wrote: > On Fri, 16 Aug 2019 21:36:49 -0400 (EDT) > Mathieu Desnoyers <firstname.lastname@example.org> wrote: > >> ----- On Aug 16, 2019, at 5:04 PM, Linus Torvalds email@example.com >> wrote: >> >> > On Fri, Aug 16, 2019 at 1:49 PM Thomas Gleixner <firstname.lastname@example.org> 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
next prev parent 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.email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --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.