locking/qspinlock, x86: Improve determinism for x86
Subsystems (4)
Peter Zijlstra
maintainerv2 cover locking/qspinlock, x86: Improve determinism for x86
Back when Will did his qspinlock determinism patches, we were left with one cmpxchg loop on x86 due to the use of atomic_fetch_or(). Will proposed a nifty trick:
http://lkml.kernel.org/r/[email protected]
While that didn't quite work, this series implements that basic idea.
Changes since v1:
- Adjusted comments in #2, wildea01
- 'Simplified' GEN_*_RMWcc, peterz
- Rewrote _the_ patch to use BTS, wildea01
Peter Zijlstra
maintainerPATCH v2 1/4 locking/qspinlock: Re-order code
Flip the branch condition after atomic_fetch_or_acquire(_Q_PENDING_VAL) such that we loose the indent. This also result in a more natural code flow IMO.
--- kernel/locking/qspinlock.c | 56 +++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 29 deletions(-)
kernel/locking/qspinlock.c +27 -29
@@ -330,39 +330,37 @@ void queued_spin_lock_slowpath(struct qs* 0,0,1 -> 0,1,1 ; pending */ val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); -if (!(val & ~_Q_LOCKED_MASK)) { -/* -* We're pending, wait for the owner to go away. -* -* *,1,1 -> *,1,0 -* -* this wait loop must be a load-acquire such that we match the -* store-release that clears the locked bit and create lock -* sequentiality; this is because not all -* clear_pending_set_locked() implementations imply full -* barriers. -*/ -if (val & _Q_LOCKED_MASK) { -atomic_cond_read_acquire (&lock->val, - !(VAL & _Q_LOCKED_MASK)); - } - -/* -* take ownership and clear the pending bit. -* -* *,1,0 -> *,0,1 -*/ -clear_pending_set_locked (lock); -qstat_inc (qstat_lock_pending, true); -return ; +/* +* If we observe any contention; undo and queue. +*/ +if (unlikely (val & ~_Q_LOCKED_MASK)) { +if (!(val & _Q_PENDING_MASK)) +clear_pending (lock); + goto queue; }/* -* If pending was clear but there are waiters in the queue, then -* we need to undo our setting of pending before we queue ourselves. +* We're pending, wait for the owner to go away. +* +* 0,1,1 -> 0,1,0 +* +* this wait loop must be a load-acquire such that we match the +* store-release that clears the locked bit and create lock +* sequentiality; this is because not all +* clear_pending_set_locked() implementations imply full +* barriers. +*/ +if (val & _Q_LOCKED_MASK) +atomic_cond_read_acquire (&lock->val, !(VAL & _Q_LOCKED_MASK)); + +/* +* take ownership and clear the pending bit. +* +* 0,1,0 -> 0,0,1 */ -if (!(val & _Q_PENDING_MASK)) -clear_pending (lock); +clear_pending_set_locked (lock); +qstat_inc (qstat_lock_pending, true); +return ;/* * End of pending bit optimistic spinning and beginning of MCS
Peter Zijlstra
maintainerPATCH v2 2/4 locking/qspinlock: Rework some comments
In reply to Peter ZijlstraWhile working my way through the code again; I felt the comments could use help.
--- kernel/locking/qspinlock.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)
kernel/locking/qspinlock.c +27 -11
@@ -326,16 +326,23 @@ void queued_spin_lock_slowpath(struct qs/* * trylock || pending * -* 0,0,0 -> 0,0,1 ; trylock -* 0,0,1 -> 0,1,1 ; pending +* 0,0,* -> 0,1,* -> 0,0,1 pending, trylock */ val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); +/* -* If we observe any contention; undo and queue. +* If we observe contention, there is a concurrent locker. +* +* Undo and queue; our setting of PENDING might have made the +* n,0,0 -> 0,0,0 transition fail and it will now be waiting +* on @next to become !NULL. */ if (unlikely (val & ~_Q_LOCKED_MASK)) { + +/* Undo PENDING if we set it. */ if (!(val & _Q_PENDING_MASK))clear_pending (lock); + goto queue; }
@@ -474,16 +481,25 @@ void queued_spin_lock_slowpath(struct qs*/ /* -* In the PV case we might already have _Q_LOCKED_VAL set. +* In the PV case we might already have _Q_LOCKED_VAL set, because +* of lock stealing; therefore we must also allow: * -* The atomic_cond_read_acquire() call above has provided the -* necessary acquire semantics required for locking. -*/ -if (((val & _Q_TAIL_MASK) == tail) && -atomic_try_cmpxchg_relaxed (&lock->val, &val, _Q_LOCKED_VAL)) - goto release;/* No contention */ +* n,0,1 -> 0,0,1 +* +* Note: at this point: (val & _Q_PENDING_MASK) == 0, because of the +* above wait condition, therefore any concurrent setting of +* PENDING will make the uncontended transition fail. +*/ +if ((val & _Q_TAIL_MASK) == tail) { +if (atomic_try_cmpxchg_relaxed (&lock->val, &val, _Q_LOCKED_VAL)) + goto release;/* No contention */ + } -/* Either somebody is queued behind us or _Q_PENDING_VAL is set */ +/* +* Either somebody is queued behind us or _Q_PENDING_VAL got set +* which will then detect the remaining tail and queue behind us +* ensuring we'll see a @next. +*/ set_locked (lock);/*
Re: [PATCH v2 2/4] locking/qspinlock: Rework some comments
Quoted 9 lines · Peter Zijlstra
While working my way through the code again; I felt the comments could use help.
Quoted 3 lines
--- kernel/locking/qspinlock.c | 38 +++++++++++++++++++++++++++----------- 1 file changed, 27 insertions(+), 11 deletions(-)
Will
Peter Zijlstra
maintainerPATCH v2 3/4 x86/asm: Simplify GEN_*_RMWcc() macros
In reply to Peter ZijlstraCurrently the GEN_*_RMWcc() macros include a return statement, which pretty much mandates we directly wrap them in a (inline) function.
Macros with return statements are tricky and, as per the above, limit use, so remove the return statement and make them statement-expressions. This allows them to be used more widely.
Also, shuffle the arguments a bit. Place the @cc argument as 3rd, this makes it consistent between UNARY and BINARY, but more importantly, it makes the @arg0 argument last.
Since the @arg0 argument is now last, we can do CPP trickery and make it an optional argument, simplifying the users; 17 out of 18 occurences do not need this argument.
Finally, change to asm symbolic names, instead of the numeric ordering of operands, which allows us to get rid of __BINARY_RMWcc_ARG and get cleaner code overall.
--- arch/x86/include/asm/atomic.h | 8 ++-- arch/x86/include/asm/atomic64_64.h | 8 ++-- arch/x86/include/asm/bitops.h | 9 +---- arch/x86/include/asm/local.h | 8 ++-- arch/x86/include/asm/preempt.h | 2 - arch/x86/include/asm/refcount.h | 18 +++++----- arch/x86/include/asm/rmwcc.h | 66 +++++++++++++++++++++---------------- 7 files changed, 64 insertions(+), 55 deletions(-)
arch/x86/include/asm/atomic.h +4 -4
@@ -82,7 +82,7 @@ static __always_inline void arch_atomic_*/ static __always_inline bool arch_atomic_sub_and_test(int i, atomic_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "subl" , v->counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (LOCK_PREFIX "subl" , v->counter, e,"er" , i); }#define arch_atomic_sub_and_test arch_atomic_sub_and_test
@@ -122,7 +122,7 @@ static __always_inline void arch_atomic_*/ static __always_inline bool arch_atomic_dec_and_test(atomic_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "decl" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "decl" , v->counter, e); }#define arch_atomic_dec_and_test arch_atomic_dec_and_test
@@ -136,7 +136,7 @@ static __always_inline bool arch_atomic_*/ static __always_inline bool arch_atomic_inc_and_test(atomic_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "incl" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "incl" , v->counter, e); }#define arch_atomic_inc_and_test arch_atomic_inc_and_test
@@ -151,7 +151,7 @@ static __always_inline bool arch_atomic_*/ static __always_inline bool arch_atomic_add_negative(int i, atomic_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "addl" , v->counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (LOCK_PREFIX "addl" , v->counter, s,"er" , i); }#define arch_atomic_add_negative arch_atomic_add_negative
arch/x86/include/asm/atomic64_64.h +4 -4
@@ -73,7 +73,7 @@ static inline void arch_atomic64_sub(lon*/ static inline bool arch_atomic64_sub_and_test (long i,atomic64_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "subq" , v->counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (LOCK_PREFIX "subq" , v->counter, e,"er" , i); }#define arch_atomic64_sub_and_test arch_atomic64_sub_and_test
@@ -115,7 +115,7 @@ static __always_inline void arch_atomic6*/ static inline bool arch_atomic64_dec_and_test (atomic64_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "decq" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "decq" , v->counter, e); }#define arch_atomic64_dec_and_test arch_atomic64_dec_and_test
@@ -129,7 +129,7 @@ static inline bool arch_atomic64_dec_and*/ static inline bool arch_atomic64_inc_and_test (atomic64_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "incq" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "incq" , v->counter, e); }#define arch_atomic64_inc_and_test arch_atomic64_inc_and_test
@@ -144,7 +144,7 @@ static inline bool arch_atomic64_inc_and*/ static inline bool arch_atomic64_add_negative (long i,atomic64_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "addq" , v->counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (LOCK_PREFIX "addq" , v->counter, s,"er" , i); }#define arch_atomic64_add_negative arch_atomic64_add_negative
arch/x86/include/asm/bitops.h +3 -6
@@ -217,8 +217,7 @@ static __always_inline void change_bit(l*/ static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr); }/**
@@ -264,8 +263,7 @@ static __always_inline bool __test_and_s*/ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr); }/**
@@ -318,8 +316,7 @@ static __always_inline bool __test_and_c*/ static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr); } static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
arch/x86/include/asm/local.h +4 -4
@@ -53,7 +53,7 @@ static inline void local_sub(long i, loc*/ static inline bool local_sub_and_test (long i,local_t *l) { -GEN_BINARY_RMWcc (_ASM_SUB, l->a.counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (_ASM_SUB, l->a.counter, e,"er" , i); }/**
@@ -66,7 +66,7 @@ static inline bool local_sub_and_test(lo*/ static inline bool local_dec_and_test (local_t *l) { -GEN_UNARY_RMWcc (_ASM_DEC, l->a.counter,"%0" , e); +return GEN_UNARY_RMWcc (_ASM_DEC, l->a.counter, e); }/**
@@ -79,7 +79,7 @@ static inline bool local_dec_and_test(lo*/ static inline bool local_inc_and_test (local_t *l) { -GEN_UNARY_RMWcc (_ASM_INC, l->a.counter,"%0" , e); +return GEN_UNARY_RMWcc (_ASM_INC, l->a.counter, e); }/**
@@ -93,7 +93,7 @@ static inline bool local_inc_and_test(lo*/ static inline bool local_add_negative (long i,local_t *l) { -GEN_BINARY_RMWcc (_ASM_ADD, l->a.counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (_ASM_ADD, l->a.counter, s,"er" , i); }/**
arch/x86/include/asm/preempt.h +1 -1
@@ -88,7 +88,7 @@ static __always_inline void __preempt_co*/ static __always_inline bool __preempt_count_dec_and_test(void) { -GEN_UNARY_RMWcc ("decl" , __preempt_count,__percpu_arg (0 ), e); + return GEN_UNARY_RMWcc("decl", __preempt_count, e, __percpu_arg([var])); }/*
arch/x86/include/asm/refcount.h +9 -9
@@ -17,7 +17,7 @@*/ #define _REFCOUNT_EXCEPTION \ ".pushsection .text..refcount\n" \ - "111:\tlea %[counter], %%" _ASM_CX "\n" \ + "111:\tlea %[var], %%" _ASM_CX "\n" \ "112:\t" ASM_UD2 "\n" \ ASM_UNREACHABLE \ ".popsection\n" \
@@ -43,7 +43,7 @@ static __always_inline void refcount_add { asmvolatile (LOCK_PREFIX "addl %1,%0\n\t" REFCOUNT_CHECK_LT_ZERO - : [counter]"+m" (r->refs.counter) + : [var]"+m" (r->refs.counter) :"ir" (i) :"cc" ,"cx" ); }
@@ -52,7 +52,7 @@ static __always_inline void refcount_inc { asmvolatile (LOCK_PREFIX "incl %0\n\t" REFCOUNT_CHECK_LT_ZERO - : [counter]"+m" (r->refs.counter) + : [var]"+m" (r->refs.counter) : :"cc" ,"cx" ); }
@@ -60,21 +60,21 @@ static __always_inline void refcount_dec { asmvolatile (LOCK_PREFIX "decl %0\n\t" REFCOUNT_CHECK_LE_ZERO - : [counter]"+m" (r->refs.counter) + : [var]"+m" (r->refs.counter) : :"cc" ,"cx" ); }static __always_inline __must_check bool refcount_sub_and_test(unsigned int i, refcount_t *r) { -GEN_BINARY_SUFFIXED_RMWcc (LOCK_PREFIX "subl" ,REFCOUNT_CHECK_LT_ZERO , - r->refs.counter,"er" , i,"%0" , e,"cx" ); +return GEN_BINARY_SUFFIXED_RMWcc (LOCK_PREFIX "subl" , +REFCOUNT_CHECK_LT_ZERO , r->refs.counter, e,"er" , i,"cx" ); } static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r) { -GEN_UNARY_SUFFIXED_RMWcc (LOCK_PREFIX "decl" ,REFCOUNT_CHECK_LT_ZERO , - r->refs.counter,"%0" , e,"cx" ); +return GEN_UNARY_SUFFIXED_RMWcc (LOCK_PREFIX "decl" , +REFCOUNT_CHECK_LT_ZERO , r->refs.counter, e,"cx" ); } static __always_inline __must_check
@@ -92,7 +92,7 @@ bool refcount_add_not_zero(unsigned int/* Did we try to increment from/to an undesirable state? */ if (unlikely (c <0 || c ==INT_MAX || result < c)) { asm volatile(REFCOUNT_ERROR - : : [counter] "m" (r->refs.counter) + : : [var] "m" (r->refs.counter) : "cc", "cx"); break; }
arch/x86/include/asm/rmwcc.h +39 -27
95 lines not on this page · raw message
Peter Zijlstra
maintainerRe: [PATCH v2 3/4] x86/asm: Simplify GEN_*_RMWcc() macros
arch/x86/include/asm/rmwcc.h line 27
Quoted 10 lines · Peter Zijlstra
arch/x86/include/asm/rmwcc.h +7
-0+#define __GEN_RMWcc (fullop, _var, cc, clobbers, ...) \ +({ \ + __label__ cc_label; \ asm_volatile_goto (fullop "; j" #cc " %l[cc_label]" \ + : : [var] "m" (_var), ## __VA_ARGS__ \ : clobbers : cc_label); \ + 0; \ cc_label: \ + 1; \ +})
That's obviously crap...
This one seems to actually compile and generate identical code:
#define __GEN_RMWcc(fullop, _var, cc, clobbers, ...) \
({ \
bool c = false; \
asm_volatile_goto (fullop "; j" #cc " %l[cc_label]" \
: : [var] "m" (_var), ## __VA_ARGS__ \
: clobbers : cc_label); \
if (0) { \
cc_label: c = true; \
} \
c; \
})tip-bot for Peter Zijlstra
merge bot[tip:locking/core] x86/asm: 'Simplify' GEN_*_RMWcc() macros
In reply to Peter Zijlstrax86/asm: 'Simplify' GEN_*_RMWcc() macros
Commit-ID: 288e4521f0f6717909933116563e66bb894ae2af Gitweb: https://git.kernel.org/tip/288e4521f0f6717909933116563e66bb894ae2af Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 3 Oct 2018 12:34:10 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 16 Oct 2018 17:33:54 +0200
x86/asm: 'Simplify' GEN_*_RMWcc() macros
Currently the GEN_*_RMWcc() macros include a return statement, which pretty much mandates we directly wrap them in a (inline) function.
Macros with return statements are tricky and, as per the above, limit use, so remove the return statement and make them statement-expressions. This allows them to be used more widely.
Also, shuffle the arguments a bit. Place the @cc argument as 3rd, this makes it consistent between UNARY and BINARY, but more importantly, it makes the @arg0 argument last.
Since the @arg0 argument is now last, we can do CPP trickery and make it an optional argument, simplifying the users; 17 out of 18 occurences do not need this argument.
Finally, change to asm symbolic names, instead of the numeric ordering of operands, which allows us to get rid of __BINARY_RMWcc_ARG and get cleaner code overall.
--- arch/x86/include/asm/atomic.h | 8 ++--- arch/x86/include/asm/atomic64_64.h | 8 ++--- arch/x86/include/asm/bitops.h | 9 ++--- arch/x86/include/asm/local.h | 8 ++--- arch/x86/include/asm/preempt.h | 2 +- arch/x86/include/asm/refcount.h | 13 +++---- arch/x86/include/asm/rmwcc.h | 69 ++++++++++++++++++++++---------------- 7 files changed, 64 insertions(+), 53 deletions(-)
arch/x86/include/asm/atomic.h +4 -4
@@ -82,7 +82,7 @@ static __always_inline void arch_atomic_sub(int i, atomic_t *v)*/ static __always_inline bool arch_atomic_sub_and_test(int i, atomic_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "subl" , v->counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (LOCK_PREFIX "subl" , v->counter, e,"er" , i); }#define arch_atomic_sub_and_test arch_atomic_sub_and_test
@@ -122,7 +122,7 @@ static __always_inline void arch_atomic_dec(atomic_t *v)*/ static __always_inline bool arch_atomic_dec_and_test(atomic_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "decl" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "decl" , v->counter, e); }#define arch_atomic_dec_and_test arch_atomic_dec_and_test
@@ -136,7 +136,7 @@ static __always_inline bool arch_atomic_dec_and_test(atomic_t *v)*/ static __always_inline bool arch_atomic_inc_and_test(atomic_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "incl" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "incl" , v->counter, e); }#define arch_atomic_inc_and_test arch_atomic_inc_and_test
@@ -151,7 +151,7 @@ static __always_inline bool arch_atomic_inc_and_test(atomic_t *v)*/ static __always_inline bool arch_atomic_add_negative(int i, atomic_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "addl" , v->counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (LOCK_PREFIX "addl" , v->counter, s,"er" , i); }#define arch_atomic_add_negative arch_atomic_add_negative
arch/x86/include/asm/atomic64_64.h +4 -4
@@ -73,7 +73,7 @@ static inline void arch_atomic64_sub(long i, atomic64_t *v)*/ static inline bool arch_atomic64_sub_and_test (long i,atomic64_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "subq" , v->counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (LOCK_PREFIX "subq" , v->counter, e,"er" , i); }#define arch_atomic64_sub_and_test arch_atomic64_sub_and_test
@@ -115,7 +115,7 @@ static __always_inline void arch_atomic64_dec(atomic64_t *v)*/ static inline bool arch_atomic64_dec_and_test (atomic64_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "decq" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "decq" , v->counter, e); }#define arch_atomic64_dec_and_test arch_atomic64_dec_and_test
@@ -129,7 +129,7 @@ static inline bool arch_atomic64_dec_and_test(atomic64_t *v)*/ static inline bool arch_atomic64_inc_and_test (atomic64_t *v) { -GEN_UNARY_RMWcc (LOCK_PREFIX "incq" , v->counter,"%0" , e); +return GEN_UNARY_RMWcc (LOCK_PREFIX "incq" , v->counter, e); }#define arch_atomic64_inc_and_test arch_atomic64_inc_and_test
@@ -144,7 +144,7 @@ static inline bool arch_atomic64_inc_and_test(atomic64_t *v)*/ static inline bool arch_atomic64_add_negative (long i,atomic64_t *v) { -GEN_BINARY_RMWcc (LOCK_PREFIX "addq" , v->counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (LOCK_PREFIX "addq" , v->counter, s,"er" , i); }#define arch_atomic64_add_negative arch_atomic64_add_negative
arch/x86/include/asm/bitops.h +3 -6
@@ -217,8 +217,7 @@ static __always_inline void change_bit(long nr, volatile unsigned long *addr)*/ static __always_inline bool test_and_set_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(bts), *addr, c, "Ir", nr); }/**
@@ -264,8 +263,7 @@ static __always_inline bool __test_and_set_bit(long nr, volatile unsigned long **/ static __always_inline bool test_and_clear_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btr), *addr, c, "Ir", nr); }/**
@@ -318,8 +316,7 @@ static __always_inline bool __test_and_change_bit(long nr, volatile unsigned lon*/ static __always_inline bool test_and_change_bit(long nr, volatile unsigned long *addr) { - GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), - *addr, "Ir", nr, "%0", c); + return GEN_BINARY_RMWcc(LOCK_PREFIX __ASM_SIZE(btc), *addr, c, "Ir", nr); } static __always_inline bool constant_test_bit(long nr, const volatile unsigned long *addr)
arch/x86/include/asm/local.h +4 -4
@@ -53,7 +53,7 @@ static inline void local_sub(long i, local_t *l)*/ static inline bool local_sub_and_test (long i,local_t *l) { -GEN_BINARY_RMWcc (_ASM_SUB, l->a.counter,"er" , i,"%0" , e); +return GEN_BINARY_RMWcc (_ASM_SUB, l->a.counter, e,"er" , i); }/**
@@ -66,7 +66,7 @@ static inline bool local_sub_and_test(long i, local_t *l)*/ static inline bool local_dec_and_test (local_t *l) { -GEN_UNARY_RMWcc (_ASM_DEC, l->a.counter,"%0" , e); +return GEN_UNARY_RMWcc (_ASM_DEC, l->a.counter, e); }/**
@@ -79,7 +79,7 @@ static inline bool local_dec_and_test(local_t *l)*/ static inline bool local_inc_and_test (local_t *l) { -GEN_UNARY_RMWcc (_ASM_INC, l->a.counter,"%0" , e); +return GEN_UNARY_RMWcc (_ASM_INC, l->a.counter, e); }/**
@@ -93,7 +93,7 @@ static inline bool local_inc_and_test(local_t *l)*/ static inline bool local_add_negative (long i,local_t *l) { -GEN_BINARY_RMWcc (_ASM_ADD, l->a.counter,"er" , i,"%0" , s); +return GEN_BINARY_RMWcc (_ASM_ADD, l->a.counter, s,"er" , i); }/**
arch/x86/include/asm/preempt.h +1 -1
@@ -88,7 +88,7 @@ static __always_inline void __preempt_count_sub(int val)*/ static __always_inline bool __preempt_count_dec_and_test(void) { -GEN_UNARY_RMWcc ("decl" , __preempt_count,__percpu_arg (0 ), e); + return GEN_UNARY_RMWcc("decl", __preempt_count, e, __percpu_arg([var])); }/*
arch/x86/include/asm/refcount.h +7 -6
@@ -79,16 +79,17 @@ static __always_inline void refcount_dec(refcount_t *r)static __always_inline __must_check bool refcount_sub_and_test(unsigned int i, refcount_t *r) { -GEN_BINARY_SUFFIXED_RMWcc (LOCK_PREFIX "subl" , -"REFCOUNT_CHECK_LT_ZERO counter=\"%0\"" , - r->refs.counter,"er" , i,"%0" , e,"cx" ); + +return GEN_BINARY_SUFFIXED_RMWcc (LOCK_PREFIX "subl" , +"REFCOUNT_CHECK_LT_ZERO counter=\"%[var]\"" , + r->refs.counter, e,"er" , i,"cx" ); } static __always_inline __must_check bool refcount_dec_and_test(refcount_t *r) { -GEN_UNARY_SUFFIXED_RMWcc (LOCK_PREFIX "decl" , -"REFCOUNT_CHECK_LT_ZERO counter=\"%0\"" , - r->refs.counter,"%0" , e,"cx" ); +return GEN_UNARY_SUFFIXED_RMWcc (LOCK_PREFIX "decl" , +"REFCOUNT_CHECK_LT_ZERO counter=\"%[var]\"" , + r->refs.counter, e,"cx" ); } static __always_inline __must_check
arch/x86/include/asm/rmwcc.h +41 -28
97 lines not on this page · raw message
Peter Zijlstra
maintainerPATCH v2 4/4 locking/qspinlock, x86: Provide liveness guarantee
In reply to Peter ZijlstraOn 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, it cannot be an xchg8 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.
--- arch/x86/include/asm/qspinlock.h | 15 +++++++++++++++ kernel/locking/qspinlock.c | 16 +++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)
arch/x86/include/asm/qspinlock.h +15 -0
@@ -6,9 +6,24 @@#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 u32 queued_fetch_set_pending_acquire(struct qspinlock *lock) +{ +u32 val =0 ; + +if (GEN_BINARY_RMWcc (LOCK_PREFIX "btsl" , lock->val.counter, c, +"I" , _Q_PENDING_OFFSET)) + val |= _Q_PENDING_VAL; + + val |=atomic_read (&lock->val) & ~_Q_PENDING_MASK; + +return val; +} +#ifdef CONFIG_PARAVIRT_SPINLOCKS extern void native_queued_spin_lock_slowpath (struct qspinlock *lock,u32 val);extern void __pv_init_lock_hash (void );
kernel/locking/qspinlock.c +15 -1
@@ -232,6 +232,20 @@ static __always_inline u32 xchg_tail(str#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 *
@@ -328,7 +342,7 @@ void queued_spin_lock_slowpath(struct qs* * 0,0,* -> 0,1,* -> 0,0,1 pending, trylock */ - val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); + val =queued_fetch_set_pending_acquire (lock);/* * If we observe contention, there is a concurrent locker.
Re: [PATCH v2 4/4] locking/qspinlock, x86: Provide liveness guarantee
Quoted 61 lines · Peter Zijlstra
On x86 we cannot do
fetch_orwith a single instruction and thus end up using a cmpxchg loop, this reduces determinism. Replace thefetch_orwith a composite operation: tas-pending + load.Using two instructions of course opens a window we previously did not have. Consider the scenario:
CPU0 CPU1 CPU21) 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 36) 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, it cannot be an xchg8 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 thatxchg_tailhaving taken place before the tas-pending.Suggested-by Will Deacon <[email protected]>Signed-off-by Peter Zijlstra (Intel) <[email protected]>
Quoted 4 lines
--- arch/x86/include/asm/qspinlock.h | 15 +++++++++++++++ kernel/locking/qspinlock.c | 16 +++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)
I've failed to break this by thinking really hard, so I've updated Catalin's TLA model to see if the tools are still happy. I'll get back to you once they've finished chewing on it.
Will
Will Deacon
Reviewed-byRe: [PATCH v2 4/4] locking/qspinlock, x86: Provide liveness guarantee
Quoted 8 lines · Will Deacon
Quoted 3 lines · Peter Zijlstra
arch/x86/include/asm/qspinlock.h | 15 +++++++++++++++ kernel/locking/qspinlock.c | 16 +++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)I've failed to break this by thinking really hard, so I've updated Catalin's TLA model to see if the tools are still happy. I'll get back to you once they've finished chewing on it.
The model is happy, so this doesn't introduce any "bad" SC behaviours.
Will
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/qspinlock, x86: Provide liveness guarantee
In reply to Peter Zijlstralocking/qspinlock, x86: Provide liveness guarantee
Commit-ID: 7aa54be2976550f17c11a1c3e3630002dea39303 Gitweb: https://git.kernel.org/tip/7aa54be2976550f17c11a1c3e3630002dea39303 Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 26 Sep 2018 13:01:20 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 16 Oct 2018 17:33:54 +0200
locking/qspinlock, x86: Provide liveness guarantee
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.
--- arch/x86/include/asm/qspinlock.h | 15 +++++++++++++++ kernel/locking/qspinlock.c | 16 +++++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-)
arch/x86/include/asm/qspinlock.h +15 -0
@@ -6,9 +6,24 @@#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 u32 queued_fetch_set_pending_acquire(struct qspinlock *lock) +{ +u32 val =0 ; + +if (GEN_BINARY_RMWcc (LOCK_PREFIX "btsl" , lock->val.counter, c, +"I" , _Q_PENDING_OFFSET)) + val |= _Q_PENDING_VAL; + + val |=atomic_read (&lock->val) & ~_Q_PENDING_MASK; + +return val; +} +#ifdef CONFIG_PARAVIRT_SPINLOCKS extern void native_queued_spin_lock_slowpath (struct qspinlock *lock,u32 val);extern void __pv_init_lock_hash (void );
kernel/locking/qspinlock.c +15 -1
@@ -231,6 +231,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
@@ -328,7 +342,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)* * 0,0,* -> 0,1,* -> 0,0,1 pending, trylock */ - val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); + val =queued_fetch_set_pending_acquire (lock);/* * If we observe contention, there is a concurrent locker.
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/qspinlock: Re-order code
In reply to Peter Zijlstralocking/qspinlock: Re-order code
Commit-ID: 53bf57fab7321fb42b703056a4c80fc9d986d170 Gitweb: https://git.kernel.org/tip/53bf57fab7321fb42b703056a4c80fc9d986d170 Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 26 Sep 2018 13:01:18 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 16 Oct 2018 17:33:53 +0200
locking/qspinlock: Re-order code
Flip the branch condition after atomic_fetch_or_acquire(_Q_PENDING_VAL) such that we loose the indent. This also result in a more natural code flow IMO.
--- kernel/locking/qspinlock.c | 56 ++++++++++++++++++++++------------------------ 1 file changed, 27 insertions(+), 29 deletions(-)
kernel/locking/qspinlock.c +27 -29
@@ -330,39 +330,37 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)* 0,0,1 -> 0,1,1 ; pending */ val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); -if (!(val & ~_Q_LOCKED_MASK)) { -/* -* We're pending, wait for the owner to go away. -* -* *,1,1 -> *,1,0 -* -* this wait loop must be a load-acquire such that we match the -* store-release that clears the locked bit and create lock -* sequentiality; this is because not all -* clear_pending_set_locked() implementations imply full -* barriers. -*/ -if (val & _Q_LOCKED_MASK) { -atomic_cond_read_acquire (&lock->val, - !(VAL & _Q_LOCKED_MASK)); - } - -/* -* take ownership and clear the pending bit. -* -* *,1,0 -> *,0,1 -*/ -clear_pending_set_locked (lock); -qstat_inc (qstat_lock_pending, true); -return ; +/* +* If we observe any contention; undo and queue. +*/ +if (unlikely (val & ~_Q_LOCKED_MASK)) { +if (!(val & _Q_PENDING_MASK)) +clear_pending (lock); + goto queue; }/* -* If pending was clear but there are waiters in the queue, then -* we need to undo our setting of pending before we queue ourselves. +* We're pending, wait for the owner to go away. +* +* 0,1,1 -> 0,1,0 +* +* this wait loop must be a load-acquire such that we match the +* store-release that clears the locked bit and create lock +* sequentiality; this is because not all +* clear_pending_set_locked() implementations imply full +* barriers. +*/ +if (val & _Q_LOCKED_MASK) +atomic_cond_read_acquire (&lock->val, !(VAL & _Q_LOCKED_MASK)); + +/* +* take ownership and clear the pending bit. +* +* 0,1,0 -> 0,0,1 */ -if (!(val & _Q_PENDING_MASK)) -clear_pending (lock); +clear_pending_set_locked (lock); +qstat_inc (qstat_lock_pending, true); +return ;/* * End of pending bit optimistic spinning and beginning of MCS
tip-bot for Peter Zijlstra
merge bot[tip:locking/core] locking/qspinlock: Rework some comments
In reply to Peter Zijlstralocking/qspinlock: Rework some comments
Commit-ID: 756b1df4c2c82a1cdffeafa9d2aa76c92e7fb405 Gitweb: https://git.kernel.org/tip/756b1df4c2c82a1cdffeafa9d2aa76c92e7fb405 Author: Peter Zijlstra <[email protected]> AuthorDate: Wed, 26 Sep 2018 13:01:19 +0200 Committer: Ingo Molnar <[email protected]> CommitDate: Tue, 16 Oct 2018 17:33:54 +0200
locking/qspinlock: Rework some comments
While working my way through the code again; I felt the comments could use help.
--- kernel/locking/qspinlock.c | 36 ++++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-)
kernel/locking/qspinlock.c +26 -10
@@ -326,16 +326,23 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)/* * trylock || pending * -* 0,0,0 -> 0,0,1 ; trylock -* 0,0,1 -> 0,1,1 ; pending +* 0,0,* -> 0,1,* -> 0,0,1 pending, trylock */ val =atomic_fetch_or_acquire (_Q_PENDING_VAL, &lock->val); +/* -* If we observe any contention; undo and queue. +* If we observe contention, there is a concurrent locker. +* +* Undo and queue; our setting of PENDING might have made the +* n,0,0 -> 0,0,0 transition fail and it will now be waiting +* on @next to become !NULL. */ if (unlikely (val & ~_Q_LOCKED_MASK)) { + +/* Undo PENDING if we set it. */ if (!(val & _Q_PENDING_MASK))clear_pending (lock); + goto queue; }
@@ -474,16 +481,25 @@ locked:*/ /* -* In the PV case we might already have _Q_LOCKED_VAL set. +* In the PV case we might already have _Q_LOCKED_VAL set, because +* of lock stealing; therefore we must also allow: * -* The atomic_cond_read_acquire() call above has provided the -* necessary acquire semantics required for locking. +* n,0,1 -> 0,0,1 +* +* Note: at this point: (val & _Q_PENDING_MASK) == 0, because of the +* above wait condition, therefore any concurrent setting of +* PENDING will make the uncontended transition fail. */ -if (((val & _Q_TAIL_MASK) == tail) && -atomic_try_cmpxchg_relaxed (&lock->val, &val, _Q_LOCKED_VAL)) - goto release;/* No contention */ +if ((val & _Q_TAIL_MASK) == tail) { +if (atomic_try_cmpxchg_relaxed (&lock->val, &val, _Q_LOCKED_VAL)) + goto release;/* No contention */ + } -/* Either somebody is queued behind us or _Q_PENDING_VAL is set */ +/* +* Either somebody is queued behind us or _Q_PENDING_VAL got set +* which will then detect the remaining tail and queue behind us +* ensuring we'll see a @next. +*/ set_locked (lock);/*