diff mbox

Remove signal handling for nanosleep (bug 16364)

Message ID 1447023171-31542-1-git-send-email-adhemerval.zanella@linaro.com
State New
Headers show

Commit Message

Adhemerval Zanella Netto Nov. 8, 2015, 10:52 p.m. UTC
Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD
vs. SIG_IGN for nanosleep which make it feasible to use it for sleep
implementation without requiring any hacking to handle the spurious
wake up.  This patch simplifies the sleep code to call nanosleep
directly.

Checked on x86_64, ppc64le, and aarch64.

	[BZ #16364]
	* sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use
	nanosleep without requiring to handle spurious wakeups.
---
 sysdeps/unix/sysv/linux/sleep.c | 127 ++--------------------------------------
 2 files changed, 11 insertions(+), 122 deletions(-)

Comments

Florian Weimer Nov. 9, 2015, 7:53 a.m. UTC | #1
On 11/08/2015 11:52 PM, Adhemerval Zanella wrote:
> Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD
> vs. SIG_IGN for nanosleep which make it feasible to use it for sleep
> implementation without requiring any hacking to handle the spurious
> wake up.  This patch simplifies the sleep code to call nanosleep
> directly.
> 
> Checked on x86_64, ppc64le, and aarch64.

Do you know the kernel commit which fixed this?

Thanks,
Florian
Adhemerval Zanella Netto Nov. 9, 2015, 12:18 p.m. UTC | #2
On 09-11-2015 05:53, Florian Weimer wrote:
> On 11/08/2015 11:52 PM, Adhemerval Zanella wrote:
>> Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD
>> vs. SIG_IGN for nanosleep which make it feasible to use it for sleep
>> implementation without requiring any hacking to handle the spurious
>> wake up.  This patch simplifies the sleep code to call nanosleep
>> directly.
>>
>> Checked on x86_64, ppc64le, and aarch64.
> 
> Do you know the kernel commit which fixed this?

I tried to track down the specific commit that fixed it, but I was unable
to pin it down.  uClibc developers also seemed to do same [1], but also
they could not find out the exact commit (which I think might be the reason
they are using glibc strategy still). musl uses plain nanosleep as the
patch.

[1] http://git.uclibc.org/uClibc/tree/libc/unistd/sleep.c

> 
> Thanks,
> Florian
>
Florian Weimer Nov. 9, 2015, 12:45 p.m. UTC | #3
On 11/09/2015 01:18 PM, Adhemerval Zanella wrote:
> 
> 
> On 09-11-2015 05:53, Florian Weimer wrote:
>> On 11/08/2015 11:52 PM, Adhemerval Zanella wrote:
>>> Linux 2.6.32 and forward do not show the issue regarding SysV SIGCHLD
>>> vs. SIG_IGN for nanosleep which make it feasible to use it for sleep
>>> implementation without requiring any hacking to handle the spurious
>>> wake up.  This patch simplifies the sleep code to call nanosleep
>>> directly.
>>>
>>> Checked on x86_64, ppc64le, and aarch64.
>>
>> Do you know the kernel commit which fixed this?
> 
> I tried to track down the specific commit that fixed it, but I was unable
> to pin it down.  uClibc developers also seemed to do same [1], but also
> they could not find out the exact commit (which I think might be the reason
> they are using glibc strategy still). musl uses plain nanosleep as the
> patch.
> 
> [1] http://git.uclibc.org/uClibc/tree/libc/unistd/sleep.c

Could you write a glibc test case for this?  I don't feel comfortable
with removing the workaround without a test, and I can't find an
existing one.

Thanks,
Florian
Roland McGrath Nov. 9, 2015, 7:03 p.m. UTC | #4
Please start with just removing sysdeps/unix/sysv/linux/sleep.c altogether.
Then sysdeps/posix/sleep.c will be used.  After that, if the code in that
file can be simplified further, do that separately.
diff mbox

Patch

diff --git a/sysdeps/unix/sysv/linux/sleep.c b/sysdeps/unix/sysv/linux/sleep.c
index 2a06d5a..3885a34 100644
--- a/sysdeps/unix/sysv/linux/sleep.c
+++ b/sysdeps/unix/sysv/linux/sleep.c
@@ -17,133 +17,16 @@ 
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
 #include <time.h>
-#include <signal.h>
-#include <string.h>	/* For the real memset prototype.  */
 #include <unistd.h>
-#include <sys/param.h>
-#include <nptl/pthreadP.h>
+#include <sysdep-cancel.h>
 
-
-#if 0
-static void
-cl (void *arg)
-{
-  (void) __sigprocmask (SIG_SETMASK, arg, (sigset_t *) NULL);
-}
-#endif
-
-
-/* We are going to use the `nanosleep' syscall of the kernel.  But the
-   kernel does not implement the stupid SysV SIGCHLD vs. SIG_IGN
-   behaviour for this syscall.  Therefore we have to emulate it here.  */
 unsigned int
 __sleep (unsigned int seconds)
 {
-  const unsigned int max
-    = (unsigned int) (((unsigned long int) (~((time_t) 0))) >> 1);
-  struct timespec ts;
-  sigset_t set, oset;
-  unsigned int result;
-
-  /* This is not necessary but some buggy programs depend on this.  */
-  if (__glibc_unlikely (seconds == 0))
-    {
-#ifdef CANCELLATION_P
-      CANCELLATION_P (THREAD_SELF);
-#endif
-      return 0;
-    }
-
-  ts.tv_sec = 0;
-  ts.tv_nsec = 0;
- again:
-  if (sizeof (ts.tv_sec) <= sizeof (seconds))
-    {
-      /* Since SECONDS is unsigned assigning the value to .tv_sec can
-	 overflow it.  In this case we have to wait in steps.  */
-      ts.tv_sec += MIN (seconds, max);
-      seconds -= (unsigned int) ts.tv_sec;
-    }
-  else
-    {
-      ts.tv_sec = (time_t) seconds;
-      seconds = 0;
-    }
-
-  /* Linux will wake up the system call, nanosleep, when SIGCHLD
-     arrives even if SIGCHLD is ignored.  We have to deal with it
-     in libc.  We block SIGCHLD first.  */
-  __sigemptyset (&set);
-  __sigaddset (&set, SIGCHLD);
-  if (__sigprocmask (SIG_BLOCK, &set, &oset))
-    return -1;
-
-  /* If SIGCHLD is already blocked, we don't have to do anything.  */
-  if (!__sigismember (&oset, SIGCHLD))
-    {
-      int saved_errno;
-      struct sigaction oact;
-
-      __sigemptyset (&set);
-      __sigaddset (&set, SIGCHLD);
-
-      /* We get the signal handler for SIGCHLD.  */
-      if (__sigaction (SIGCHLD, (struct sigaction *) NULL, &oact) < 0)
-	{
-	  saved_errno = errno;
-	  /* Restore the original signal mask.  */
-	  (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-	  __set_errno (saved_errno);
-	  return -1;
-	}
-
-      /* Note the sleep() is a cancellation point.  But since we call
-	 nanosleep() which itself is a cancellation point we do not
-	 have to do anything here.  */
-      if (oact.sa_handler == SIG_IGN)
-	{
-	  //__libc_cleanup_push (cl, &oset);
-
-	  /* We should leave SIGCHLD blocked.  */
-	  while (1)
-	    {
-	      result = __nanosleep (&ts, &ts);
-
-	      if (result != 0 || seconds == 0)
-		break;
-
-	      if (sizeof (ts.tv_sec) <= sizeof (seconds))
-		{
-		  ts.tv_sec = MIN (seconds, max);
-		  seconds -= (unsigned int) ts.tv_nsec;
-		}
-	    }
-
-	  //__libc_cleanup_pop (0);
-
-	  saved_errno = errno;
-	  /* Restore the original signal mask.  */
-	  (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-	  __set_errno (saved_errno);
-
-	  goto out;
-	}
-
-      /* We should unblock SIGCHLD.  Restore the original signal mask.  */
-      (void) __sigprocmask (SIG_SETMASK, &oset, (sigset_t *) NULL);
-    }
-
-  result = __nanosleep (&ts, &ts);
-  if (result == 0 && seconds != 0)
-    goto again;
-
- out:
-  if (result != 0)
-    /* Round remaining time.  */
-    result = seconds + (unsigned int) ts.tv_sec + (ts.tv_nsec >= 500000000L);
-
-  return result;
+  struct timespec ts = { .tv_sec = seconds, .tv_nsec = 0 };
+  if (__nanosleep (&ts, &ts))
+    return ts.tv_sec;
+  return 0;
 }
 weak_alias (__sleep, sleep)