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

[David Vrabel <david.vrabel@xxxxxxxxxx>]

On 23/04/15 15:58, Jan Beulich wrote:
>>>> On 23.04.15 at 16:43, <tim@xxxxxxx> wrote:
>> At 14:54 +0100 on 23 Apr (1429800874), Jan Beulich wrote:
>>>>>> On 23.04.15 at 14:03, <tim@xxxxxxx> 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.)

David

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel