Re: [PATCH 2/2]: atomic_t: Remove volatile from atomic_t definition

[Linus Torvalds <torvalds@linux-foundation.org>]

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