lkml.org 
[lkml]   [2016]   [Apr]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC][PATCH 3/3] locking,arm64: Introduce cmpwait()
    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

    \
     
     \ /
      Last update: 2016-04-12 19:21    [W:3.872 / U:0.192 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site