| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160601093158.GN3190@twins.programming.kicks-ass.net>
Date: Wed, 1 Jun 2016 11:31:58 +0200
From: Peter Zijlstra <peterz@...radead.org>
To: Waiman Long <waiman.long@....com>
Cc: linux-kernel@...r.kernel.org, torvalds@...ux-foundation.org,
manfred@...orfullife.com, dave@...olabs.net,
paulmck@...ux.vnet.ibm.com, will.deacon@....com,
boqun.feng@...il.com, tj@...nel.org, pablo@...filter.org,
kaber@...sh.net, davem@...emloft.net, oleg@...hat.com,
netfilter-devel@...r.kernel.org, sasha.levin@...cle.com,
hofrat@...dl.org
Subject: Re: [PATCH -v3 7/8] locking: Move smp_cond_load_acquire() and
friends into asm-generic/barrier.h
On Tue, May 31, 2016 at 04:01:06PM -0400, Waiman Long wrote:
> You are doing two READ_ONCE's in the smp_cond_load_acquire loop. Can we
> change it to do just one READ_ONCE, like
>
> --- a/include/asm-generic/barrier.h
> +++ b/include/asm-generic/barrier.h
> @@ -229,12 +229,18 @@ do {
> * value; some architectures can do this in hardware.
> */
> #ifndef cmpwait
> +#define cmpwait(ptr, val) ({ \
> typeof (ptr) __ptr = (ptr); \
> + typeof (val) __old = (val); \
> + typeof (val) __new; \
> + for (;;) { \
> + __new = READ_ONCE(*__ptr); \
> + if (__new != __old) \
> + break; \
> cpu_relax(); \
> + } \
> + __new; \
> +})
> #endif
>
> /**
> @@ -251,12 +257,11 @@ do {
> #ifndef smp_cond_load_acquire
> #define smp_cond_load_acquire(ptr, cond_expr) ({ \
> typeof(ptr) __PTR = (ptr); \
> + typeof(*ptr) VAL = READ_ONCE(*__PTR); \
> for (;;) { \
> if (cond_expr) \
> break; \
> + VAL = cmpwait(__PTR, VAL); \
> } \
> smp_acquire__after_ctrl_dep(); \
> VAL; \
Yes, that generates slightly better code, but now that you made me look
at it, I think we need to kill the cmpwait() in the generic version and
only keep it for arch versions.
/me ponders...
So cmpwait() as implemented here has strict semantics; but arch
implementations as previously proposed have less strict semantics; and
the use here follows that less strict variant.
The difference being that the arch implementations of cmpwait can have
false positives (ie. return early, without a changed value)
smp_cond_load_acquire() can deal with these false positives seeing how
its in a loop and does its own (more specific) comparison.
Exposing cmpwait(), with the documented semantics, means that arch
versions need an additional loop inside to match these strict semantics,
or we need to weaken the cmpwait() semantics, at which point I'm not
entirely sure its worth keeping as a generic primitive...
Hmm, so if we can find a use for the weaker cmpwait() outside of
smp_cond_load_acquire() I think we can make a case for keeping it, and
looking at qspinlock.h there's two sites we can replace cpu_relax() with
it.
Will, since ARM64 seems to want to use this, does the below make sense
to you?
---
include/asm-generic/barrier.h | 15 ++++++---------
kernel/locking/qspinlock.c | 4 ++--
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/include/asm-generic/barrier.h b/include/asm-generic/barrier.h
index be9222b10d17..05feda5c22e6 100644
--- a/include/asm-generic/barrier.h
+++ b/include/asm-generic/barrier.h
@@ -221,20 +221,17 @@ do { \
#endif
/**
- * cmpwait - compare and wait for a variable to change
+ * cmpwait - compare and wait for a variable to 'change'
* @ptr: pointer to the variable to wait on
* @val: the value it should change from
*
- * A simple constuct that waits for a variable to change from a known
- * value; some architectures can do this in hardware.
+ * A 'better' cpu_relax(), some architectures can avoid polling and have event
+ * based wakeups on variables. Such constructs allow false positives on the
+ * 'change' and can return early. Therefore this reduces to cpu_relax()
+ * without hardware assist.
*/
#ifndef cmpwait
-#define cmpwait(ptr, val) do { \
- typeof (ptr) __ptr = (ptr); \
- typeof (val) __val = (val); \
- while (READ_ONCE(*__ptr) == __val) \
- cpu_relax(); \
-} while (0)
+#define cmpwait(ptr, val) cpu_relax()
#endif
/**
diff --git a/kernel/locking/qspinlock.c b/kernel/locking/qspinlock.c
index e98e5bf679e9..60a811d56406 100644
--- a/kernel/locking/qspinlock.c
+++ b/kernel/locking/qspinlock.c
@@ -311,7 +311,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
*/
if (val == _Q_PENDING_VAL) {
while ((val = atomic_read(&lock->val)) == _Q_PENDING_VAL)
- cpu_relax();
+ cmpwait(&lock->val.counter, _Q_PENDING_VAL);
}
/*
@@ -481,7 +481,7 @@ void queued_spin_lock_slowpath(struct qspinlock *lock, u32 val)
*/
if (!next) {
while (!(next = READ_ONCE(node->next)))
- cpu_relax();
+ cmpwait(&node->next, NULL);
}
arch_mcs_spin_unlock_contended(&next->locked);
Powered by blists - more mailing lists