Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1764351AbXHHW1W (ORCPT ); Wed, 8 Aug 2007 18:27:22 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1757296AbXHHW1I (ORCPT ); Wed, 8 Aug 2007 18:27:08 -0400 Received: from mtagate3.de.ibm.com ([195.212.29.152]:48577 "EHLO mtagate3.de.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753667AbXHHW1G (ORCPT ); Wed, 8 Aug 2007 18:27:06 -0400 Date: Thu, 9 Aug 2007 00:27:03 +0200 From: Heiko Carstens To: Andrew Morton , Linus Torvalds , Andi Kleen Cc: Chris Snook , andi@firstfloor.org, David Miller , linux-kernel@vger.kernel.org, netdev@vger.kernel.org, schwidefsky@de.ibm.com, wensong@linux-vs.org, horms@verge.net.au Subject: Re: [patch] ipvs: force read of atomic_t in while loop Message-ID: <20070808222703.GA11359@osiris.ibm.com> References: <20070808093300.GA14530@osiris.boeblingen.de.ibm.com> <20070808.032131.35507346.davem@davemloft.net> <20070808102835.GC14530@osiris.boeblingen.de.ibm.com> <46BA30DC.20207@redhat.com> <20070808143115.de539511.akpm@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20070808143115.de539511.akpm@linux-foundation.org> User-Agent: Mutt/1.5.15 (2007-04-06) Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3685 Lines: 92 On Wed, Aug 08, 2007 at 02:31:15PM -0700, Andrew Morton wrote: > On Wed, 08 Aug 2007 17:08:44 -0400 > Chris Snook wrote: > > > Heiko Carstens wrote: > > > On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote: > > >> From: Heiko Carstens > > >> Date: Wed, 8 Aug 2007 11:33:00 +0200 > > >> > > >>> Just saw this while grepping for atomic_reads in a while loops. > > >>> Maybe we should re-add the volatile to atomic_t. Not sure. > > >> I think whatever the choice, it should be done consistently > > >> on every architecture. > > >> > > >> It's just asking for trouble if your arch does it differently from > > >> every other. > > > > > > Well..currently it's i386/x86_64 and s390 which have no volatile > > > in atomic_t. And yes, of course I agree it should be consistent > > > across all architectures. But it isn't. > > > > Based on recent discussion, it's pretty clear that there's a lot of > > confusion about this. A lot of people (myself included, until I thought > > about it long and hard) will reasonably assume that calling > > atomic_read() will actually read the value from memory. Leaving out the > > volatile declaration seems like a pessimization to me. If you force > > people to use barrier() everywhere they're working with atomic_t, it > > will force re-reads of all the non-atomic data in use as well, which > > will cause more memory fetches of things that generally don't need > > barrier(). That and it's a bug waiting to happen. > > > > Andi -- your thoughts on the matter? > > I'm not Andi, but this not-Andi thinks that permitting the compiler to cache > the results of atomic_read() is dumb. Ok, how about this: Subject: [PATCH] Add 'volatile' to atomic_t again. From: Heiko Carstens This basically reverts f9e9dcb38f5106fa8cdac04a9e967d5487f1cd20 which removed 'volatile' from atomic_t for i386/x86_64. Reason for this is to make sure that code like while (atomic_read(&whatever)); continues to work. Otherwise the compiler might generate code that will loop forever. Also this makes sure atomic_t is the same across all architectures. Cc: Andi Kleen Cc: Martin Schwidefsky Signed-off-by: Heiko Carstens --- s390 patch will go in via Martin if this is accepted. include/asm-i386/atomic.h | 2 +- include/asm-x86_64/atomic.h | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) Index: linux-2.6/include/asm-i386/atomic.h =================================================================== --- linux-2.6.orig/include/asm-i386/atomic.h +++ linux-2.6/include/asm-i386/atomic.h @@ -15,7 +15,7 @@ * on us. We need to use _exactly_ the address the user gave us, * not some alias that contains the same information. */ -typedef struct { int counter; } atomic_t; +typedef struct { volatile int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } Index: linux-2.6/include/asm-x86_64/atomic.h =================================================================== --- linux-2.6.orig/include/asm-x86_64/atomic.h +++ linux-2.6/include/asm-x86_64/atomic.h @@ -22,7 +22,7 @@ * on us. We need to use _exactly_ the address the user gave us, * not some alias that contains the same information. */ -typedef struct { int counter; } atomic_t; +typedef struct { volatile int counter; } atomic_t; #define ATOMIC_INIT(i) { (i) } - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/