From: Peter Zijlstra <peterz@infradead.org>
To: Michael Matz <matz@suse.de>
Cc: "Paul E. McKenney" <paulmck@kernel.org>,
Frederic Weisbecker <frederic@kernel.org>,
LKML <linux-kernel@vger.kernel.org>,
Boqun Feng <boqun.feng@gmail.com>,
Joel Fernandes <joel@joelfernandes.org>,
Josh Triplett <josh@joshtriplett.org>,
Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
Neeraj Upadhyay <neeraj.upadhyay@amd.com>,
Steven Rostedt <rostedt@goodmis.org>,
Uladzislau Rezki <urezki@gmail.com>, rcu <rcu@vger.kernel.org>,
Zqiang <qiang.zhang1211@gmail.com>,
"Liam R . Howlett" <Liam.Howlett@oracle.com>,
ubizjak@gmail.com
Subject: Re: [PATCH 2/4] rcu/tasks: Handle new PF_IDLE semantics
Date: Tue, 31 Oct 2023 17:23:53 +0100 [thread overview]
Message-ID: <20231031162353.GF15024@noisy.programming.kicks-ass.net> (raw)
In-Reply-To: <alpine.LSU.2.20.2310311523290.15233@wotan.suse.de>
On Tue, Oct 31, 2023 at 03:55:20PM +0000, Michael Matz wrote:
> > There's a pile of situations where a RmW instruction is actively
> > different vs a load-store split, esp for volatile variables that are
> > explicitly expected to change asynchronously.
> >
> > The original RmW instruction is IRQ-safe, while the load-store version
> > is not. If an interrupt lands in between the load and store and also
> > modifies the variable then the store after interrupt-return will
> > over-write said modification.
> >
> > These are not equivalent.
>
> Okay, then there you have it. Namely that LLVM has a bug (but see next
> paragraph). For volatile x, x++ _must_ expand to a separate read and
> write, because the abstract machine of C says so. If a RmW isn't
> equivalent to that, then it can't be used in this situation. If you
> _have_ to use a RmW for other reasons like interrupt safety, then a
> volatile variable is not the way to force this, as C simply doesn't have
> that concept and hence can't talk about it. (Of course it can't, as not
> all architectures could implement such, if it were required).
Yeah, RISC archs typically lack the RmW ops. I can understand C not
mandating their use. However, on architectures that do have them, using
them makes a ton of sense.
For us living in the real world, this C abstract machine is mostly a
pain in the arse :-)
> (If an RmW merely gives you more guarantees than a split load-store then
> of course LLVM doesn't have a bug, but you said not-equivalent, so I'm
> assuming the worst, that RmW also has fewer (other) guarantees)
RmW is strict superset of load-store, and as such not equivalent :-)
Specifically, using volatile degrades the guarantees -- which is counter
intuitive.
> So, are RMW ops a strict superset (vis the guarantees they give) of split
> load-store? If so we can at least say that using RMW is a valid
> optimization :) Still, an optmization only.
This.
> > > But all that seems to be a side-track anyway, what's your real worry with
> > > the code sequence generated by GCC?
> >
> > In this case it's sub-optimal code, both larger and possibly slower for
> > having two memops.
> >
> > The reason to have volatile is because that's what Linux uses to
> > dis-allow store-tearing, something that doesn't happen in this case. A
> > suitably insane but conforming compiler could compile a non-volatile
> > memory increment into something insane like:
> >
> > load byte-0, r1
> > increment r1
> > store r1, byte-0
> > jno done
> > load byte-1, r1
> > increment ri
> > store r1, byte 1
> > jno done
> > ...
> > done:
> >
> > We want to explicitly dis-allow this.
>
> Yeah, I see. Within C you don't have much choice than volatile for this
> :-/ Funny thing: on some architectures this is actually what is generated
> sometimes, even if it has multi-byte loads/stores. This came up
> recently on the gcc list and the byte-per-byte sequence was faster ;-)
> (it was rather: load-by-bytes, form whole value via shifts, increment,
> store-by-bytes)
> Insane codegen for insane micro-architectures!
*groan*
> > I know C has recently (2011) grown this _Atomic thing, but that has
> > other problems.
>
> Yeah.
>
> So, hmm, I don't quite know what to say, you're between a rock and a hard
> place, I guess. You have to use volatile for its effects but then are
> unhappy about its effects :)
Notably, Linux uses a *ton* of volatile and there has historically been
a lot of grumbling about the GCC stance of 'stupid' codegen the moment
it sees volatile.
It really would help us (the Linux community) if GCC were to be less
offended by the whole volatile thing and would try to generate better
code.
Paul has been on the C/C++ committee meetings and keeps telling me them
folks hate volatile with a passion up to the point of proposing to
remove it from the language or somesuch. But the reality is that Linux
very heavily relies on it and _Atomic simply cannot replace it.
> If you can confirm the above about validity of the optimization, then at
> least there'd by a point for adding a peephole in GCC for this, even if
> current codegen isn't a bug, but I still wouldn't hold my breath.
> volatile is so ... ewww, it's best left alone.
Confirmed, and please, your SMP computer only works becuase of volatile,
it *is* important.
next prev parent reply other threads:[~2023-10-31 16:24 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20231027144050.110601-1-frederic@kernel.org>
[not found] ` <20231027144050.110601-3-frederic@kernel.org>
2023-10-27 19:20 ` [PATCH 2/4] rcu/tasks: Handle new PF_IDLE semantics Peter Zijlstra
2023-10-27 21:23 ` Paul E. McKenney
2023-10-27 22:46 ` Peter Zijlstra
2023-10-27 23:41 ` Paul E. McKenney
2023-10-30 8:21 ` Peter Zijlstra
2023-10-30 20:11 ` Paul E. McKenney
2023-10-31 9:52 ` Peter Zijlstra
2023-10-31 14:16 ` Michael Matz
2023-10-31 14:43 ` Paul E. McKenney
2023-10-31 15:16 ` Peter Zijlstra
2023-10-31 15:55 ` Michael Matz
2023-10-31 16:23 ` Peter Zijlstra [this message]
2023-10-31 16:49 ` Michael Matz
2023-10-31 17:10 ` Paul E. McKenney
2023-10-31 14:24 ` Paul E. McKenney
2023-10-31 15:20 ` Peter Zijlstra
2023-10-31 18:12 ` Paul E. McKenney
2023-10-27 19:23 ` [PATCH 0/4] rcu: Fix PF_IDLE related issues v3 Peter Zijlstra
2023-10-24 21:46 [PATCH 0/4] rcu: Fix PF_IDLE related issues v2 Frederic Weisbecker
2023-10-24 21:46 ` [PATCH 2/4] rcu/tasks: Handle new PF_IDLE semantics Frederic Weisbecker
2023-10-25 8:40 ` Peter Zijlstra
2023-10-25 10:31 ` Frederic Weisbecker
2023-10-26 12:15 ` Z qiang
2023-10-26 14:34 ` Frederic Weisbecker
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=20231031162353.GF15024@noisy.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=Liam.Howlett@oracle.com \
--cc=boqun.feng@gmail.com \
--cc=frederic@kernel.org \
--cc=joel@joelfernandes.org \
--cc=josh@joshtriplett.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mathieu.desnoyers@efficios.com \
--cc=matz@suse.de \
--cc=neeraj.upadhyay@amd.com \
--cc=paulmck@kernel.org \
--cc=qiang.zhang1211@gmail.com \
--cc=rcu@vger.kernel.org \
--cc=rostedt@goodmis.org \
--cc=ubizjak@gmail.com \
--cc=urezki@gmail.com \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).