Re: [Xen-devel] [PATCHv4 3/5] xen: use ticket locks for spin locks

[Tim Deegan <tim@xxxxxxx>]

At 21:05 +0100 on 14 May (1431637535), Jan Beulich wrote:
> >>> Tim Deegan <tim@xxxxxxx> 05/14/15 12:36 PM >>>
> >At 15:37 +0100 on 11 May (1431358623), David Vrabel wrote:
> >>  void _spin_barrier(spinlock_t *lock)
> >>  {
> >> +    spinlock_tickets_t sample;
> >>  #ifdef LOCK_PROFILE
> >>      s_time_t block = NOW();
> >> -    u64      loop = 0;
> >> +#endif
> >>  
> >>      check_barrier(&lock->debug);
> >> -    do { smp_mb(); loop++;} while ( _raw_spin_is_locked(&lock->raw) );
> >> -    if ((loop > 1) && lock->profile)
> >> +    smp_mb();
> >> +    sample = observe_lock(&lock->tickets);
> >> +    if ( sample.head != sample.tail )
> >>      {
> >> -        lock->profile->time_block += NOW() - block;
> >> -        lock->profile->block_cnt++;
> >> -    }
> >> -#else
> >> -    check_barrier(&lock->debug);
> >> -    do { smp_mb(); } while ( _raw_spin_is_locked(&lock->raw) );
> >> +        while ( observe_head(&lock->tickets) != sample.tail )
> >
> >This test should be "observe_head(&lock->tickets) == sample.head",
> >i.e. wait until the thread that held the lock has released it.
> >Checking for it to reach the tail is unnecessary (other threads that
> >were queueing for the lock at the sample time don't matter) and
> >dangerous (on a contended lock head might pass sample.tail without us
> >happening to observe it being == ).
> 
> The observation of there being a problem is correct, but the suggested 
> solution
> doesn't seem to be. The new code being
> 
>     if ( sample.head != sample.tail )
>     {
>         while ( observe_head(&lock->tickets) == sample.tail )
>             cpu_relax();
> 
> means that if head didn't change between the full sample and the head sample
> we'd end the loop right away, which can't be right. We really need to wait for
> head to reach or pass the sampled tail.

I think you misread what I asked for.  We wait until the observed head
doesn't match the sampled _head_, i.e. for whoever had the lock when
we sampled it to realease it:

     if ( sample.head != sample.tail )
     {
         while ( observe_head(&lock->tickets) == sample.head )
             cpu_relax();

Cheers,

Tim.

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