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

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.04.15 at 17:33, <david.vrabel@xxxxxxxxxx> wrote:
>  int _spin_trylock(spinlock_t *lock)
>  {
> +    spinlock_tickets_t old, new;
> +
>      check_lock(&lock->debug);
> -    if ( !_raw_spin_trylock(&lock->raw) )
> +    old = observe_lock(&lock->tickets);
> +    if ( old.head != old.tail )
> +        return 0;
> +    new = old;
> +    new.tail++;
> +    if ( cmpxchg(&lock->tickets.head_tail, old.head_tail, new.head_tail)
> +         != old.head_tail )
>          return 0;

Maybe worth a comment here that arch_lock_acquire_barrier() is
not needed (implicitly done by cmpxchg())?

> @@ -213,27 +213,32 @@ int _spin_trylock(spinlock_t *lock)
>  
>  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)
> +    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 )
> +        {
> +#ifdef LOCK_PROFILE
> +            if ( lock->profile )
> +            {
> +                lock->profile->time_block += NOW() - block;
> +                lock->profile->block_cnt++;
> +            }

If you want to keep this inside the loop (which seems a bad idea)
you'd need to update "block" in each iteration and increment
->block_cnt only the first time through.

>  #endif

Wouldn't there be a cpu_relax() on order here?

> +        }
> +    }
>      smp_mb();
>  }

The old code had smp_mb() before _and_ after the check - is it really
correct to drop the one before (or effectively replace it by smp_rmb()
in observe_{lock,head}())?

> @@ -256,8 +261,17 @@ int _spin_trylock_recursive(spinlock_t *lock)
>  
>  void _spin_lock_recursive(spinlock_t *lock)
>  {
> -    while ( !spin_trylock_recursive(lock) )
> -        cpu_relax();
> +    unsigned int cpu = smp_processor_id();
> +
> +    if ( likely(lock->recurse_cpu != cpu) )
> +    {
> +        spin_lock(lock);

While benign with what xen/spinlock.h currently has, it would seem
to me that in a function named _spin_lock_recursive() this ought to
be _spin_lock().

Jan


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