Re: [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest

[Waiman Long <waiman.long@hp.com>]

On 03/13/2014 11:15 AM, Peter Zijlstra wrote:
> On Wed, Mar 12, 2014 at 02:54:52PM -0400, Waiman Long wrote:
>> +static inline void arch_spin_lock(struct qspinlock *lock)
>> +{
>> +	if (static_key_false(&paravirt_unfairlocks_enabled))
>> +		queue_spin_lock_unfair(lock);
>> +	else
>> +		queue_spin_lock(lock);
>> +}
> So I would have expected something like:
>
> 	if (static_key_false(&paravirt_spinlock)) {
> 		while (!queue_spin_trylock(lock))
> 			cpu_relax();
> 		return;
> 	}
>
> At the top of queue_spin_lock_slowpath().

I don't like the idea of constantly spinning on the lock. That can cause 
all sort of performance issues. My version of the unfair lock tries to 
grab the lock ignoring if there are others waiting in the queue or not. 
So instead of the doing a cmpxchg of the whole 32-bit word, I just do a 
cmpxchg of the lock byte in the unfair version. A CPU has only one 
chance to steal the lock. If it can't, it will be lined up in the queue 
just like the fair version. It is not as unfair as the other unfair 
locking schemes that spins on the lock repetitively. So lock starvation 
should be less a problem.

On the other hand, it may not perform as well as the other unfair 
locking schemes. It is a compromise to provide some lock unfairness 
without sacrificing the good cacheline behavior of the queue spinlock.

>> +static inline int arch_spin_trylock(struct qspinlock *lock)
>> +{
>> +	if (static_key_false(&paravirt_unfairlocks_enabled))
>> +		return queue_spin_trylock_unfair(lock);
>> +	else
>> +		return queue_spin_trylock(lock);
>> +}
> That just doesn't make any kind of sense; a trylock cannot be fair or
> unfair.

Because I use a different cmpxchg for the fair and unfair versions, I 
also need a different version for trylock.

-Longman