From: Heiko Carstens <[email protected]>
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 <[email protected]>
Cc: Simon Horman <[email protected]>
Cc: David Miller <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Signed-off-by: Heiko Carstens <[email protected]>
---
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);
On Wed, Aug 08, 2007 at 11:33:00AM +0200, Heiko Carstens wrote:
> From: Heiko Carstens <[email protected]>
>
> 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 <[email protected]>
> Cc: Simon Horman <[email protected]>
> Cc: David Miller <[email protected]>
> Cc: Martin Schwidefsky <[email protected]>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
>
> 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 <[email protected]>
>
> 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/
From: Heiko Carstens <[email protected]>
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.
On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> From: Heiko Carstens <[email protected]>
> 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.
Heiko Carstens wrote:
> On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
>> From: Heiko Carstens <[email protected]>
>> 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?
On Wed, 08 Aug 2007 17:08:44 -0400
Chris Snook <[email protected]> wrote:
> Heiko Carstens wrote:
> > On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> >> From: Heiko Carstens <[email protected]>
> >> 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.
On Wed, Aug 08, 2007 at 02:31:15PM -0700, Andrew Morton wrote:
> On Wed, 08 Aug 2007 17:08:44 -0400
> Chris Snook <[email protected]> wrote:
>
> > Heiko Carstens wrote:
> > > On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
> > >> From: Heiko Carstens <[email protected]>
> > >> 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 <[email protected]>
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 <[email protected]>
Cc: Martin Schwidefsky <[email protected]>
Signed-off-by: Heiko Carstens <[email protected]>
---
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) }
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 <[email protected]> wrote:
>>
>>> Heiko Carstens wrote:
>>>> On Wed, Aug 08, 2007 at 03:21:31AM -0700, David Miller wrote:
>>>>> From: Heiko Carstens <[email protected]>
>>>>> 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 <[email protected]>
>
> 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 <[email protected]>
> Cc: Martin Schwidefsky <[email protected]>
> Signed-off-by: Heiko Carstens <[email protected]>
> ---
>
> 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
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 <[email protected]>
> >>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
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 <[email protected]>
> > >>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.
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 <[email protected]>
>>>>> 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
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.
> 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