[v2] Remove signal handling for nanosleep (bug 16364)

  From: Adhemerval Zanella <adhemerval.zanella@linaro.com>

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.  The issue is likely being fixed before 2.6 and git
history [1] [2].

This patch simplifies the sleep code to call nanosleep directly by
using the posix default version.  It also removes the early cancellation
tests for zero argument, since nanosleep will handle cancellation
in this case.

[1] https://lkml.org/lkml/2004/11/25/5
[2] https://lkml.org/lkml/2003/11/8/50

Checked on x86_64, ppc64le, and aarch64.

	[BZ #16364]
	* sysdeps/unix/sysv/linux/sleep.c: Remove file
	* sysdeps/posix/sleep.c (__sleep): Simplify cancellation handling.
	* posix/tst-nanosleep-signal.c: New file.
	* posix/Makefile (test): Add tst-nanosleep-signal.
---
 posix/Makefile                  |   4 +-
 posix/tst-nanosleep-signal.c    | 113 ++++++++++++++++++++++++++++++
 sysdeps/posix/sleep.c           |   9 ---
 sysdeps/unix/sysv/linux/sleep.c | 149 ----------------------------------------
 5 files changed, 123 insertions(+), 160 deletions(-)
 create mode 100644 posix/tst-nanosleep-signal.c
 delete mode 100644 sysdeps/unix/sysv/linux/sleep.c
  

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
> +   for 0.5s.  */
> +static int
> +check_nanosleep_base (void)
> +{
> +  int ret = 0;
> +  pid_t f = fork ();
> +  if (f == 0)
> +    {
> +      // child
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
> +      nanosleep (&tv, &tv);
> +      _exit (EXIT_SUCCESS);
> +    }
> +  else if (f > 0)
> +    {
> +      // parent
> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
> +      ret = nanosleep (&tv, &tv);
> +    }
> +  else if (f == -1)
> +    {
> +      puts ("error: fork failed");
> +      _exit (EXIT_FAILURE);
> +    }
> +  return ret;
> +}

You should wait for the child before returning.
How do you avoid the race between the parent and the child?

Andreas.
  

On 10-11-2015 11:16, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
>> +   for 0.5s.  */
>> +static int
>> +check_nanosleep_base (void)
>> +{
>> +  int ret = 0;
>> +  pid_t f = fork ();
>> +  if (f == 0)
>> +    {
>> +      // child
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
>> +      nanosleep (&tv, &tv);
>> +      _exit (EXIT_SUCCESS);
>> +    }
>> +  else if (f > 0)
>> +    {
>> +      // parent
>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
>> +      ret = nanosleep (&tv, &tv);
>> +    }
>> +  else if (f == -1)
>> +    {
>> +      puts ("error: fork failed");
>> +      _exit (EXIT_FAILURE);
>> +    }
>> +  return ret;
>> +}
> 
> You should wait for the child before returning.
> How do you avoid the race between the parent and the child?
> 

That's the hole points of the previous discussion in v1 patch while
Florian also pointed this racy.  He neither I could devise a race-free
testcase to check for this issue so my questioning was if someone have
a way to remove the race or if we really should push for this test. 

> Andreas.
>
  

On 10-11-2015 11:29, Adhemerval Zanella wrote:
> 
> 
> On 10-11-2015 11:16, Andreas Schwab wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> +/* Fork and terminate the child in 0.25s while parent waits on a nanosleep
>>> +   for 0.5s.  */
>>> +static int
>>> +check_nanosleep_base (void)
>>> +{
>>> +  int ret = 0;
>>> +  pid_t f = fork ();
>>> +  if (f == 0)
>>> +    {
>>> +      // child
>>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 250000000UL };
>>> +      nanosleep (&tv, &tv);
>>> +      _exit (EXIT_SUCCESS);
>>> +    }
>>> +  else if (f > 0)
>>> +    {
>>> +      // parent
>>> +      struct timespec tv = { .tv_sec = 0, .tv_nsec = 500000000UL };
>>> +      ret = nanosleep (&tv, &tv);
>>> +    }
>>> +  else if (f == -1)
>>> +    {
>>> +      puts ("error: fork failed");
>>> +      _exit (EXIT_FAILURE);
>>> +    }
>>> +  return ret;
>>> +}
>>
>> You should wait for the child before returning.
>> How do you avoid the race between the parent and the child?
>>
> 
> That's the hole points of the previous discussion in v1 patch while

s/hole points/whole point

> Florian also pointed this racy.  He neither I could devise a race-free
> testcase to check for this issue so my questioning was if someone have
> a way to remove the race or if we really should push for this test. 
> 
>> Andreas.
>>
  

Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:

> That's the hole points of the previous discussion in v1 patch while
> Florian also pointed this racy.  He neither I could devise a race-free
> testcase to check for this issue so my questioning was if someone have
> a way to remove the race or if we really should push for this test. 

A racy test is as good as a non-existing test.  Everyone will ignore it.

Andreas.
  

On 11/10/2015 03:22 PM, Andreas Schwab wrote:
> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
> 
>> That's the hole points of the previous discussion in v1 patch while
>> Florian also pointed this racy.  He neither I could devise a race-free
>> testcase to check for this issue so my questioning was if someone have
>> a way to remove the race or if we really should push for this test. 
> 
> A racy test is as good as a non-existing test.  Everyone will ignore it.

I disagree very strongly.  It depends on the frequency of the race, and
it which direction it errs (FAIL even without the bug, or PASS with the
bug).  Some properties are impossible to test without theoretic races.
It really depends on the rate of inappropriate FAILs whether such tests
have value or not.

Florian
  

On 10-11-2015 15:48, Florian Weimer wrote:
> On 11/10/2015 03:22 PM, Andreas Schwab wrote:
>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>
>>> That's the hole points of the previous discussion in v1 patch while
>>> Florian also pointed this racy.  He neither I could devise a race-free
>>> testcase to check for this issue so my questioning was if someone have
>>> a way to remove the race or if we really should push for this test. 
>>
>> A racy test is as good as a non-existing test.  Everyone will ignore it.
> 
> I disagree very strongly.  It depends on the frequency of the race, and
> it which direction it errs (FAIL even without the bug, or PASS with the
> bug).  Some properties are impossible to test without theoretic races.
> It really depends on the rate of inappropriate FAILs whether such tests
> have value or not.

Regarding to this specific test, IMHO I would prefer to not add it since
it clearly a kernel issue which has been fixed in a long time and it is
quite unlike to regress.  Also, any regression would be flagged a kernel
defect and I do not see this being deployed in any kernel release.

Now regarding the racy test, I see we need to assess by case basis. I
also I do not see strong reasoning to block this patch altogether: we
can evaluate/push the version v3 which do not have the testcase and 
if we decide this race-test is valuable I can prepare another patch 
to add it.  

> 
> Florian
>
  

On 11-11-2015 10:42, Adhemerval Zanella wrote:
> 
> 
> On 10-11-2015 15:48, Florian Weimer wrote:
>> On 11/10/2015 03:22 PM, Andreas Schwab wrote:
>>> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes:
>>>
>>>> That's the hole points of the previous discussion in v1 patch while
>>>> Florian also pointed this racy.  He neither I could devise a race-free
>>>> testcase to check for this issue so my questioning was if someone have
>>>> a way to remove the race or if we really should push for this test. 
>>>
>>> A racy test is as good as a non-existing test.  Everyone will ignore it.
>>
>> I disagree very strongly.  It depends on the frequency of the race, and
>> it which direction it errs (FAIL even without the bug, or PASS with the
>> bug).  Some properties are impossible to test without theoretic races.
>> It really depends on the rate of inappropriate FAILs whether such tests
>> have value or not.
> 
> Regarding to this specific test, IMHO I would prefer to not add it since
> it clearly a kernel issue which has been fixed in a long time and it is
> quite unlike to regress.  Also, any regression would be flagged a kernel
> defect and I do not see this being deployed in any kernel release.
> 
> Now regarding the racy test, I see we need to assess by case basis. I
> also I do not see strong reasoning to block this patch altogether: we
> can evaluate/push the version v3 which do not have the testcase and 
> if we decide this race-test is valuable I can prepare another patch 
> to add it.  

It appears this discussion has been stalled and to move forward at least
with the patch core discussion (nanosleep refactor), I would like to
continue discuss solely the code modification at its third version [1].

I will send another patch with the only the testcase and then we can
continue discuss if it worth or not to include possible racy testcase
in GLIBC.

[1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html

> 
>>
>> Florian
>>
  

On 11/17/2015 03:08 PM, Adhemerval Zanella wrote:

> It appears this discussion has been stalled and to move forward at least
> with the patch core discussion (nanosleep refactor), I would like to
> continue discuss solely the code modification at its third version [1].
> 
> I will send another patch with the only the testcase and then we can
> continue discuss if it worth or not to include possible racy testcase
> in GLIBC.
> 
> [1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html

I have no objections to this patch as a first step.  (“Remove file” in
changelog should be followed by a “.”.)

Thanks,
Florian
  

On 17-11-2015 12:18, Florian Weimer wrote:
> On 11/17/2015 03:08 PM, Adhemerval Zanella wrote:
> 
>> It appears this discussion has been stalled and to move forward at least
>> with the patch core discussion (nanosleep refactor), I would like to
>> continue discuss solely the code modification at its third version [1].
>>
>> I will send another patch with the only the testcase and then we can
>> continue discuss if it worth or not to include possible racy testcase
>> in GLIBC.
>>
>> [1] https://sourceware.org/ml/libc-alpha/2015-11/msg00206.html
> 
> I have no objections to this patch as a first step.  (“Remove file” in
> changelog should be followed by a “.”.)

Thanks, I will push the patch and resend the testcase.

> 
> Thanks,
> Florian
>