apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <...@manyfish.co.uk>
Subject Re: [PATCH] Thread Locks and SMP boxes
Date Wed, 30 Jul 2003 23:07:29 GMT
On Wed, Jul 30, 2003 at 11:21:21AM -0500, William Rowe 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.

Why do you say "nested (default)"? The pools code is the only place I
can find which actually uses nested mutexes. By default they are not
nested.
 
> 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.

This code looks like it is making lots of dubious assumptions:

- pthread_t can be a structure, that's why the memset is used I presume,
assinging 0 to a pthread_t definitely wins no prizes
- who says a memset-to-zero'ed pthread_t isn't a valid pthread_t?  No
reason why it couldn't be.  If it is *not* a valid pthread_t, then
passing it to pthread_equal gives undefined behaviour anyway!
- let's hope and pray that pthread_equal() is an atomic operation

> Many more eyes on this code would be welcomed, I expect to
> discover that we repeat this mistake elsewhere in the mutex
> code...

Like Aaron I'm not sure why the comments about reentrancy are relevant:
you can't use pthread mutexes (and hence apr mutexes :) from a signal
handler, anyone doing that is SOL.

You've made lots of changes in the patch:

1. changing some incr++ to ++incr which makes no differences
2. re-ordered the mutex_unlock code so that the ownership changes occur
inside the critical section.
3. added some "protection" against races

1 and 3 look bogus; 2 looks like the important change here.  The only
effect of your (3) changes would seem to be to hide races if there still
are any?

(2) does introduce a semantic change in that the caller cannot rely on
pthread_mutex_unlock() having defined behaviour if called from a thread
which does not own the mutex lock, but I expect that's no big deal.

It's worth noting that most modern POSIX implementations offer
"recursive" mutexes in which pthread_mutex_lock/unlock do all this work
internally.

joe

Mime
View raw message