On Thu, Jan 22, 2009 at 12:42:56AM -0500, Daniel Eischen wrote:
> On Wed, 21 Jan 2009, David Schultz wrote:
>
> >I think there *is* a real bug here, but there's two distinct ways
> >to fix it. When a threaded process forks, malloc acquires all its
> >locks so that its state is consistent after a fork. However, the
> >post-fork hook that's supposed to release these locks fails to do
> >so in the child because the child process isn't threaded, and
> >malloc_mutex_unlock() is optimized to be a no-op in
> >single-threaded processes. If the child *stays* single-threaded,
> >malloc() works by accident even with all the locks held because
> >malloc_mutex_lock() is also a no-op in single-threaded processes.
> >But if the child goes multi-threaded, then things break.
> >
> >Solution 1 is to actually unlock the locks in the child process,
> >which is what Brian is proposing.
> >
> >Solution 2 is to take the position that all of this pre- and
> >post-fork bloat in the fork() path is gratuitous and should be
> >removed. The rationale here is that if you fork with multiple
> >running threads, there's scads of ways in which the child's heap
> >could be inconsistent; fork hooks would be needed not just in
> >malloc(), but in stdio, third party libraries, etc. Why should
> >malloc() be special? It's the programmer's job to quiesce all the
> >threads before calling fork(), and if the programmer doesn't do
> >this, then POSIX only guarantees that async-signal-safe functions
> >will work.
> >
> >Note that Solution 2 also fixes Brian's problem if he quiesces all
> >of his worker threads before forking (as he should!) With the
> >pre-fork hook removed, all the locks will start out free in the
> >child. So that's what I vote for...
>
> The problem is that our own libraries (libthr included)
> need to malloc() for themselves, even after a fork() in
> the child. After a fork(), the malloc locks should be
> reinitialized in the child if it was threaded, so that
> our implementation actually works for all the async
> signal calls, fork(), exec(), etc. I forget the exact
> failure modes for very common cases, but if you remove
> the re-initialization of the malloc locks, I'm sure
> you will have problems.
>
> Perhaps much of this malloc() stuff goes away when we
> move to pthread locks that are not pointers to allocated
> objects, but instead are actual objects/structures.
> This needs to be done in order for mutexes/CVs/etc
> to be PTHREAD_PROCESS_SHARED (placed in shared memory
> and used by multiple processes). In other words,
> pthread_mutex_t goes from this:
>
> typedef struct pthread_mutex *pthread_mutex_t;
>
> to something like this:
>
> struct __pthread_mutex {
> uint32_t lock;
> ...
> }
> typedef struct __pthread_mutex pthread_mutex_t;
>
> Same thing for CVs, and we probably should convert any other
> locks used internally by libc/libpthread (spinlocks).
>
> So after a fork(), there is no need to reallocate anything,
> it can just be reinitialized if necessary.
>
I looked at the issue once more recently, and I propose the following
much less intrusive patch. It is somewhat hackish, but I think that
it would be good to have this working. Most other Unixes do have
working thread library after the fork. Any objections ?
diff --git a/lib/libthr/thread/thr_fork.c b/lib/libthr/thread/thr_fork.c
index bc410d1..ae6b9ad 100644
--- a/lib/libthr/thread/thr_fork.c
+++ b/lib/libthr/thread/thr_fork.c
@@ -173,14 +173,19 @@ _fork(void)
/* Ready to continue, unblock signals. */
_thr_signal_unblock(curthread);
- if (unlock_malloc)
+ if (unlock_malloc) {
+ __isthreaded = 1;
_malloc_postfork();
+ __isthreaded = 0;
+ }
/* Run down atfork child handlers. */
TAILQ_FOREACH(af, &_thr_atfork_list, qe) {
if (af->child != NULL)
af->child();
}
+
+ THR_UMUTEX_UNLOCK(curthread, &_thr_atfork_lock);
} else {
/* Parent process */
errsave = errno;
pgplLb1p3doD4.pgp
Description: PGP signature
