Remove signal handling for nanosleep (bug 16364)

  On 09-11-2015 10:45, Florian Weimer wrote:
> 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.

What about:

	[BZ #16364]
	* sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use
	nanosleep without requiring to handle spurious wakeups.
	* posix/tst-nanosleep-signal.c: New file.
	* posix/Makefile (test): Add tst-nanosleep-signal.

--
  

On 11/09/2015 03:07 PM, Adhemerval Zanella wrote:

>> 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.
> 
> What about:
> 
> 	[BZ #16364]
> 	* sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use
> 	nanosleep without requiring to handle spurious wakeups.
> 	* posix/tst-nanosleep-signal.c: New file.
> 	* posix/Makefile (test): Add tst-nanosleep-signal.

Thanks, but see below for details.

> +  int ret;
> +  pid_t f = fork ();
> +  if (f == 0)
> +    {
> +      // child
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
> +      nanosleep (&tv, &tv);
> +      exit (0);


This needs to be “_exit (0);”, to avoid running termination handlers twice.

> +    }
> +  else
> +    {
> +      // parent
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
> +      ret = nanosleep (&tv, &tv);
> +    }
> +  return ret;
> +}

The parent needs to check for fork errors.

> +
> +void
> +check_nanosleep_sigdfl(void)

Space before paren.


> +{
> +  /* For SIG_DFL nanosleep should not be interrupted by SIGCHLD.  */
> +  signal(SIGCHLD, SIG_DFL);

Space before paren.

> +  if (check_nanosleep_base ())
> +    {
> +      puts ("error: nanosleep was interrupted with SIG_DFL");
> +      errors = 1;
> +    }
> +}

This should check that errno is EINTR.

> +
> +void
> +check_nanosleep_dummy(void)

Space before paren.

> +{
> +  /* With a dummy handler nanosleep should be interrupted.  */
> +  signal(SIGCHLD, dummy);

Space before paren.

> +  int ret = check_nanosleep_base ();
> +  if (ret == 0)
> +    {
> +      puts ("error: nanosleep was not interrupted with dummy handler");
> +      errors = 1;
> +    }
> +}

Missing EINTR check.

> +
> +void
> +check_nanosleep_sigign(void)

Space before paren.

> +{
> +  /* For SIG_IGN nanosleep should not be interrupted by SIGCHLD.  */
> +  signal(SIGCHLD, SIG_IGN);

Space before paren.

The test is racy in two ways: the child could exit before nanosleep in
the parent starts, or the child exit could be delayed after nanosleep in
the parent ends.  I'm not sure if there is way to make this more reliable.

The larger question is whether the EINTR check is sufficient, or if a
time-based check is needed as well.  That is, if the kernel bug
consistent of silent early termination of nanosleep.

Florian
  

<https://sourceware.org/ml/libc-hacker/1998-11/msg00210.html> has what was 
claimed to be a test for the kernel bug (albeit one with "I believe", not 
asserted to have actually been tested).

I found some claims that nanosleep restarting in some cases was fixed in 
2.6(.0), e.g. <https://lkml.org/lkml/2004/11/25/5> 
<https://lkml.org/lkml/2003/11/8/50>, so before the git history.
  

On 09-11-2015 12:28, Florian Weimer wrote:
> On 11/09/2015 03:07 PM, Adhemerval Zanella wrote:
> 
>>> 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.
>>
>> What about:
>>
>> 	[BZ #16364]
>> 	* sysdeps/unix/sysv/linux/sleep.c (__sleep): Simplify code to use
>> 	nanosleep without requiring to handle spurious wakeups.
>> 	* posix/tst-nanosleep-signal.c: New file.
>> 	* posix/Makefile (test): Add tst-nanosleep-signal.
> 
> Thanks, but see below for details.
> 
>> +  int ret;
>> +  pid_t f = fork ();
>> +  if (f == 0)
>> +    {
>> +      // child
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
>> +      nanosleep (&tv, &tv);
>> +      exit (0);
> 
> 
> This needs to be “_exit (0);”, to avoid running termination handlers twice.
> 

I will change it.

>> +    }
>> +  else
>> +    {
>> +      // parent
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
>> +      ret = nanosleep (&tv, &tv);
>> +    }
>> +  return ret;
>> +}
> 
> The parent needs to check for fork errors.
> 

Right, I will change it.

>> +
>> +void
>> +check_nanosleep_sigdfl(void)
> 
> Space before paren.
> 
> 
>> +{
>> +  /* For SIG_DFL nanosleep should not be interrupted by SIGCHLD.  */
>> +  signal(SIGCHLD, SIG_DFL);
> 
> Space before paren.
> 
>> +  if (check_nanosleep_base ())
>> +    {
>> +      puts ("error: nanosleep was interrupted with SIG_DFL");
>> +      errors = 1;
>> +    }
>> +}
> 
> This should check that errno is EINTR.

I do not think this check is strictly required: the other failures (EFAULT and
EINVAL) should also indicate something very flaky on the system.

> 
>> +
>> +void
>> +check_nanosleep_dummy(void)
> 
> Space before paren.
> 
>> +{
>> +  /* With a dummy handler nanosleep should be interrupted.  */
>> +  signal(SIGCHLD, dummy);
> 
> Space before paren.
> 
>> +  int ret = check_nanosleep_base ();
>> +  if (ret == 0)
>> +    {
>> +      puts ("error: nanosleep was not interrupted with dummy handler");
>> +      errors = 1;
>> +    }
>> +}
> 
> Missing EINTR check.

Also the idea here is to indicate any failure for nanosleep (same as before).

> 
>> +
>> +void
>> +check_nanosleep_sigign(void)
> 
> Space before paren.
> 
>> +{
>> +  /* For SIG_IGN nanosleep should not be interrupted by SIGCHLD.  */
>> +  signal(SIGCHLD, SIG_IGN);
> 
> Space before paren.
> 
> The test is racy in two ways: the child could exit before nanosleep in
> the parent starts, or the child exit could be delayed after nanosleep in
> the parent ends.  I'm not sure if there is way to make this more reliable.

I am aware of that, that's why I try to mitigate this by making the time on
parent twice for the child and some magnitude higher than the syscalls itself.
However I also can't see a way to make this test entirely reliable: even by
doing some synchronization (either by shared semaphores or pthread barrier)
and/or sending the SIGCHLD directly using signal the two race scenarios you 
describe will still have a small window to occur.  I am not sure which 
strategy will be better and I think we should not rely or add hacks to try
to mitigate for such kernel failures.

> 
> The larger question is whether the EINTR check is sufficient, or if a
> time-based check is needed as well.  That is, if the kernel bug
> consistent of silent early termination of nanosleep.

My understanding is on old kernels the nanosleep calls was not restarted
in a nanosleep call (with the restart_syscall), so it nanosleep will
early terminate.

> 
> Florian
>
  

On 11/09/2015 05:30 PM, Adhemerval Zanella wrote:

> I do not think this check is strictly required: the other failures (EFAULT and
> EINVAL) should also indicate something very flaky on the system.

We should always check for unexpected errors.  We might have been
overlooking something.

>> The test is racy in two ways: the child could exit before nanosleep in
>> the parent starts, or the child exit could be delayed after nanosleep in
>> the parent ends.  I'm not sure if there is way to make this more reliable.
> 
> I am aware of that, that's why I try to mitigate this by making the time on
> parent twice for the child and some magnitude higher than the syscalls itself.
> However I also can't see a way to make this test entirely reliable: even by
> doing some synchronization (either by shared semaphores or pthread barrier)
> and/or sending the SIGCHLD directly using signal the two race scenarios you 
> describe will still have a small window to occur.  I am not sure which 
> strategy will be better and I think we should not rely or add hacks to try
> to mitigate for such kernel failures.

I think you could make it more likely to hit the window if you forked
several child processes.

>> The larger question is whether the EINTR check is sufficient, or if a
>> time-based check is needed as well.  That is, if the kernel bug
>> consistent of silent early termination of nanosleep.
> 
> My understanding is on old kernels the nanosleep calls was not restarted
> in a nanosleep call (with the restart_syscall), so it nanosleep will
> early terminate.

To be sure, you should check how much time has elapsed in the nanosleep
calls, with clock_gettime(CLOCK_MONOTONIC).  This would cover both the
-1/EINTR case and the return value 0 case.  The existing code does not
look at EINTR, so if there was a bug, it was the return value 0 scenario.

Florian
  

On 09-11-2015 15:18, Florian Weimer wrote:
> On 11/09/2015 05:30 PM, Adhemerval Zanella wrote:
> 
>> I do not think this check is strictly required: the other failures (EFAULT and
>> EINVAL) should also indicate something very flaky on the system.
> 
> We should always check for unexpected errors.  We might have been
> overlooking something.
> 

Fair enough, I will change that.

>>> The test is racy in two ways: the child could exit before nanosleep in
>>> the parent starts, or the child exit could be delayed after nanosleep in
>>> the parent ends.  I'm not sure if there is way to make this more reliable.
>>
>> I am aware of that, that's why I try to mitigate this by making the time on
>> parent twice for the child and some magnitude higher than the syscalls itself.
>> However I also can't see a way to make this test entirely reliable: even by
>> doing some synchronization (either by shared semaphores or pthread barrier)
>> and/or sending the SIGCHLD directly using signal the two race scenarios you 
>> describe will still have a small window to occur.  I am not sure which 
>> strategy will be better and I think we should not rely or add hacks to try
>> to mitigate for such kernel failures.
> 
> I think you could make it more likely to hit the window if you forked
> several child processes.

Or if the system load is high enough.  My question is if we should push or
not this test even with this racy condition and if so which strategy would
be better to mitigate it.

> 
>>> The larger question is whether the EINTR check is sufficient, or if a
>>> time-based check is needed as well.  That is, if the kernel bug
>>> consistent of silent early termination of nanosleep.
>>
>> My understanding is on old kernels the nanosleep calls was not restarted
>> in a nanosleep call (with the restart_syscall), so it nanosleep will
>> early terminate.
> 
> To be sure, you should check how much time has elapsed in the nanosleep
> calls, with clock_gettime(CLOCK_MONOTONIC).  This would cover both the
> -1/EINTR case and the return value 0 case.  The existing code does not
> look at EINTR, so if there was a bug, it was the return value 0 scenario.

For nanosleep check itself, glibc already have posix/tst-nanosleep.c 
(which uses gettimeofday.c).

However my main questioning and the reason of this bikeshedding regarding
is if GLIBC should continue to provide a hack for non-compliant kernels
and if we should really check for this issue with a possible racy testcase.

> 
> Florian
> 
>
  

On 09-11-2015 14:19, Joseph Myers wrote:
> <https://sourceware.org/ml/libc-hacker/1998-11/msg00210.html> has what was 
> claimed to be a test for the kernel bug (albeit one with "I believe", not 
> asserted to have actually been tested).

This test is essentially what I have added, it just spawn more children
with different sleep periods.

> 
> I found some claims that nanosleep restarting in some cases was fixed in 
> 2.6(.0), e.g. <https://lkml.org/lkml/2004/11/25/5> 
> <https://lkml.org/lkml/2003/11/8/50>, so before the git history.
> 

Thanks, I tried to track down the kernel source which handles the nanosleep
(kernel/posix-cpu-timers.c), but found it has been fixed also before git
history.

I will resend the patch with this information.