>>> On 30.04.15 at 17:33, <david.vra...@citrix.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
