Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1370591 > unrolled thread

[RFC][PATCH 3/3] locking,arm64: Introduce cmpwait()

Started byPeter Zijlstra <peterz@infradead.org>
First post2016-04-04 14:40 +0200
Last post2016-04-13 15:00 +0200
Articles 4 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait() Peter Zijlstra <peterz@infradead.org> - 2016-04-04 14:40 +0200
    Re: [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait() Peter Zijlstra <peterz@infradead.org> - 2016-04-04 15:20 +0200
    Re: [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait() Will Deacon <will.deacon@arm.com> - 2016-04-12 19:00 +0200
      Re: [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait() Peter Zijlstra <peterz@infradead.org> - 2016-04-13 15:00 +0200

#1370591 — [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait()

FromPeter Zijlstra <peterz@infradead.org>
Date2016-04-04 14:40 +0200
Subject[RFC][PATCH 3/3] locking,arm64: Introduce cmpwait()
Message-ID<rkbXs-31q-7@gated-at.bofh.it>
Provide the cmpwait() primitive, which will 'spin' wait for a variable
to change. Use it to implement smp_cond_load_acquire() and provide an
ARM64 implementation.

The ARM64 implementation uses LDXR+WFE to avoid most spinning by
letting the hardware go idle while waiting for the exclusive load of
the variable to be cancelled (as anybody changing the value must).

I've misplaced my arm64 compiler, so this is not even compile tested.

Suggested-by: Will Deacon <will.deacon@arm.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/arm64/include/asm/cmpxchg.h |   36 +++++++++++++++++++++++++++++
 include/linux/atomic.h           |   47 +++++++++++++++++++++++++++++++++++++++
 include/linux/compiler.h         |   30 ------------------------
 3 files changed, 83 insertions(+), 30 deletions(-)

--- a/arch/arm64/include/asm/cmpxchg.h
+++ b/arch/arm64/include/asm/cmpxchg.h
@@ -224,4 +224,40 @@ __CMPXCHG_GEN(_mb)
 	__ret;								\
 })
 
+#define __CMPWAIT_GEN(w, sz, name)					\
+void __cmpwait_case_##name(volatile void *ptr, unsigned long val)	\
+{									\
+	unsigned long tmp;						\
+									\
+	asm volatile(							\
+	"	ldxr" #sz "\t%" #w "[tmp], %[v]\n"			\
+	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
+	"	cbnz	%" #w "[tmp], 1f\n"				\
+	"	wfe\n"							\
+	"1:"								\
+	: [tmp] "=&r" (tmp), [val] "=&r" (val),				\
+	  [v] "+Q" (*(unsigned long *)ptr));				\
+}
+
+__CMPWAIT_GEN(w, b, 1);
+__CMPWAIT_GEN(w, h, 2);
+__CMPWAIT_GEN(w,  , 4);
+__CMPWAIT_GEN( ,  , 8);
+
+static inline void __cmpwait(volatile void *ptr, unsigned long val, int size)
+{
+	switch (size) {
+	case 1: return __cmpwait_case_1(ptr, val);
+	case 2: return __cmpwait_case_2(ptr, val);
+	case 4: return __cmpwait_case_4(ptr, val);
+	case 8: return __cmpwait_case_8(ptr, val);
+	default: BUILD_BUG();
+	}
+
+	unreachable();
+}
+
+#define cmpwait(ptr, val) \
+	__cmpwait((ptr), (unsigned long)(val), sizeof(*(ptr)))
+
 #endif	/* __ASM_CMPXCHG_H */
--- a/include/linux/atomic.h
+++ b/include/linux/atomic.h
@@ -30,6 +30,53 @@
 #define  atomic_set_release(v, i)	smp_store_release(&(v)->counter, (i))
 #endif
 
+/**
+ * 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.
+ */
+#ifndef cmpwait
+#define cmpwait(ptr, val) do {					\
+	typeof (ptr) __ptr = (ptr);				\
+	typeof (val) __val = (val);				\
+	while (READ_ONCE(*__ptr) == __val)			\
+		cpu_relax();					\
+} while (0)
+#endif
+
+/**
+ * smp_cond_load_acquire() - (Spin) wait for cond with ACQUIRE ordering
+ * @ptr: pointer to the variable to wait on
+ * @cond: boolean expression to wait for
+ *
+ * Equivalent to using smp_load_acquire() on the condition variable but employs
+ * the control dependency of the wait to reduce the barrier on many platforms.
+ *
+ * Due to C lacking lambda expressions we load the value of *ptr into a
+ * pre-named variable @VAL to be used in @cond.
+ *
+ * The control dependency provides a LOAD->STORE order, the additional RMB
+ * provides LOAD->LOAD order, together they provide LOAD->{LOAD,STORE} order,
+ * aka. ACQUIRE.
+ */
+#ifndef smp_cond_load_acquire
+#define smp_cond_load_acquire(ptr, cond_expr) ({		\
+	typeof(ptr) __PTR = (ptr);				\
+	typeof(*ptr) VAL;					\
+	for (;;) {						\
+		VAL = READ_ONCE(*__PTR);			\
+		if (cond_expr)					\
+			break;					\
+		cmpwait(__PTR, VAL);				\
+	}							\
+	smp_rmb(); /* ctrl + rmb := acquire */			\
+	VAL;							\
+})
+#endif
+
 /*
  * The idea here is to build acquire/release variants by adding explicit
  * barriers on top of the relaxed variant. In the case where the relaxed
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -304,36 +304,6 @@ static __always_inline void __write_once
 	__u.__val;					\
 })
 
-/**
- * smp_cond_load_acquire() - (Spin) wait for cond with ACQUIRE ordering
- * @ptr: pointer to the variable to wait on
- * @cond: boolean expression to wait for
- *
- * Equivalent to using smp_load_acquire() on the condition variable but employs
- * the control dependency of the wait to reduce the barrier on many platforms.
- *
- * Due to C lacking lambda expressions we load the value of *ptr into a
- * pre-named variable @VAL to be used in @cond.
- *
- * The control dependency provides a LOAD->STORE order, the additional RMB
- * provides LOAD->LOAD order, together they provide LOAD->{LOAD,STORE} order,
- * aka. ACQUIRE.
- */
-#ifndef smp_cond_load_acquire
-#define smp_cond_load_acquire(ptr, cond_expr) ({		\
-	typeof(ptr) __PTR = (ptr);				\
-	typeof(*ptr) VAL;					\
-	for (;;) {						\
-		VAL = READ_ONCE(*__PTR);			\
-		if (cond_expr)					\
-			break;					\
-		cpu_relax();					\
-	}							\
-	smp_rmb(); /* ctrl + rmb := acquire */			\
-	VAL;							\
-})
-#endif
-
 #endif /* __KERNEL__ */
 
 #endif /* __ASSEMBLY__ */

[toc] | [next] | [standalone]

#1370610

FromPeter Zijlstra <peterz@infradead.org>
Date2016-04-04 15:20 +0200
Message-ID<rkcAb-3vK-33@gated-at.bofh.it>
In reply to#1370591
On Mon, Apr 04, 2016 at 02:22:53PM +0200, Peter Zijlstra wrote:
> +#define __CMPWAIT_GEN(w, sz, name)					\

 +static inline  \

> +void __cmpwait_case_##name(volatile void *ptr, unsigned long val)	\
> +{									\
> +	unsigned long tmp;						\
> +									\
> +	asm volatile(							\
> +	"	ldxr" #sz "\t%" #w "[tmp], %[v]\n"			\
> +	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
> +	"	cbnz	%" #w "[tmp], 1f\n"				\
> +	"	wfe\n"							\
> +	"1:"								\
> +	: [tmp] "=&r" (tmp), [val] "=&r" (val),				\
> +	  [v] "+Q" (*(unsigned long *)ptr));				\

And this probably wants a "memory" clobber to force reload values after
this returns.

> +}

[toc] | [prev] | [next] | [standalone]

#1377021

FromWill Deacon <will.deacon@arm.com>
Date2016-04-12 19:00 +0200
Message-ID<rn9Ps-27p-23@gated-at.bofh.it>
In reply to#1370591
Hi Peter,

Thanks for looking at this!

On Mon, Apr 04, 2016 at 02:22:53PM +0200, Peter Zijlstra wrote:
> Provide the cmpwait() primitive, which will 'spin' wait for a variable
> to change. Use it to implement smp_cond_load_acquire() and provide an
> ARM64 implementation.
> 
> The ARM64 implementation uses LDXR+WFE to avoid most spinning by
> letting the hardware go idle while waiting for the exclusive load of
> the variable to be cancelled (as anybody changing the value must).
> 
> I've misplaced my arm64 compiler, so this is not even compile tested.

Guess what? ;)

init/do_mounts_initrd.o: In function `__cmpwait_case_1':
/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:242: multiple definition of `__cmpwait_case_1'
init/do_mounts.o:/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:242: first defined here
init/do_mounts_initrd.o: In function `__cmpwait_case_2':
/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:243: multiple definition of `__cmpwait_case_2'
init/do_mounts.o:/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:243: first defined here
init/do_mounts_initrd.o: In function `__cmpwait_case_4':
/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:244: multiple definition of `__cmpwait_case_4'
init/do_mounts.o:/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:244: first defined here
init/do_mounts_initrd.o: In function `__cmpwait_case_8':
/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:245: multiple definition of `__cmpwait_case_8'
init/do_mounts.o:/home/will/work/aarch32/linux/linux/./arch/arm64/include/asm/cmpxchg.h:245: first defined here
make[1]: *** [init/mounts.o] Error 1
make: *** [init] Error 2
make: *** Waiting for unfinished jobs....

(and lot of similar errors).

Looks like you're just missing an #undef in cmpxchg.h.

FWIW, you can pick up arm64 toolchain binaries from:

  https://releases.linaro.org/components/toolchain/binaries/latest-5/

> Suggested-by: Will Deacon <will.deacon@arm.com>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
>  arch/arm64/include/asm/cmpxchg.h |   36 +++++++++++++++++++++++++++++
>  include/linux/atomic.h           |   47 +++++++++++++++++++++++++++++++++++++++
>  include/linux/compiler.h         |   30 ------------------------
>  3 files changed, 83 insertions(+), 30 deletions(-)
> 
> --- a/arch/arm64/include/asm/cmpxchg.h
> +++ b/arch/arm64/include/asm/cmpxchg.h
> @@ -224,4 +224,40 @@ __CMPXCHG_GEN(_mb)
>  	__ret;								\
>  })
>  
> +#define __CMPWAIT_GEN(w, sz, name)					\
> +void __cmpwait_case_##name(volatile void *ptr, unsigned long val)	\
> +{									\
> +	unsigned long tmp;						\
> +									\
> +	asm volatile(							\
> +	"	ldxr" #sz "\t%" #w "[tmp], %[v]\n"			\
> +	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
> +	"	cbnz	%" #w "[tmp], 1f\n"				\

Shouldn't this be cbz? (i.e. branch over the wfe if the value is equal
to what we wanted?).

> +	"	wfe\n"							\
> +	"1:"								\
> +	: [tmp] "=&r" (tmp), [val] "=&r" (val),				\

We only read val, so it can be an input operand, no?

> +	  [v] "+Q" (*(unsigned long *)ptr));				\
> +}
> +
> +__CMPWAIT_GEN(w, b, 1);
> +__CMPWAIT_GEN(w, h, 2);
> +__CMPWAIT_GEN(w,  , 4);
> +__CMPWAIT_GEN( ,  , 8);
> +
> +static inline void __cmpwait(volatile void *ptr, unsigned long val, int size)
> +{
> +	switch (size) {
> +	case 1: return __cmpwait_case_1(ptr, val);
> +	case 2: return __cmpwait_case_2(ptr, val);
> +	case 4: return __cmpwait_case_4(ptr, val);
> +	case 8: return __cmpwait_case_8(ptr, val);
> +	default: BUILD_BUG();
> +	}
> +
> +	unreachable();
> +}
> +
> +#define cmpwait(ptr, val) \
> +	__cmpwait((ptr), (unsigned long)(val), sizeof(*(ptr)))

We might want to call this cmpwait_relaxed, in case we decide to add
fenced versions in the future. Or just make it cmpwait_acquire and
remove the smp_rmb() from smp_cond_load_acquire(). Dunno.

Will

[toc] | [prev] | [next] | [standalone]

#1377909

FromPeter Zijlstra <peterz@infradead.org>
Date2016-04-13 15:00 +0200
Message-ID<rnsyL-1lV-41@gated-at.bofh.it>
In reply to#1377021
On Tue, Apr 12, 2016 at 05:59:41PM +0100, Will Deacon wrote:
> Thanks for looking at this!

n/p, had to se what it would look like etc.. :-)

> > I've misplaced my arm64 compiler, so this is not even compile tested.
> 
> Guess what? ;)
> 

> make: *** [init] Error 2
> make: *** Waiting for unfinished jobs....
> 
> (and lot of similar errors).
> 
> Looks like you're just missing an #undef in cmpxchg.h.
> 
> FWIW, you can pick up arm64 toolchain binaries from:
> 
>   https://releases.linaro.org/components/toolchain/binaries/latest-5/

Ah, I usually build a whole set from sources; for some reason arm64
didn't build in the latest run. I'll have to kick it.

> > +#define __CMPWAIT_GEN(w, sz, name)					\
> > +void __cmpwait_case_##name(volatile void *ptr, unsigned long val)	\
> > +{									\
> > +	unsigned long tmp;						\
> > +									\
> > +	asm volatile(							\
> > +	"	ldxr" #sz "\t%" #w "[tmp], %[v]\n"			\
> > +	"	eor	%" #w "[tmp], %" #w "[tmp], %" #w "[val]\n"	\
> > +	"	cbnz	%" #w "[tmp], 1f\n"				\
> 
> Shouldn't this be cbz? (i.e. branch over the wfe if the value is equal
> to what we wanted?).

Indeed so.

> > +	"	wfe\n"							\
> > +	"1:"								\
> > +	: [tmp] "=&r" (tmp), [val] "=&r" (val),				\
> 
> We only read val, so it can be an input operand, no?

True.. :-)

> > +#define cmpwait(ptr, val) \
> > +	__cmpwait((ptr), (unsigned long)(val), sizeof(*(ptr)))
> 
> We might want to call this cmpwait_relaxed, in case we decide to add
> fenced versions in the future. Or just make it cmpwait_acquire and
> remove the smp_rmb() from smp_cond_load_acquire(). Dunno.

This is something I'll very much leave up to you. I have no idea on the
tradeoffs involved here.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web