Return-Path: Delivered-To: apmail-apr-dev-archive@apr.apache.org Received: (qmail 52071 invoked by uid 500); 23 Jul 2001 04:04:51 -0000 Mailing-List: contact dev-help@apr.apache.org; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: Delivered-To: mailing list dev@apr.apache.org Received: (qmail 52060 invoked from network); 23 Jul 2001 04:04:50 -0000 Date: Sun, 22 Jul 2001 21:04:31 -0700 From: Justin Erenkrantz To: dev@apr.apache.org Subject: [PATCH] Fix thread-safety in SMS... Message-ID: <20010722210431.L26972@ebuilt.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.2.5i X-AntiVirus: scanned for viruses by AMaViS 0.2.1-pre3 (http://amavis.org/) X-Spam-Rating: h31.sny.collab.net 1.6.2 0/1000/N 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)