Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Eric Dumazet]

Le lundi 02 janvier 2012 à 17:02 +0000, Jan Beulich a écrit :
> Just like the per-CPU ones they had several problems/shortcomings:
> 
> Only the first memory operand was mentioned in the asm() operands, and
> the 2x64-bit version didn't have a memory clobber while the 2x32-bit
> one did. The former allowed the compiler to not recognize the need to
> re-load the data in case it had it cached in some register, while the
> latter was overly destructive.
> 
> The types of the local copies of the old and new values were incorrect
> (the types of the pointed-to variables should be used here, to make
> sure the respective old/new variable types are compatible).
> 
> The __dummy/__junk variables were pointless, given that local copies
> of the inputs already existed (and can hence be used for discarded
> outputs).
> 
> The 32-bit variant of cmpxchg_double_local() referenced
> cmpxchg16b_local().
> 
> At once also
> - change the return value type to what it really is: 'bool'
> - unify 32- and 64-bit variants
> - abstract out the common part of the 'normal' and 'local' variants
> 
> Signed-off-by: Jan Beulich <jbeulich@suse.com>

While looking at your patch, I discovered that atomic64_add() /
atomic64_inc() on 32bit are completely buggy. Oh well...

Generated code :

c03bc00c <atomic64_add_return_cx8>:
c03bc00c:       55                      push   %ebp
c03bc00d:       53                      push   %ebx
c03bc00e:       56                      push   %esi
c03bc00f:       57                      push   %edi
c03bc010:       89 c6                   mov    %eax,%esi
c03bc012:       89 d7                   mov    %edx,%edi
c03bc014:       89 cd                   mov    %ecx,%ebp
c03bc016:       89 d8                   mov    %ebx,%eax
c03bc018:       89 ca                   mov    %ecx,%edx
c03bc01a:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
c03bc01f:       89 c3                   mov    %eax,%ebx
c03bc021:       89 d1                   mov    %edx,%ecx
c03bc023:       01 f3                   add    %esi,%ebx
c03bc025:       11 f9                   adc    %edi,%ecx
c03bc027:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
c03bc02c:       75 f9                   jne    c03bc027 <atomic64_add_return_cx8+0x1b>
c03bc02e:       89 d8                   mov    %ebx,%eax
c03bc030:       89 ca                   mov    %ecx,%edx
c03bc032:       5f                      pop    %edi
c03bc033:       5e                      pop    %esi
c03bc034:       5b                      pop    %ebx
c03bc035:       5d                      pop    %ebp
c03bc036:       c3                      ret

The ' jne c03bc027' should really be 'jne c03bc01f'

No idea how old is this bug.

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Eric Dumazet]

Le mardi 03 janvier 2012 à 16:00 +0100, Eric Dumazet a écrit :

> While looking at your patch, I discovered that atomic64_add() /
> atomic64_inc() on 32bit are completely buggy. Oh well...
> 
> Generated code :
> 
> c03bc00c <atomic64_add_return_cx8>:
> c03bc00c:       55                      push   %ebp
> c03bc00d:       53                      push   %ebx
> c03bc00e:       56                      push   %esi
> c03bc00f:       57                      push   %edi
> c03bc010:       89 c6                   mov    %eax,%esi
> c03bc012:       89 d7                   mov    %edx,%edi
> c03bc014:       89 cd                   mov    %ecx,%ebp
> c03bc016:       89 d8                   mov    %ebx,%eax
> c03bc018:       89 ca                   mov    %ecx,%edx
> c03bc01a:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
> c03bc01f:       89 c3                   mov    %eax,%ebx
> c03bc021:       89 d1                   mov    %edx,%ecx
> c03bc023:       01 f3                   add    %esi,%ebx
> c03bc025:       11 f9                   adc    %edi,%ecx
> c03bc027:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
> c03bc02c:       75 f9                   jne    c03bc027 <atomic64_add_return_cx8+0x1b>
> c03bc02e:       89 d8                   mov    %ebx,%eax
> c03bc030:       89 ca                   mov    %ecx,%edx
> c03bc032:       5f                      pop    %edi
> c03bc033:       5e                      pop    %esi
> c03bc034:       5b                      pop    %ebx
> c03bc035:       5d                      pop    %ebp
> c03bc036:       c3                      ret
> 
> The ' jne c03bc027' should really be 'jne c03bc01f'
> 
> No idea how old is this bug.
> 

Very old it seems...

arch/x86/lib/atomic64_cx8_32.S

all "jxx 1b" are wrong if a LOCK_PREFIX is included after the 1: label

1:
	inst1
	LOCK_PREFIX
	cmpxchg8b (%ebp)
	jne 1b  / jumps to beginning of LOCK_PREFIX, inst1 is not replayed

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Eric Dumazet]

Le mardi 03 janvier 2012 à 16:15 +0100, Eric Dumazet a écrit :

> Very old it seems...
> 
> arch/x86/lib/atomic64_cx8_32.S
> 
> all "jxx 1b" are wrong if a LOCK_PREFIX is included after the 1: label
> 
> 1:
> 	inst1
> 	LOCK_PREFIX
> 	cmpxchg8b (%ebp)
> 	jne 1b  / jumps to beginning of LOCK_PREFIX, inst1 is not replayed
> 
> 
> 
> 

A possible fix would be to not use "1" label in LOCK_PREFIX macro,
but 672 magic value.

Not sure if we can use a local label in a macro ?

 arch/x86/include/asm/alternative-asm.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/alternative-asm.h b/arch/x86/include/asm/alternative-asm.h
index 091508b..952bd01 100644
--- a/arch/x86/include/asm/alternative-asm.h
+++ b/arch/x86/include/asm/alternative-asm.h
@@ -4,10 +4,10 @@
 
 #ifdef CONFIG_SMP
 	.macro LOCK_PREFIX
-1:	lock
+672:	lock
 	.section .smp_locks,"a"
 	.balign 4
-	.long 1b - .
+	.long 672b - .
 	.previous
 	.endm
 #else

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Jan Beulich]

>>> On 03.01.12 at 16:41, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mardi 03 janvier 2012 à 16:15 +0100, Eric Dumazet a écrit :
> 
>> Very old it seems...
>> 
>> arch/x86/lib/atomic64_cx8_32.S
>> 
>> all "jxx 1b" are wrong if a LOCK_PREFIX is included after the 1: label
>> 
>> 1:
>> 	inst1
>> 	LOCK_PREFIX
>> 	cmpxchg8b (%ebp)
>> 	jne 1b  / jumps to beginning of LOCK_PREFIX, inst1 is not replayed
>> 
>> 
>> 
>> 
> 
> A possible fix would be to not use "1" label in LOCK_PREFIX macro,
> but 672 magic value.
> 
> Not sure if we can use a local label in a macro ?

"1" and "672" are both local labels, so both are okay. As long as there's
no other (colliding) use of 672 anywhere, that would seem to be the
preferred fix (feel free to put my ack on the patch when you formally
submit it).

Jan

>  arch/x86/include/asm/alternative-asm.h |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/include/asm/alternative-asm.h 
> b/arch/x86/include/asm/alternative-asm.h
> index 091508b..952bd01 100644
> --- a/arch/x86/include/asm/alternative-asm.h
> +++ b/arch/x86/include/asm/alternative-asm.h
> @@ -4,10 +4,10 @@
>  
>  #ifdef CONFIG_SMP
>  	.macro LOCK_PREFIX
> -1:	lock
> +672:	lock
>  	.section .smp_locks,"a"
>  	.balign 4
> -	.long 1b - .
> +	.long 672b - .
>  	.previous
>  	.endm
>  #else

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Eric Dumazet]

Le mardi 03 janvier 2012 à 16:08 +0000, Jan Beulich a écrit :

> "1" and "672" are both local labels, so both are okay. As long as there's
> no other (colliding) use of 672 anywhere, that would seem to be the
> preferred fix (feel free to put my ack on the patch when you formally
> submit it).

I was referring the use of a label local to the macro itself, with
restricted scope.

following psudi code would trigger an asm error :

	.macro FOO
	.local_label 1
1:	lock
        .section .smp_locks,"a"
        .balign 4
        .long 1b - .
        .previous
	.endm


	FOO
	jne 1b

[PATCH] x86: fix atomic64_xxx_cx8() functions

[Eric Dumazet]

It appears about all functions in arch/x86/lib/atomic64_cx8_32.S are
wrong in case cmpxchg8b must be restarted, because LOCK_PREFIX macro
defines a label "1" clashing with other local labels :

1:
	some_instructions
	LOCK_PREFIX
	cmpxchg8b (%ebp)
	jne 1b  / jumps to beginning of LOCK_PREFIX !

A possible fix is to use a magic label "672" in LOCK_PREFIX asm
definition, similar to the "671" one we defined in LOCK_PREFIX_HERE. 

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
Acked-by: Jan Beulich <JBeulich@suse.com>
CC: Christoph Lameter <cl@linux.com>
CC: Ingo Molnar <mingo@elte.hu>
CC: H. Peter Anvin <hpa@zytor.com>
Cc: stable@vger.kernel.org  [2.6.35+]
---
 arch/x86/include/asm/alternative-asm.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/arch/x86/include/asm/alternative-asm.h b/arch/x86/include/asm/alternative-asm.h
index 091508b..952bd01 100644
--- a/arch/x86/include/asm/alternative-asm.h
+++ b/arch/x86/include/asm/alternative-asm.h
@@ -4,10 +4,10 @@
 
 #ifdef CONFIG_SMP
 	.macro LOCK_PREFIX
-1:	lock
+672:	lock
 	.section .smp_locks,"a"
 	.balign 4
-	.long 1b - .
+	.long 672b - .
 	.previous
 	.endm
 #else

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Jan Beulich]

>>> On 03.01.12 at 16:00, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le lundi 02 janvier 2012 à 17:02 +0000, Jan Beulich a écrit :
>> Just like the per-CPU ones they had several problems/shortcomings:
>> 
>> Only the first memory operand was mentioned in the asm() operands, and
>> the 2x64-bit version didn't have a memory clobber while the 2x32-bit
>> one did. The former allowed the compiler to not recognize the need to
>> re-load the data in case it had it cached in some register, while the
>> latter was overly destructive.
>> 
>> The types of the local copies of the old and new values were incorrect
>> (the types of the pointed-to variables should be used here, to make
>> sure the respective old/new variable types are compatible).
>> 
>> The __dummy/__junk variables were pointless, given that local copies
>> of the inputs already existed (and can hence be used for discarded
>> outputs).
>> 
>> The 32-bit variant of cmpxchg_double_local() referenced
>> cmpxchg16b_local().
>> 
>> At once also
>> - change the return value type to what it really is: 'bool'
>> - unify 32- and 64-bit variants
>> - abstract out the common part of the 'normal' and 'local' variants
>> 
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> 
> While looking at your patch, I discovered that atomic64_add() /
> atomic64_inc() on 32bit are completely buggy. Oh well...
> 
> Generated code :
> 
> c03bc00c <atomic64_add_return_cx8>:
> c03bc00c:       55                      push   %ebp
> c03bc00d:       53                      push   %ebx
> c03bc00e:       56                      push   %esi
> c03bc00f:       57                      push   %edi
> c03bc010:       89 c6                   mov    %eax,%esi
> c03bc012:       89 d7                   mov    %edx,%edi
> c03bc014:       89 cd                   mov    %ecx,%ebp
> c03bc016:       89 d8                   mov    %ebx,%eax
> c03bc018:       89 ca                   mov    %ecx,%edx
> c03bc01a:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
> c03bc01f:       89 c3                   mov    %eax,%ebx
> c03bc021:       89 d1                   mov    %edx,%ecx
> c03bc023:       01 f3                   add    %esi,%ebx
> c03bc025:       11 f9                   adc    %edi,%ecx
> c03bc027:       f0 0f c7 4d 00          lock cmpxchg8b 0x0(%ebp)
> c03bc02c:       75 f9                   jne    c03bc027 
> <atomic64_add_return_cx8+0x1b>
> c03bc02e:       89 d8                   mov    %ebx,%eax
> c03bc030:       89 ca                   mov    %ecx,%edx
> c03bc032:       5f                      pop    %edi
> c03bc033:       5e                      pop    %esi
> c03bc034:       5b                      pop    %ebx
> c03bc035:       5d                      pop    %ebp
> c03bc036:       c3                      ret
> 
> The ' jne c03bc027' should really be 'jne c03bc01f'

Indeed, and that's the same for all other routines in this file that
incorrectly use 1: together with LOCK_PREFIX between the label and
an intended jump to that label.

> No idea how old is this bug.

The file (and with it the bug) was introduced in 2.6.35.

While looking at this I also noticed this comment in read64: "we need
LOCK_PREFIX since otherwise cmpxchg8b always does the write",
which is saying quite the opposite of the Intel manual: "This instruction
can be used with a LOCK prefix to allow the instruction to be executed
atomically. To simplify the interface to the processor’s bus, the
destination operand receives a write cycle without regard to the result
of the comparison. The destination operand is written back if the
comparison fails; otherwise, the source operand is written into the
destination. (The processor never produces a locked read without
also producing a locked write.)" - I would conclude the LOCK prefix
actually hurts there.

And in atomic64_set_cx8 it's the other way around: The comment
explains why supposedly no LOCK prefix is needed, but that's again
in conflict with above quoted paragraph from the manual.

Jan

Re: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Eric Dumazet]

Le mardi 03 janvier 2012 à 15:35 +0000, Jan Beulich a écrit :

> And in atomic64_set_cx8 it's the other way around: The comment
> explains why supposedly no LOCK prefix is needed, but that's again
> in conflict with above quoted paragraph from the manual.

BTW atomic64_set() asm() contraints are wrong :

static inline void atomic64_set(atomic64_t *v, long long i)
{
        unsigned high = (unsigned)(i >> 32);
        unsigned low = (unsigned)i;
        asm volatile(ATOMIC64_ALTERNATIVE(set)
                     : "+b" (low), "+c" (high)
                     : "S" (v)
                     : "eax", "edx", "memory"
                     );
}


ebx/ecx registers are not modified by cmpxchg8b (or the atomic64_set_386
emulation). Only eax/edx can be modified.

RE: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[David Laight]

 
> BTW atomic64_set() asm() contraints are wrong :
> 
> static inline void atomic64_set(atomic64_t *v, long long i)
> {
>         unsigned high = (unsigned)(i >> 32);
>         unsigned low = (unsigned)i;
>         asm volatile(ATOMIC64_ALTERNATIVE(set)
>                      : "+b" (low), "+c" (high)
>                      : "S" (v)
>                      : "eax", "edx", "memory"
>                      );
> }
> 
> 
> ebx/ecx registers are not modified by cmpxchg8b (or the 
> atomic64_set_386 emulation). Only eax/edx can be modified.

Isn't it also possible to constrain the "memory"
constraint to only apply to '*v' not all of memory?
I can't remember the syntax off hand though...

	David

RE: [PATCH] x86: fix and improve cmpxchg_double{,_local}()

[Jan Beulich]

 >>> On 04.01.12 at 11:36, "David Laight" <David.Laight@ACULAB.COM> wrote:

>> BTW atomic64_set() asm() contraints are wrong :
>> 
>> static inline void atomic64_set(atomic64_t *v, long long i)
>> {
>>         unsigned high = (unsigned)(i >> 32);
>>         unsigned low = (unsigned)i;
>>         asm volatile(ATOMIC64_ALTERNATIVE(set)
>>                      : "+b" (low), "+c" (high)
>>                      : "S" (v)
>>                      : "eax", "edx", "memory"
>>                      );
>> }
>> 
>> 
>> ebx/ecx registers are not modified by cmpxchg8b (or the 
>> atomic64_set_386 emulation). Only eax/edx can be modified.

Same would be true for atomic64_xchg() and the use of "+c" (v)
in subsequent functions (whether unnecessarily strict or too lax
varies).

> Isn't it also possible to constrain the "memory"
> constraint to only apply to '*v' not all of memory?
> I can't remember the syntax off hand though...

Absolutely - "=m" (v->counter) would be the right way to specify
this.

Jan