Re: [PATCHv2 4/6] x86: provide xadd()

[David Vrabel <david.vrabel@citrix.com>]

On 16/04/15 12:25, Tim Deegan wrote:
> At 15:19 +0100 on 10 Apr (1428679195), David Vrabel wrote:
>> xadd() atomically adds a value and returns the previous value.  This
>> is needed to implement ticket locks.
>>
>> Signed-off-by: David Vrabel <david.vrabel@citrix.com>
>> ---
>>  xen/include/asm-x86/system.h |   29 +++++++++++++++++++++++++++++
>>  1 file changed, 29 insertions(+)
>>
>> diff --git a/xen/include/asm-x86/system.h b/xen/include/asm-x86/system.h
>> index 7111329..1e6c6a8 100644
>> --- a/xen/include/asm-x86/system.h
>> +++ b/xen/include/asm-x86/system.h
>> @@ -117,6 +117,35 @@ static always_inline unsigned long __cmpxchg(
>>                                     (unsigned long)__n,sizeof(*(ptr)))); \
>>  })
>>  
>> +static always_inline unsigned long __xadd(
>> +    volatile void *ptr, unsigned long v, int size)
>> +{
>> +    switch ( size )
>> +    {
>> +    case 1:
>> +        asm volatile ( "lock; xaddb %b0,%1"
>> +                       : "+r" (v), "+m" (*__xg((volatile void *)ptr)));
>> +        return v;
>> +    case 2:
>> +        asm volatile ( "lock; xaddw %w0,%1"
>> +                       : "+r" (v), "+m" (*__xg((volatile void *)ptr)));
>> +        return v;
>> +    case 4:
>> +        asm volatile ( "lock; xaddl %k0,%1"
>> +                       : "+r" (v), "+m" (*__xg((volatile void *)ptr)));
>> +        return v;
>> +    case 8:
>> +        asm volatile ( "lock; xaddq %q0,%1"
>> +                       : "+r" (v), "+m" (*__xg((volatile void *)ptr)));
>> +        return v;
>> +    }
>> +    return 0;
> 
> ASSERT_UNREACHABLE(), rather?  And some appropriate BUILD_BUG_ON?
> 
> But also: AFAICS the GCC builtin __sync_fetch_and_add() does almost
> exactly this (the difference being that those are also compiler
> barriers where this is only a CPU barrier).  Should we be using it
> instead?

I wasn't aware of the GCC built-ins -- I'll try them out. I think asm
volatile is a compiler barrier though.

David