apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Aaron Bannert <aa...@clove.org>
Subject Re: [PATCH] Thread Locks and SMP boxes
Date Wed, 30 Jul 2003 18:15:04 GMT
Comments below...


On Wednesday, July 30, 2003, at 09:21  AM, William A. Rowe, Jr. wrote:

> Attached is a patch that appears to be as close as thread-safe
> (but altogether not reentrant) as I can accomplish.
>
> The existing unix/thread_mutex.c code is neither thread-safe
> nor reentrant when using the 'nested' (default) thread mutexes.
> The bug is most often expressed on SMP boxes, where they
> are so massively parallel that the subtle flaws of modifying the
> mutex object AFTER we have released the lock become very
> apparent.
>
> Examples include a number of error (45) Deadlock Avoided
> alerts using Apache2/mod_ssl on Solaris, where there are both
> internal thread locks and global locks which contain a nested
> thread lock.
>
> Many more eyes on this code would be welcomed, I expect to
> discover that we repeat this mistake elsewhere in the mutex
> code...

> Index: srclib/apr/thread_mutex.c
> ===================================================================
> RCS file: /home/cvs/apr/locks/unix/thread_mutex.c,v
> retrieving revision 1.18
> diff -u -r1.18 thread_mutex.c
> --- srclib/apr/thread_mutex.c	8 Jun 2003 13:42:20 -0000	1.18
> +++ srclib/apr/thread_mutex.c	30 Jul 2003 16:09:41 -0000
> @@ -108,8 +108,11 @@
>      apr_status_t rv;
>
>      if (mutex->nested) {
> +        /*
> +         * WARNING: although threadsafe, this test is NOT reentrant
> +         */

I don't know what you mean by reentrant here, could you explain?

>          if (apr_os_thread_equal(mutex->owner, 
> apr_os_thread_current())) {
> -            mutex->owner_ref++;
> +            ++mutex->owner_ref;

Does this make a difference? (I think an incr is not threadsafe any way 
we
look at it, but I could be wrong (It's really a load/incr/store, 
right?).)

>              return APR_SUCCESS;
>          }
>
> @@ -121,8 +124,13 @@
>              return rv;
>          }
>
> +        if (++mutex->owner_ref != 1) {
> +            /* The owner_ref should be zero when the lock is not held,
> +             * if owner_ref was non-zero we have a mutex reference 
> bug.
> +             */
> +            mutex->owner_ref = 1;
> +        }
>          mutex->owner = apr_os_thread_current();
> -        mutex->owner_ref = 1;
>          return rv;
>      }
>      else {
> @@ -141,8 +149,11 @@
>      apr_status_t rv;
>
>      if (mutex->nested) {
> +        /*
> +         * WARNING: although threadsafe, this test is NOT reentrant
> +         */
>          if (apr_os_thread_equal(mutex->owner, 
> apr_os_thread_current())) {
> -            mutex->owner_ref++;
> +            ++mutex->owner_ref;
>              return APR_SUCCESS;
>          }
>
> @@ -154,8 +165,13 @@
>              return (rv == EBUSY) ? APR_EBUSY : rv;
>          }
>
> +        if (++mutex->owner_ref != 1) {
> +            /* The owner_ref should be zero when the lock is not held,
> +             * if owner_ref was non-zero we have a mutex reference 
> bug.
> +             */
> +            mutex->owner_ref = 1;
> +        }
>          mutex->owner = apr_os_thread_current();
> -        mutex->owner_ref = 1;
>      }
>      else {
>          rv = pthread_mutex_trylock(&mutex->mutex);
> @@ -176,31 +192,30 @@
>
>      if (mutex->nested) {
>          if (apr_os_thread_equal(mutex->owner, 
> apr_os_thread_current())) {
> -            mutex->owner_ref--;
> -            if (mutex->owner_ref > 0)
> +            /*
> +             * WARNING: although threadsafe, this test is NOT 
> reentrant
> +             */
> +            if (--mutex->owner_ref > 0)
>                  return APR_SUCCESS;
>          }
> -        status = pthread_mutex_unlock(&mutex->mutex);
> -        if (status) {
> -#ifdef PTHREAD_SETS_ERRNO
> -            status = errno;
> -#endif
> -            return status;
> -        }
> +        mutex->owner = 0;
>
> -        memset(&mutex->owner, 0, sizeof mutex->owner);
> -        mutex->owner_ref = 0;
> -        return status;
> +        if (mutex->owner_ref < 0) {
> +            /* The owner_ref should be zero when the lock is finally 
> released,
> +             * if owner_ref is non-zero we have a mutex reference bug.
> +             */
> +            mutex->owner_ref = 0;
> +        }
>      }
> -    else {
> -        status = pthread_mutex_unlock(&mutex->mutex);
> +
> +    status = pthread_mutex_unlock(&mutex->mutex);
>  #ifdef PTHREAD_SETS_ERRNO
> -        if (status) {
> -            status = errno;
> -        }
> -#endif
> -        return status;
> +    if (status) {
> +        status = errno;
>      }
> +#endif
> +
> +    return status;
>  }
>
>  APR_DECLARE(apr_status_t) apr_thread_mutex_destroy(apr_thread_mutex_t 
> *mutex)


This last chunk seems to be the meat of at least one of the problems.
Still, even though this change is good in that we only mess with
the mutex->owner stuff when the lock is held, we aren't protecting
mutex->owner in other places (eg. in the places above that you pointed 
out)

Do we really need nested mutexes? The correct solution might be to add 
another
internal pthread_mutex that only gets used when we want nested mutexes,
and only gets locked when we want to read or modify those mutex->owner
variables. This might make things a little slower than they would be
now (only slightly), but it would be correct if nested mutexes are
really a necessary feature.

-aaron


Mime
View raw message