On Wed, 19 May 2010, Nick Piggin wrote:
>
> I wonder, Linus, is there a good reason to use volatile for these at
> all?
>
> I asked you about it quite a while back, and IIRC you said it might
> be OK to remove volatile from bitops, provided that callers were audited
> (ie. that nobody used bitops on volatile variables).
The bitops volatiles are different. They are there to allow for the C type
system (ie "const volatile *" just means that it accepts any kind of
pointer without complaining about implicit casting of const -> non-const
or volatile -> non-volatile).
For atomic_read(), and for the test_bit(), the _internal_ volatiles are
there just to get that ACCESS_ONCE() behavior, so that you can do things
like
while (test_bit(..)) {
..
}
and know that the compiler doesn't think it can do things like move the
atomic or bit read outside the loop or whatever.
Now, I do agree that _normally_ we should have memory barriers or similar
that guarantee that the compiler won't do odd things, but atomics and the
bitops are basically designed to work in the _absense_ of any other
serialization, so that's why it makes sense to have ACCESS_ONCE()
semantics for them.
> For atomic_read it shouldn't matter unless gcc is *really* bad at it.
> Ah, for atomic_read, the required semantic is surely ACCESS_ONCE, so
> that's where the volatile is needed? (maybe it would be clearer to
> explicitly use ACCESS_ONCE?)
Exactly. The volatile access on read inside those macros/functions (as
opposed to the "volatiles" that are there for C type reasons) is basically
the same as ACCESS_ONCE(). We could replace it with ACCESS_ONCE, although
I don't think it makes much difference as long as you always just think of
volatile as ACCESS_ONCE and just always put it in code (rather than on the
data structures)).
And replacing it with ACCESS_ONCE always has the header file dependency
issues, so..
> The case I was thinking about for bitops was for multiple non-atomic
> bitops, which would be nice to combine. In reality a lot of performance
> critical code (like page allocator) bites the bullet and does the
> open-coded bitwise ops. But it would be nice if that just worked for
> __set_bit / __clear_bit too.
__set_bit / __clear_bit should probably just be done as regular C code.
And yeah, we should remove the volatile from them. They aren't even valid
on anything that isn't locked anyway, so if somebody uses them on
something they have marked volatile, it's a bug.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-arch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
