Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1574355
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Newsgroups | linux.kernel |
| Subject | Re: [PATCH 4/5] atomic: Introduce atomic_try_cmpxchg() |
| Date | 2017-02-06 05:30 +0100 |
| Message-ID | <t7J69-8py-3@gated-at.bofh.it> (permalink) |
| References | <t6MfM-29e-5@gated-at.bofh.it> <t6MfM-29e-9@gated-at.bofh.it> |
| Organization | linux.* mail to news gateway |
[Multipart message — attachments visible in raw view] - view raw
On Fri, Feb 03, 2017 at 02:26:02PM +0100, Peter Zijlstra wrote:
> Add a new cmpxchg interface:
>
> bool try_cmpxchg(u{8,16,32,64} *ptr, u{8,16,32,64} *val, u{8,16,32,64} new);
>
> Where the boolean returns the result of the compare; and thus if the
> exchange happened; and in case of failure, the new value of *ptr is
> returned in *val.
>
> This allows simplification/improvement of loops like:
>
> for (;;) {
> new = val $op $imm;
> old = cmpxchg(ptr, val, new);
> if (old == val)
> break;
> val = old;
> }
>
> into:
>
> for (;;) {
> new = val $op $imm;
> if (try_cmpxchg(ptr, &val, new))
> break;
> }
>
> while also generating better code (GCC6 and onwards).
>
But switching to try_cmpxchg() will make @val a memory location, which
could not be put in a register. And this will generate unnecessary
memory accesses on archs having enough registers(PPC, e.g.).
> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
> ---
> --- a/arch/x86/include/asm/atomic.h
> +++ b/arch/x86/include/asm/atomic.h
> @@ -186,6 +186,12 @@ static __always_inline int atomic_cmpxch
> return cmpxchg(&v->counter, old, new);
> }
>
> +#define atomic_try_cmpxchg atomic_try_cmpxchg
> +static __always_inline bool atomic_try_cmpxchg(atomic_t *v, int *old, int new)
> +{
> + return try_cmpxchg(&v->counter, old, new);
> +}
> +
> static inline int atomic_xchg(atomic_t *v, int new)
> {
> return xchg(&v->counter, new);
> --- a/arch/x86/include/asm/cmpxchg.h
> +++ b/arch/x86/include/asm/cmpxchg.h
> @@ -153,6 +153,75 @@ extern void __add_wrong_size(void)
> #define cmpxchg_local(ptr, old, new) \
> __cmpxchg_local(ptr, old, new, sizeof(*(ptr)))
>
> +
> +#define __raw_try_cmpxchg(_ptr, _pold, _new, size, lock) \
> +({ \
> + bool success; \
> + __typeof__(_ptr) _old = (_pold); \
> + __typeof__(*(_ptr)) __old = *_old; \
> + __typeof__(*(_ptr)) __new = (_new); \
> + switch (size) { \
> + case __X86_CASE_B: \
> + { \
> + volatile u8 *__ptr = (volatile u8 *)(_ptr); \
> + asm volatile(lock "cmpxchgb %[new], %[ptr]" \
> + CC_SET(z) \
> + : CC_OUT(z) (success), \
> + [ptr] "+m" (*__ptr), \
> + [old] "+a" (__old) \
> + : [new] "q" (__new) \
> + : "memory"); \
> + break; \
> + } \
> + case __X86_CASE_W: \
> + { \
> + volatile u16 *__ptr = (volatile u16 *)(_ptr); \
> + asm volatile(lock "cmpxchgw %[new], %[ptr]" \
> + CC_SET(z) \
> + : CC_OUT(z) (success), \
> + [ptr] "+m" (*__ptr), \
> + [old] "+a" (__old) \
> + : [new] "r" (__new) \
> + : "memory"); \
> + break; \
> + } \
> + case __X86_CASE_L: \
> + { \
> + volatile u32 *__ptr = (volatile u32 *)(_ptr); \
> + asm volatile(lock "cmpxchgl %[new], %[ptr]" \
> + CC_SET(z) \
> + : CC_OUT(z) (success), \
> + [ptr] "+m" (*__ptr), \
> + [old] "+a" (__old) \
> + : [new] "r" (__new) \
> + : "memory"); \
> + break; \
> + } \
> + case __X86_CASE_Q: \
> + { \
> + volatile u64 *__ptr = (volatile u64 *)(_ptr); \
> + asm volatile(lock "cmpxchgq %[new], %[ptr]" \
> + CC_SET(z) \
> + : CC_OUT(z) (success), \
> + [ptr] "+m" (*__ptr), \
> + [old] "+a" (__old) \
> + : [new] "r" (__new) \
> + : "memory"); \
> + break; \
> + } \
> + default: \
> + __cmpxchg_wrong_size(); \
> + } \
> + *_old = __old; \
> + success; \
> +})
> +
> +#define __try_cmpxchg(ptr, pold, new, size) \
> + __raw_try_cmpxchg((ptr), (pold), (new), (size), LOCK_PREFIX)
> +
> +#define try_cmpxchg(ptr, pold, new) \
> + __try_cmpxchg((ptr), (pold), (new), sizeof(*(ptr)))
> +
> /*
> * xadd() adds "inc" to "*ptr" and atomically returns the previous
> * value of "*ptr".
> --- a/include/linux/atomic.h
> +++ b/include/linux/atomic.h
> @@ -423,6 +423,28 @@
> #endif
> #endif /* atomic_cmpxchg_relaxed */
>
> +#ifndef atomic_try_cmpxchg
> +
> +#define __atomic_try_cmpxchg(type, _p, _po, _n) \
> +({ \
> + typeof(_po) __po = (_po); \
> + typeof(*(_po)) __o = *__po; \
> + bool success = (atomic_cmpxchg##type((_p), __o, (_n)) == __o); \
> + *__po = __o; \
Besides, is this part correct? atomic_cmpxchg_*() wouldn't change the
value of __o, so *__po wouldn't be changed.. IOW, in case of failure,
*ptr wouldn't be updated to a new value.
Maybe this should be:
bool success;
*__po = atomic_cmpxchg##type((_p), __o, (_n));
sucess = (*__po == _o);
, right?
Regards,
Boqun
> + success; \
> +})
> +
> +#define atomic_try_cmpxchg(_p, _po, _n) __atomic_try_cmpxchg(, _p, _po, _n)
> +#define atomic_try_cmpxchg_relaxed(_p, _po, _n) __atomic_try_cmpxchg(_relaxed, _p, _po, _n)
> +#define atomic_try_cmpxchg_acquire(_p, _po, _n) __atomic_try_cmpxchg(_acquire, _p, _po, _n)
> +#define atomic_try_cmpxchg_release(_p, _po, _n) __atomic_try_cmpxchg(_release, _p, _po, _n)
> +
> +#else /* atomic_try_cmpxchg */
> +#define atomic_try_cmpxchg_relaxed atomic_try_cmpxchg
> +#define atomic_try_cmpxchg_acquire atomic_try_cmpxchg
> +#define atomic_try_cmpxchg_release atomic_try_cmpxchg
> +#endif /* atomic_try_cmpxchg */
> +
> /* cmpxchg_relaxed */
> #ifndef cmpxchg_relaxed
> #define cmpxchg_relaxed cmpxchg
>
>
Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread
[PATCH 0/5] refcount_t and various related bits Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
[PATCH 4/5] atomic: Introduce atomic_try_cmpxchg() Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
Re: [PATCH 4/5] atomic: Introduce atomic_try_cmpxchg() Boqun Feng <boqun.feng@gmail.com> - 2017-02-06 05:30 +0100
Re: [PATCH 4/5] atomic: Introduce atomic_try_cmpxchg() Boqun Feng <boqun.feng@gmail.com> - 2017-02-06 07:40 +0100
Re: [PATCH 4/5] atomic: Introduce atomic_try_cmpxchg() Peter Zijlstra <peterz@infradead.org> - 2017-02-06 09:20 +0100
[PATCH 2/5] kref: Implement using refcount_t Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
Re: [PATCH 2/5] kref: Implement using refcount_t Greg KH <gregkh@linuxfoundation.org> - 2017-02-06 14:10 +0100
[PATCH 5/5] refcount: Use atomic_try_cmpxchg() Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
[PATCH 1/5] refcount_t: A special purpose refcount type Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
Re: [PATCH 1/5] refcount_t: A special purpose refcount type Kees Cook <keescook@chromium.org> - 2017-02-03 19:10 +0100
Re: [PATCH 1/5] refcount_t: A special purpose refcount type Kees Cook <keescook@chromium.org> - 2017-02-04 00:40 +0100
[PATCH 3/5] x86: Implement __WARN using UD2 Peter Zijlstra <peterz@infradead.org> - 2017-02-03 14:40 +0100
csiph-web