[prev in list] [next in list] [prev in thread] [next in thread]
List: apr-dev
Subject: [PATCH] Bug in apr locking assumptions and optimization
From: "William A. Rowe, Jr." <wrowe () rowe-clan ! net>
Date: 2002-05-29 15:19:52
[Download RAW message or body]
Attached is a patch to introduce a new locking semantic to our thread_mutex.
Right now we presume the 'default' is unnested. Well, that saves 30% of the
processing time on Unix, but it would cripple Win32 (which gets a critical
section
in 10 instructions or so IF there is no contention... and it's always
nested...)
Sebastian tossed me a really interesting and invalid assumption I made
when apr'izing mod_isapi...
At 09:38 AM 5/25/2002, Sebastian Hantsch wrote to me:
>[...] the current Async emulation won't work as designed:
>
>This is a snippet from isapi_handler:
>comp = cid->completed;
>if (cid->completed && (rv == APR_SUCCESS)) {
> rv = apr_thread_mutex_lock(comp);
>}
>/* The completion port is now locked. When we regain the
>* lock, we may destroy the request.
>*/
>if (cid->completed && (rv == APR_SUCCESS)) {
> rv = apr_thread_mutex_lock(comp);
>}
>break;
>
>apr_thread_mutex functions internally use Critical Sections on win32 platform.
>In the Win32 SDK at
>http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dllproc/synchro_3xym.asp
>is the following statement:
> > After a thread has ownership of a critical section, it can make
> additional calls to
> > EnterCriticalSection or TryEnterCriticalSection without blocking its
> execution. This
> > prevents a thread from deadlocking itself while waiting for a critical
> section that it already owns.
>
>So, the method currently used in mod_isapi.c does not block as supposed
>until a HSE_REQ_DONE_WITH_SESSION message arrives.
>I would suggest to use SetEvent/CreateEvent/WaitForSingleObject as in
>2.0.36, that synchronization methods work for me.
Today we have no unnested locks on Win32. That's problem one to address.
The attached patch should make this really trivial and unambiguous, while
leaving
all platforms to choose the obvious semantic for _DEFAULT locks for
performance.
Comments?
Index: include/apr_thread_mutex.h
===================================================================
RCS file: /home/cvs/apr/include/apr_thread_mutex.h,v
retrieving revision 1.10
diff -u -r1.10 apr_thread_mutex.h
--- include/apr_thread_mutex.h 9 Apr 2002 06:56:55 -0000 1.10
+++ include/apr_thread_mutex.h 29 May 2002 15:09:57 -0000
@@ -79,6 +79,7 @@
#define APR_THREAD_MUTEX_DEFAULT 0x0
#define APR_THREAD_MUTEX_NESTED 0x1
+#define APR_THREAD_MUTEX_UNNESTED 0x2
/* Delayed the include to avoid a circular reference */
#include "apr_pools.h"
@@ -89,10 +90,14 @@
* stored.
* @param flags Or'ed value of:
* <PRE>
- * APR_THREAD_MUTEX_DEFAULT normal lock behavior (non-recursive).
+ * APR_THREAD_MUTEX_DEFAULT platform-optimal lock behavior.
* APR_THREAD_MUTEX_NESTED enable nested (recursive) locks.
+ * APR_THREAD_MUTEX_UNNESTED disable nested locks (non-recursive).
* </PRE>
* @param pool the pool from which to allocate the mutex.
+ * @tip Be cautious in using APR_THREAD_MUTEX_DEFAULT. While this is the
+ * most optimial mutex based on a given platform's performance charateristics,
+ * it will behave as either a nested or an unnested lock.
*/
APR_DECLARE(apr_status_t) apr_thread_mutex_create(apr_thread_mutex_t **mutex,
unsigned int flags,
Index: locks/netware/thread_mutex.c
===================================================================
RCS file: /home/cvs/apr/locks/netware/thread_mutex.c,v
retrieving revision 1.7
diff -u -r1.7 thread_mutex.c
--- locks/netware/thread_mutex.c 13 Mar 2002 20:39:20 -0000 1.7
+++ locks/netware/thread_mutex.c 29 May 2002 15:09:57 -0000
@@ -73,6 +73,11 @@
{
apr_thread_mutex_t *new_mutex = NULL;
+ /* XXX: Implement _UNNESTED flavor and favor _DEFAULT for performance
+ */
+ if (flags & APR_THREAD_MUTEX_UNNESTED) {
+ return APR_ENOTIMPL;
+ }
new_mutex = (apr_thread_mutex_t *)apr_pcalloc(pool, sizeof(apr_thread_mutex_t));
if(new_mutex ==NULL) {
@@ -80,7 +85,6 @@
}
new_mutex->pool = pool;
- /* FIXME: only use recursive locks if (flags & APR_THREAD_MUTEX_NESTED) */
new_mutex->mutex = NXMutexAlloc(NX_MUTEX_RECURSIVE, NULL, NULL);
if(new_mutex->mutex == NULL)
Index: locks/os2/thread_mutex.c
===================================================================
RCS file: /home/cvs/apr/locks/os2/thread_mutex.c,v
retrieving revision 1.6
diff -u -r1.6 thread_mutex.c
--- locks/os2/thread_mutex.c 13 Mar 2002 20:39:21 -0000 1.6
+++ locks/os2/thread_mutex.c 29 May 2002 15:09:57 -0000
@@ -69,6 +69,9 @@
+/* XXX: Need to respect APR_THREAD_MUTEX_[UN]NESTED flags argument
+ * or return APR_ENOTIMPL!!!
+ */
APR_DECLARE(apr_status_t) apr_thread_mutex_create(apr_thread_mutex_t **mutex,
unsigned int flags,
apr_pool_t *pool)
Index: locks/unix/thread_mutex.c
===================================================================
RCS file: /home/cvs/apr/locks/unix/thread_mutex.c,v
retrieving revision 1.10
diff -u -r1.10 thread_mutex.c
--- locks/unix/thread_mutex.c 28 Apr 2002 03:21:24 -0000 1.10
+++ locks/unix/thread_mutex.c 29 May 2002 15:09:57 -0000
@@ -89,6 +89,10 @@
}
new_mutex->pool = pool;
+
+ /* Optimal default is APR_THREAD_MUTEX_UNNESTED,
+ * no additional checks required for either flag.
+ */
new_mutex->nested = flags & APR_THREAD_MUTEX_NESTED;
if ((rv = pthread_mutexattr_init(&mattr))) {
Index: locks/win32/thread_mutex.c
===================================================================
RCS file: /home/cvs/apr/locks/win32/thread_mutex.c,v
retrieving revision 1.9
diff -u -r1.9 thread_mutex.c
--- locks/win32/thread_mutex.c 13 Mar 2002 20:39:22 -0000 1.9
+++ locks/win32/thread_mutex.c 29 May 2002 15:09:57 -0000
@@ -69,15 +69,24 @@
return APR_SUCCESS;
}
+/* XXX: Need to implement explicit APR_THREAD_MUTEX_UNNESTED
+ *
+ * XXX: Need to reimplement with Mutexes or Semaphores on Win9x for
+ * apr_thread_mutex_trylock using WaitForSingleObject(timeout 0)
+ * Would be nice if that implementation overlaps _UNNESTED impl.
+ */
APR_DECLARE(apr_status_t) apr_thread_mutex_create(apr_thread_mutex_t **mutex,
unsigned int flags,
apr_pool_t *pool)
{
+ if (flags & APR_THREAD_MUTEX_UNNESTED) {
+ return APR_ENOTIMPL;
+ }
+
(*mutex) = (apr_thread_mutex_t *)apr_palloc(pool, sizeof(**mutex));
(*mutex)->pool = pool;
- /* FIXME: Implement nested (aka recursive) locks or use a native
- * win32 implementation if available. */
+
InitializeCriticalSection(&(*mutex)->section);
apr_pool_cleanup_register((*mutex)->pool, (*mutex), thread_mutex_cleanup,
apr_pool_cleanup_null);
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic