Spinlock cleanup

- Non-GCC intrinsics alternative implementation removed. Let's worry
  about the absence of the intrinsics once/if that becomes relevant.
- Spinlock release now performed using __sync_lock_release, as per
  svoj's advice.
- while-looping on the variable used as lock removed, so it no longer
  need to be volatile.
- Boolean function returns bool.
- Size of profiling counters increased.
- Risk for division-by-zero removed.
- Documentation moved from implementation to header.

include/maxscale/spinlock.h

@@ -24,6 +24,7 @@
  */
 
 #include <maxscale/cdefs.h>
+#include <stdbool.h>
 #include <maxscale/debug.h>
 
 MXS_BEGIN_DECLS
@@ -42,37 +43,73 @@ MXS_BEGIN_DECLS
  */
 typedef struct spinlock
 {
-    volatile int lock;/*< Is the lock held? */
+    int lock;              /*< Is the lock held? */
 #if SPINLOCK_PROFILE
-    int spins;        /*< Number of spins on this lock */
+    uint64_t spins;        /*< Number of spins on this lock */
-    int maxspins;     /*< Max no of spins to acquire lock */
+    uint64_t maxspins;     /*< Max no of spins to acquire lock */
-    int acquired;     /*< No. of times lock was acquired */
+    uint64_t acquired;     /*< No. of times lock was acquired */
-    int waiting;      /*< No. of threads acquiring this lock */
+    uint64_t waiting;      /*< No. of threads acquiring this lock */
-    int max_waiting;  /*< Max no of threads waiting for lock */
+    uint64_t max_waiting;  /*< Max no of threads waiting for lock */
-    int contended;    /*< No. of times acquire was contended */
+    uint64_t contended;    /*< No. of times acquire was contended */
-    THREAD owner;     /*< Last owner of this lock */
+    THREAD owner;          /*< Last owner of this lock */
 #endif
 } SPINLOCK;
 
-#ifndef TRUE
-#define TRUE    true
-#endif
-#ifndef FALSE
-#define FALSE   false
-#endif
-
 #if SPINLOCK_PROFILE
 #define SPINLOCK_INIT { 0, 0, 0, 0, 0, 0, 0, 0 }
 #else
 #define SPINLOCK_INIT { 0 }
 #endif
 
+/**
+ * Debugging macro for testing the state of a spinlock.
+ *
+ * @attention ONLY to be used in debugging context.
+ */
 #define SPINLOCK_IS_LOCKED(l) ((l)->lock != 0 ? true : false)
 
+/**
+ * Initialise a spinlock.
+ *
+ * @param lock The spinlock to initialise.
+ */
 extern void spinlock_init(SPINLOCK *lock);
+
+/**
+ * Acquire a spinlock.
+ *
+ * @param lock The spinlock to acquire
+ */
 extern void spinlock_acquire(const SPINLOCK *lock);
-extern int spinlock_acquire_nowait(const SPINLOCK *lock);
+
+/**
+ * Acquire a spinlock if it is not already locked.
+ *
+ * @param lock The spinlock to acquire
+ * @return True if the spinlock was acquired, otherwise false
+ */
+extern bool spinlock_acquire_nowait(const SPINLOCK *lock);
+
+/*
+ * Release a spinlock.
+ *
+ * @param lock The spinlock to release
+ */
 extern void spinlock_release(const SPINLOCK *lock);
+
+/**
+ * Report statistics on a spinlock. This only has an effect if the
+ * spinlock code has been compiled with the SPINLOCK_PROFILE option set.
+ *
+ * NB A callback function is used to return the data rather than
+ * merely printing to a DCB in order to avoid a dependency on the DCB
+ * form the spinlock code and also to facilitate other uses of the
+ * statistics reporting.
+ *
+ * @param lock          The spinlock to report on
+ * @param reporter      The callback function to pass the statistics to
+ * @param hdl           A handle that is passed to the reporter function
+ */
 extern void spinlock_stats(const SPINLOCK *lock, void (*reporter)(void *, char *, int), void *hdl);
 
 MXS_END_DECLS

server/core/spinlock.c

@@ -11,48 +11,27 @@
  * Public License.
  */
 
-/**
- * @file spinlock.c  -  Spinlock operations for the MariaDB Corporation MaxScale
- *
- * @verbatim
- * Revision History
- *
- * Date         Who             Description
- * 10/06/13     Mark Riddoch    Initial implementation
- *
- * @endverbatim
- */
-
 #include <maxscale/spinlock.h>
 #include <maxscale/atomic.h>
 #include <time.h>
 #include <maxscale/debug.h>
 
-/**
+
- * Initialise a spinlock.
+void spinlock_init(SPINLOCK *lock)
- *
- * @param lock The spinlock to initialise.
- */
-void
-spinlock_init(SPINLOCK *lock)
 {
     lock->lock = 0;
 #if SPINLOCK_PROFILE
     lock->spins = 0;
+    lock->maxspins = 0;
     lock->acquired = 0;
     lock->waiting = 0;
     lock->max_waiting = 0;
     lock->contended = 0;
+    lock->owner = 0;
 #endif
 }
 
-/**
+void spinlock_acquire(const SPINLOCK *const_lock)
- * Acquire a spinlock.
- *
- * @param lock The spinlock to acquire
- */
-void
-spinlock_acquire(const SPINLOCK *const_lock)
 {
     SPINLOCK *lock = (SPINLOCK*)const_lock;
 #if SPINLOCK_PROFILE
@@ -61,20 +40,14 @@ spinlock_acquire(const SPINLOCK *const_lock)
     atomic_add(&(lock->waiting), 1);
 #endif
 
-#ifdef __GNUC__
     while (__sync_lock_test_and_set(&(lock->lock), 1))
-        while (lock->lock)
-        {
-#else
-    while (atomic_add(&(lock->lock), 1) != 0)
     {
-        atomic_add(&(lock->lock), -1);
-#endif
 #if SPINLOCK_PROFILE
-            atomic_add(&(lock->spins), 1);
+        atomic_add(&(lock->spins), 1);
-            spins++;
+        spins++;
 #endif
-        }
+    }
+
 #if SPINLOCK_PROFILE
     if (spins)
     {
@@ -90,42 +63,24 @@ spinlock_acquire(const SPINLOCK *const_lock)
 #endif
 }
 
-/**
+bool
- * Acquire a spinlock if it is not already locked.
- *
- * @param lock The spinlock to acquire
- * @return True if the spinlock was acquired, otherwise false
- */
-int
 spinlock_acquire_nowait(const SPINLOCK *const_lock)
 {
     SPINLOCK *lock = (SPINLOCK*)const_lock;
-#ifdef __GNUC__
     if (__sync_lock_test_and_set(&(lock->lock), 1))
     {
-        return FALSE;
+        return false;
     }
-#else
+
-    if (atomic_add(&(lock->lock), 1) != 0)
-    {
-        atomic_add(&(lock->lock), -1);
-        return FALSE;
-    }
-#endif
 #if SPINLOCK_PROFILE
     lock->acquired++;
     lock->owner = thread_self();
 #endif
-    return TRUE;
+
+    return true;
 }
 
-/*
+void spinlock_release(const SPINLOCK *const_lock)
- * Release a spinlock.
- *
- * @param lock The spinlock to release
- */
-void
-spinlock_release(const SPINLOCK *const_lock)
 {
     SPINLOCK *lock = (SPINLOCK*)const_lock;
     ss_dassert(lock->lock != 0);
@@ -135,48 +90,32 @@ spinlock_release(const SPINLOCK *const_lock)
         lock->max_waiting = lock->waiting;
     }
 #endif
-#ifdef __GNUC__
+
-    __sync_synchronize(); /* Memory barrier. */
+    __sync_lock_release(&lock->lock);
-    lock->lock = 0;
-#else
-    atomic_add(&(lock->lock), -1);
-#endif
 }
 
-/**
+void spinlock_stats(const SPINLOCK *lock, void (*reporter)(void *, char *, int), void *hdl)
- * Report statistics on a spinlock. This only has an effect if the
- * spinlock code has been compiled with the SPINLOCK_PROFILE option set.
- *
- * NB A callback function is used to return the data rather than
- * merely printing to a DCB in order to avoid a dependency on the DCB
- * form the spinlock code and also to facilitate other uses of the
- * statistics reporting.
- *
- * @param lock          The spinlock to report on
- * @param reporter      The callback function to pass the statistics to
- * @param hdl           A handle that is passed to the reporter function
- */
-void
-spinlock_stats(const SPINLOCK *lock, void (*reporter)(void *, char *, int), void *hdl)
 {
 #if SPINLOCK_PROFILE
     reporter(hdl, "Spinlock acquired", lock->acquired);
     if (lock->acquired)
     {
         reporter(hdl, "Total no. of spins", lock->spins);
-        reporter(hdl, "Average no. of spins (overall)",
+        if (lock->acquired)
-                 lock->spins / lock->acquired);
+        {
+            reporter(hdl, "Average no. of spins (overall)", lock->spins / lock->acquired);
+        }
         if (lock->contended)
         {
-            reporter(hdl, "Average no. of spins (when contended)",
+            reporter(hdl, "Average no. of spins (when contended)", lock->spins / lock->contended);
-                     lock->spins / lock->contended);
         }
         reporter(hdl, "Maximum no. of spins", lock->maxspins);
-        reporter(hdl, "Maximim no. of blocked threads",
+        reporter(hdl, "Maximim no. of blocked threads", lock->max_waiting);
-                 lock->max_waiting);
         reporter(hdl, "Contended locks", lock->contended);
-        reporter(hdl, "Contention percentage",
+        if (lock->acquired)
-                 (lock->contended * 100) / lock->acquired);
+        {
+            reporter(hdl, "Contention percentage", (lock->contended * 100) / lock->acquired);
+        }
     }
 #endif
 }