Date: Thu, 9 Aug 2012 12:56:49 +0200 From: Jilles Tjoelker <jilles@stack.nl> To: Konstantin Belousov <kostikbel@gmail.com>, davidxu@freebsd.org Cc: freebsd-hackers@freebsd.org Subject: Re: system() using vfork() or posix_spawn() and libthr Message-ID: <20120809105648.GA79814@stack.nl> In-Reply-To: <20120806082535.GI2676@deviant.kiev.zoral.com.ua> References: <20120730102408.GA19983@stack.nl> <20120730105303.GU2676@deviant.kiev.zoral.com.ua> <20120805215432.GA28704@stack.nl> <20120806082535.GI2676@deviant.kiev.zoral.com.ua>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
--
Jilles Tjoelker
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120809105648.GA79814>