Re: [PATCHv3 3/4] xen: use ticket locks for spin locks

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

On 29/04/15 17:56, Tim Deegan wrote:
> At 16:36 +0100 on 29 Apr (1430325362), David Vrabel wrote:
>> On 23/04/15 15:58, Jan Beulich wrote:
>>>>>> On 23.04.15 at 16:43, <tim@xen.org> wrote:
>>>> At 14:54 +0100 on 23 Apr (1429800874), Jan Beulich wrote:
>>>>>>>> On 23.04.15 at 14:03, <tim@xen.org> wrote:
>>>>>> At 11:11 +0100 on 21 Apr (1429614687), David Vrabel wrote:
>>>>>>>  void _spin_unlock(spinlock_t *lock)
>>>>>>>  {
>>>>>>> +    smp_mb();
>>>>>>>      preempt_enable();
>>>>>>>      LOCK_PROFILE_REL;
>>>>>>> -    _raw_spin_unlock(&lock->raw);
>>>>>>> +    lock->tickets.head++;
>>>>>>
>>>>>> This needs to be done with an explicit atomic (though not locked)
>>>>>> write; otherwise the compiler might use some unsuitable operation that
>>>>>> clobbers .tail as well.
>>>>>
>>>>> How do you imagine that to happen? An increment of one
>>>>> structure member surely won't modify any others.
>>>>
>>>> AIUI, the '++' could end up as a word-size read, modify, and word-size
>>>> write.  If another CPU updates .tail parallel, that update could get
>>>> lost.
>>>
>>> Ah, right, compilers are allowed to do that, albeit normally wouldn't
>>> unless the architecture has no suitable loads/stores.
>>
>> lock->tickets.head++;
>>
>>   7b:   66 83 07 01             addw   $0x1,(%rdi)
>>
>> write_atomic(&lock->tickets.head, lock->tickets.head + 1);
>>
>>   7b:   0f b7 07                movzwl (%rdi),%eax
>>   7e:   83 c0 01                add    $0x1,%eax
>>   81:   66 89 07                mov    %ax,(%rdi)
> 
> :(
> 
>> Do you want a new add_atomic() operation? e.g.,
>>
>> #define add_atomic(ptr, inc) \
>>         asm volatile ("addw %1,%w" \
>>             : "+m" (*(ptr)) : "ri" (inc) : "memory")
>>
>> (but obviously handling all the different sizes.)
> 
> I guess so.  An equivalent 'inc' operation would be even shorter,
> but maybe GCC has its reasons for using addw + immediate?
> (Ah, it's in the optimization manual: addw $1 is preferred because it
> sets all the flags, whereas inc sets only some, so the inc has a
> dependence on the previous instruction to set flags.)
> 
> It needs some careful naming -- this series will add two
> new add operations, currently xadd() and add_atomic(), where xadd() is
> the more atomic of the two, IYSWIM.

Should I rename xadd() to arch_fetch_and_add() to match the GCC builtin
name?

David