[patch] ipvs: force read of atomic_t in while loop

[patch] ipvs: force read of atomic_t in while loop

[Heiko Carstens]

From: Heiko Carstens <heiko.carstens@de.ibm.com>

For architectures that don't have a volatile atomic_ts constructs like
while (atomic_read(&something)); might result in endless loops since a
barrier() is missing which forces the compiler to generate code that
actually reads memory contents.
Fix this in ipvs by using the IP_VS_WAIT_WHILE macro which resolves to
while (expr) { cpu_relax(); }
(why isn't this open coded btw?)

Cc: Wensong Zhang <wensong@linux-vs.org>
Cc: Simon Horman <horms@verge.net.au>
Cc: David Miller <davem@davemloft.net>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

Just saw this while grepping for atomic_reads in a while loops.
Maybe we should re-add the volatile to atomic_t. Not sure.

 net/ipv4/ipvs/ip_vs_ctl.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: linux-2.6/net/ipv4/ipvs/ip_vs_ctl.c
===================================================================
--- linux-2.6.orig/net/ipv4/ipvs/ip_vs_ctl.c
+++ linux-2.6/net/ipv4/ipvs/ip_vs_ctl.c
@@ -909,7 +909,7 @@ ip_vs_edit_dest(struct ip_vs_service *sv
 	write_lock_bh(&__ip_vs_svc_lock);
 
 	/* Wait until all other svc users go away */
-	while (atomic_read(&svc->usecnt) > 1) {};
+	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
 
 	/* call the update_service, because server weight may be changed */
 	svc->scheduler->update_service(svc);

Re: [patch] ipvs: force read of atomic_t in while loop

[Horms]

On Wed, Aug 08, 2007 at 11:33:00AM +0200, Heiko Carstens wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 
> For architectures that don't have a volatile atomic_ts constructs like
> while (atomic_read(&something)); might result in endless loops since a
> barrier() is missing which forces the compiler to generate code that
> actually reads memory contents.
> Fix this in ipvs by using the IP_VS_WAIT_WHILE macro which resolves to
> while (expr) { cpu_relax(); }
> (why isn't this open coded btw?)
> 
> Cc: Wensong Zhang <wensong@linux-vs.org>
> Cc: Simon Horman <horms@verge.net.au>
> Cc: David Miller <davem@davemloft.net>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
> 
> Just saw this while grepping for atomic_reads in a while loops.
> Maybe we should re-add the volatile to atomic_t. Not sure.

This looks good to me. A little wile back I noticed a few places
where IP_VS_WAIT_WHILE seemed to be curiously unused, then I got
distracted...

Signed-off-by: Simon Horman <horms@verge.net.au>

> 
>  net/ipv4/ipvs/ip_vs_ctl.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: linux-2.6/net/ipv4/ipvs/ip_vs_ctl.c
> ===================================================================
> --- linux-2.6.orig/net/ipv4/ipvs/ip_vs_ctl.c
> +++ linux-2.6/net/ipv4/ipvs/ip_vs_ctl.c
> @@ -909,7 +909,7 @@ ip_vs_edit_dest(struct ip_vs_service *sv
>  	write_lock_bh(&__ip_vs_svc_lock);
>  
>  	/* Wait until all other svc users go away */
> -	while (atomic_read(&svc->usecnt) > 1) {};
> +	IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
>  
>  	/* call the update_service, because server weight may be changed */
>  	svc->scheduler->update_service(svc);

-- 
Horms
  H: http://www.vergenet.net/~horms/
  W: http://www.valinux.co.jp/en/

Re: [patch] ipvs: force read of atomic_t in while loop

[David Miller]

From: Heiko Carstens <heiko.carstens@de.ibm.com>
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.

Re: [patch] ipvs: force read of atomic_t in while loop

[Heiko Carstens]

On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> 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.

Re: [patch] ipvs: force read of atomic_t in while loop

[Chris Snook]

Heiko Carstens wrote:
> On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
>> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>> 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?

Re: [patch] ipvs: force read of atomic_t in while loop

[Andrew Morton]

On Wed, 08 Aug 2007 17:08:44 -0400
Chris Snook <csnook@redhat.com> wrote:

> Heiko Carstens wrote:
> > On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> >> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> >> 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.

Re: [patch] ipvs: force read of atomic_t in while loop

[Heiko Carstens]

On Wed, Aug 08, 2007 at 02:31:15PM -0700, Andrew Morton wrote:
> On Wed, 08 Aug 2007 17:08:44 -0400
> Chris Snook <csnook@redhat.com> wrote:
> 
> > Heiko Carstens wrote:
> > > On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> > >> From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >> 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 <heiko.carstens@de.ibm.com>

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 <ak@suse.de>
Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
---

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) }
 

Re: [patch] ipvs: force read of atomic_t in while loop

[Chris Snook]

Heiko Carstens wrote:
> On Wed, Aug 08, 2007 at 02:31:15PM -0700, Andrew Morton wrote:
>> On Wed, 08 Aug 2007 17:08:44 -0400
>> Chris Snook <csnook@redhat.com> wrote:
>>
>>> Heiko Carstens wrote:
>>>> On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
>>>>> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>>>>> 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 <heiko.carstens@de.ibm.com>
> 
> 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 <ak@suse.de>
> Cc: Martin Schwidefsky <schwidefsky@de.ibm.com>
> Signed-off-by: Heiko Carstens <heiko.carstens@de.ibm.com>
> ---
> 
> 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) }
>  

Good so far, but we need to fix it on non-SMP architectures too, since 
drivers may use atomic_t in interrupt code.  Ideally I'd like to be able 
to remove a whole bunch of barriers, since they cause a lot of needless 
re-fetches for everything else in the loop.  We should also document the 
semantics of atomic_t to ensure consistency in the future.

	-- Chris

Re: [patch] ipvs: force read of atomic_t in while loop

[Andi Kleen]

On Wed, Aug 08, 2007 at 05:08:44PM -0400, Chris Snook wrote:
> Heiko Carstens wrote:
> >On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> >>From: Heiko Carstens <heiko.carstens@de.ibm.com>
> >>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 also think readding volatile makes sense. An alternative would be
to stick an rmb() into atomic_read() -- that would also stop speculative reads.
Disadvantage is that it clobbers all memory, not just the specific value.

But you really have to complain to Linus (cc'ed). He came up
with the volatile removale change iirc.

-Andi

Re: [patch] ipvs: force read of atomic_t in while loop

[Michael Buesch]

On Thursday 09 August 2007 02:15:33 Andi Kleen wrote:
> On Wed, Aug 08, 2007 at 05:08:44PM -0400, Chris Snook wrote:
> > Heiko Carstens wrote:
> > >On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> > >>From: Heiko Carstens <heiko.carstens@de.ibm.com>
> > >>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 also think readding volatile makes sense. An alternative would be
> to stick an rmb() into atomic_read() -- that would also stop speculative reads.
> Disadvantage is that it clobbers all memory, not just the specific value.
> 
> But you really have to complain to Linus (cc'ed). He came up
> with the volatile removale change iirc.

Isn't it possible through some inline assembly trick
that only a certain variable has to be reloaded?
So we could define something like that:

#define reload_var(x) __asm__ __volatile__ (whatever, x)

I don't know inline assembly that much, but isn't it possible
with that to kind of "fake-touch" the variable, so the compiler
must reload it (and only it) to make sure it's up to date?

-- 
Greetings Michael.

Re: [patch] ipvs: force read of atomic_t in while loop

[Chris Snook]

Michael Buesch wrote:
> On Thursday 09 August 2007 02:15:33 Andi Kleen wrote:
>> On Wed, Aug 08, 2007 at 05:08:44PM -0400, Chris Snook wrote:
>>> Heiko Carstens wrote:
>>>> On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
>>>>> From: Heiko Carstens <heiko.carstens@de.ibm.com>
>>>>> 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 also think readding volatile makes sense. An alternative would be
>> to stick an rmb() into atomic_read() -- that would also stop speculative reads.
>> Disadvantage is that it clobbers all memory, not just the specific value.
>>
>> But you really have to complain to Linus (cc'ed). He came up
>> with the volatile removale change iirc.
> 
> Isn't it possible through some inline assembly trick
> that only a certain variable has to be reloaded?
> So we could define something like that:
> 
> #define reload_var(x) __asm__ __volatile__ (whatever, x)
> 
> I don't know inline assembly that much, but isn't it possible
> with that to kind of "fake-touch" the variable, so the compiler
> must reload it (and only it) to make sure it's up to date?

We can do it in C, like this:

-#define atomic_read(v) ((v)->counter)
+#define atomic_read(v) (*(volatile int *)&(v)->counter)

By casting it volatile at the precise piece of code where we want to guarantee a 
read from memory, there's little risk of the compiler getting creative in its 
interpretation of the code.

Stay tuned for the patch set...

	-- Chris

Re: [patch] ipvs: force read of atomic_t in while loop

[Martin Schwidefsky]

On Thu, 2007-08-09 at 08:40 -0400, Chris Snook wrote:
> > #define reload_var(x) __asm__ __volatile__ (whatever, x)
> > 
> > I don't know inline assembly that much, but isn't it possible
> > with that to kind of "fake-touch" the variable, so the compiler
> > must reload it (and only it) to make sure it's up to date?
> 
> We can do it in C, like this:
> 
> -#define atomic_read(v) ((v)->counter)
> +#define atomic_read(v) (*(volatile int *)&(v)->counter)
> 
> By casting it volatile at the precise piece of code where we want to
> guarantee a read from memory, there's little risk of the compiler
> getting creative in its interpretation of the code.

To answer the inline assembler question:

asm volatile ("" : "=m" (counter)) : "m" (counter) )

will force the compiler to reload the value from memory. But the cast to
(volatile int *) is even better.

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

Re: [patch] ipvs: force read of atomic_t in while loop

[Andi Kleen]

> Isn't it possible through some inline assembly trick
> that only a certain variable has to be reloaded?

A volatile cast does that already

-Andi