On 4/30/19 6:07 PM, Corinna Vinschen wrote:
> On Apr 30 09:09, Michael Haubenwallner wrote:
>> Do not remember the child before it was successfully initialized, or we
>> would need more sophisticated cleanup on child initialization failure,
>> like cleaning up the process table and suppressing SIGCHILD delivery
>> with multiple threads ("waitproc") involved.
>> ---
>> winsup/cygwin/fork.cc | 33 ++++++++++++++++++---------------
>> 1 file changed, 18 insertions(+), 15 deletions(-)
>> [...]
>> + yield (); /* For child.remember (), to perform async thread startup. */
>
> Is that really necessary? What's that fixing and what effect does this
> have on the performance of the already very slow fork()?
Indeed, that's needless. Testing around the original patch in this thread
did lead me to the idea that the "waitproc" cygthread is started using
QueueUserAPC and async_startup, being executed by WFMO in ch.sync, which
I've reduced to yield later on. But apparently I've been wrong here and the
waitproc thread is started synchronously. Performance wise, I found it
negligible, but admitted I didn't perform the full >24h build.
Patch updated.
Thanks!
/haubi/
>From 69c21e8e5d19cc6ec205a4e44d6b84542b1fef98 Mon Sep 17 00:00:00 2001
From: Michael Haubenwallner <michael.haubenwall...@ssi-schaefer.com>
Date: Mon, 29 Apr 2019 16:03:51 +0200
Subject: [PATCH] Cygwin: fork: Remember child not before success.
Do not remember the child before it was successfully initialized, or we
would need more sophisticated cleanup on child initialization failure,
like cleaning up the process table and suppressing SIGCHILD delivery
with multiple threads ("waitproc") involved. Compared to that, the
potential slowdown due to an extra yield () call should be negligible.
---
winsup/cygwin/fork.cc | 31 ++++++++++++++++---------------
1 file changed, 16 insertions(+), 15 deletions(-)
diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 59b13806c..c69081fc0 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -181,7 +181,8 @@ frok::child (volatile char * volatile here)
cygheap->fdtab.fixup_after_fork (hParent);
/* Signal that we have successfully initialized, so the parent can
- - transfer data/bss for dynamically loaded dlls (if any), or
+ - transfer data/bss for dynamically loaded dlls (if any), and
+ - start up some tracker threads to remember the child, or
- terminate the current fork call even if the child is initialized. */
sync_with_parent ("performed fork fixups and dynamic dll loading", true);
@@ -411,20 +412,6 @@ frok::parent (volatile char * volatile stack_here)
child.hProcess = hchild;
ch.postfork (child);
- /* Hopefully, this will succeed. The alternative to doing things this
- way is to reserve space prior to calling CreateProcess and then fill
- it in afterwards. This requires more bookkeeping than I like, though,
- so we'll just do it the easy way. So, terminate any child process if
- we can't actually record the pid in the internal table. */
- if (!child.remember (false))
- {
- this_errno = EAGAIN;
-#ifdef DEBUGGING0
- error ("child remember failed");
-#endif
- goto cleanup;
- }
-
/* CHILD IS STOPPED */
debug_printf ("child is alive (but stopped)");
@@ -508,6 +495,20 @@ frok::parent (volatile char * volatile stack_here)
}
}
+ /* Hopefully, this will succeed. The alternative to doing things this
+ way is to reserve space prior to calling CreateProcess and then fill
+ it in afterwards. This requires more bookkeeping than I like, though,
+ so we'll just do it the easy way. So, terminate any child process if
+ we can't actually record the pid in the internal table. */
+ if (!child.remember (false))
+ {
+ this_errno = EAGAIN;
+#ifdef DEBUGGING0
+ error ("child remember failed");
+#endif
+ goto cleanup;
+ }
+
/* Finally start the child up. */
resume_child (forker_finished);
--
2.19.2
