Change in process.c

Change in process.c

[Richard Stallman]

What was the reason for moving the first sleep before the first wait3
call?  It looks like that will force a one-second delay even
in cases where it would succeed straightaway.

Using usleep would reduce that problem to an acceptable level,
but I am not sure all systems have usleep.  So it is risky
to make that change.

What's the issue here?

Re: Change in process.c

[Sam Steingold]

> * Richard Stallman <ezf@tah.bet> [2007-03-26 19:13:42 -0400]:
>
> What was the reason for moving the first sleep before the first wait3
> call?  It looks like that will force a one-second delay even
> in cases where it would succeed straightaway.
>
> Using usleep would reduce that problem to an acceptable level,
> but I am not sure all systems have usleep.  So it is risky
> to make that change.
>
> What's the issue here?

this has been discussed here:
http://thread.gmane.org/gmane.emacs.devel:67722

I have been observing the following behavior:
emacs hangs in sigchld_handler waiting for the child process to
terminate.
the system stops responding, loadavg goes to 5-8(!), CPU is 100% busy.
this lasts for ~10 seconds.
this happens on M-c compile and when I click on a URL (it is passed to
an existing Firefox).

the sleep (and the most recent usleep) patch fixes the problem for me.
this may be linux-specific - I have to defer to kernel experts on that.

-- 
Sam Steingold (http://sds.podval.org/) on Fedora Core release 6 (Zod)
http://openvotingconsortium.org http://jihadwatch.org http://truepeace.org
http://memri.org http://iris.org.il http://camera.org http://dhimmi.com
We're too busy mopping the floor to turn off the faucet.

Re: Change in process.c

[Andreas Schwab]

Sam Steingold <sds@gnu.org> writes:

> the system stops responding, loadavg goes to 5-8(!), CPU is 100% busy.
> this lasts for ~10 seconds.

Does this happen on _any_ other system but yours?

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Change in process.c

[Nick Roberts]

 > > the system stops responding, loadavg goes to 5-8(!), CPU is 100% busy.
 > > this lasts for ~10 seconds.
 > 
 > Does this happen on _any_ other system but yours?

Also my old machine says (hard problems on my newer one - you were right!):


       This function is obsolete. Use nanosleep(2) or setitimer(2) instead.

                                  2001-04-02                         USLEEP(3)
                                  ^^^^

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Change in process.c

[Richard Stallman]

    > the system stops responding, loadavg goes to 5-8(!), CPU is 100% busy.
    > this lasts for ~10 seconds.

    Does this happen on _any_ other system but yours?

We can't support a negative answer to that question without asking
lots more users.

I don't mind making a change to fix this,
but it must not do a full-second sleep before the first wait3,
and it must not use usleep since that is obsolete.

Re: Change in process.c

[Chong Yidong]

Richard Stallman <rms@gnu.org> writes:

> I don't mind making a change to fix this,
> but it must not do a full-second sleep before the first wait3,
> and it must not use usleep since that is obsolete.

Does this version work?

*** emacs/src/process.c.~1.508.~	2007-03-27 16:47:03.000000000 -0400
--- emacs/src/process.c	2007-03-27 17:13:31.000000000 -0400
***************
*** 6500,6516 ****
  #define WUNTRACED 0
  #endif /* no WUNTRACED */
        /* Keep trying to get a status until we get a definitive result.  */
!       do
          {
  	  /* For some reason, this sleep() prevents Emacs from sending
               loadavg to 5-8(!) for ~10 seconds.
               See http://thread.gmane.org/gmane.emacs.devel/67722 or
               http://www.google.com/search?q=busyloop+in+sigchld_handler */
!           usleep (1000);
  	  errno = 0;
  	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
  	}
-       while (pid < 0 && errno == EINTR);
  
        if (pid <= 0)
  	{
--- 6500,6518 ----
  #define WUNTRACED 0
  #endif /* no WUNTRACED */
        /* Keep trying to get a status until we get a definitive result.  */
! 
!       errno = 0;
!       pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
!       while (pid < 0 && errno == EINTR)
          {
  	  /* For some reason, this sleep() prevents Emacs from sending
               loadavg to 5-8(!) for ~10 seconds.
               See http://thread.gmane.org/gmane.emacs.devel/67722 or
               http://www.google.com/search?q=busyloop+in+sigchld_handler */
!           sleep (1);
  	  errno = 0;
  	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
  	}
  
        if (pid <= 0)
  	{

Re: Change in process.c

[Nick Roberts]

 > Does this version work?

I can't comment on th patch, but

 >   	  /* For some reason, this sleep() prevents Emacs from sending
 >                loadavg to 5-8(!) for ~10 seconds.
 >                See http://thread.gmane.org/gmane.emacs.devel/67722 or
 >                http://www.google.com/search?q=busyloop+in+sigchld_handler */

Links that might disappear, and searches whose results will change, as comments
doesn't seem a good idea.  Can't the reason for the change be summarised in
situ?

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Change in process.c

[Sam Steingold]

Chong Yidong wrote:
> Richard Stallman <rms@gnu.org> writes:
> 
>> I don't mind making a change to fix this,
>> but it must not do a full-second sleep before the first wait3,
>> and it must not use usleep since that is obsolete.
> 
> Does this version work?

no, absolutely NOT.
there must be a sleep BEFORE the first wait.
something equivalent to your version was tried and found lacking.

> *** emacs/src/process.c.~1.508.~	2007-03-27 16:47:03.000000000 -0400
> --- emacs/src/process.c	2007-03-27 17:13:31.000000000 -0400
> ***************
> *** 6500,6516 ****
>   #define WUNTRACED 0
>   #endif /* no WUNTRACED */
>         /* Keep trying to get a status until we get a definitive result.  */
> !       do
>           {
>   	  /* For some reason, this sleep() prevents Emacs from sending
>                loadavg to 5-8(!) for ~10 seconds.
>                See http://thread.gmane.org/gmane.emacs.devel/67722 or
>                http://www.google.com/search?q=busyloop+in+sigchld_handler */
> !           usleep (1000);
>   	  errno = 0;
>   	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
>   	}
> -       while (pid < 0 && errno == EINTR);
>   
>         if (pid <= 0)
>   	{
> --- 6500,6518 ----
>   #define WUNTRACED 0
>   #endif /* no WUNTRACED */
>         /* Keep trying to get a status until we get a definitive result.  */
> ! 
> !       errno = 0;
> !       pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
> !       while (pid < 0 && errno == EINTR)
>           {
>   	  /* For some reason, this sleep() prevents Emacs from sending
>                loadavg to 5-8(!) for ~10 seconds.
>                See http://thread.gmane.org/gmane.emacs.devel/67722 or
>                http://www.google.com/search?q=busyloop+in+sigchld_handler */
> !           sleep (1);
>   	  errno = 0;
>   	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
>   	}
>   
>         if (pid <= 0)
>   	{

Re: Change in process.c

[Sam Steingold]

Nick Roberts wrote:
>  >   	  /* For some reason, this sleep() prevents Emacs from sending
>  >                loadavg to 5-8(!) for ~10 seconds.
>  >                See http://thread.gmane.org/gmane.emacs.devel/67722 or
>  >                http://www.google.com/search?q=busyloop+in+sigchld_handler */
> 
> Links that might disappear, and searches whose results will change, as comments
> doesn't seem a good idea.  

the link will not disappear any more than a "gnu.org" link will.
if you have a better link, please add it.

> Can't the reason for the change be summarised in situ?

it is summarized: "or some reason, this sleep() prevents Emacs from 
sending loadavg to 5-8(!) for ~10 seconds".
40 messages of various conjectures, claims, counterclaims, rebuttals &c 
are long but of dubious value inside the source code.

Re: Change in process.c

[Andreas Schwab]

Sam Steingold <sds@gnu.org> writes:

> there must be a sleep BEFORE the first wait.

If that is the case then there is something very broken with your kernel.
After you receive a SIGCHLD it is guarateed that what3 returns immediately
with the status of the changing child.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: Change in process.c

[Chong Yidong]

Sam Steingold <sds@gnu.org> writes:

> there must be a sleep BEFORE the first wait.
> something equivalent to your version was tried and found lacking.

I can understand the explanation of why a busyloop would cause Emacs
to eat CPU; but why does issuing the wait3 before the first sleep
cause problems?  AFAIK, no one has ever offered an explanation for
this.

Re: Change in process.c

[Sam Steingold]

Andreas Schwab wrote:
> Sam Steingold <sds@gnu.org> writes:
> 
>> there must be a sleep BEFORE the first wait.
> 
> If that is the case then there is something very broken with your kernel.
> After you receive a SIGCHLD it is guarateed that what3 returns immediately
> with the status of the changing child.

this is the latest fc6 kernel - I am not the only one experiencing this.

Re: Change in process.c

[Richard Stallman]

    > I don't mind making a change to fix this,
    > but it must not do a full-second sleep before the first wait3,
    > and it must not use usleep since that is obsolete.

    Does this version work?

Your version avoids those two mistakes, at least.

Re: Change in process.c

[Jan Djärv]

Sam Steingold skrev:
> Andreas Schwab wrote:
>> Sam Steingold <sds@gnu.org> writes:
>>
>>> there must be a sleep BEFORE the first wait.
>>
>> If that is the case then there is something very broken with your kernel.
>> After you receive a SIGCHLD it is guarateed that what3 returns 
>> immediately
>> with the status of the changing child.
> 
> this is the latest fc6 kernel - I am not the only one experiencing this.

I can see this too.  But it is this code that gets executed over and over 
again (process.c, line 4810 or so):

#ifdef HAVE_PTYS
	      /* On some OSs with ptys, when the process on one end of
		 a pty exits, the other end gets an error reading with
		 errno = EIO instead of getting an EOF (0 bytes read).
		 Therefore, if we get an error reading and errno =
		 EIO, just continue, because the child process has
		 exited and should clean itself up soon (e.g. when we
		 get a SIGCHLD).

		 However, it has been known to happen that the SIGCHLD
		 got lost.  So raise the signl again just in case.
		 It can't hurt.  */
	      else if (nread == -1 && errno == EIO)
		kill (getpid (), SIGCHLD);
#endif				/* HAVE_PTYS */


So emacs gets swamped with SIGCHILD and hogs the CPU.  The (u)sleep inserted 
just lets the forked process get some CPU time to die properly.  Now why the 
PTY return EIO repeatedly is another matter.

	Jan D.

Re: Change in process.c

[Kim F. Storm]

Jan Djärv <jan.h.d@swipnet.se> writes:

> I can see this too.  But it is this code that gets executed over and
> over again (process.c, line 4810 or so):

Good catch!!

Does the following patch give good results?

*** process.c	28 Mar 2007 11:18:56 +0200	1.508
--- process.c	28 Mar 2007 22:55:11 +0200	
***************
*** 4820,4826 ****
  		 got lost.  So raise the signl again just in case.
  		 It can't hurt.  */
  	      else if (nread == -1 && errno == EIO)
! 		kill (getpid (), SIGCHLD);
  #endif				/* HAVE_PTYS */
  	      /* If we can detect process termination, don't consider the process
  		 gone just because its pipe is closed.  */
--- 4820,4832 ----
  		 got lost.  So raise the signl again just in case.
  		 It can't hurt.  */
  	      else if (nread == -1 && errno == EIO)
! 		{
! 		  /* Clear the descriptor now, so we only raise the signal once.  */
! 		  FD_CLR (channel, &input_wait_mask);
! 		  FD_CLR (channel, &non_keyboard_wait_mask);
! 
! 		  kill (getpid (), SIGCHLD);
! 		}
  #endif				/* HAVE_PTYS */
  	      /* If we can detect process termination, don't consider the process
  		 gone just because its pipe is closed.  */
***************
*** 6502,6512 ****
        /* Keep trying to get a status until we get a definitive result.  */
        do
          {
- 	  /* For some reason, this sleep() prevents Emacs from sending
-              loadavg to 5-8(!) for ~10 seconds.
-              See http://thread.gmane.org/gmane.emacs.devel/67722 or
-              http://www.google.com/search?q=busyloop+in+sigchld_handler */
-           usleep (1000);
  	  errno = 0;
  	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
  	}
--- 6508,6513 ----

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk

Re: Change in process.c

[Nick Roberts]

 > > Can't the reason for the change be summarised in situ?
 > 
 > it is summarized: "or some reason, this sleep() prevents Emacs from 
 > sending loadavg to 5-8(!) for ~10 seconds".
 > 40 messages of various conjectures, claims, counterclaims, rebuttals &c 
 > are long but of dubious value inside the source code.

If we don't understand why a change works then I'm not sure that it's the
right fix.

-- 
Nick                                           http://www.inet.net.nz/~nickrob

Re: Change in process.c

[Jan Djärv]

Kim F. Storm skrev:
> Jan Djärv <jan.h.d@swipnet.se> writes:
> 
>> I can see this too.  But it is this code that gets executed over and
>> over again (process.c, line 4810 or so):
> 
> Good catch!!
> 
> Does the following patch give good results?

Yes, the problem is completely gone.  Can the original poster (Sam Steingold? 
I forgot) also confirm?

	Jan D.

> 
> *** process.c	28 Mar 2007 11:18:56 +0200	1.508
> --- process.c	28 Mar 2007 22:55:11 +0200	
> ***************
> *** 4820,4826 ****
>   		 got lost.  So raise the signl again just in case.
>   		 It can't hurt.  */
>   	      else if (nread == -1 && errno == EIO)
> ! 		kill (getpid (), SIGCHLD);
>   #endif				/* HAVE_PTYS */
>   	      /* If we can detect process termination, don't consider the process
>   		 gone just because its pipe is closed.  */
> --- 4820,4832 ----
>   		 got lost.  So raise the signl again just in case.
>   		 It can't hurt.  */
>   	      else if (nread == -1 && errno == EIO)
> ! 		{
> ! 		  /* Clear the descriptor now, so we only raise the signal once.  */
> ! 		  FD_CLR (channel, &input_wait_mask);
> ! 		  FD_CLR (channel, &non_keyboard_wait_mask);
> ! 
> ! 		  kill (getpid (), SIGCHLD);
> ! 		}
>   #endif				/* HAVE_PTYS */
>   	      /* If we can detect process termination, don't consider the process
>   		 gone just because its pipe is closed.  */
> ***************
> *** 6502,6512 ****
>         /* Keep trying to get a status until we get a definitive result.  */
>         do
>           {
> - 	  /* For some reason, this sleep() prevents Emacs from sending
> -              loadavg to 5-8(!) for ~10 seconds.
> -              See http://thread.gmane.org/gmane.emacs.devel/67722 or
> -              http://www.google.com/search?q=busyloop+in+sigchld_handler */
> -           usleep (1000);
>   	  errno = 0;
>   	  pid = wait3 (&w, WNOHANG | WUNTRACED, 0);
>   	}
> --- 6508,6513 ----
> 

Re: Change in process.c

[Randal L. Schwartz]

>>>>> "Richard" == Richard Stallman <rms@gnu.org> writes:

Richard> What was the reason for moving the first sleep before the first wait3
Richard> call?  It looks like that will force a one-second delay even
Richard> in cases where it would succeed straightaway.

Richard> Using usleep would reduce that problem to an acceptable level,
Richard> but I am not sure all systems have usleep.  So it is risky
Richard> to make that change.

Richard> What's the issue here?

And some recent change (perhaps two or three weeks ago) has made the *first*
child process (after starting emacs) on OSX always fail.  I've learned to work
around it, so I forgot to report it.  But the first of anything (M-!, M-x
shell, etc) just does... nothing.  No error.  But it's a total throw away.

-- 
Randal L. Schwartz - Stonehenge Consulting Services, Inc. - +1 503 777 0095
<merlyn@stonehenge.com> <URL:http://www.stonehenge.com/merlyn/>
Perl/Unix/security consulting, Technical writing, Comedy, etc. etc.
See PerlTraining.Stonehenge.com for onsite and open-enrollment Perl training!

Re: Change in process.c

[Kim F. Storm]

Jan Djärv <jan.h.d@swipnet.se> writes:

> Kim F. Storm skrev:
>> Jan Djärv <jan.h.d@swipnet.se> writes:
>>
>>> I can see this too.  But it is this code that gets executed over and
>>> over again (process.c, line 4810 or so):
>>
>> Good catch!!
>>
>> Does the following patch give good results?
>
> Yes, the problem is completely gone.  Can the original poster (Sam
> Steingold? I forgot) also confirm?

I have committed the changes (including removing the sleep from the
sigchld handler).

Sam, can you pls. check that it fixes the problem for you.

-- 
Kim F. Storm <storm@cua.dk> http://www.cua.dk