Skip to content

Commit

Permalink
locking/qspinlock, x86: Provide liveness guarantee
Browse files Browse the repository at this point in the history
commit 7aa54be upstream.

On x86 we cannot do fetch_or() with a single instruction and thus end up
using a cmpxchg loop, this reduces determinism. Replace the fetch_or()
with a composite operation: tas-pending + load.

Using two instructions of course opens a window we previously did not
have. Consider the scenario:

	CPU0		CPU1		CPU2

 1)	lock
	  trylock -> (0,0,1)

 2)			lock
			  trylock /* fail */

 3)	unlock -> (0,0,0)

 4)					lock
					  trylock -> (0,0,1)

 5)			  tas-pending -> (0,1,1)
			  load-val <- (0,1,0) from 3

 6)			  clear-pending-set-locked -> (0,0,1)

			  FAIL: _2_ owners

where 5) is our new composite operation. When we consider each part of
the qspinlock state as a separate variable (as we can when
_Q_PENDING_BITS == 8) then the above is entirely possible, because
tas-pending will only RmW the pending byte, so the later load is able
to observe prior tail and lock state (but not earlier than its own
trylock, which operates on the whole word, due to coherence).

To avoid this we need 2 things:

 - the load must come after the tas-pending (obviously, otherwise it
   can trivially observe prior state).

 - the tas-pending must be a full word RmW instruction, it cannot be an XCHGB for
   example, such that we cannot observe other state prior to setting
   pending.

On x86 we can realize this by using "LOCK BTS m32, r32" for
tas-pending followed by a regular load.

Note that observing later state is not a problem:

 - if we fail to observe a later unlock, we'll simply spin-wait for
   that store to become visible.

 - if we observe a later xchg_tail(), there is no difference from that
   xchg_tail() having taken place before the tas-pending.

Suggested-by: Will Deacon <[email protected]>
Reported-by: Thomas Gleixner <[email protected]>
Signed-off-by: Peter Zijlstra (Intel) <[email protected]>
Reviewed-by: Will Deacon <[email protected]>
Cc: Linus Torvalds <[email protected]>
Cc: Peter Zijlstra <[email protected]>
Cc: [email protected]
Cc: [email protected]
Fixes: 59fb586 ("locking/qspinlock: Remove unbounded cmpxchg() loop from locking slowpath")
Link: https://lkml.kernel.org/r/[email protected]
Signed-off-by: Ingo Molnar <[email protected]>
[bigeasy: GEN_BINARY_RMWcc macro redo]
Signed-off-by: Sebastian Andrzej Siewior <[email protected]>
Signed-off-by: Sasha Levin <[email protected]>
  • Loading branch information
Peter Zijlstra authored and gregkh committed Dec 21, 2018
1 parent 4e21502 commit 5d01e06
Show file tree
Hide file tree
Showing 2 changed files with 37 additions and 1 deletion.
21 changes: 21 additions & 0 deletions arch/x86/include/asm/qspinlock.h
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,30 @@
#include <asm/cpufeature.h>
#include <asm-generic/qspinlock_types.h>
#include <asm/paravirt.h>
#include <asm/rmwcc.h>

#define _Q_PENDING_LOOPS (1 << 9)

#define queued_fetch_set_pending_acquire queued_fetch_set_pending_acquire

static __always_inline bool __queued_RMW_btsl(struct qspinlock *lock)
{
GEN_BINARY_RMWcc(LOCK_PREFIX "btsl", lock->val.counter,
"I", _Q_PENDING_OFFSET, "%0", c);
}

static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lock)
{
u32 val = 0;

if (__queued_RMW_btsl(lock))
val |= _Q_PENDING_VAL;

val |= atomic_read(&lock->val) & ~_Q_PENDING_MASK;

return val;
}

#define queued_spin_unlock queued_spin_unlock
/**
* queued_spin_unlock - release a queued spinlock
Expand Down
17 changes: 16 additions & 1 deletion kernel/locking/qspinlock.c
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,20 @@ static __always_inline u32 xchg_tail(struct qspinlock *lock, u32 tail)
}
#endif /* _Q_PENDING_BITS == 8 */

/**
* queued_fetch_set_pending_acquire - fetch the whole lock value and set pending
* @lock : Pointer to queued spinlock structure
* Return: The previous lock value
*
* *,*,* -> *,1,*
*/
#ifndef queued_fetch_set_pending_acquire
static __always_inline u32 queued_fetch_set_pending_acquire(struct qspinlock *lock)
{
return atomic_fetch_or_acquire(_Q_PENDING_VAL, &lock->val);
}
#endif

/**
* set_locked - Set the lock bit and own the lock
* @lock: Pointer to queued spinlock structure
Expand Down Expand Up @@ -323,7 +337,8 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
* 0,0,0 -> 0,0,1 ; trylock
* 0,0,1 -> 0,1,1 ; pending
*/
val = atomic_fetch_or_acquire(_Q_PENDING_VAL, &lock->val);
val = queued_fetch_set_pending_acquire(lock);

/*
* If we observe any contention; undo and queue.
*/
Expand Down

0 comments on commit 5d01e06

Please sign in to comment.