Date: Fri, 16 Jan 2009 16:19:59 -0500 From: Brian Fundakowski Feldman <green@freebsd.org> To: Julian Elischer <julian@elischer.org> Cc: hackers@freebsd.org, jasone@freebsd.org Subject: Re: threaded, forked, rethreaded processes will deadlock Message-ID: <20090116211959.GA12007@green.homeunix.org> In-Reply-To: <49678BBC.8050306@elischer.org> References: <20090109031942.GA2825@green.homeunix.org> <Pine.GSO.4.64.0901082237001.28531@sea.ntplx.net> <20090109053117.GB2825@green.homeunix.org> <4966F81C.3070406@elischer.org> <20090109163426.GC2825@green.homeunix.org> <49678BBC.8050306@elischer.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jan 09, 2009 at 09:39:08AM -0800, Julian Elischer wrote:
> Brian Fundakowski Feldman wrote:
>> On Thu, Jan 08, 2009 at 11:09:16PM -0800, Julian Elischer wrote:
>>> Brian Fundakowski Feldman wrote:
>>>> On Thu, Jan 08, 2009 at 10:44:20PM -0500, Daniel Eischen wrote:
>>>>> On Thu, 8 Jan 2009, Brian Fundakowski Feldman wrote:
>>>>>
>>>>>> It appears that the post-fork hooks for malloc(3) are somewhat broken such that
>>>>>> when a threaded program forks, and then its child attempts to go threaded, it
>>>>>> deadlocks because it already appears to have locks held. I am not familiar
>>>>>> enough with the current libthr/libc/rtld-elf interaction that I've been able
>>>>>> to fix it myself, unfortunately.
>>>>> There's really nothing to fix - according to POSIX you are only
>>>>> allowed to call async-signal-safe functions in the child forked
>>>>> from a threaded process. If you are trying to do anything other
>>>>> than that, it may or may not work on FreeBSD, but it is not
>>>>> guaranteed and is not portable.
>>>>>
>>>>> The rationale is that what is the point of forking and creating
>>>>> more threads, when you can just as easily create more threads in
>>>>> the parent without forking? The only reason to fork from a threaded
>>>>> process is to call one of the exec() functions.
>>>> Well, it worked until the last major set of changes to malloc. For me, the point
>>>> was that I was able to have transparent background worker threads in any program
>>>> regardless of its architecture, using the standard pthread fork hooks. Could you
>>>> point me to the POSIX section covering fork and threads? If it's really not
>>>> supposed to work then that's fine, but there's an awful lot of code there dedicated
>>>> to support going threaded again after a fork.
>>>>
>>> Practically, you can't go threaded again after a fork
>>> (by which I mean creating new threads or use things
>>> like mutexes etc.) in any posix system I know of.
>>>
>>> It would require that:
>>> The forking thread stop until:
>>> Every other thread has released every resource it owns
>>> and reports itself to be in a "safe quiescent state",
>>> or at least report every resource it owns, especially
>>> locks,
>>> and
>>> After the fork:
>>> The child, post fork, to take ownership of all
>>> of them, and free them.
>>>
>>> You might be able to do that in a simple
>>> threaded program, but consider then that the libraries may have
>>> threads running in them of which you are unaware, and that
>>> some of the resources may interract with resources owned by the
>>> forking thread.
>>>
>>> Add to this that there may be a signal thrown into this mix as well
>>>
>>> (signals are the bane of thread developement)
>>
>> Well, I wouldn't mind showing all of you what I can of what I had been doing
>> with the background threads -- it works pretty well modulo this particular
>> malloc lock bug. Due to it being inappropriate to share library resources
>> between a child and parent for an open socket connection, I always considered
>> the only "safe" behavior to be going single-threaded for the duration of the fork
>> processes in both the parent and child, and the pthread_atfork(3) hooks have been
>> sufficient to do so.
>
>
> ahhhh
> well going single threaded for the duration of the fork, changes
> everything!
Could you, and anyone else who would care to, check this out? It's a regression
fix but it also makes the code a little bit clearer. Thanks!
Index: lib/libc/stdlib/malloc.c
===================================================================
--- lib/libc/stdlib/malloc.c (revision 187160)
+++ lib/libc/stdlib/malloc.c (working copy)
@@ -415,6 +415,7 @@
/* Set to true once the allocator has been initialized. */
static bool malloc_initialized = false;
+static bool malloc_during_fork = false;
/* Used to avoid initialization races. */
static malloc_mutex_t init_lock = {_SPINLOCK_INITIALIZER};
@@ -1205,7 +1206,7 @@
malloc_mutex_lock(malloc_mutex_t *mutex)
{
- if (__isthreaded)
+ if (__isthreaded || malloc_during_fork)
_SPINLOCK(&mutex->lock);
}
@@ -1213,7 +1214,7 @@
malloc_mutex_unlock(malloc_mutex_t *mutex)
{
- if (__isthreaded)
+ if (__isthreaded || malloc_during_fork)
_SPINUNLOCK(&mutex->lock);
}
@@ -1260,7 +1261,7 @@
{
unsigned ret = 0;
- if (__isthreaded) {
+ if (__isthreaded || malloc_during_fork) {
if (_pthread_mutex_trylock(lock) != 0) {
/* Exponentially back off if there are multiple CPUs. */
if (ncpus > 1) {
@@ -1296,7 +1297,7 @@
malloc_spin_unlock(pthread_mutex_t *lock)
{
- if (__isthreaded)
+ if (__isthreaded || malloc_during_fork)
_pthread_mutex_unlock(lock);
}
@@ -5515,9 +5516,8 @@
void
_malloc_prefork(void)
{
+ arena_t *larenas[narenas];
bool again;
- unsigned i, j;
- arena_t *larenas[narenas], *tarenas[narenas];
/* Acquire all mutexes in a safe order. */
@@ -5530,19 +5530,23 @@
*/
memset(larenas, 0, sizeof(arena_t *) * narenas);
do {
+ unsigned int i;
+
again = false;
malloc_spin_lock(&arenas_lock);
for (i = 0; i < narenas; i++) {
if (arenas[i] != larenas[i]) {
+ arena_t *tarenas[narenas];
+ unsigned int j;
+
memcpy(tarenas, arenas, sizeof(arena_t *) *
narenas);
malloc_spin_unlock(&arenas_lock);
for (j = 0; j < narenas; j++) {
if (larenas[j] != tarenas[j]) {
larenas[j] = tarenas[j];
- malloc_spin_lock(
- &larenas[j]->lock);
+ malloc_spin_lock(&larenas[j]->lock);
}
}
again = true;
@@ -5558,6 +5562,7 @@
#ifdef MALLOC_DSS
malloc_mutex_lock(&dss_mtx);
#endif
+ malloc_during_fork = true;
}
void
@@ -5582,6 +5587,12 @@
if (larenas[i] != NULL)
malloc_spin_unlock(&larenas[i]->lock);
}
+ /*
+ * This ends the special post-__isthreaded exemption behavior for
+ * malloc stuff. We should really be single threaded right now
+ * in effect regardless of __isthreaded status.
+ */
+ malloc_during_fork = false;
}
/*
Index: lib/libthr/thread/thr_fork.c
===================================================================
--- lib/libthr/thread/thr_fork.c (revision 187160)
+++ lib/libthr/thread/thr_fork.c (working copy)
@@ -105,7 +105,7 @@
struct pthread_atfork *af;
pid_t ret;
int errsave;
- int unlock_malloc;
+ int was_threaded;
int rtld_locks[MAX_RTLD_LOCKS];
if (!_thr_is_inited())
@@ -122,16 +122,16 @@
}
/*
- * Try our best to protect memory from being corrupted in
- * child process because another thread in malloc code will
- * simply be kill by fork().
+ * All bets are off as to what should happen soon if the parent
+ * process was not so kindly as to set up pthread fork hooks to
+ * relinquish all running threads.
*/
if (_thr_isthreaded() != 0) {
- unlock_malloc = 1;
+ was_threaded = 1;
_malloc_prefork();
_rtld_atfork_pre(rtld_locks);
} else {
- unlock_malloc = 0;
+ was_threaded = 0;
}
/*
@@ -159,7 +159,7 @@
_thr_umutex_init(&curthread->lock);
_thr_umutex_init(&_thr_atfork_lock);
- if (unlock_malloc)
+ if (was_threaded)
_rtld_atfork_post(rtld_locks);
_thr_setthreaded(0);
@@ -173,7 +173,7 @@
/* Ready to continue, unblock signals. */
_thr_signal_unblock(curthread);
- if (unlock_malloc)
+ if (was_threaded)
_malloc_postfork();
/* Run down atfork child handlers. */
@@ -188,7 +188,7 @@
/* Ready to continue, unblock signals. */
_thr_signal_unblock(curthread);
- if (unlock_malloc) {
+ if (was_threaded) {
_rtld_atfork_post(rtld_locks);
_malloc_postfork();
}
Index: tools/regression/pthread/malloc_thread_fork_survival/no_deadlock.c
===================================================================
--- tools/regression/pthread/malloc_thread_fork_survival/no_deadlock.c (revision 0)
+++ tools/regression/pthread/malloc_thread_fork_survival/no_deadlock.c (revision 0)
@@ -0,0 +1,55 @@
+/*
+ * Public domain, originally by Brian Fundakowski Feldman <green@FreeBSD.org>.
+ * $FreeBSD$
+ */
+#include <sys/types.h>
+#include <sys/wait.h>
+
+#include <pthread.h>
+#include <signal.h>
+#include <stdlib.h>
+#include <unistd.h>
+
+void *
+noop(void *unused)
+{
+ return (NULL);
+}
+
+void *
+exit0(void *unused)
+{
+ const char ok[] = "ok\n";
+ write(1, ok, sizeof(ok));
+ exit(0);
+}
+
+void *
+exitN(int code)
+{
+ if (code == 0) {
+ exit0(NULL);
+ } else {
+ const char not_ok[] = "not ok\n";
+ write(1, not_ok, sizeof(not_ok));
+ exit(code);
+ }
+}
+
+int
+main()
+{
+ pthread_t thread;
+ int exited;
+
+ pthread_create(&thread, NULL, noop, NULL);
+ pthread_join(thread, NULL);
+ if (fork() == 0) {
+ alarm(1);
+ pthread_create(&thread, NULL, exit0, NULL);
+ pthread_join(thread, NULL);
+ /* UNREACHED */
+ }
+ wait(&exited);
+ exitN(WIFSIGNALED(exited) ? WTERMSIG(exited) : WEXITSTATUS(exited));
+}
Index: tools/regression/pthread/malloc_thread_fork_survival/Makefile
===================================================================
--- tools/regression/pthread/malloc_thread_fork_survival/Makefile (revision 0)
+++ tools/regression/pthread/malloc_thread_fork_survival/Makefile (working copy)
@@ -1,8 +1,8 @@
# $FreeBSD$
-PROG= cv_cancel1
+PROG= no_deadlock
NO_MAN=
-LDADD= -lpthread
+LDADD= -lthr
.include <bsd.prog.mk>
--
Brian Fundakowski Feldman \'[ FreeBSD ]''''''''''\
<> green@FreeBSD.org \ The Power to Serve! \
Opinions expressed are my own. \,,,,,,,,,,,,,,,,,,,,,,\
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090116211959.GA12007>