Peter Zijlstra
2014-Mar-13 15:15 UTC
[PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
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(¶virt_unfairlocks_enabled)) > + queue_spin_lock_unfair(lock); > + else > + queue_spin_lock(lock); > +}So I would have expected something like: if (static_key_false(¶virt_spinlock)) { while (!queue_spin_trylock(lock)) cpu_relax(); return; } At the top of queue_spin_lock_slowpath().> +static inline int arch_spin_trylock(struct qspinlock *lock) > +{ > + if (static_key_false(¶virt_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.
Waiman Long
2014-Mar-13 20:05 UTC
[PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
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(¶virt_unfairlocks_enabled)) >> + queue_spin_lock_unfair(lock); >> + else >> + queue_spin_lock(lock); >> +} > So I would have expected something like: > > if (static_key_false(¶virt_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(¶virt_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
Peter Zijlstra
2014-Mar-14 08:30 UTC
[PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
On Thu, Mar 13, 2014 at 04:05:19PM -0400, Waiman Long wrote:> 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(¶virt_unfairlocks_enabled)) > >>+ queue_spin_lock_unfair(lock); > >>+ else > >>+ queue_spin_lock(lock); > >>+} > >So I would have expected something like: > > > > if (static_key_false(¶virt_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.Its bloody virt; _that_ is a performance issue to begin with. Anybody half sane stops using virt (esp. if they care about performance).> 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.But but but,.. any kind of queueing gets you into a world of hurt with virt. The simple test-and-set lock (as per the above) still sucks due to lock holder preemption, but at least the suckage doesn't queue. Because with queueing you not only have to worry about the lock holder getting preemption, but also the waiter(s). Take the situation of 3 (v)CPUs where cpu0 holds the lock but is preempted. cpu1 queues, cpu2 queues. Then cpu1 gets preempted, after which cpu0 gets back online. The simple test-and-set lock will now let cpu2 acquire. Your queue however will just sit there spinning, waiting for cpu1 to come back from holiday. I think you're way over engineering this. Just do the simple test-and-set lock for virt && !paravirt (as I think Paolo Bonzini suggested RHEL6 already does).
Apparently Analagous Threads
- [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
- [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
- [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
- [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest
- [PATCH v6 05/11] pvqspinlock, x86: Allow unfair spinlock in a PV guest