From: Eric Dumazet <hidden> Date: 2012-01-03 15:00:39
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 <redacted>
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.
From: Eric Dumazet <hidden> Date: 2012-01-03 15:15:23
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
From: Jan Beulich <hidden> Date: 2012-01-03 15:34:53
quoted
quoted
On 03.01.12 at 16:00, Eric Dumazet [off-list ref] wrote:
Le lundi 02 janvier 2012 à 17:02 +0000, Jan Beulich a écrit :
quoted
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 <redacted>
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
From: Eric Dumazet <hidden> Date: 2012-01-03 15:41:38
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(-)
From: Jan Beulich <hidden> Date: 2012-01-03 16:07:53
quoted
quoted
On 03.01.12 at 16:41, Eric Dumazet [off-list ref] wrote:
Le mardi 03 janvier 2012 à 16:15 +0100, Eric Dumazet a écrit :
quoted
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
From: Eric Dumazet <hidden> Date: 2012-01-03 16:13:48
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
From: Eric Dumazet <hidden> Date: 2012-01-03 16:35:46
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 <redacted>
Acked-by: Jan Beulich <redacted>
CC: Christoph Lameter <redacted>
CC: Ingo Molnar <redacted>
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(-)
From: Eric Dumazet <hidden> Date: 2012-01-03 17:31:46
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.
From: David Laight <hidden> Date: 2012-01-04 10:38:23
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