On Tue, Jun 17, 2014 at 05:07:29PM -0400, Konrad Rzeszutek Wilk wrote:
> On Tue, Jun 17, 2014 at 04:51:57PM -0400, Waiman Long wrote:
> > On 06/17/2014 04:36 PM, Konrad Rzeszutek Wilk wrote:
> > >On Sun, Jun 15, 2014 at 02:47:00PM +0200, Peter Zijlstra wrote:
> > >>Because the qspinlock needs to touch a second cacheline; add a pending
> > >>bit and allow a single in-word spinner before we punt to the second
> > >>cacheline.
> > >Could you add this in the description please:
> > >
> > >And by second cacheline we mean the local 'node'. That is the:
> > >mcs_nodes[0] and mcs_nodes[idx]
> > >
> > >Perhaps it might be better then to split this in the header file
> > >as this is trying to not be a slowpath code - but rather - a
> > >pre-slow-path-lets-try-if-we can do another cmpxchg in case
> > >the unlocker has just unlocked itself.
> > >
> > >So something like:
> > >
> > >diff --git a/include/asm-generic/qspinlock.h b/include/asm-generic/qspinlock.h
> > >index e8a7ae8..29cc9c7 100644
> > >--- a/include/asm-generic/qspinlock.h
> > >+++ b/include/asm-generic/qspinlock.h
> > >@@ -75,11 +75,21 @@ extern void queue_spin_lock_slowpath(struct qspinlock *lock, u32 val);
> > > */
> > > static __always_inline void queue_spin_lock(struct qspinlock *lock)
> > > {
> > >- u32 val;
> > >+ u32 val, new;
> > >
> > > val = atomic_cmpxchg(&lock->val, 0, _Q_LOCKED_VAL);
> > > if (likely(val == 0))
> > > return;
> > >+
> > >+ /* One more attempt - but if we fail mark it as pending. */
> > >+ if (val == _Q_LOCKED_VAL) {
> > >+ new = Q_LOCKED_VAL |_Q_PENDING_VAL;
> > >+
> > >+ old = atomic_cmpxchg(&lock->val, val, new);
> > >+ if (old == _Q_LOCKED_VAL) /* YEEY! */
> > >+ return;
> >
> > No, it can leave like that. The unlock path will not clear the pending bit.
>
> Err, you are right. It needs to go back in the slowpath.
What I should have wrote is:
if (old == 0) /* YEEY */
return;
As that would the same thing as this patch does on the pending bit - that
is if we can on the second compare and exchange set the pending bit (and the
lock) and the lock has been released - we are good.
And it is a quick path.
>
> > We are trying to make the fastpath as simple as possible as it may be
> > inlined. The complexity of the queue spinlock is in the slowpath.
>
> Sure, but then it shouldn't be called slowpath anymore as it is not
> slow. It is a combination of fast path (the potential chance of
> grabbing the lock and setting the pending lock) and the real slow
> path (the queuing). Perhaps it should be called 'queue_spinlock_complex' ?
>
I forgot to mention - that was the crux of my comments - just change
the slowpath to complex name at that point to better reflect what
it does.
> >
> > Moreover, an cmpxchg followed immediately followed by another cmpxchg will
> > just increase the level of memory contention when a lock is fairly
> > contended. The chance of second cmpxchg() succeeding will be pretty low.
>
> Then why even do the pending bit - which is what the slowpath does
> for the first time. And if it grabs it (And sets the pending bit) it
> immediately exits. Why not perculate that piece of code in-to this header.
>
> And the leave all that slow code (queing, mcs_lock access, etc) in the slowpath.
>
> >
> > -Longman
> >
> >
_______________________________________________
Virtualization mailing list
Virtualization@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
