apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Justin Erenkrantz <jerenkra...@ebuilt.com>
Subject [PATCH] Fix thread-safety in SMS...
Date Mon, 23 Jul 2001 04:04:31 GMT
As Will Rowe has discussed, I think it is time to enable SMS by 
default in APR.

I just spent the past 5 hours debugging the SMS code with a threaded
MPM.  Basically, with the cross-process patch I've already committed and
this patch, a SMS-enabled threaded httpd on Linux 2.4 doesn't SEGV
or leak descriptors.

On a performance standpoint, we still need to separate the per-thread 
SMS from the per-process SMS.  Then, we can see what the performance 
gain is.  But, I can't really do that until APR's thread API is 
cleaned up (i.e. the child threads have access to the thread's pool).

This patch does the following things:
- Throws out the "private lock" structure.  I don't think we need this
  when the sms already has a lock.  This bloats the size of the patch
  a bit.
- ALWAYS make sure that the parent is lock-safe before we attempt to
  be create a lock.  This is probably the root of the problem.
  Previously, we'd walk up the tree and try to do locks, but with
  a SMS-enabled httpd, we could hit race conditions before we even
  created a lock on our parent.

So in the meantime, Dave and Sander and other interested parties can 
review this patch.  I'll plan on committing this sometime tomorrow
unless someone screams.  -- justin

Index: memory/unix/apr_sms.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_sms.c,v
retrieving revision 1.48
diff -u -r1.48 apr_sms.c
--- memory/unix/apr_sms.c	2001/07/11 14:20:02	1.48
+++ memory/unix/apr_sms.c	2001/07/23 03:42:03
@@ -319,8 +319,7 @@
     rv = APR_SUCCESS;
     
 #if APR_HAS_THREADS
-    if (sms->thread_register_fn)
-        rv = sms->thread_register_fn(sms, apr_os_thread_current());
+    apr_sms_thread_register(sms, apr_os_thread_current());
 #endif /* APR_HAS_THREADS */
     
 #if APR_DEBUG_SHOW_FUNCTIONS
@@ -612,7 +611,11 @@
         sms->pre_destroy_fn(sms);
 
     if (sms->sms_lock)
+    {
         apr_lock_destroy(sms->sms_lock);
+        if (pms->free_fn)
+            return apr_sms_free(sms->parent, sms->sms_lock);
+    }
     
 #ifndef APR_POOLS_ARE_SMS
     /* XXX - This should eventually be removed */
@@ -856,38 +859,44 @@
                                                   apr_os_thread_t thread)
 {
     apr_status_t rv;
-    
-    do {
+  
+    /* Before attempting to acquire a lock for us, we must ensure that our
+     * parent is lock-safe. 
+     */
+    if (sms->parent) 
+    {
+        apr_sms_thread_register(sms->parent, thread);
+
         if (!sms->sms_lock) {
             /* Create the sms framework lock we'll use. */
             apr_lock_create(&sms->sms_lock, APR_MUTEX, APR_LOCKALL,
-                            NULL, sms->pool);
+                            NULL, sms->parent->pool);
         }
+    }
 
+    if (sms->sms_lock)
         apr_lock_acquire(sms->sms_lock);
 
-        sms->threads++;
+    sms->threads++;
 
-        /* let the sms know about the thread if it is
-         * interested (so it can protect its private
-         * data with its own lock)
-         *
-         * if the sms is doesn't have a thread register
-         * function, or it wasn't able to register the
-         * thread, we should bomb out!
-         * XXX - not sure how to implement the bombing out
-         */
-        rv = APR_ENOTIMPL;
-        if (sms->thread_register_fn)
-            rv = sms->thread_register_fn(sms, thread);
+    /* let the sms know about the thread if it is
+     * interested (so it can protect its private
+     * data with its own lock)
+     *
+     * if the sms is doesn't have a thread register
+     * function, or it wasn't able to register the
+     * thread, we should bomb out!
+     * XXX - not sure how to implement the bombing out
+     */
+    rv = APR_ENOTIMPL;
+    if (sms->thread_register_fn)
+        rv = sms->thread_register_fn(sms, thread);
 
+    if (sms->sms_lock)
         apr_lock_release(sms->sms_lock);
 
-        if (rv != APR_SUCCESS)
-            return rv;
-
-        sms = sms->parent;
-    } while (sms);
+    if (rv != APR_SUCCESS)
+        return rv;
 
     return APR_SUCCESS;    
 }
Index: memory/unix/apr_sms_trivial.c
===================================================================
RCS file: /home/cvs/apr/memory/unix/apr_sms_trivial.c,v
retrieving revision 1.17
diff -u -r1.17 apr_sms_trivial.c
--- memory/unix/apr_sms_trivial.c	2001/07/19 17:31:05	1.17
+++ memory/unix/apr_sms_trivial.c	2001/07/23 03:42:05
@@ -96,7 +96,6 @@
     apr_size_t           min_alloc;
     apr_size_t           min_free;
     apr_size_t           max_free;
-    apr_lock_t          *lock;
 } apr_sms_trivial_t;
 
 #define SIZEOF_BLOCK_T       APR_ALIGN_DEFAULT(sizeof(block_t))
@@ -129,8 +128,8 @@
     /* Round up the size to the next 8 byte boundary */
     size = APR_ALIGN_DEFAULT(size) + SIZEOF_BLOCK_T;
 
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_acquire(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_acquire(sms->sms_lock);
     
     node = SMS_TRIVIAL_T(sms)->used_sentinel.prev;
 
@@ -140,8 +139,8 @@
         node->first_avail += size;
         node->count++;
 
-        if (SMS_TRIVIAL_T(sms)->lock)
-            apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+        if (sms->sms_lock)
+            apr_lock_release(sms->sms_lock);
     
         BLOCK_T(mem)->node = node;
         mem = (char *)mem + SIZEOF_BLOCK_T;
@@ -184,8 +183,8 @@
         node->first_avail += size;
         node->count = 1;
 
-        if (SMS_TRIVIAL_T(sms)->lock)
-            apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+        if (sms->sms_lock)
+            apr_lock_release(sms->sms_lock);
 
         BLOCK_T(mem)->node = node;
         mem = (char *)mem + SIZEOF_BLOCK_T;
@@ -207,8 +206,8 @@
         node->first_avail += node->avail_size;
         node->avail_size   = 0;
     
-        if (SMS_TRIVIAL_T(sms)->lock)
-            apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+        if (sms->sms_lock)
+            apr_lock_release(sms->sms_lock);
         
         return NULL;
     }
@@ -225,8 +224,8 @@
     node->avail_size = node_size - (node->first_avail - (char *)node);
     node->count = 1;
 
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_release(sms->sms_lock);
 
     BLOCK_T(mem)->node = node;
     mem = (char *)mem + SIZEOF_BLOCK_T;
@@ -240,14 +239,14 @@
 
     node = BLOCK_T((char *)mem - SIZEOF_BLOCK_T)->node;
 
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_acquire(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_acquire(sms->sms_lock);
 
     node->count--;
 
     if (node->count) {
-        if (SMS_TRIVIAL_T(sms)->lock)
-            apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+        if (sms->sms_lock)
+            apr_lock_release(sms->sms_lock);
 
         return APR_SUCCESS;
     }
@@ -262,8 +261,8 @@
         if (sms->parent->free_fn &&
             node->avail_size > SMS_TRIVIAL_T(sms)->max_free &&
             node != SMS_TRIVIAL_T(sms)->self) {
-            if (SMS_TRIVIAL_T(sms)->lock)
-                apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+            if (sms->sms_lock)
+                apr_lock_release(sms->sms_lock);
 
             return apr_sms_free(sms->parent, node);
         }
@@ -278,8 +277,8 @@
             SMS_TRIVIAL_T(sms)->max_free -= node->avail_size;
     }
 
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_release(sms->sms_lock);
 
     return APR_SUCCESS;
 }
@@ -324,8 +323,8 @@
     
     free_list = NULL;
     
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_acquire(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_acquire(sms->sms_lock);
 
     /* Always reset our base node as this can't be reclaimed. */
     node = SMS_TRIVIAL_T(sms)->self;
@@ -397,8 +396,8 @@
     node->next = node->prev = used_sentinel;
     used_sentinel->next = used_sentinel->prev = node;
 
-    if (SMS_TRIVIAL_T(sms)->lock)
-        apr_lock_release(SMS_TRIVIAL_T(sms)->lock);
+    if (sms->sms_lock)
+        apr_lock_release(sms->sms_lock);
 
     while ((node = free_list) != NULL) {
         free_list = node->next;
@@ -413,16 +412,9 @@
     /* This function WILL always be called.  However, be aware that the
      * main sms destroy function knows that it's not wise to try and destroy
      * the same piece of memory twice, so the destroy function in a child won't
-     * neccesarily be called.  To guarantee we destroy the lock it's therefore
-     * destroyed here.
+     * neccesarily be called.  
      */
-        
-    if (SMS_TRIVIAL_T(sms)->lock) {
-        apr_lock_acquire(SMS_TRIVIAL_T(sms)->lock);
-        apr_lock_destroy(SMS_TRIVIAL_T(sms)->lock);
-        SMS_TRIVIAL_T(sms)->lock = NULL;
-    }
-    
+
     return APR_SUCCESS;    
 }
 
@@ -456,11 +448,6 @@
 static apr_status_t apr_sms_trivial_thread_register(apr_sms_t *sms,
                                                     apr_os_thread_t thread)
 {
-    if (!SMS_TRIVIAL_T(sms)->lock && sms->threads > 1)
-        return apr_lock_create(&SMS_TRIVIAL_T(sms)->lock,
-                               APR_MUTEX, APR_LOCKALL,
-                               NULL, sms->pool);
-
     return APR_SUCCESS;
 }
 
@@ -483,7 +470,7 @@
     apr_status_t rv;
 
     *sms = NULL;
-    
+
     min_alloc = APR_ALIGN_DEFAULT(min_alloc);
     min_free  = APR_ALIGN_DEFAULT(min_free);
     if (min_free < SIZEOF_NODE_T)


Mime
View raw message