apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Philip Martin <phi...@codematters.co.uk>
Subject [PATCH] apr_atomic_cas broken in 0.9.x
Date Sat, 23 Sep 2006 02:17:08 GMT
"William A. Rowe, Jr." <wrowe@rowe-clan.net> writes:

> Philip or Troy - would you care to prepare and test the backport to ensure
> we can commit this for the next 0.9 release, coming within days?

The 1.2.x branch has changed the name/signature to apr_atomic_cas32
and no longer passes longs; it's not really clear what the 0.9.x
branch should do with the high bits of the longs.

There's another problem with the 0.9.x code, the threaded
implementation provides no indication to the caller when mutex
operations fail, although most mutex implementations are unlikely to
fail.  The 1.2.x code aborts when a mutex fails, so assuming the same
should occur when high bits are set here's a patch.  I don't have an
IA64 but I believe the code gets used on x86:

APR Atomic Test
===============

Initializing the context                                    OK
Initializing the atomics                                    OK
testing apr_atomic_dec                                      OK
testing CAS                                                 OK
testing CAS - match non-null                                OK
testing CAS - no match                                      OK
testing CAS for pointers                                    OK
testing CAS for pointers - match non-null                   OK
testing CAS for pointers - no match                         OK
testing add                                                 OK
testing add/inc                                             OK
Starting all the threads                                    OK
Waiting for threads to exit                                 
(Note that this may take a while to complete.)              OK
Checking if atomic worked                                   OK
Checking if nolock worked                                   no surprise
The no-locks didn't work. z = 864045 instead of 1000000

Port some of the atomic code from 1.2.x to 0.9.x, in particular make
mutex operations that fail cause an abort and make the generic C
implementation of apr_atomic_cas work on 64 bit platforms.

Index: atomic/unix/apr_atomic.c
===================================================================
--- atomic/unix/apr_atomic.c	(revision 449122)
+++ atomic/unix/apr_atomic.c	(working copy)
@@ -18,6 +18,8 @@
 #include "apr_atomic.h"
 #include "apr_thread_mutex.h"
 
+#include <stdlib.h>
+
 #if !defined(apr_atomic_init) && !defined(APR_OVERRIDE_ATOMIC_INIT)
 
 #if APR_HAS_THREADS
@@ -46,18 +48,18 @@
 }
 #endif /*!defined(apr_atomic_init) && !defined(APR_OVERRIDE_ATOMIC_INIT) */
 
+/* abort() if 'x' does not evaluate to APR_SUCCESS. */
+#define CHECK(x) do { if ((x) != APR_SUCCESS) abort(); } while (0)
+
 #if !defined(apr_atomic_add) && !defined(APR_OVERRIDE_ATOMIC_ADD)
 void apr_atomic_add(volatile apr_atomic_t *mem, apr_uint32_t val) 
 {
 #if APR_HAS_THREADS
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
-    apr_uint32_t prev;
        
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        prev = *mem;
-        *mem += val;
-        apr_thread_mutex_unlock(lock);
-    }
+    CHECK(apr_thread_mutex_lock(lock));
+    *mem += val;
+    CHECK(apr_thread_mutex_unlock(lock));
 #else
     *mem += val;
 #endif /* APR_HAS_THREADS */
@@ -69,13 +71,10 @@
 {
 #if APR_HAS_THREADS
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
-    apr_uint32_t prev;
 
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        prev = *mem;
-        *mem = val;
-        apr_thread_mutex_unlock(lock);
-    }
+    CHECK(apr_thread_mutex_lock(lock));
+    *mem = val;
+    CHECK(apr_thread_mutex_unlock(lock));
 #else
     *mem = val;
 #endif /* APR_HAS_THREADS */
@@ -87,13 +86,10 @@
 {
 #if APR_HAS_THREADS
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
-    apr_uint32_t prev;
 
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        prev = *mem;
-        (*mem)++;
-        apr_thread_mutex_unlock(lock);
-    }
+    CHECK(apr_thread_mutex_lock(lock));
+    (*mem)++;
+    CHECK(apr_thread_mutex_unlock(lock));
 #else
     (*mem)++;
 #endif /* APR_HAS_THREADS */
@@ -107,12 +103,11 @@
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
     apr_uint32_t new;
 
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        (*mem)--;
-        new = *mem;
-        apr_thread_mutex_unlock(lock);
-        return new; 
-    }
+    CHECK(apr_thread_mutex_lock(lock));
+    (*mem)--;
+    new = *mem;
+    CHECK(apr_thread_mutex_unlock(lock));
+    return new; 
 #else
     (*mem)--;
 #endif /* APR_HAS_THREADS */
@@ -123,26 +118,28 @@
 #if !defined(apr_atomic_cas) && !defined(APR_OVERRIDE_ATOMIC_CAS)
 apr_uint32_t apr_atomic_cas(volatile apr_uint32_t *mem, long with, long cmp)
 {
-    long prev;
+    apr_uint32_t prev;
 #if APR_HAS_THREADS
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
 
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        prev = *(long*)mem;
-        if (prev == cmp) {
-            *(long*)mem = with;
-        }
-        apr_thread_mutex_unlock(lock);
-        return prev;
+    if ((long)(apr_uint32_t)with != with || (long)(apr_uint32_t)cmp != cmp)
+        abort();
+    CHECK(apr_thread_mutex_lock(lock));
+    prev = *mem;
+    if (prev == (apr_uint32_t)cmp) {
+      *mem = (apr_uint32_t)with;
     }
-    return *(long*)mem;
+    CHECK(apr_thread_mutex_unlock(lock));
 #else
-    prev = *(long*)mem;
-    if (prev == cmp) {
-        *(long*)mem = with;
+    if ((long)(apr_uint32_t)with != with || (long)(apr_uint32_t)cmp != cmp) {
+        abort();
     }
-    return prev;
+    prev = *mem;
+    if (prev == (apr_uint32_t)cmp) {
+        *mem = (apr_uint32_t)with;
+    }
 #endif /* APR_HAS_THREADS */
+    return prev;
 }
 #endif /*!defined(apr_atomic_cas) && !defined(APR_OVERRIDE_ATOMIC_CAS) */
 
@@ -153,21 +150,18 @@
 #if APR_HAS_THREADS
     apr_thread_mutex_t *lock = hash_mutex[ATOMIC_HASH(mem)];
 
-    if (apr_thread_mutex_lock(lock) == APR_SUCCESS) {
-        prev = *(void **)mem;
-        if (prev == cmp) {
-            *mem = with;
-        }
-        apr_thread_mutex_unlock(lock);
-        return prev;
+    CHECK(apr_thread_mutex_lock(lock));
+    prev = *(void **)mem;
+    if (prev == cmp) {
+      *mem = with;
     }
-    return *(void **)mem;
+    CHECK(apr_thread_mutex_unlock(lock));
 #else
     prev = *(void **)mem;
     if (prev == cmp) {
         *mem = with;
     }
-    return prev;
 #endif /* APR_HAS_THREADS */
+    return prev;
 }
 #endif /*!defined(apr_atomic_cas) && !defined(APR_OVERRIDE_ATOMIC_CAS) */


-- 
Philip Martin

Mime
View raw message