Re: [ros-dev] [ros-diffs] [tkreuzer] 51748: [NTOSKRNL] Patch by Paolo Bonzini <bonzini [at] gnu [dot] org> Fix implementation of ExInterlockedAddLargeStatistic The old version wasn't really atomic. See issue #6223 for more ...

[Alex Ionescu]

Congrats on once again breaking another key part of Windows compatibility.

This function is DOCUMENTED to tear. It was DESIGNED that way on purpose.

Utter failure, once more.

Best regards,
Alex Ionescu


On Sat, May 14, 2011 at 12:29 PM, <tkreuzer(a)svn.reactos.org> wrote:

>   Author: tkreuzer
>  Date: Sat May 14 19:29:42 2011
>  New Revision: 51748
>
>  URL: http://svn.reactos.org/svn/reactos?rev=51748&view=rev
>  Log:
>  [NTOSKRNL]
>  Patch by Paolo Bonzini <bonzini [at] gnu [dot] org>
>  Fix implementation of ExInterlockedAddLargeStatistic
>  The old version wasn't really atomic.
>
>  See issue #6223 for more details.
>
>  Modified:
>     trunk/reactos/ntoskrnl/ex/i386/fastinterlck_asm.S
>
>  Modified: trunk/reactos/ntoskrnl/ex/i386/fastinterlck_asm.S
>  URL:
>
> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/ex/i386/fastinter…
>
>  ==============================================================================
>  --- trunk/reactos/ntoskrnl/ex/i386/fastinterlck_asm.S [iso-8859-1]
>  (original)
>  +++ trunk/reactos/ntoskrnl/ex/i386/fastinterlck_asm.S [iso-8859-1] Sat May
>  14 19:29:42 2011
>  @@ -4,8 +4,9 @@
>   * FILE:            ntoskrnl/ex/i386/fastinterlck_asm.S
>   * PURPOSE:         FASTCALL Interlocked Functions
>   * PROGRAMMERS:     Alex Ionescu (alex(a)relsoft.net)
>  - */
>  -
>  + *                  Paolo Bonzini <bonzini [at] gnu [dot] org>
>  + */
>  +
>   /* INCLUDES
>  ******************************************************************/
>
>   #include <asm.inc>
>  @@ -31,25 +32,55 @@
>   */
>   PUBLIC @ExInterlockedAddLargeStatistic@8
>   @ExInterlockedAddLargeStatistic@8:
>  -
>  -#ifdef CONFIG_SMP
>  -    /* Do the addition */
>  -    lock add [ecx], edx
>  -
>  -    /* Check for carry bit and return */
>  -    jb .l1
>  -    ret
>  -
>  -.l1:
>  -    /* Add carry */
>  -    lock adc dword ptr [ecx+4], 0
>  -#else
>  -    /* Do the addition and add the carry */
>  -    add dword ptr [ecx], edx
>  -    adc dword ptr [ecx+4], 0
>  -#endif
>  -    /* Return */
>  -    ret
>  +    push ebp
>  +    push ebx
>  +    mov  ebp, ecx
>  +
>  +Again:
>  +    /* Load comparand in eax for cmpxchg */
>  +    mov  eax, [ebp]
>  +
>  +    /* Compute low word of the result in ebx */
>  +    mov  ebx, edx
>  +    add  ebx, eax
>  +
>  +    /* Carry needs cmpxchg8b */
>  +    jc   Slow
>  +
>  +    /* Fast path still needs to be atomic, so use cmpxchg and retry if it
>  fails
>  +     * Hopefully it will still get through this path :) */
>  +    LOCK cmpxchg [ecx], ebx
>  +    jnz  Again
>  +
>  +    /* Thats it */
>  +    pop  ebx
>  +    pop  ebp
>  +    ret
>  +
>  +Slow:
>  +    /* Save increment across cmpxchg8b */
>  +    push edx
>  +
>  +    /* Finish loading comparand in edx:eax */
>  +    mov  edx, [ebp+4]
>  +
>  +    /* Result in ecx:ebx (we know there's carry) */
>  +    lea  ecx, [edx+1]
>  +
>  +    /* Do a full exchange */
>  +    LOCK cmpxchg8b [ebp]
>  +
>  +    /* restore increment */
>  +    pop  edx
>  +
>  +    /* Need to retry */
>  +    jnz  Again
>  +
>  +    /* Thats it */
>  +    pop  ebx
>  +    pop  ebp
>  +    ret
>  +
>
>   /*ULONG
>   *FASTCALL
>
>
>