This is the mail archive of the
cygwin-patches
mailing list for the Cygwin project.
Re: [PATCH] Possibly correct fix to strace phantom process entry
On 04/24/2017 07:19 AM, Corinna Vinschen wrote:
Hi Daniel,
On Apr 24 04:37, Daniel Santos wrote:
The root cause of problem with strace causing long delays when any
process enumerates the process database appears to be calling
myself.thisproc () from child_info_spawn::handle_spawn() when we've
dynamically loaded cygwin1.dll. It definately fixes the problem, but I
don't konw what other processes dynamically load cygwin1.dll and, thus,
what other side-effects that this may have. Please verify correctness.
Please see discussion here: https://cygwin.com/ml/cygwin/2017-04/msg00240.html
Daniel
Signed-off-by: Daniel Santos <daniel.santos@pobox.com>
---
I was just looking into this myself, but I was looking into the weird
Sleep loop itself and was wondering if that makes any sense at all.
Assuming pinfo::init is called from process enumeration (winpids::add)
then there's no good reason to handle this procinfo block at all. It
doesn't resolve into an existing "real" Cygwin process. And that's
exactly the case that hangs.
Yeah, and it doesn't represent a unique windows process either.
So my curent fix would have been this:
diff --git a/winsup/cygwin/pinfo.cc b/winsup/cygwin/pinfo.cc
index e43082d..090fcb9 100644
--- a/winsup/cygwin/pinfo.cc
+++ b/winsup/cygwin/pinfo.cc
@@ -314,12 +314,18 @@ pinfo::init (pid_t n, DWORD flag, HANDLE h0)
/* Detect situation where a transitional memory block is being retrieved.
If the block has been allocated with PINFO_REDIR_SIZE but not yet
updated with a PID_EXECED state then we'll retry. */
- if (!created && !(flag & PID_NEW))
- /* If not populated, wait 2 seconds for procinfo to become populated.
- Would like to wait with finer granularity but that is not easily
- doable. */
- for (int i = 0; i < 200 && !procinfo->ppid; i++)
- Sleep (10);
+ if (!created && !(flag & PID_NEW) && !procinfo->ppid)
+ {
+ /* We're fetching process info for /proc or ps so we can just
+ ignore this procinfo. */
+ if (flag & PID_NOREDIR)
+ break;
+ /* If not populated, wait 2 seconds for procinfo to become populated.
+ Would like to wait with finer granularity but that is not easily
+ doable. */
+ for (int i = 0; i < 200 && !procinfo->ppid; i++)
+ Sleep (10);
+ }
Yeah, I hacked this loop up many times, mostly to diagnose the problem.
I presume that it was originally added for a reason, but as I said
before, I know that on x86 procinfo->ppid is almost certain to compile
into a mov that will be atomic, but when I expect another thread to
change something larger than one byte, I prefer to use a macro or
function that is always atomic and conveys the intention.
winsup/cygwin/dcrt0.cc | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/winsup/cygwin/dcrt0.cc b/winsup/cygwin/dcrt0.cc
index ea6adcbbd..bbab08725 100644
--- a/winsup/cygwin/dcrt0.cc
+++ b/winsup/cygwin/dcrt0.cc
@@ -664,7 +664,8 @@ child_info_spawn::handle_spawn ()
my_wr_proc_pipe = wr_proc_pipe;
rd_proc_pipe = wr_proc_pipe = NULL;
- myself.thisproc (h);
+ if (!dynamically_loaded)
+ myself.thisproc (h);
__argc = moreinfo->argc;
__argv = moreinfo->argv;
envp = moreinfo->envp;
--
2.11.0
Your patch looks simple and elegant. I'm just not sure about the side
effects it may have if a process is missing its procinfo. No problem
for strace and ldd, apparently, but other processes...?
This makes me curious if ld produces this problem as well, only that
it's very brief in execution.
We could try it for a while. I'm off all of May anyway and I could
create a test build for that time...
Btw., would you mind to send a BSD waiver per
https://cygwin.com/contrib.html and
https://cygwin.com/git/?p=newlib-cygwin.git;f=winsup/CONTRIBUTORS;hb=HEAD
Your patches are covered by the "trivial patch" rule yet, but if you
look into providing more patches you don't have to care anymore.
Thanks, I had overlooked that. Is this sufficient?
I am providing my patches to the Cygwin sources under the 2-clause BSD
license.
Daniel