From owner-freebsd-hackers@FreeBSD.ORG Tue Aug 14 04:42:14 2012 Return-Path: Delivered-To: freebsd-hackers@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id E36781065677 for ; Tue, 14 Aug 2012 04:42:13 +0000 (UTC) (envelope-from davidxu@freebsd.org) Received: from freefall.freebsd.org (freefall.freebsd.org [IPv6:2001:4f8:fff6::28]) by mx1.freebsd.org (Postfix) with ESMTP id CB3CE8FC1B; Tue, 14 Aug 2012 04:42:13 +0000 (UTC) Received: from xyf.my.dom (localhost [127.0.0.1]) by freefall.freebsd.org (8.14.5/8.14.5) with ESMTP id q7E4gBov057251; Tue, 14 Aug 2012 04:42:12 GMT (envelope-from davidxu@freebsd.org) Message-ID: <5029D727.2090105@freebsd.org> Date: Tue, 14 Aug 2012 12:42:15 +0800 From: David Xu User-Agent: Mozilla/5.0 (X11; FreeBSD i386; rv:9.0) Gecko/20111229 Thunderbird/9.0 MIME-Version: 1.0 To: Jilles Tjoelker References: <20120730102408.GA19983@stack.nl> <20120730105303.GU2676@deviant.kiev.zoral.com.ua> <20120805215432.GA28704@stack.nl> <20120806082535.GI2676@deviant.kiev.zoral.com.ua> <20120809105648.GA79814@stack.nl> In-Reply-To: <20120809105648.GA79814@stack.nl> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit Cc: Konstantin Belousov , freebsd-hackers@freebsd.org Subject: Re: system() using vfork() or posix_spawn() and libthr X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 14 Aug 2012 04:42:14 -0000 On 2012/08/09 18:56, Jilles Tjoelker wrote: > On Mon, Aug 06, 2012 at 11:25:35AM +0300, Konstantin Belousov wrote: >> On Sun, Aug 05, 2012 at 11:54:32PM +0200, Jilles Tjoelker wrote: >>> On Mon, Jul 30, 2012 at 01:53:03PM +0300, Konstantin Belousov wrote: >>>> On Mon, Jul 30, 2012 at 12:24:08PM +0200, Jilles Tjoelker wrote: >>>>> People sometimes use system() from large address spaces where it would >>>>> improve performance greatly to use vfork() instead of fork(). > >>>>> A simple approach is to change fork() to vfork(), although I have not >>>>> tried this. It seems safe enough to use sigaction and sigprocmask system >>>>> calls in the vforked process. > >>>>> Alternatively, we can have posix_spawn() do the vfork() with signal >>>>> changes. This avoids possible whining from compilers and static >>>>> analyzers about using vfork() in system.c. However, I do not like the >>>>> tricky code for signals and that it adds lines of code. > >>>>> This is lightly tested. > >>>> It is interesting to note that for some time our vfork(2) no longer >>>> stops the whole forked process (parent), only the forking thread is >>>> waiting for the child exit or exec. I am not sure is this point >>>> important for system(3), but determined code can notice the difference >>>> from the fork->vfork switch. > >>> Neither fork nor vfork call thread_single(SINGLE_BOUNDARY), so this is >>> not a difference. >> It is the difference, because vforked child shares parent address space. > >>> Thread singling may be noticeable from a failing execve() (but only in >>> the process doing execve()) and in the rare case of rfork() without >>> RFPROC. > >> No, other running threads in parent affect vforked child till exec or exit. >> In fact, I would classify this as bug, but not a serious one. > > There are some ugly ways this parallel execution is depended on. If the > vforked child calls sigaction() while another thread is also in > sigaction() for that signal, the vforked child needs to wait for the > other thread to release the lock. > > This uses a per-process lock to synchronize threads in different > processes, which may not work properly. > > If the vforked child is killed (such as by SIGKILL) while holding the > lock, the parent is not killed but its _thr_sigact is damaged. > > These problems could be avoided in libthr by skipping the lock in > _sigaction() if a signal action is being set to SIG_DFL or SIG_IGN and > the old action is not queried. In those cases, _thr_sigact is not > touched so no lock is required. This change also helps applications, > provided they call sigaction() and not signal(). > > Alternatively, posix_spawn() and system() could use the sigaction system > call directly, bypassing libthr (if present). However, this will not > help applications that call vfork() and sigaction() themselves (such as > a shell that wants to implement ...& using vfork()). > > posix_spawn() likely still needs some adjustment so that having it reset > all signals (sigfillset()) to the default action will not cause it to > [EINVAL] because libthr does not allow changing SIGTHR's disposition. > > Index: lib/libthr/thread/thr_sig.c > =================================================================== > --- lib/libthr/thread/thr_sig.c (revision 238970) > +++ lib/libthr/thread/thr_sig.c (working copy) > @@ -519,8 +519,16 @@ > return (-1); > } > > - if (act) > + if (act) { > newact = *act; > + /* > + * Short-circuit cases where we do not touch _thr_sigact. > + * This allows performing these safely in a vforked child. > + */ > + if ((newact.sa_handler == SIG_DFL || > + newact.sa_handler == SIG_IGN)&& oact == NULL) > + return (__sys_sigaction(sig,&newact, NULL)); > + } > > __sys_sigprocmask(SIG_SETMASK,&_thr_maskset,&oldset); > _thr_rwl_wrlock(&_thr_sigact[sig-1].lock); > I simply duplicated idea from OpenSolaris, here is my patch which has similar feature as your patch, and it also tries to prevent vforked child from corrupting parent's data: http://people.freebsd.org/~davidxu/patch/libthr-vfork.diff