From 2a14b8d614427ab0648db16c6da9c80b54427df4 Mon Sep 17 00:00:00 2001 From: Sam Lantinga Date: Tue, 25 Jan 2011 17:40:06 -0800 Subject: [PATCH] Improvements based on feedback from Anthony Williams --- configure.in | 2 + include/SDL_atomic.h | 120 ++++++++++++++++++++++++-------------- src/atomic/SDL_atomic.c | 79 +------------------------ src/atomic/SDL_spinlock.c | 18 ++++-- 4 files changed, 93 insertions(+), 126 deletions(-) diff --git a/configure.in b/configure.in index 98bb7b166..2bbff60d7 100644 --- a/configure.in +++ b/configure.in @@ -301,6 +301,7 @@ if test x$enable_gcc_atomics = xyes; then int a; void *x, *y, *z; __sync_lock_test_and_set(&a, 4); + __sync_lock_test_and_set(&x, y); __sync_fetch_and_add(&a, 1); __sync_bool_compare_and_swap(&a, 5, 10); __sync_bool_compare_and_swap(&x, y, z); @@ -317,6 +318,7 @@ if test x$enable_gcc_atomics = xyes; then ],[ int a; __sync_lock_test_and_set(&a, 1); + __sync_lock_release(&a); ],[ have_gcc_sync_lock_test_and_set=yes ]) diff --git a/include/SDL_atomic.h b/include/SDL_atomic.h index 9f339c7cb..8342489d0 100644 --- a/include/SDL_atomic.h +++ b/include/SDL_atomic.h @@ -38,8 +38,13 @@ * SDL_AtomicDecRef() * * Seriously, here be dragons! + * ^^^^^^^^^^^^^^^^^^^^^^^^^^^ * - * These operations may, or may not, actually be implemented using + * You can find out a little more about lockless programming and the + * subtle issues that can arise here: + * http://msdn.microsoft.com/en-us/library/ee418650%28v=vs.85%29.aspx + * + * These operations may or may not actually be implemented using * processor specific atomic operations. When possible they are * implemented as true processor specific atomic operations. When that * is not possible the are implemented using locks that *do* use the @@ -114,66 +119,54 @@ extern DECLSPEC void SDLCALL SDL_AtomicUnlock(SDL_SpinLock *lock); /*@}*//*SDL AtomicLock*/ + +/* The compiler barrier prevents the compiler from reordering + reads and writes to globally visible variables across the call. +*/ +#ifdef _MSC_VER +void _ReadWriteBarrier(void); +#pragma intrinsic(_ReadWriteBarrier) +#define SDL_CompilerBarrier() _ReadWriteBarrier() +#elif __GNUC__ +#define SDL_CompilerBarrier() __asm__ __volatile__ ("" : : : "memory") +#else +#define SDL_CompilerBarrier() \ +({ SDL_SpinLock _tmp = 0; SDL_AtomicLock(&_tmp); SDL_AtomicUnlock(&_tmp); }) +#endif + /* Platform specific optimized versions of the atomic functions, * you can disable these by defining SDL_DISABLE_ATOMIC_INLINE */ #ifndef SDL_DISABLE_ATOMIC_INLINE -#if defined(HAVE_MSC_ATOMICS) +#if HAVE_MSC_ATOMICS #define SDL_AtomicSet(a, v) _InterlockedExchange((long*)&(a)->value, (v)) -#define SDL_AtomicGet(a) ((a)->value) #define SDL_AtomicAdd(a, v) _InterlockedExchangeAdd((long*)&(a)->value, (v)) #define SDL_AtomicCAS(a, oldval, newval) (_InterlockedCompareExchange((long*)&(a)->value, (newval), (oldval)) == (oldval)) -#define SDL_AtomicSetPtr(a, v) (void)_InterlockedExchangePointer((a), (v)) -#define SDL_AtomicGetPtr(a) (*(a)) +#define SDL_AtomicSetPtr(a, v) _InterlockedExchangePointer((a), (v)) #if _M_IX86 #define SDL_AtomicCASPtr(a, oldval, newval) (_InterlockedCompareExchange((long*)(a), (long)(newval), (long)(oldval)) == (long)(oldval)) #else #define SDL_AtomicCASPtr(a, oldval, newval) (_InterlockedCompareExchangePointer((a), (newval), (oldval)) == (oldval)) #endif -#elif defined(__MACOSX__) +#elif __MACOSX__ #include -#define SDL_AtomicSet(a, v) \ -({ \ - int oldvalue; \ - \ - do { \ - oldvalue = (a)->value; \ - } while (!OSAtomicCompareAndSwap32Barrier(oldvalue, v, &(a)->value)); \ - \ - oldvalue; \ -}) -#define SDL_AtomicGet(a) ((a)->value) -#define SDL_AtomicAdd(a, v) \ -({ \ - int oldvalue; \ - \ - do { \ - oldvalue = (a)->value; \ - } while (!OSAtomicCompareAndSwap32Barrier(oldvalue, oldvalue+v, &(a)->value)); \ - \ - oldvalue; \ -}) -#define SDL_AtomicCAS(a, oldval, newval) OSAtomicCompareAndSwap32Barrier(oldval, newval, &(a)->value) -#define SDL_AtomicSetPtr(a, v) (*(a) = v, OSMemoryBarrier()) -#define SDL_AtomicGetPtr(a) (*(a)) +#define SDL_AtomicCAS(a, oldval, newval) OSAtomicCompareAndSwap32Barrier((oldval), (newval), &(a)->value) #if SIZEOF_VOIDP == 4 #define SDL_AtomicCASPtr(a, oldval, newval) OSAtomicCompareAndSwap32Barrier((int32_t)(oldval), (int32_t)(newval), (int32_t*)(a)) #elif SIZEOF_VOIDP == 8 #define SDL_AtomicCASPtr(a, oldval, newval) OSAtomicCompareAndSwap64Barrier((int64_t)(oldval), (int64_t)(newval), (int64_t*)(a)) #endif -#elif defined(HAVE_GCC_ATOMICS) +#elif HAVE_GCC_ATOMICS #define SDL_AtomicSet(a, v) __sync_lock_test_and_set(&(a)->value, v) -#define SDL_AtomicGet(a) ((a)->value) #define SDL_AtomicAdd(a, v) __sync_fetch_and_add(&(a)->value, v) +#define SDL_AtomicSetPtr(a, v) __sync_lock_test_and_set(a, v) #define SDL_AtomicCAS(a, oldval, newval) __sync_bool_compare_and_swap(&(a)->value, oldval, newval) -#define SDL_AtomicSetPtr(a, v) (*(a) = v, __sync_synchronize()) -#define SDL_AtomicGetPtr(a) (*(a)) #define SDL_AtomicCASPtr(a, oldval, newval) __sync_bool_compare_and_swap(a, oldval, newval) #endif @@ -195,40 +188,61 @@ typedef struct { int value; } SDL_atomic_t; * \return The previous value of the atomic variable. */ #ifndef SDL_AtomicSet -extern DECLSPEC int SDLCALL SDL_AtomicSet(SDL_atomic_t *a, int value); +#define SDL_AtomicSet(a, v) \ +({ \ + int _value; \ + do { \ + _value = (a)->value; \ + } while (!SDL_AtomicCAS(a, _value, (v))); \ + _value; \ +}) #endif /** * \brief Get the value of an atomic variable */ #ifndef SDL_AtomicGet -extern DECLSPEC int SDLCALL SDL_AtomicGet(SDL_atomic_t *a); +#define SDL_AtomicGet(a) \ +({ \ + int _value = (a)->value; \ + SDL_CompilerBarrier(); \ + _value; \ +}) #endif /** - * \brief Add to an atomic variable. + * \brief Add to an atomic variable. * * \return The previous value of the atomic variable. + * + * \note This same style can be used for any number operation */ #ifndef SDL_AtomicAdd -extern DECLSPEC int SDLCALL SDL_AtomicAdd(SDL_atomic_t *a, int value); +#define SDL_AtomicAdd(a, v) \ +({ \ + int _value; \ + do { \ + _value = (a)->value; \ + } while (!SDL_AtomicCAS(a, _value, (_value + (v)))); \ + _value; \ +}) #endif /** * \brief Increment an atomic variable used as a reference count. */ #ifndef SDL_AtomicIncRef -extern DECLSPEC void SDLCALL SDL_AtomicIncRef(SDL_atomic_t *a); +#define SDL_AtomicIncRef(a) SDL_AtomicAdd(a, 1) #endif /** * \brief Decrement an atomic variable used as a reference count. * - * \return SDL_TRUE if the variable has reached zero after decrementing, + * \return SDL_TRUE if the variable reached zero after decrementing, * SDL_FALSE otherwise */ #ifndef SDL_AtomicDecRef -extern DECLSPEC SDL_bool SDLCALL SDL_AtomicDecRef(SDL_atomic_t *a); +#define SDL_AtomicDecRef(a) (SDL_AtomicAdd(a, -1) == 1) #endif /** @@ -239,21 +253,36 @@ extern DECLSPEC SDL_bool SDLCALL SDL_AtomicDecRef(SDL_atomic_t *a); * \note If you don't know what this function is for, you shouldn't use it! */ #ifndef SDL_AtomicCAS -extern DECLSPEC SDL_bool SDLCALL SDL_AtomicCAS(SDL_atomic_t *a, int oldval, int newval); +#define SDL_AtomicCAS SDL_AtomicCAS_ #endif +extern DECLSPEC SDL_bool SDLCALL SDL_AtomicCAS_(SDL_atomic_t *a, int oldval, int newval); /** * \brief Set a pointer to a value atomically. + * + * \return The previous value of the pointer. */ #ifndef SDL_AtomicSetPtr -extern DECLSPEC void SDLCALL SDL_AtomicSetPtr(void** a, void* value); +#define SDL_AtomicSetPtr(a, v) \ +({ \ + void* _value; \ + do { \ + _value = *(a); \ + } while (!SDL_AtomicCASPtr(a, _value, (v))); \ + _value; \ +}) #endif /** * \brief Get the value of a pointer atomically. */ #ifndef SDL_AtomicGetPtr -extern DECLSPEC void* SDLCALL SDL_AtomicGetPtr(void** a); +#define SDL_AtomicGetPtr(a) \ +({ \ + void* _value = *(a); \ + SDL_CompilerBarrier(); \ + _value; \ +}) #endif /** @@ -264,8 +293,9 @@ extern DECLSPEC void* SDLCALL SDL_AtomicGetPtr(void** a); * \note If you don't know what this function is for, you shouldn't use it! */ #ifndef SDL_AtomicCASPtr -extern DECLSPEC SDL_bool SDLCALL SDL_AtomicCASPtr(void **a, void *oldval, void *newval); +#define SDL_AtomicCASPtr SDL_AtomicCASPtr_ #endif +extern DECLSPEC SDL_bool SDLCALL SDL_AtomicCASPtr_(void **a, void *oldval, void *newval); /* Ends C function definitions when using C++ */ #ifdef __cplusplus diff --git a/src/atomic/SDL_atomic.c b/src/atomic/SDL_atomic.c index 50cc7da36..7790823ed 100644 --- a/src/atomic/SDL_atomic.c +++ b/src/atomic/SDL_atomic.c @@ -70,61 +70,8 @@ leaveLock(void *a) SDL_AtomicUnlock(&locks[index]); } -#undef SDL_AtomicSet -int -SDL_AtomicSet(SDL_atomic_t *a, int value) -{ - int oldvalue; - - enterLock(a); - oldvalue = a->value; - a->value = value; - leaveLock(a); - - return oldvalue; -} - -#undef SDL_AtomicGet -int -SDL_AtomicGet(SDL_atomic_t *a) -{ - /* Assuming integral reads on this platform, we're safe here since the - functions that set the variable have the necessary memory barriers. - */ - return a->value; -} - -#undef SDL_AtomicAdd -int -SDL_AtomicAdd(SDL_atomic_t *a, int value) -{ - int oldvalue; - - enterLock(a); - oldvalue = a->value; - a->value += value; - leaveLock(a); - - return oldvalue; -} - -#undef SDL_AtomicIncRef -void -SDL_AtomicIncRef(SDL_atomic_t *a) -{ - SDL_AtomicAdd(a, 1); -} - -#undef SDL_AtomicDecRef -SDL_bool -SDL_AtomicDecRef(SDL_atomic_t *a) -{ - return SDL_AtomicAdd(a, -1) == 1; -} - -#undef SDL_AtomicCAS SDL_bool -SDL_AtomicCAS(SDL_atomic_t *a, int oldval, int newval) +SDL_AtomicCAS_(SDL_atomic_t *a, int oldval, int newval) { SDL_bool retval = SDL_FALSE; @@ -138,28 +85,8 @@ SDL_AtomicCAS(SDL_atomic_t *a, int oldval, int newval) return retval; } -#undef SDL_AtomicSetPtr -void -SDL_AtomicSetPtr(void** a, void* value) -{ - void *prevval; - do { - prevval = *a; - } while (!SDL_AtomicCASPtr(a, prevval, value)); -} - -#undef SDL_AtomicGetPtr -void* -SDL_AtomicGetPtr(void** a) -{ - /* Assuming integral reads on this platform, we're safe here since the - functions that set the pointer have the necessary memory barriers. - */ - return *a; -} - -#undef SDL_AtomicCASPtr -SDL_bool SDL_AtomicCASPtr(void **a, void *oldval, void *newval) +SDL_bool +SDL_AtomicCASPtr_(void **a, void *oldval, void *newval) { SDL_bool retval = SDL_FALSE; diff --git a/src/atomic/SDL_spinlock.c b/src/atomic/SDL_spinlock.c index 25fdca27a..3580455ba 100644 --- a/src/atomic/SDL_spinlock.c +++ b/src/atomic/SDL_spinlock.c @@ -37,20 +37,20 @@ SDL_AtomicTryLock(SDL_SpinLock *lock) SDL_COMPILE_TIME_ASSERT(locksize, sizeof(*lock) == sizeof(long)); return (InterlockedExchange((long*)lock, 1) == 0); -#elif defined(__MACOSX__) +#elif __MACOSX__ return OSAtomicCompareAndSwap32Barrier(0, 1, lock); -#elif defined(HAVE_GCC_ATOMICS) || defined(HAVE_GCC_SYNC_LOCK_TEST_AND_SET) +#elif HAVE_GCC_ATOMICS || HAVE_GCC_SYNC_LOCK_TEST_AND_SET return (__sync_lock_test_and_set(lock, 1) == 0); -#elif defined(__GNUC__) && defined(__arm__) && defined(__ARM_ARCH_5__) +#elif __GNUC__ && __arm__ && __ARM_ARCH_5__ int result; __asm__ __volatile__ ( "swp %0, %1, [%2]\n" : "=&r,&r" (result) : "r,0" (1), "r,r" (lock) : "memory"); return (result == 0); -#elif defined(__GNUC__) && defined(__arm__) +#elif __GNUC__ && __arm__ int result; __asm__ __volatile__ ( "ldrex %0, [%2]\nteq %0, #0\nstrexeq %0, %1, [%2]" @@ -75,8 +75,16 @@ SDL_AtomicLock(SDL_SpinLock *lock) void SDL_AtomicUnlock(SDL_SpinLock *lock) { - /* Assuming atomic assignment operation and full memory barrier in lock */ +#if defined(_MSC_VER) + _ReadWriteBarrier(); + *lock = 0; + +#elif HAVE_GCC_ATOMICS || HAVE_GCC_SYNC_LOCK_TEST_AND_SET + __sync_lock_release(lock); + +#else *lock = 0; +#endif } /* vi: set ts=4 sw=4 expandtab: */