[PATCH 0/2] silent fork retry with shm (broke emacs-X11)

classic Classic list List threaded Threaded
10 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 0/2] silent fork retry with shm (broke emacs-X11)

Michael Haubenwallner-2
Hi,

following up https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html

It turns out that fixup_shms_after_fork does require the child pinfo to
be "remember"ed, while the fork retry to be silent on failure requires
the child to not be "attach"ed yet.

As current pinfo.remember performs both "remember" and "attach" at once,
the first patch does introduce pinfo.remember_without_attach, to not
change current behaviour of pinfo.remember and keep patches small.

However, my first thought was to clean up pinfo API a little and have
remember not do both "remember+attach" at once, but introduce some new
remember_and_attach method instead.  But then, when 'bool detach' is
true, the "_and_attach" does feel wrong.

Thoughts?

Thanks!
/haubi/


Reply | Threaded
Open this post in threaded view
|

[PATCH 1/2] Cygwin: pinfo: new remember_without_attach method

Michael Haubenwallner-2
During fork, the child process requires the process table to be
initialized for fixup_shms_after_fork, while still allowing subsequent
dlls.load_after_fork to fail silently (for when the "forkable" hardlinks
are not created yet).

Prepares to improve "Cygwin: fork: Remember child not before success."
commit f03ea8e1c57bd5cea83f6cd47fa02870bdfeb1c5, which leads to fork
problems if cygserver is running:

https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
---
 winsup/cygwin/pinfo.h    | 9 ++++++++-
 winsup/cygwin/sigproc.cc | 4 +---
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/winsup/cygwin/pinfo.h b/winsup/cygwin/pinfo.h
index 8cf1bba2e..35f6f5ac6 100644
--- a/winsup/cygwin/pinfo.h
+++ b/winsup/cygwin/pinfo.h
@@ -195,13 +195,20 @@ public:
     destroy = res ? false : true;
     return res;
   }
-  int remember (bool detach)
+  int remember_without_attach (bool detach)
   {
     int res = proc_subproc (detach ? PROC_DETACHED_CHILD : PROC_ADDCHILD,
     (uintptr_t) this);
     destroy = res ? false : true;
     return res;
   }
+  int remember (bool detach)
+  {
+    int res = remember_without_attach (detach);
+    if (res && !detach)
+      res = reattach ();
+    return res;
+  }
 #endif
   HANDLE shared_handle () {return h;}
   HANDLE shared_winpid_handle () {return winpid_hdl;}
diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index 900facd58..9af47ce48 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -216,9 +216,7 @@ proc_subproc (DWORD what, uintptr_t val)
   vchild->process_state |= PID_INITIALIZING;
   vchild->ppid = what == PROC_DETACHED_CHILD ? 1 : myself->pid; /* always set last */
  }
-      if (what == PROC_DETACHED_CHILD)
- break;
-      /* fall through intentionally */
+      break;
 
     case PROC_REATTACH_CHILD:
       procs[nprocs] = vchild;
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH 2/2] Cygwin: fork: Attach child not before success.

Michael Haubenwallner-2
In reply to this post by Michael Haubenwallner-2
Do not attach to the child before it was successfully initialized, or we
would need more sophisticated cleanup on child initialization failure,
like suppressing SIGCHILD delivery with multiple threads ("waitproc")
involved.

Improves "Cygwin: fork: Remember child not before success.",
commit f03ea8e1c57bd5cea83f6cd47fa02870bdfeb1c5, which leads to fork
problems if cygserver is running:

https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
---
 winsup/cygwin/fork.cc | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 59b13806c..c6ba8cf65 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -416,11 +416,11 @@ frok::parent (volatile char * volatile stack_here)
      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))
+  if (!child.remember_without_attach (false))
     {
       this_errno = EAGAIN;
 #ifdef DEBUGGING0
-      error ("child remember failed");
+      error ("child remember_without_attach failed");
 #endif
       goto cleanup;
     }
@@ -508,6 +508,15 @@ frok::parent (volatile char * volatile stack_here)
  }
     }
 
+  if (!child.reattach ())
+    {
+      this_errno = EAGAIN;
+#ifdef DEBUGGING0
+      error ("child reattach failed");
+#endif
+      goto cleanup;
+    }
+
   /* Finally start the child up. */
   resume_child (forker_finished);
 
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH 0/2] silent fork retry with shm (broke emacs-X11)

Corinna Vinschen-2
In reply to this post by Michael Haubenwallner-2
Hi Michael,

On Jul 30 17:22, Michael Haubenwallner wrote:

> Hi,
>
> following up https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
>
> It turns out that fixup_shms_after_fork does require the child pinfo to
> be "remember"ed, while the fork retry to be silent on failure requires
> the child to not be "attach"ed yet.
>
> As current pinfo.remember performs both "remember" and "attach" at once,
> the first patch does introduce pinfo.remember_without_attach, to not
> change current behaviour of pinfo.remember and keep patches small.
>
> However, my first thought was to clean up pinfo API a little and have
> remember not do both "remember+attach" at once, but introduce some new
> remember_and_attach method instead.  But then, when 'bool detach' is
> true, the "_and_attach" does feel wrong.
I'd prefer to drop the reattach call from remember, calling both of them
where appropriate.


Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

[PATCH v2 0/2] silent fork retry with shm (broke emacs-X11)

Michael Haubenwallner-2
Hi Corinna,

On 7/30/19 6:07 PM, Corinna Vinschen wrote:

> Hi Michael,
>
> On Jul 30 17:22, Michael Haubenwallner wrote:
>> Hi,
>>
>> following up
>> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
>>
>> It turns out that fixup_shms_after_fork does require the child pinfo to
>> be "remember"ed, while the fork retry to be silent on failure requires
>> the child to not be "attach"ed yet.
>>
>> As current pinfo.remember performs both "remember" and "attach" at once,
>> the first patch does introduce pinfo.remember_without_attach, to not
>> change current behaviour of pinfo.remember and keep patches small.
>>
>> However, my first thought was to clean up pinfo API a little and have
>> remember not do both "remember+attach" at once, but introduce some new
>> remember_and_attach method instead.  But then, when 'bool detach' is
>> true, the "_and_attach" does feel wrong.
>
> I'd prefer to drop the reattach call from remember, calling both of them
> where appropriate.
>

Fine with me, even if that looks a little more complicated for spawn.

Thanks!
/haubi/


Reply | Threaded
Open this post in threaded view
|

[PATCH v2 1/2] Cygwin: pinfo: stop remember doing reattach

Michael Haubenwallner-2
During fork, the child process requires the process table to be
initialized for fixup_shms_after_fork, while still allowing subsequent
dlls.load_after_fork to fail silently (for when the "forkable" hardlinks
are not created yet).
pinfo::remember not performing reattach anymore requires explicit
pinfo::reattach now where appropriate.

Prepares to improve "Cygwin: fork: Remember child not before success."
commit f03ea8e1c57bd5cea83f6cd47fa02870bdfeb1c5, which leads to fork
problems if cygserver is running:

https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
---
 winsup/cygwin/fork.cc    | 8 ++++++++
 winsup/cygwin/sigproc.cc | 7 +++----
 winsup/cygwin/spawn.cc   | 4 +++-
 3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 59b13806c..0119581df 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -421,6 +421,14 @@ frok::parent (volatile char * volatile stack_here)
       this_errno = EAGAIN;
 #ifdef DEBUGGING0
       error ("child remember failed");
+#endif
+      goto cleanup;
+    }
+  if (!child.reattach ())
+    {
+      this_errno = EAGAIN;
+#ifdef DEBUGGING0
+      error ("child reattach failed");
 #endif
       goto cleanup;
     }
diff --git a/winsup/cygwin/sigproc.cc b/winsup/cygwin/sigproc.cc
index 900facd58..8003e2db6 100644
--- a/winsup/cygwin/sigproc.cc
+++ b/winsup/cygwin/sigproc.cc
@@ -216,9 +216,7 @@ proc_subproc (DWORD what, uintptr_t val)
   vchild->process_state |= PID_INITIALIZING;
   vchild->ppid = what == PROC_DETACHED_CHILD ? 1 : myself->pid; /* always set last */
  }
-      if (what == PROC_DETACHED_CHILD)
- break;
-      /* fall through intentionally */
+      break;
 
     case PROC_REATTACH_CHILD:
       procs[nprocs] = vchild;
@@ -873,7 +871,8 @@ void
 child_info_spawn::wait_for_myself ()
 {
   postfork (myself);
-  myself.remember (false);
+  if (myself.remember (false))
+    myself.reattach ();
   WaitForSingleObject (ev, INFINITE);
 }
 
diff --git a/winsup/cygwin/spawn.cc b/winsup/cygwin/spawn.cc
index 579b3c9c3..f719410a8 100644
--- a/winsup/cygwin/spawn.cc
+++ b/winsup/cygwin/spawn.cc
@@ -779,7 +779,9 @@ child_info_spawn::worker (const char *prog_arg, const char *const *argv,
   child->start_time = time (NULL); /* Register child's starting time. */
   child->nice = myself->nice;
   postfork (child);
-  if (!child.remember (mode == _P_DETACH))
+  if (mode == _P_DETACH ?
+      !child.remember (true) :
+      !(child.remember (false) && child.reattach ()))
     {
       /* FIXME: Child in strange state now */
       CloseHandle (pi.hProcess);
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

[PATCH v2 2/2] Cygwin: fork: attach child not before success

Michael Haubenwallner-2
In reply to this post by Michael Haubenwallner-2
Do not attach to the child before it was successfully initialized, or we
would need more sophisticated cleanup on child initialization failure,
like suppressing SIGCHILD delivery with multiple threads ("waitproc")
involved.

Improves "Cygwin: fork: Remember child not before success.",
commit f03ea8e1c57bd5cea83f6cd47fa02870bdfeb1c5, which leads to fork
problems if cygserver is running:

https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
---
 winsup/cygwin/fork.cc | 19 +++++++++++--------
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/winsup/cygwin/fork.cc b/winsup/cygwin/fork.cc
index 0119581df..7080144b9 100644
--- a/winsup/cygwin/fork.cc
+++ b/winsup/cygwin/fork.cc
@@ -421,14 +421,6 @@ frok::parent (volatile char * volatile stack_here)
       this_errno = EAGAIN;
 #ifdef DEBUGGING0
       error ("child remember failed");
-#endif
-      goto cleanup;
-    }
-  if (!child.reattach ())
-    {
-      this_errno = EAGAIN;
-#ifdef DEBUGGING0
-      error ("child reattach failed");
 #endif
       goto cleanup;
     }
@@ -516,6 +508,17 @@ frok::parent (volatile char * volatile stack_here)
  }
     }
 
+  /* Do not attach to the child before it has successfully initialized.
+     Otherwise we may wait forever, or deliver an orphan SIGCHILD. */
+  if (!child.reattach ())
+    {
+      this_errno = EAGAIN;
+#ifdef DEBUGGING0
+      error ("child reattach failed");
+#endif
+      goto cleanup;
+    }
+
   /* Finally start the child up. */
   resume_child (forker_finished);
 
--
2.21.0

Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] silent fork retry with shm (broke emacs-X11)

Corinna Vinschen-2
In reply to this post by Michael Haubenwallner-2
On Jul 31 12:35, Michael Haubenwallner wrote:

> Hi Corinna,
>
> On 7/30/19 6:07 PM, Corinna Vinschen wrote:
> > Hi Michael,
> >
> > On Jul 30 17:22, Michael Haubenwallner wrote:
> >> Hi,
> >>
> >> following up
> >> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
> >>
> >> It turns out that fixup_shms_after_fork does require the child pinfo to
> >> be "remember"ed, while the fork retry to be silent on failure requires
> >> the child to not be "attach"ed yet.
> >>
> >> As current pinfo.remember performs both "remember" and "attach" at once,
> >> the first patch does introduce pinfo.remember_without_attach, to not
> >> change current behaviour of pinfo.remember and keep patches small.
> >>
> >> However, my first thought was to clean up pinfo API a little and have
> >> remember not do both "remember+attach" at once, but introduce some new
> >> remember_and_attach method instead.  But then, when 'bool detach' is
> >> true, the "_and_attach" does feel wrong.
> >
> > I'd prefer to drop the reattach call from remember, calling both of them
> > where appropriate.
> >
>
> Fine with me, even if that looks a little more complicated for spawn.
Pushed, with just a small formatting tweak.


Thanks,
Corinna

--
Corinna Vinschen
Cygwin Maintainer

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] silent fork retry with shm (broke emacs-X11)

Ken Brown-6
On 7/31/2019 12:59 PM, Corinna Vinschen wrote:

> On Jul 31 12:35, Michael Haubenwallner wrote:
>> Hi Corinna,
>>
>> On 7/30/19 6:07 PM, Corinna Vinschen wrote:
>>> Hi Michael,
>>>
>>> On Jul 30 17:22, Michael Haubenwallner wrote:
>>>> Hi,
>>>>
>>>> following up
>>>> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
>>>>
>>>> It turns out that fixup_shms_after_fork does require the child pinfo to
>>>> be "remember"ed, while the fork retry to be silent on failure requires
>>>> the child to not be "attach"ed yet.
>>>>
>>>> As current pinfo.remember performs both "remember" and "attach" at once,
>>>> the first patch does introduce pinfo.remember_without_attach, to not
>>>> change current behaviour of pinfo.remember and keep patches small.
>>>>
>>>> However, my first thought was to clean up pinfo API a little and have
>>>> remember not do both "remember+attach" at once, but introduce some new
>>>> remember_and_attach method instead.  But then, when 'bool detach' is
>>>> true, the "_and_attach" does feel wrong.
>>>
>>> I'd prefer to drop the reattach call from remember, calling both of them
>>> where appropriate.
>>>
>>
>> Fine with me, even if that looks a little more complicated for spawn.
>
> Pushed, with just a small formatting tweak.

I can confirm that this fixes the problem I reported in
https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html.

Ken
Reply | Threaded
Open this post in threaded view
|

Re: [PATCH v2 0/2] silent fork retry with shm (broke emacs-X11)

Michael Haubenwallner-2
On 7/31/19 7:25 PM, Ken Brown wrote:

> On 7/31/2019 12:59 PM, Corinna Vinschen wrote:
>> On Jul 31 12:35, Michael Haubenwallner wrote:
>>> On 7/30/19 6:07 PM, Corinna Vinschen wrote:
>>>> On Jul 30 17:22, Michael Haubenwallner wrote:
>>>>> Hi,
>>>>>
>>>>> following up
>>>>> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html
>>>>>
>>>>> It turns out that fixup_shms_after_fork does require the child pinfo to
>>>>> be "remember"ed, while the fork retry to be silent on failure requires
>>>>> the child to not be "attach"ed yet.
>>>>>
>>>>> As current pinfo.remember performs both "remember" and "attach" at once,
>>>>> the first patch does introduce pinfo.remember_without_attach, to not
>>>>> change current behaviour of pinfo.remember and keep patches small.
>>>>>
>>>>> However, my first thought was to clean up pinfo API a little and have
>>>>> remember not do both "remember+attach" at once, but introduce some new
>>>>> remember_and_attach method instead.  But then, when 'bool detach' is
>>>>> true, the "_and_attach" does feel wrong.
>>>>
>>>> I'd prefer to drop the reattach call from remember, calling both of them
>>>> where appropriate.
>>>>
>>>
>>> Fine with me, even if that looks a little more complicated for spawn.
>>
>> Pushed, with just a small formatting tweak.
>
> I can confirm that this fixes the problem I reported in
> https://cygwin.com/ml/cygwin-patches/2019-q2/msg00155.html.
>

Corinna, Ken: Thanks a lot!
/haubi/