* [PATCH] x86: simplify non-atomic bitops
@ 2015-02-11 13:39 Jan Beulich
2015-02-11 15:14 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-02-11 13:39 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 4773 bytes --]
- being non-atomic, their pointer arguments shouldn't be volatile-
qualified
- their (half fake) memory operands can be a single "+m" instead of
being both an output and an input
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop "+m" related sentence from comment at the top of the file as
being wrong (the referenced indication in gcc's documentation got
removed quite some time ago too).
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -14,8 +14,7 @@
* operand is both read from and written to. Since the operand is in fact a
* word array, we also specify "memory" in the clobbers list to indicate that
* words other than the one directly addressed by the memory operand may be
- * modified. We don't use "+m" because the gcc manual says that it should be
- * used only when the constraint allows the operand to reside in a register.
+ * modified.
*/
#define ADDR (*(volatile long *) addr)
@@ -55,12 +54,9 @@ static inline void set_bit(int nr, volat
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __set_bit(int nr, volatile void *addr)
+static inline void __set_bit(int nr, void *addr)
{
- asm volatile (
- "btsl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __set_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -95,12 +91,9 @@ static inline void clear_bit(int nr, vol
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __clear_bit(int nr, volatile void *addr)
+static inline void __clear_bit(int nr, void *addr)
{
- asm volatile (
- "btrl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __clear_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -116,12 +109,9 @@ static inline void __clear_bit(int nr, v
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __change_bit(int nr, volatile void *addr)
+static inline void __change_bit(int nr, void *addr)
{
- asm volatile (
- "btcl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __change_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -181,14 +171,14 @@ static inline int test_and_set_bit(int n
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_set_bit(int nr, volatile void *addr)
+static inline int __test_and_set_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btsl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_set_bit(nr, addr) ({ \
@@ -228,14 +218,14 @@ static inline int test_and_clear_bit(int
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_clear_bit(int nr, volatile void *addr)
+static inline int __test_and_clear_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btrl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_clear_bit(nr, addr) ({ \
@@ -244,14 +234,14 @@ static inline int __test_and_clear_bit(i
})
/* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_bit(int nr, volatile void *addr)
+static inline int __test_and_change_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btcl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_change_bit(nr, addr) ({ \
[-- Attachment #2: x86-non-atomic-bitops.patch --]
[-- Type: text/plain, Size: 4804 bytes --]
x86: simplify non-atomic bitops
- being non-atomic, their pointer arguments shouldn't be volatile-
qualified
- their (half fake) memory operands can be a single "+m" instead of
being both an output and an input
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
v2: Drop "+m" related sentence from comment at the top of the file as
being wrong (the referenced indication in gcc's documentation got
removed quite some time ago too).
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -14,8 +14,7 @@
* operand is both read from and written to. Since the operand is in fact a
* word array, we also specify "memory" in the clobbers list to indicate that
* words other than the one directly addressed by the memory operand may be
- * modified. We don't use "+m" because the gcc manual says that it should be
- * used only when the constraint allows the operand to reside in a register.
+ * modified.
*/
#define ADDR (*(volatile long *) addr)
@@ -55,12 +54,9 @@ static inline void set_bit(int nr, volat
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __set_bit(int nr, volatile void *addr)
+static inline void __set_bit(int nr, void *addr)
{
- asm volatile (
- "btsl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __set_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -95,12 +91,9 @@ static inline void clear_bit(int nr, vol
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __clear_bit(int nr, volatile void *addr)
+static inline void __clear_bit(int nr, void *addr)
{
- asm volatile (
- "btrl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __clear_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -116,12 +109,9 @@ static inline void __clear_bit(int nr, v
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __change_bit(int nr, volatile void *addr)
+static inline void __change_bit(int nr, void *addr)
{
- asm volatile (
- "btcl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __change_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -181,14 +171,14 @@ static inline int test_and_set_bit(int n
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_set_bit(int nr, volatile void *addr)
+static inline int __test_and_set_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btsl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_set_bit(nr, addr) ({ \
@@ -228,14 +218,14 @@ static inline int test_and_clear_bit(int
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_clear_bit(int nr, volatile void *addr)
+static inline int __test_and_clear_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btrl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_clear_bit(nr, addr) ({ \
@@ -244,14 +234,14 @@ static inline int __test_and_clear_bit(i
})
/* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_bit(int nr, volatile void *addr)
+static inline int __test_and_change_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btcl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR)
+ : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_change_bit(nr, addr) ({ \
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: simplify non-atomic bitops
2015-02-11 13:39 [PATCH] x86: simplify non-atomic bitops Jan Beulich
@ 2015-02-11 15:14 ` Andrew Cooper
2015-02-11 15:41 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2015-02-11 15:14 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 11/02/15 13:39, Jan Beulich wrote:
> - being non-atomic, their pointer arguments shouldn't be volatile-
> qualified
> - their (half fake) memory operands can be a single "+m" instead of
> being both an output and an input
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> v2: Drop "+m" related sentence from comment at the top of the file as
> being wrong (the referenced indication in gcc's documentation got
> removed quite some time ago too).
>
> --- a/xen/include/asm-x86/bitops.h
> +++ b/xen/include/asm-x86/bitops.h
> @@ -14,8 +14,7 @@
> * operand is both read from and written to. Since the operand is in fact a
> * word array, we also specify "memory" in the clobbers list to indicate that
> * words other than the one directly addressed by the memory operand may be
> - * modified. We don't use "+m" because the gcc manual says that it should be
> - * used only when the constraint allows the operand to reside in a register.
> + * modified.
> */
>
> #define ADDR (*(volatile long *) addr)
> @@ -55,12 +54,9 @@ static inline void set_bit(int nr, volat
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __set_bit(int nr, volatile void *addr)
> +static inline void __set_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btsl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
You presumably want to s/ADDR/addr/ to avoid re-gaining the volatile
attribute on the pointer?
~Andrew
> }
> #define __set_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -95,12 +91,9 @@ static inline void clear_bit(int nr, vol
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __clear_bit(int nr, volatile void *addr)
> +static inline void __clear_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btrl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
> }
> #define __clear_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -116,12 +109,9 @@ static inline void __clear_bit(int nr, v
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __change_bit(int nr, volatile void *addr)
> +static inline void __change_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btcl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
> }
> #define __change_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -181,14 +171,14 @@ static inline int test_and_set_bit(int n
> * If two examples of this operation race, one can appear to succeed
> * but actually fail. You must protect multiple accesses with a lock.
> */
> -static inline int __test_and_set_bit(int nr, volatile void *addr)
> +static inline int __test_and_set_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btsl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR)
> + : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_set_bit(nr, addr) ({ \
> @@ -228,14 +218,14 @@ static inline int test_and_clear_bit(int
> * If two examples of this operation race, one can appear to succeed
> * but actually fail. You must protect multiple accesses with a lock.
> */
> -static inline int __test_and_clear_bit(int nr, volatile void *addr)
> +static inline int __test_and_clear_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btrl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR)
> + : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_clear_bit(nr, addr) ({ \
> @@ -244,14 +234,14 @@ static inline int __test_and_clear_bit(i
> })
>
> /* WARNING: non atomic and it can be reordered! */
> -static inline int __test_and_change_bit(int nr, volatile void *addr)
> +static inline int __test_and_change_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btcl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR)
> + : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_change_bit(nr, addr) ({ \
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: simplify non-atomic bitops
2015-02-11 15:14 ` Andrew Cooper
@ 2015-02-11 15:41 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2015-02-11 15:41 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 11.02.15 at 16:14, <andrew.cooper3@citrix.com> wrote:
> On 11/02/15 13:39, Jan Beulich wrote:
>> @@ -55,12 +54,9 @@ static inline void set_bit(int nr, volat
>> * If it's called on the same region of memory simultaneously, the effect
>> * may be that only one operation succeeds.
>> */
>> -static inline void __set_bit(int nr, volatile void *addr)
>> +static inline void __set_bit(int nr, void *addr)
>> {
>> - asm volatile (
>> - "btsl %1,%0"
>> - : "=m" (ADDR)
>> - : "Ir" (nr), "m" (ADDR) : "memory");
>> + asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
>
> You presumably want to s/ADDR/addr/ to avoid re-gaining the volatile
> attribute on the pointer?
I don't think it makes a difference, but yes, that would get things
into more consistent shape. Looking at it again the memory
operand could also legitimately be just an input, as we have a
memory clobber.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH] x86: simplify non-atomic bitops
@ 2015-01-19 15:52 Jan Beulich
2015-01-19 17:21 ` Andrew Cooper
0 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2015-01-19 15:52 UTC (permalink / raw)
To: xen-devel; +Cc: Andrew Cooper, Keir Fraser
[-- Attachment #1: Type: text/plain, Size: 4081 bytes --]
- being non-atomic, their pointer arguments shouldn't be volatile-
qualified
- their (half fake) memory operands can be a single "+m" instead of
being both an output and an input
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -53,12 +53,9 @@ static inline void set_bit(int nr, volat
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __set_bit(int nr, volatile void *addr)
+static inline void __set_bit(int nr, void *addr)
{
- asm volatile (
- "btsl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __set_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -93,12 +90,9 @@ static inline void clear_bit(int nr, vol
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __clear_bit(int nr, volatile void *addr)
+static inline void __clear_bit(int nr, void *addr)
{
- asm volatile (
- "btrl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __clear_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -114,12 +108,9 @@ static inline void __clear_bit(int nr, v
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __change_bit(int nr, volatile void *addr)
+static inline void __change_bit(int nr, void *addr)
{
- asm volatile (
- "btcl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __change_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -179,14 +170,13 @@ static inline int test_and_set_bit(int n
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_set_bit(int nr, volatile void *addr)
+static inline int __test_and_set_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btsl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_set_bit(nr, addr) ({ \
@@ -226,14 +216,13 @@ static inline int test_and_clear_bit(int
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_clear_bit(int nr, volatile void *addr)
+static inline int __test_and_clear_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btrl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_clear_bit(nr, addr) ({ \
@@ -242,14 +231,13 @@ static inline int __test_and_clear_bit(i
})
/* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_bit(int nr, volatile void *addr)
+static inline int __test_and_change_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btcl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_change_bit(nr, addr) ({ \
[-- Attachment #2: x86-non-atomic-bitops.patch --]
[-- Type: text/plain, Size: 4110 bytes --]
x86: simplify non-atomic bitops
- being non-atomic, their pointer arguments shouldn't be volatile-
qualified
- their (half fake) memory operands can be a single "+m" instead of
being both an output and an input
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/include/asm-x86/bitops.h
+++ b/xen/include/asm-x86/bitops.h
@@ -53,12 +53,9 @@ static inline void set_bit(int nr, volat
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __set_bit(int nr, volatile void *addr)
+static inline void __set_bit(int nr, void *addr)
{
- asm volatile (
- "btsl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __set_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -93,12 +90,9 @@ static inline void clear_bit(int nr, vol
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __clear_bit(int nr, volatile void *addr)
+static inline void __clear_bit(int nr, void *addr)
{
- asm volatile (
- "btrl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __clear_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -114,12 +108,9 @@ static inline void __clear_bit(int nr, v
* If it's called on the same region of memory simultaneously, the effect
* may be that only one operation succeeds.
*/
-static inline void __change_bit(int nr, volatile void *addr)
+static inline void __change_bit(int nr, void *addr)
{
- asm volatile (
- "btcl %1,%0"
- : "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
}
#define __change_bit(nr, addr) ({ \
if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
@@ -179,14 +170,13 @@ static inline int test_and_set_bit(int n
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_set_bit(int nr, volatile void *addr)
+static inline int __test_and_set_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btsl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_set_bit(nr, addr) ({ \
@@ -226,14 +216,13 @@ static inline int test_and_clear_bit(int
* If two examples of this operation race, one can appear to succeed
* but actually fail. You must protect multiple accesses with a lock.
*/
-static inline int __test_and_clear_bit(int nr, volatile void *addr)
+static inline int __test_and_clear_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btrl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_clear_bit(nr, addr) ({ \
@@ -242,14 +231,13 @@ static inline int __test_and_clear_bit(i
})
/* WARNING: non atomic and it can be reordered! */
-static inline int __test_and_change_bit(int nr, volatile void *addr)
+static inline int __test_and_change_bit(int nr, void *addr)
{
int oldbit;
asm volatile (
"btcl %2,%1\n\tsbbl %0,%0"
- : "=r" (oldbit), "=m" (ADDR)
- : "Ir" (nr), "m" (ADDR) : "memory");
+ : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
return oldbit;
}
#define __test_and_change_bit(nr, addr) ({ \
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: simplify non-atomic bitops
2015-01-19 15:52 Jan Beulich
@ 2015-01-19 17:21 ` Andrew Cooper
2015-01-20 8:09 ` Jan Beulich
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2015-01-19 17:21 UTC (permalink / raw)
To: Jan Beulich, xen-devel; +Cc: Keir Fraser
On 19/01/15 15:52, Jan Beulich wrote:
> - being non-atomic, their pointer arguments shouldn't be volatile-
> qualified
> - their (half fake) memory operands can be a single "+m" instead of
> being both an output and an input
>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
There is a note at the top of the file describing why "+m" is
specifically not used. I have not looked into the reason yet, but a
patch like this at leasts needs an adjustment to the comment if you
believe it to be safe.
~Andrew
>
> --- a/xen/include/asm-x86/bitops.h
> +++ b/xen/include/asm-x86/bitops.h
> @@ -53,12 +53,9 @@ static inline void set_bit(int nr, volat
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __set_bit(int nr, volatile void *addr)
> +static inline void __set_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btsl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btsl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
> }
> #define __set_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -93,12 +90,9 @@ static inline void clear_bit(int nr, vol
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __clear_bit(int nr, volatile void *addr)
> +static inline void __clear_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btrl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btrl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
> }
> #define __clear_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -114,12 +108,9 @@ static inline void __clear_bit(int nr, v
> * If it's called on the same region of memory simultaneously, the effect
> * may be that only one operation succeeds.
> */
> -static inline void __change_bit(int nr, volatile void *addr)
> +static inline void __change_bit(int nr, void *addr)
> {
> - asm volatile (
> - "btcl %1,%0"
> - : "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + asm volatile ( "btcl %1,%0" : "+m" (ADDR) : "Ir" (nr) : "memory" );
> }
> #define __change_bit(nr, addr) ({ \
> if ( bitop_bad_size(addr) ) __bitop_bad_size(); \
> @@ -179,14 +170,13 @@ static inline int test_and_set_bit(int n
> * If two examples of this operation race, one can appear to succeed
> * but actually fail. You must protect multiple accesses with a lock.
> */
> -static inline int __test_and_set_bit(int nr, volatile void *addr)
> +static inline int __test_and_set_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btsl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_set_bit(nr, addr) ({ \
> @@ -226,14 +216,13 @@ static inline int test_and_clear_bit(int
> * If two examples of this operation race, one can appear to succeed
> * but actually fail. You must protect multiple accesses with a lock.
> */
> -static inline int __test_and_clear_bit(int nr, volatile void *addr)
> +static inline int __test_and_clear_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btrl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_clear_bit(nr, addr) ({ \
> @@ -242,14 +231,13 @@ static inline int __test_and_clear_bit(i
> })
>
> /* WARNING: non atomic and it can be reordered! */
> -static inline int __test_and_change_bit(int nr, volatile void *addr)
> +static inline int __test_and_change_bit(int nr, void *addr)
> {
> int oldbit;
>
> asm volatile (
> "btcl %2,%1\n\tsbbl %0,%0"
> - : "=r" (oldbit), "=m" (ADDR)
> - : "Ir" (nr), "m" (ADDR) : "memory");
> + : "=r" (oldbit), "+m" (ADDR) : "Ir" (nr) : "memory" );
> return oldbit;
> }
> #define __test_and_change_bit(nr, addr) ({ \
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] x86: simplify non-atomic bitops
2015-01-19 17:21 ` Andrew Cooper
@ 2015-01-20 8:09 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2015-01-20 8:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Keir Fraser
>>> On 19.01.15 at 18:21, <andrew.cooper3@citrix.com> wrote:
> On 19/01/15 15:52, Jan Beulich wrote:
>> - being non-atomic, their pointer arguments shouldn't be volatile-
>> qualified
>> - their (half fake) memory operands can be a single "+m" instead of
>> being both an output and an input
>>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>
> There is a note at the top of the file describing why "+m" is
> specifically not used. I have not looked into the reason yet, but a
> patch like this at leasts needs an adjustment to the comment if you
> believe it to be safe.
You mean this: "We don't use "+m" because the gcc manual says
that it should be used only when the constraint allows the operand
to reside in a register"? It looks wrong anyway, and in the current
gcc doc I verified that the respective statement (which indeed was
there in older versions) got deleted. I'm convinced that it was
there because on architectures where read-modify-write operations
on memory operands don't exist, this indeed is necessary. But we're
talking about x86-specific code here.
Linux changed this in 2006 (commit 92934bcbf9), but later added a
workaround for problems with gcc below 4.1 (which we don't care
about). Interestingly it was also in 2006 that Keir changed our copy
(12280:e10a13f13207).
Bottom line - I'll simply remove the comment as being wrong.
Jan
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2015-02-11 15:41 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-02-11 13:39 [PATCH] x86: simplify non-atomic bitops Jan Beulich
2015-02-11 15:14 ` Andrew Cooper
2015-02-11 15:41 ` Jan Beulich
-- strict thread matches above, loose matches on Subject: below --
2015-01-19 15:52 Jan Beulich
2015-01-19 17:21 ` Andrew Cooper
2015-01-20 8:09 ` Jan Beulich
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.