lkml.org 
[lkml]   [2017]   [Mar]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: locking/atomic: Introduce atomic_try_cmpxchg()
    On Fri, Mar 24, 2017 at 6:23 PM, Peter Zijlstra <peterz@infradead.org> wrote:
    > On Fri, Mar 24, 2017 at 09:54:46AM -0700, Andy Lutomirski wrote:
    >> > So the first snipped I tested regressed like so:
    >> >
    >> >
    >> > 0000000000000000 <T_refcount_inc>: 0000000000000000 <T_refcount_inc>:
    >> > 0: 8b 07 mov (%rdi),%eax 0: 8b 17 mov (%rdi),%edx
    >> > 2: 83 f8 ff cmp $0xffffffff,%eax 2: 83 fa ff cmp $0xffffffff,%edx
    >> > 5: 74 13 je 1a <T_refcount_inc+0x1a> 5: 74 1a je 21 <T_refcount_inc+0x21>
    >> > 7: 85 c0 test %eax,%eax 7: 85 d2 test %edx,%edx
    >> > 9: 74 0d je 18 <T_refcount_inc+0x18> 9: 74 13 je 1e <T_refcount_inc+0x1e>
    >> > b: 8d 50 01 lea 0x1(%rax),%edx b: 8d 4a 01 lea 0x1(%rdx),%ecx
    >> > e: f0 0f b1 17 lock cmpxchg %edx,(%rdi) e: 89 d0 mov %edx,%eax
    >> > 12: 75 ee jne 2 <T_refcount_inc+0x2> 10: f0 0f b1 0f lock cmpxchg %ecx,(%rdi)
    >> > 14: ff c2 inc %edx 14: 74 04 je 1a <T_refcount_inc+0x1a>
    >> > 16: 75 02 jne 1a <T_refcount_inc+0x1a> 16: 89 c2 mov %eax,%edx
    >> > 18: 0f 0b ud2 18: eb e8 jmp 2 <T_refcount_inc+0x2>
    >> > 1a: c3 retq 1a: ff c1 inc %ecx
    >> > 1c: 75 03 jne 21 <T_refcount_inc+0x21>
    >> > 1e: 0f 0b ud2
    >> > 20: c3 retq
    >> > 21: c3 retq
    >>
    >> Can you re-send the better asm you got earlier?
    >
    > On the left?
    >
    >> If I pretend to be a dumb compiler, I wonder if you'd get better results with:
    >>
    >> if (!success) {
    >> *_old = __old;
    >> return false;
    >> } else {
    >> return true;
    >> }
    >>
    >> or however you jam that into a statement expression. That way you
    >> aren't relying on the compiler to merge the branches.
    >
    > I tried a few variants, but nothing really made it better.
    >
    > Find the tiny.c file below; I'm using:
    >
    > gcc (Debian 6.3.0-5) 6.3.0 20170124
    >
    > it has both an inline and an stmt-expr try_cmpxchg variant to play with;
    > the 'expected' output is at the bottom (same as above left).
    >
    > Note that clang doesn't compile this stuff due to missing features.
    >
    > ---
    >
    > /* gcc -Os -std=gnu99 -fno-strict-overflow -falign-jumps=1 -falign-loops=1 -c tiny.c; objdump -dr tiny.o */
    >
    > typedef _Bool bool;
    >
    > static inline bool try_cmpxchg(unsigned int *ptr, unsigned int *val, unsigned int new)
    > {
    > unsigned int old = *val;
    > bool success;
    >
    > asm volatile("lock cmpxchgl %[new], %[ptr]"
    > : "=@ccz" (success),
    > [ptr] "+m" (*ptr),
    > [old] "+a" (old)
    > : [new] "r" (new)
    > : "memory");
    >
    > // if (!success)
    > *val = old;
    >
    > return success;
    > }
    >
    > #define __try_cmpxchg(_ptr, _pold, _new) \
    > ({ \
    > bool success; \
    > __typeof__(_ptr) _old = (_pold); \
    > __typeof__(*(_ptr)) __old = *_old; \
    > __typeof__(*(_ptr)) __new = (_new); \
    > volatile unsigned int * __ptr = (volatile unsigned int *)(_ptr);\
    > \
    > asm volatile("lock cmpxchgl %[new], %[ptr]" \
    > : "=@ccz" (success), \
    > [ptr] "+m" (*__ptr), \
    > [old] "+a" (__old) \
    > : [new] "r" (__new) \
    > : "memory"); \
    > /* if (!success) */ \
    > *_old = __old; \
    > success; \
    > })
    >
    > #define EXCEPTION_VALUE(val, handler) asm volatile ("ud2 # %0" : : "r" (val))
    >
    > #define UINT_MAX (~0U)
    >
    > #define likely(x) __builtin_expect(!!(x), 1)
    > #define unlikely(x) __builtin_expect(!!(x), 0)
    >
    > static inline void refcount_inc(unsigned int *r)
    > {
    > unsigned int new, val = *(unsigned int volatile *)r;
    >
    > do {
    > if (unlikely(val == UINT_MAX)) /* saturated */
    > return;
    >
    > if (unlikely(!val)) /* use-after-free */
    > goto exception;
    >
    > /* cannot overflow because we already checked UINT_MAX */
    > new = val + 1;
    >
    > } while (unlikely(!try_cmpxchg(r, &val, new)));
    >
    > if (unlikely(new == UINT_MAX))
    > exception: EXCEPTION_VALUE(val, __refcount_warn);
    > }
    >
    > void T_refcount_inc(unsigned int *r)
    > {
    > refcount_inc(r);
    > }
    >
    > /*
    > 0: 8b 07 mov (%rdi),%eax
    > 2: 83 f8 ff cmp $0xffffffff,%eax
    > 5: 74 13 je 1a <T_refcount_inc+0x1a>
    > 7: 85 c0 test %eax,%eax
    > 9: 74 0d je 18 <T_refcount_inc+0x18>
    > b: 8d 50 01 lea 0x1(%rax),%edx
    > e: f0 0f b1 17 lock cmpxchg %edx,(%rdi)
    > 12: 75 ee jne 2 <T_refcount_inc+0x2>
    > 14: ff c2 inc %edx
    > 16: 75 02 jne 1a <T_refcount_inc+0x1a>
    > 18: 0f 0b ud2
    > 1a: c3 retq
    > */



    This seems to help ;)

    #define try_cmpxchg(ptr, pold, new) __atomic_compare_exchange_n(ptr,
    pold, new, 0, __ATOMIC_SEQ_CST, __ATOMIC_SEQ_CST)

    \
     
     \ /
      Last update: 2017-03-24 18:52    [W:4.408 / U:0.064 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site