Message-ID: <46BB31A6.4080507@redhat.com>
Date:	Thu, 09 Aug 2007 11:24:22 -0400
From:	Chris Snook <csnook@...hat.com>
To:	paulmck@...ux.vnet.ibm.com
CC:	linux-kernel@...r.kernel.org, linux-arch@...r.kernel.org,
	torvalds@...ux-foundation.org, netdev@...r.kernel.org,
	akpm@...ux-foundation.org, ak@...e.de, heiko.carstens@...ibm.com,
	davem@...emloft.net, schwidefsky@...ibm.com, wensong@...ux-vs.org,
	horms@...ge.net.au, wjiang@...ilience.com, cfriesen@...tel.com,
	zlynx@....org, rpjday@...dspring.com, jesper.juhl@...il.com
Subject: Re: [PATCH 1/24] make atomic_read() behave consistently on alpha

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 majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists