You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@apr.apache.org by Justin Erenkrantz <je...@ebuilt.com> on 2001/07/23 06:04:31 UTC
[PATCH] Fix thread-safety in SMS...
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)