Paul E. McKenney wrote:
On Thu, Aug 09, 2007 at 10:53:14AM -0400, Chris Snook wrote:
Paul E. McKenney wrote:
Why not the same access-once semantics for atomic_set() as
for atomic_read()? As this patch stands, it might introduce
architecture-specific compiler-induced bugs due to the fact that
atomic_set() used to imply volatile behavior but no longer does.
When we make the volatile cast in atomic_read(), we're casting an rvalue to
volatile. This unambiguously tells the compiler that we want to re-load
that register from memory. What's "volatile behavior" for an lvalue?
I was absolutely -not- suggesting volatile behavior for lvalues.
Instead, I am asking for volatile behavior from an -rvalue-. In the
case of atomic_read(), it is the atomic_t being read from. In the case
of atomic_set(), it is the atomic_t being written to. As suggested in
my previous email:
#define atomic_set(v,i) ((*(volatile int *)&(v)->counter) = (i))
#define atomic64_set(v,i) ((*(volatile long *)&(v)->counter) = (i))
That looks like a volatile lvalue to me. I confess I didn't exactly ace
compilers. Care to explain this?
Again, the architectures that used to have their "counter" declared
as volatile will lose volatile semantics on atomic_set() with your
patch, which might result in bugs due to overly imaginative compiler
optimizations. The above would prevent any such bugs from appearing.
A
write to an lvalue already implies an eventual write to memory, so this
would be a no-op. Maybe you'll write to the register a few times before
flushing it to memory, but it will happen eventually. With an rvalue,
there's no guarantee that it will *ever* load from memory, which is what
volatile fixes.
I think what you have in mind is LOCK_PREFIX behavior, which is not the
purpose of atomic_set. We use LOCK_PREFIX in the inline assembly for the
atomic_* operations that read, modify, and write a value, only because it
is necessary to perform that entire transaction atomically.
No LOCK_PREFIX, thank you!!! I just want to make sure that the compiler
doesn't push the store down out of a loop, split the store, allow the
store to happen twice (e.g., to allow different code paths to be merged),
and all the other tricks that the C standard permits compilers to pull.
We can't have split stores because we don't use atomic64_t on 32-bit
architectures. volatile won't save you from pushing stores out of loops unless
you're also doing reads. This is why we use reads to flush writes to mmio
registers. In this case, an atomic_read() with volatile in it will suffice.
Storing twice is perfectly legal here, though it's unlikely that an optimizing
compiler smart enough to create the problem we're addressing would ever do that.
-- Chris
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
