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

["Jan Beulich" <jbeulich@xxxxxxxx>]

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

Jan


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