Date: Wed, 18 Mar 2009 18:32:22 +0200 From: Kostik Belousov <kostikbel@gmail.com> To: Daniel Eischen <deischen@freebsd.org> Cc: David Schultz <das@freebsd.org>, hackers@freebsd.org, davidxu@freebsd.org, Jason Evans <jasone@freebsd.org>, Julian Elischer <julian@elischer.org> Subject: Re: threaded, forked, rethreaded processes will deadlock Message-ID: <20090318163222.GE7716@deviant.kiev.zoral.com.ua> In-Reply-To: <Pine.GSO.4.64.0901220021320.4150@sea.ntplx.net> References: <4966F81C.3070406@elischer.org> <20090109163426.GC2825@green.homeunix.org> <49678BBC.8050306@elischer.org> <20090116211959.GA12007@green.homeunix.org> <49710BD6.7040705@FreeBSD.org> <20090120004135.GB12007@green.homeunix.org> <20090121230033.GC12007@green.homeunix.org> <Pine.GSO.4.64.0901211831210.4150@sea.ntplx.net> <20090122045637.GA61058@zim.MIT.EDU> <Pine.GSO.4.64.0901220021320.4150@sea.ntplx.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--N1GIdlSm9i+YlY4t Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Thu, Jan 22, 2009 at 12:42:56AM -0500, Daniel Eischen wrote: > On Wed, 21 Jan 2009, David Schultz wrote: >=20 > >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... >=20 > 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. >=20 > 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: >=20 > typedef struct pthread_mutex *pthread_mutex_t; >=20 > to something like this: >=20 > struct __pthread_mutex { > uint32_t lock; > ... > } > typedef struct __pthread_mutex pthread_mutex_t; >=20 > Same thing for CVs, and we probably should convert any other > locks used internally by libc/libpthread (spinlocks). >=20 > So after a fork(), there is no need to reallocate anything, > it can just be reinitialized if necessary. >=20 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. */=20 _thr_signal_unblock(curthread); =20 - if (unlock_malloc) + if (unlock_malloc) { + __isthreaded =3D 1; _malloc_postfork(); + __isthreaded =3D 0; + } =20 /* Run down atfork child handlers. */ TAILQ_FOREACH(af, &_thr_atfork_list, qe) { if (af->child !=3D NULL) af->child(); } + + THR_UMUTEX_UNLOCK(curthread, &_thr_atfork_lock); } else { /* Parent process */ errsave =3D errno; --N1GIdlSm9i+YlY4t Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.9 (FreeBSD) iEYEARECAAYFAknBIhUACgkQC3+MBN1Mb4gSTwCeIIAdoAw9tSKhJ1ttiGe8LNwo 5zoAoOx5my0Upyo9shFZ1P/irQ60mREW =y+gs -----END PGP SIGNATURE----- --N1GIdlSm9i+YlY4t--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090318163222.GE7716>