Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote: On Tue, 24 Jul 2007, Benjamin Herrenschmidt wrote: Besides, as Nick pointed out, it prevents some valid optimizations. No it doesn't. Not the ones on the functions that just do an inline asm. The only valid optimization it might break is for "constant_test_bit()", which isn't even using inline asm. The constant case is probably most used (at least for page flags), and is most important for me. constant_test_bit may not be using inline asm, but the volatile pointer target means that it reloads the value and can't do much optimisation over it. BTW. once volatile goes away, i386 really should start using the C versions of __set_bit and __clear_bit as well IMO. (at least for the constant bitnr case), so gcc can potentially optimise better. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote:
And before we remove that "volatile", we'd better make damn sure that
there isn't any driver that does
/* Wait for the command queue to be cleared by DMA */
while (test_bit(...))
;
or similar.
Yes, it's annoying, but this is a scary and subtle area. And we sadly
_have_ had code that does things like that.
I certainly cannot speak for all the grotty drivers out there, but I've
never ever seen anything like the above. I would consider anyone using
kernel bit operations on DMA memory to be more than a little crazy. But
that's just me :)
Usually you will see
while (1) {
rmb();
if (software_index == hardware_index_in_DMA_memory)
break;
... handle a unit of work ...
}
Though ISTR being told that even rmb() was not sufficient in all cases
[nonetheless, that's what I use in net and SATA drivers and email
recommendations, and people seem happy]
Jeff
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Tue, 24 Jul 2007, Benjamin Herrenschmidt wrote: > > - This "volatile" will allow to pass pointers to volatile data to the > bitops. > > - Most users of "volatile" in the kenrel (except maybe jiffies) are > bogus > > - Thus let's remove it -as a type safety thing- to catch more of those > stupid volatile that shouldn't be ? :-) Quite frankly, I'd really really rather kill the "volatile" data structures independently. Once that is done, it doesn't really matter any more. > Besides, as Nick pointed out, it prevents some valid optimizations. No it doesn't. Not the ones on the functions that just do an inline asm. The only valid optimization it might break is for "constant_test_bit()", which isn't even using inline asm. And before we remove that "volatile", we'd better make damn sure that there isn't any driver that does /* Wait for the command queue to be cleared by DMA */ while (test_bit(...)) ; or similar. Yes, it's annoying, but this is a scary and subtle area. And we sadly _have_ had code that does things like that. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
> The "const volatile" is so that you can pass an arbitrary pointer. The > only kind of abritraty pointer is "const volatile". > > In other words, the "volatile" has nothing at all to do with whether the > memory is volatile or not (the same way "const" has nothing to do with it: > it's purely a C type *safety* issue, exactly the same way "const" is a > type safety issue. > > A "const" on a pointer doesn't mean that the thing it points to cannot > change. When you pass a source pointer to "strlen()", it doesn't have to > be constant. But "strlen()" takes a "const" pointer, because it work son > constant pointers *too*. However... What about that: - This "volatile" will allow to pass pointers to volatile data to the bitops. - Most users of "volatile" in the kenrel (except maybe jiffies) are bogus - Thus let's remove it -as a type safety thing- to catch more of those stupid volatile that shouldn't be ? :-) Besides, as Nick pointed out, it prevents some valid optimizations. Cheers, Ben. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Satyam Sharma wrote: On Tue, 24 Jul 2007, Nick Piggin wrote: Linus Torvalds wrote: Of course, if we remove all "volatiles" in data in the kernel (with the possible exception of "jiffies"), we can then remove them from function declarations too, but it should be done in that order. Well, regardless, it still forces the function to treat the pointer target as volatile, won't it? It definitely prevents valid optimisations that would be useful for me in mm/page_alloc.c where page flags are being set up or torn down or checked with non-atomic bitops. Yes, and yes. But I think what he meant there is that we'd need to audit the kernel for all users of set_bit and friends and see if callers actually pass in any _data_ that _is_ volatile. So we have to kill them there first, and then in the function declarations here. I think I'll put that on my long-term todo list, but see below. Yeah that is probably what he meant. Anyway by type safety, do you mean it will stop the compiler from warning if a pointer to a volatile is passed to the bitop? The compiler would start warning for all those cases (passing in a pointer to volatile data, when the bitops have lost the volatile casting from their function declarations), actually. Something like "passing argument discards qualifiers from pointer type" ... but considering I didn't see *any* of those warnings after these patches, I'm confused as to what exactly Linus meant here ... and what exactly do we need to do "kill the volatiles". Because even with an allyesconfig, your compile isn't testing the entire kernel. So given the relatively minor benefit of removing the volatiles, I suppose we shouldn't risk slipping a bug in. If you can make gcc throw an error in that case it would be a different story. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Tue, 24 Jul 2007, Nick Piggin wrote: > Linus Torvalds wrote: > > > > On Mon, 23 Jul 2007, Satyam Sharma wrote: > > > > > > > [4/8] i386: bitops: Kill volatile-casting of memory addresses > > > > > > This is wrong. > > > > The "const volatile" is so that you can pass an arbitrary pointer. The only > > kind of abritraty pointer is "const volatile". > > > > In other words, the "volatile" has nothing at all to do with whether the > > memory is volatile or not (the same way "const" has nothing to do with it: > > it's purely a C type *safety* issue, exactly the same way "const" is a type > > safety issue. > > > > A "const" on a pointer doesn't mean that the thing it points to cannot > > change. When you pass a source pointer to "strlen()", it doesn't have to be > > constant. But "strlen()" takes a "const" pointer, because it work son > > constant pointers *too*. > > > > Same deal here. > > > > Admittedly this may be mostly historic, but regardless - the "volatiles" are > > right. > > > > Using volatile on *data* is generally considered incorrect and bad taste, > > but using it in situations like this potentially makes sense. > > > > Of course, if we remove all "volatiles" in data in the kernel (with the > > possible exception of "jiffies"), we can then remove them from function > > declarations too, but it should be done in that order. > > Well, regardless, it still forces the function to treat the pointer > target as volatile, won't it? It definitely prevents valid optimisations > that would be useful for me in mm/page_alloc.c where page flags are > being set up or torn down or checked with non-atomic bitops. Yes, and yes. But I think what he meant there is that we'd need to audit the kernel for all users of set_bit and friends and see if callers actually pass in any _data_ that _is_ volatile. So we have to kill them there first, and then in the function declarations here. I think I'll put that on my long-term todo list, but see below. > Anyway by type safety, do you mean it will stop the compiler from > warning if a pointer to a volatile is passed to the bitop? The compiler would start warning for all those cases (passing in a pointer to volatile data, when the bitops have lost the volatile casting from their function declarations), actually. Something like "passing argument discards qualifiers from pointer type" ... but considering I didn't see *any* of those warnings after these patches, I'm confused as to what exactly Linus meant here ... and what exactly do we need to do "kill the volatiles". > If so, then > why don't we just kill all the volatiles out of here and fix any > warnings that comeup? I doubt there would be many, and of those, some > might show up real synchronisation problems. Yes, but see above. > OK, not the i386 functions as much because they are all in asm anwyay, > but in general (btw. why does i386 or any architecture define their own > non-atomic bitops? If the version in asm-generic/bitops/non-atomic.h > is not good enough then surely it is a bug in gcc or that file?) Satyam - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Tue, 24 Jul 2007, Nick Piggin wrote: Linus Torvalds wrote: On Mon, 23 Jul 2007, Satyam Sharma wrote: [4/8] i386: bitops: Kill volatile-casting of memory addresses This is wrong. The const volatile is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is const volatile. In other words, the volatile has nothing at all to do with whether the memory is volatile or not (the same way const has nothing to do with it: it's purely a C type *safety* issue, exactly the same way const is a type safety issue. A const on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to strlen(), it doesn't have to be constant. But strlen() takes a const pointer, because it work son constant pointers *too*. Same deal here. Admittedly this may be mostly historic, but regardless - the volatiles are right. Using volatile on *data* is generally considered incorrect and bad taste, but using it in situations like this potentially makes sense. Of course, if we remove all volatiles in data in the kernel (with the possible exception of jiffies), we can then remove them from function declarations too, but it should be done in that order. Well, regardless, it still forces the function to treat the pointer target as volatile, won't it? It definitely prevents valid optimisations that would be useful for me in mm/page_alloc.c where page flags are being set up or torn down or checked with non-atomic bitops. Yes, and yes. But I think what he meant there is that we'd need to audit the kernel for all users of set_bit and friends and see if callers actually pass in any _data_ that _is_ volatile. So we have to kill them there first, and then in the function declarations here. I think I'll put that on my long-term todo list, but see below. Anyway by type safety, do you mean it will stop the compiler from warning if a pointer to a volatile is passed to the bitop? The compiler would start warning for all those cases (passing in a pointer to volatile data, when the bitops have lost the volatile casting from their function declarations), actually. Something like passing argument discards qualifiers from pointer type ... but considering I didn't see *any* of those warnings after these patches, I'm confused as to what exactly Linus meant here ... and what exactly do we need to do kill the volatiles. If so, then why don't we just kill all the volatiles out of here and fix any warnings that comeup? I doubt there would be many, and of those, some might show up real synchronisation problems. Yes, but see above. OK, not the i386 functions as much because they are all in asm anwyay, but in general (btw. why does i386 or any architecture define their own non-atomic bitops? If the version in asm-generic/bitops/non-atomic.h is not good enough then surely it is a bug in gcc or that file?) Satyam - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Satyam Sharma wrote: On Tue, 24 Jul 2007, Nick Piggin wrote: Linus Torvalds wrote: Of course, if we remove all volatiles in data in the kernel (with the possible exception of jiffies), we can then remove them from function declarations too, but it should be done in that order. Well, regardless, it still forces the function to treat the pointer target as volatile, won't it? It definitely prevents valid optimisations that would be useful for me in mm/page_alloc.c where page flags are being set up or torn down or checked with non-atomic bitops. Yes, and yes. But I think what he meant there is that we'd need to audit the kernel for all users of set_bit and friends and see if callers actually pass in any _data_ that _is_ volatile. So we have to kill them there first, and then in the function declarations here. I think I'll put that on my long-term todo list, but see below. Yeah that is probably what he meant. Anyway by type safety, do you mean it will stop the compiler from warning if a pointer to a volatile is passed to the bitop? The compiler would start warning for all those cases (passing in a pointer to volatile data, when the bitops have lost the volatile casting from their function declarations), actually. Something like passing argument discards qualifiers from pointer type ... but considering I didn't see *any* of those warnings after these patches, I'm confused as to what exactly Linus meant here ... and what exactly do we need to do kill the volatiles. Because even with an allyesconfig, your compile isn't testing the entire kernel. So given the relatively minor benefit of removing the volatiles, I suppose we shouldn't risk slipping a bug in. If you can make gcc throw an error in that case it would be a different story. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
The const volatile is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is const volatile. In other words, the volatile has nothing at all to do with whether the memory is volatile or not (the same way const has nothing to do with it: it's purely a C type *safety* issue, exactly the same way const is a type safety issue. A const on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to strlen(), it doesn't have to be constant. But strlen() takes a const pointer, because it work son constant pointers *too*. However... What about that: - This volatile will allow to pass pointers to volatile data to the bitops. - Most users of volatile in the kenrel (except maybe jiffies) are bogus - Thus let's remove it -as a type safety thing- to catch more of those stupid volatile that shouldn't be ? :-) Besides, as Nick pointed out, it prevents some valid optimizations. Cheers, Ben. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Tue, 24 Jul 2007, Benjamin Herrenschmidt wrote: - This volatile will allow to pass pointers to volatile data to the bitops. - Most users of volatile in the kenrel (except maybe jiffies) are bogus - Thus let's remove it -as a type safety thing- to catch more of those stupid volatile that shouldn't be ? :-) Quite frankly, I'd really really rather kill the volatile data structures independently. Once that is done, it doesn't really matter any more. Besides, as Nick pointed out, it prevents some valid optimizations. No it doesn't. Not the ones on the functions that just do an inline asm. The only valid optimization it might break is for constant_test_bit(), which isn't even using inline asm. And before we remove that volatile, we'd better make damn sure that there isn't any driver that does /* Wait for the command queue to be cleared by DMA */ while (test_bit(...)) ; or similar. Yes, it's annoying, but this is a scary and subtle area. And we sadly _have_ had code that does things like that. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote:
And before we remove that volatile, we'd better make damn sure that
there isn't any driver that does
/* Wait for the command queue to be cleared by DMA */
while (test_bit(...))
;
or similar.
Yes, it's annoying, but this is a scary and subtle area. And we sadly
_have_ had code that does things like that.
I certainly cannot speak for all the grotty drivers out there, but I've
never ever seen anything like the above. I would consider anyone using
kernel bit operations on DMA memory to be more than a little crazy. But
that's just me :)
Usually you will see
while (1) {
rmb();
if (software_index == hardware_index_in_DMA_memory)
break;
... handle a unit of work ...
}
Though ISTR being told that even rmb() was not sufficient in all cases
[nonetheless, that's what I use in net and SATA drivers and email
recommendations, and people seem happy]
Jeff
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote: On Tue, 24 Jul 2007, Benjamin Herrenschmidt wrote: Besides, as Nick pointed out, it prevents some valid optimizations. No it doesn't. Not the ones on the functions that just do an inline asm. The only valid optimization it might break is for constant_test_bit(), which isn't even using inline asm. The constant case is probably most used (at least for page flags), and is most important for me. constant_test_bit may not be using inline asm, but the volatile pointer target means that it reloads the value and can't do much optimisation over it. BTW. once volatile goes away, i386 really should start using the C versions of __set_bit and __clear_bit as well IMO. (at least for the constant bitnr case), so gcc can potentially optimise better. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote: On Mon, 23 Jul 2007, Satyam Sharma wrote: [4/8] i386: bitops: Kill volatile-casting of memory addresses This is wrong. The "const volatile" is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is "const volatile". In other words, the "volatile" has nothing at all to do with whether the memory is volatile or not (the same way "const" has nothing to do with it: it's purely a C type *safety* issue, exactly the same way "const" is a type safety issue. A "const" on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to "strlen()", it doesn't have to be constant. But "strlen()" takes a "const" pointer, because it work son constant pointers *too*. Same deal here. Admittedly this may be mostly historic, but regardless - the "volatiles" are right. Using volatile on *data* is generally considered incorrect and bad taste, but using it in situations like this potentially makes sense. Of course, if we remove all "volatiles" in data in the kernel (with the possible exception of "jiffies"), we can then remove them from function declarations too, but it should be done in that order. Well, regardless, it still forces the function to treat the pointer target as volatile, won't it? It definitely prevents valid optimisations that would be useful for me in mm/page_alloc.c where page flags are being set up or torn down or checked with non-atomic bitops. OK, not the i386 functions as much because they are all in asm anwyay, but in general (btw. why does i386 or any architecture define their own non-atomic bitops? If the version in asm-generic/bitops/non-atomic.h is not good enough then surely it is a bug in gcc or that file?) Anyway by type safety, do you mean it will stop the compiler from warning if a pointer to a volatile is passed to the bitop? If so, then why don't we just kill all the volatiles out of here and fix any warnings that comeup? I doubt there would be many, and of those, some might show up real synchronisation problems. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Mon, 23 Jul 2007, Satyam Sharma wrote: > > [4/8] i386: bitops: Kill volatile-casting of memory addresses This is wrong. The "const volatile" is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is "const volatile". In other words, the "volatile" has nothing at all to do with whether the memory is volatile or not (the same way "const" has nothing to do with it: it's purely a C type *safety* issue, exactly the same way "const" is a type safety issue. A "const" on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to "strlen()", it doesn't have to be constant. But "strlen()" takes a "const" pointer, because it work son constant pointers *too*. Same deal here. Admittedly this may be mostly historic, but regardless - the "volatiles" are right. Using volatile on *data* is generally considered incorrect and bad taste, but using it in situations like this potentially makes sense. Of course, if we remove all "volatiles" in data in the kernel (with the possible exception of "jiffies"), we can then remove them from function declarations too, but it should be done in that order. Linus - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
From: Satyam Sharma <[EMAIL PROTECTED]>
[4/8] i386: bitops: Kill volatile-casting of memory addresses
All the occurrences of "volatile" that are used to qualify access to the
passed bit-string pointer/address must be removed, because "volatile" is
crazy, doesn't really work anyway, has nothing to do with locking and
atomicity, and is in general wholly unnecessary for these operations:
* In the case of the unlocked variants, we're not providing any guarantees
whatsoever, so we want the caller to do any necessary locking surrounding
the use of that function anyway.
* In case of the primitives where we *do* make locking/atomicity/reordering
(three very different, but related, concepts) guarantees, we're doing that
properly by using the lock instruction-prefix and "__asm__ __volatile__"
anyway (and with "addr" always being constrained with "m" in the assembly,
gcc doesn't bring it into a local register either).
So let's just kill the pointless use of volatile.
Signed-off-by: Satyam Sharma <[EMAIL PROTECTED]>
Cc: David Howells <[EMAIL PROTECTED]>
Cc: Nick Piggin <[EMAIL PROTECTED]>
---
include/asm-i386/bitops.h | 60 +
1 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/include/asm-i386/bitops.h b/include/asm-i386/bitops.h
index d623fd9..0f5634b 100644
--- a/include/asm-i386/bitops.h
+++ b/include/asm-i386/bitops.h
@@ -26,8 +26,6 @@
* on single-dword quantities.
*/
-#define ADDR (*(volatile long *) addr)
-
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
@@ -38,11 +36,11 @@
*
* See __set_bit() if you do not require the atomic guarantees.
*/
-static inline void set_bit(int nr, volatile unsigned long * addr)
+static inline void set_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
"btsl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -58,11 +56,11 @@ static inline void set_bit(int nr, volatile unsigned long *
addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __set_bit(int nr, volatile unsigned long * addr)
+static inline void __set_bit(int nr, unsigned long *addr)
{
__asm__(
"btsl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -76,11 +74,11 @@ static inline void __set_bit(int nr, volatile unsigned long
* addr)
*
* See __clear_bit() if you do not require the atomic guarantees.
*/
-static inline void clear_bit(int nr, volatile unsigned long * addr)
+static inline void clear_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
"btrl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -96,11 +94,11 @@ static inline void clear_bit(int nr, volatile unsigned long
* addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __clear_bit(int nr, volatile unsigned long * addr)
+static inline void __clear_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__(
"btrl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -123,11 +121,11 @@ static inline void __clear_bit(int nr, volatile unsigned
long * addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __change_bit(int nr, volatile unsigned long * addr)
+static inline void __change_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__(
"btcl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -141,11 +139,11 @@ static inline void __change_bit(int nr, volatile unsigned
long * addr)
*
* See __change_bit() if you do not require the atomic guarantees.
*/
-static inline void change_bit(int nr, volatile unsigned long * addr)
+static inline void change_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
"btcl %1,%0"
- :"=m" (ADDR)
+ :"=m" (*addr)
:"r" (nr));
}
@@ -159,13 +157,13 @@ static inline void change_bit(int nr, volatile unsigned
long * addr)
*
* See __test_and_set_bit() if you do not require the atomic guarantees.
*/
-static inline int test_and_set_bit(int nr, volatile unsigned long * addr)
+static inline int test_and_set_bit(int nr, unsigned long *addr)
{
int oldbit;
__asm__ __volatile__( LOCK_PREFIX
"btsl %2,%1\n\tsbbl %0,%0"
- :"=r" (oldbit),"=m" (ADDR)
+ :"=r" (oldbit),"=m" (*addr)
:"r" (nr) : "memory");
[PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
From: Satyam Sharma [EMAIL PROTECTED]
[4/8] i386: bitops: Kill volatile-casting of memory addresses
All the occurrences of volatile that are used to qualify access to the
passed bit-string pointer/address must be removed, because volatile is
crazy, doesn't really work anyway, has nothing to do with locking and
atomicity, and is in general wholly unnecessary for these operations:
* In the case of the unlocked variants, we're not providing any guarantees
whatsoever, so we want the caller to do any necessary locking surrounding
the use of that function anyway.
* In case of the primitives where we *do* make locking/atomicity/reordering
(three very different, but related, concepts) guarantees, we're doing that
properly by using the lock instruction-prefix and __asm__ __volatile__
anyway (and with addr always being constrained with m in the assembly,
gcc doesn't bring it into a local register either).
So let's just kill the pointless use of volatile.
Signed-off-by: Satyam Sharma [EMAIL PROTECTED]
Cc: David Howells [EMAIL PROTECTED]
Cc: Nick Piggin [EMAIL PROTECTED]
---
include/asm-i386/bitops.h | 60 +
1 files changed, 28 insertions(+), 32 deletions(-)
diff --git a/include/asm-i386/bitops.h b/include/asm-i386/bitops.h
index d623fd9..0f5634b 100644
--- a/include/asm-i386/bitops.h
+++ b/include/asm-i386/bitops.h
@@ -26,8 +26,6 @@
* on single-dword quantities.
*/
-#define ADDR (*(volatile long *) addr)
-
/**
* set_bit - Atomically set a bit in memory
* @nr: the bit to set
@@ -38,11 +36,11 @@
*
* See __set_bit() if you do not require the atomic guarantees.
*/
-static inline void set_bit(int nr, volatile unsigned long * addr)
+static inline void set_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
btsl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -58,11 +56,11 @@ static inline void set_bit(int nr, volatile unsigned long *
addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __set_bit(int nr, volatile unsigned long * addr)
+static inline void __set_bit(int nr, unsigned long *addr)
{
__asm__(
btsl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -76,11 +74,11 @@ static inline void __set_bit(int nr, volatile unsigned long
* addr)
*
* See __clear_bit() if you do not require the atomic guarantees.
*/
-static inline void clear_bit(int nr, volatile unsigned long * addr)
+static inline void clear_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
btrl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -96,11 +94,11 @@ static inline void clear_bit(int nr, volatile unsigned long
* addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __clear_bit(int nr, volatile unsigned long * addr)
+static inline void __clear_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__(
btrl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -123,11 +121,11 @@ static inline void __clear_bit(int nr, volatile unsigned
long * addr)
* This is a fast and unlocked operation, and only suitable for callers
* that already implement higher-level locking to protect access.
*/
-static inline void __change_bit(int nr, volatile unsigned long * addr)
+static inline void __change_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__(
btcl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -141,11 +139,11 @@ static inline void __change_bit(int nr, volatile unsigned
long * addr)
*
* See __change_bit() if you do not require the atomic guarantees.
*/
-static inline void change_bit(int nr, volatile unsigned long * addr)
+static inline void change_bit(int nr, unsigned long *addr)
{
__asm__ __volatile__( LOCK_PREFIX
btcl %1,%0
- :=m (ADDR)
+ :=m (*addr)
:r (nr));
}
@@ -159,13 +157,13 @@ static inline void change_bit(int nr, volatile unsigned
long * addr)
*
* See __test_and_set_bit() if you do not require the atomic guarantees.
*/
-static inline int test_and_set_bit(int nr, volatile unsigned long * addr)
+static inline int test_and_set_bit(int nr, unsigned long *addr)
{
int oldbit;
__asm__ __volatile__( LOCK_PREFIX
btsl %2,%1\n\tsbbl %0,%0
- :=r (oldbit),=m (ADDR)
+ :=r (oldbit),=m (*addr)
:r (nr) : memory);
return oldbit;
}
@@ -182,13 +180,13 @@ static inline int
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
On Mon, 23 Jul 2007, Satyam Sharma wrote: [4/8] i386: bitops: Kill volatile-casting of memory addresses This is wrong. The const volatile is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is const volatile. In other words, the volatile has nothing at all to do with whether the memory is volatile or not (the same way const has nothing to do with it: it's purely a C type *safety* issue, exactly the same way const is a type safety issue. A const on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to strlen(), it doesn't have to be constant. But strlen() takes a const pointer, because it work son constant pointers *too*. Same deal here. Admittedly this may be mostly historic, but regardless - the volatiles are right. Using volatile on *data* is generally considered incorrect and bad taste, but using it in situations like this potentially makes sense. Of course, if we remove all volatiles in data in the kernel (with the possible exception of jiffies), we can then remove them from function declarations too, but it should be done in that order. Linus - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 4/8] i386: bitops: Kill volatile-casting of memory addresses
Linus Torvalds wrote: On Mon, 23 Jul 2007, Satyam Sharma wrote: [4/8] i386: bitops: Kill volatile-casting of memory addresses This is wrong. The const volatile is so that you can pass an arbitrary pointer. The only kind of abritraty pointer is const volatile. In other words, the volatile has nothing at all to do with whether the memory is volatile or not (the same way const has nothing to do with it: it's purely a C type *safety* issue, exactly the same way const is a type safety issue. A const on a pointer doesn't mean that the thing it points to cannot change. When you pass a source pointer to strlen(), it doesn't have to be constant. But strlen() takes a const pointer, because it work son constant pointers *too*. Same deal here. Admittedly this may be mostly historic, but regardless - the volatiles are right. Using volatile on *data* is generally considered incorrect and bad taste, but using it in situations like this potentially makes sense. Of course, if we remove all volatiles in data in the kernel (with the possible exception of jiffies), we can then remove them from function declarations too, but it should be done in that order. Well, regardless, it still forces the function to treat the pointer target as volatile, won't it? It definitely prevents valid optimisations that would be useful for me in mm/page_alloc.c where page flags are being set up or torn down or checked with non-atomic bitops. OK, not the i386 functions as much because they are all in asm anwyay, but in general (btw. why does i386 or any architecture define their own non-atomic bitops? If the version in asm-generic/bitops/non-atomic.h is not good enough then surely it is a bug in gcc or that file?) Anyway by type safety, do you mean it will stop the compiler from warning if a pointer to a volatile is passed to the bitop? If so, then why don't we just kill all the volatiles out of here and fix any warnings that comeup? I doubt there would be many, and of those, some might show up real synchronisation problems. -- SUSE Labs, Novell Inc. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
