Re: [PATCH] make atomic_t volatile on all architectures

From: Chris Snook
Date: Wed Aug 08 2007 - 19:32:22 EST


Jesper Juhl wrote:
On 09/08/2007, Chris Snook <csnook@xxxxxxxxxx> wrote:
From: Chris Snook <csnook@xxxxxxxxxx>

Some architectures currently do not declare the contents of an atomic_t to be
volatile. This causes confusion since atomic_read() might not actually read
anything if an optimizing compiler re-uses a value stored in a register, which
can break code that loops until something external changes the value of an
atomic_t. Avoiding such bugs requires using barrier(), which causes re-loads
of all registers used in the loop, thus hurting performance instead of helping
it, particularly on architectures where it's unnecessary. Since we generally
want to re-read the contents of an atomic variable on every access anyway,
let's standardize the behavior across all architectures and avoid the
performance and correctness problems of requiring the use of barrier() in
loops that expect atomic_t variables to change externally. This is relevant
even on non-smp architectures, since drivers may use atomic operations in
interrupt handlers.

Signed-off-by: Chris Snook <csnook@xxxxxxxxxx>


Hmm, I thought we were trying to move away from volatile since it is
very weakly defined and towards explicit barriers and locks...
Points to --> Documentation/volatile-considered-harmful.txt

This is a special case. Usually, the use of volatile is just lazy. In this case, it's probably necessary on at least some architectures, so we can't remove it everywhere unless we want to rewrite atomic.h completely in inline assembler for several architectures, and painstakingly verify all that work. I would hope it's obvious that having consistent behavior on all architectures, or even at all compiler optimization levels within an architecture, can be agreed to be good. Additionally, atomic_t variables are a rare exception, in that we pretty much always want to work with the latest value in RAM on every access. Making this atomic will allow us to remove a bunch of barriers which do nothing but slow things down on most architectures.

I agree that the use of atomic_t in .c files is generally bad, but in certain special cases, hiding it inside defined data types may be worth the slight impurity, just as we sometimes tolerate lines longer than 80 columns when "fixing" it makes things unreadable.

-- Chris
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/