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

[Tim Deegan <tim@xxxxxxx>]

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@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.)

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.

Cheers,

Tim.

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