From: Linus Torvalds <torvalds@osdl.org>
To: Tim Schmielau <tim@physik3.uni-rostock.de>
Cc: Andrew Morton <akpm@osdl.org>,
James Bottomley <James.Bottomley@SteelEye.com>,
johnstul@us.ibm.com, lkml <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] fix get_jiffies_64 to work on voyager
Date: Tue, 6 Jan 2004 10:02:18 -0800 (PST) [thread overview]
Message-ID: <Pine.LNX.4.58.0401060945090.9166@home.osdl.org> (raw)
In-Reply-To: <Pine.LNX.4.53.0401061807070.9108@gockel.physik3.uni-rostock.de>
[ This is a big rant against using "volatile" on data structures. Feel
free to ignore it, but the fact is, I'm right. You should never EVER use
"volatile" on a data structure. ]
On Tue, 6 Jan 2004, Tim Schmielau wrote:
>
> We then need to make jiffies_64 volatile, since we violate the rule to
> never read it.
No, we should _never_ make anything volatile. There just isn't any reason
to. The compiler will never do any "better" with a volatile, it will only
ever do worse.
If there are memory ordering constraints etc, the compiler won't be able
to handle them anyway, and volatile will be a no-op. That's why we have
"barrier()" and "mb()" and friends.
The _only_ acceptable use of "volatile" is basically:
- in _code_ (not data structures), where we might mark a place as making
a special type of access. For example, in the PCI MMIO read functions,
rather than use inline assembly to force the particular access (which
would work equally well), we can force the pointer to a volatile type.
Similarly, we force this for "test_bit()" macros etc, because they are
documented to work on SMP-safe. But it's the _code_ that works that
way, not the data structures.
And this is an important distinctions: there are specific pieces of
_code_ that may be SMP-safe (for whatever reasons the writer thinks).
Data structures themselves are never SMP safe.
Ergo: never mark data structures "volatile". It's a sure sign of a bug
if the thing isn't a memory-mapped IO register (and even then it's
likely a bug, since you really should be using the proper functions).
(Some driver writers use "volatile" for mailboxes that are updated by
DMA from the hardware. It _can_ be correct there, but the fact is, you
might as well put the "volatile" in the code just out of principle).
That said, the "sure sign of a bug" case has one specific sub-case:
- to paste over bugs that you really don't tink are worth fixing any
other way. This is why "jiffies" itself is declared volatile: just to
let people write code that does "while (time_before(xxx, jiffies))".
But the "jiffies" case is safe only _exactly_ because it's an atomic read.
You always get a valid value - so it's actually "safe" to mark jiffies as
baing volatile. It allows people to be sloppy (bad), but at least it
allows people to be sloppy in a safe way.
In contrast, "jiffies_64" is _not_ an area where you can safely let the
compiler read a unstable value, so "volatile" is fundamentally wrong for
it. You need to have some locking, or to explicitly say "we don't care in
this case", and in both cases it would be wrong to call the thing
"volatile". With locking, it _isn't_ volatile, and with "we don't care",
it had better not make any difference. In either case the "volatile" is
wrong.
We had absolutely _tons_ of bugs in the original networking code, where
clueless people thougth that "volatile" somehow means that you don't need
locking. EVERY SINGLE CASE, and I mean EVERY ONE, was a bug.
There are some other cases where the "volatile" keyword is fine, notably
inline asm has a specific meaning that is pretty well-defined and very
useful. But in all other cases I'd suggest having the volatile be part of
the code, possibly with an explanation _why_ it is ok to use volatile
there unless it is obvious.
Linus
next prev parent reply other threads:[~2004-01-06 18:02 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2004-01-06 16:04 [PATCH] fix get_jiffies_64 to work on voyager James Bottomley
2004-01-06 16:19 ` Andrew Morton
2004-01-06 16:26 ` James Bottomley
2004-01-06 17:05 ` Andrew Morton
2004-01-06 18:53 ` James Bottomley
2004-01-06 16:29 ` Linus Torvalds
2004-01-06 17:05 ` Andrew Morton
2004-01-06 17:11 ` Tim Schmielau
2004-01-06 17:22 ` Andrew Morton
2004-01-06 17:37 ` Tim Schmielau
2004-01-06 18:02 ` Linus Torvalds [this message]
2004-01-06 16:30 ` Tim Schmielau
2004-01-06 16:26 ` Linus Torvalds
2004-01-06 18:29 ` Paulo Marques
2004-01-06 18:46 ` Linus Torvalds
2004-01-06 19:04 ` Paulo Marques
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=Pine.LNX.4.58.0401060945090.9166@home.osdl.org \
--to=torvalds@osdl.org \
--cc=James.Bottomley@SteelEye.com \
--cc=akpm@osdl.org \
--cc=johnstul@us.ibm.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tim@physik3.uni-rostock.de \
/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 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.