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

[Tim Deegan <tim@xxxxxxx>]

At 18:00 +0100 on 29 Apr (1430330400), David Vrabel wrote:
> 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@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.
> 
> Should I rename xadd() to arch_fetch_and_add() to match the GCC builtin
> name?

Yes, that sounds good.  And maybe the new one could be add_sized() or
similar?  Just to avoid anyone using it when they wanted a locked op.

Tim.

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