diff mbox series

[2/3] Refactor nanosleep in terms of clock_nanosleep

Message ID 20191106125245.28102-2-adhemerval.zanella@linaro.org
State Accepted
Commit 3537ecb49cf7177274607004c562d6f9ecc99474
Headers show
Series [1/3] nptl: Move nanosleep implementation to libc | expand

Commit Message

Adhemerval Zanella Netto Nov. 6, 2019, 12:52 p.m. UTC
The generic version is straightforward.  For Hurd, its nanosleep
implementation is moved to clock_nanosleep with adjustments from
generic unix implementation.

The generic clock_nanosleep unix version is also removed since
it calls nanosleep.

Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.
---
 include/time.h                            |  3 +
 posix/nanosleep.c                         | 13 ++--
 sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------
 sysdeps/mach/nanosleep.c                  | 79 -----------------------
 sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-
 sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------
 time/clock_nanosleep.c                    |  2 +-
 7 files changed, 71 insertions(+), 138 deletions(-)
 rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)
 delete mode 100644 sysdeps/mach/nanosleep.c
 delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

-- 
2.17.1

Comments

Florian Weimer Nov. 6, 2019, 1:08 p.m. UTC | #1
* Adhemerval Zanella:

> +++ b/sysdeps/mach/clock_nanosleep.c


> +static struct timespec

> +timespec_sub (struct timespec a, struct timespec b)

> +{

> +  struct timespec result = { a.tv_sec - b.tv_sec,

> +			     a.tv_nsec - b.tv_nsec };

> +  if (result.tv_nsec < 0)

> +    {

> +      --result.tv_sec;

> +      result.tv_nsec += 1000000000ul;

> +    }

> +  return result;

> +}


Can you use the version in <posix-timers.h>?

Thanks,
Florian
Adhemerval Zanella Netto Nov. 6, 2019, 2:17 p.m. UTC | #2
On 06/11/2019 10:08, Florian Weimer wrote:
> * Adhemerval Zanella:

> 

>> +++ b/sysdeps/mach/clock_nanosleep.c

> 

>> +static struct timespec

>> +timespec_sub (struct timespec a, struct timespec b)

>> +{

>> +  struct timespec result = { a.tv_sec - b.tv_sec,

>> +			     a.tv_nsec - b.tv_nsec };

>> +  if (result.tv_nsec < 0)

>> +    {

>> +      --result.tv_sec;

>> +      result.tv_nsec += 1000000000ul;

>> +    }

>> +  return result;

>> +}

> 

> Can you use the version in <posix-timers.h>?


Ack.
Dmitry V. Levin Nov. 28, 2019, 11:44 p.m. UTC | #3
On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:
> The generic version is straightforward.  For Hurd, its nanosleep

> implementation is moved to clock_nanosleep with adjustments from

> generic unix implementation.

> 

> The generic clock_nanosleep unix version is also removed since

> it calls nanosleep.

> 

> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.

> ---

>  include/time.h                            |  3 +

>  posix/nanosleep.c                         | 13 ++--

>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------

>  sysdeps/mach/nanosleep.c                  | 79 -----------------------

>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-

>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------

>  time/clock_nanosleep.c                    |  2 +-

>  7 files changed, 71 insertions(+), 138 deletions(-)

>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)

>  delete mode 100644 sysdeps/mach/nanosleep.c

>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

> 

> diff --git a/include/time.h b/include/time.h

> index 8ac58e891b..b3e635395d 100644

> --- a/include/time.h

> +++ b/include/time.h

> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)

>  extern __typeof (clock_settime) __clock_settime;

>  libc_hidden_proto (__clock_settime)

>  

> +extern __typeof (clock_nanosleep) __clock_nanosleep;

> +libc_hidden_proto (__clock_nanosleep);

> +

>  #ifdef __linux__

>  extern __typeof (clock_adjtime) __clock_adjtime;

>  libc_hidden_proto (__clock_adjtime);

> diff --git a/posix/nanosleep.c b/posix/nanosleep.c

> index d8564c7119..ed41c8cce7 100644

> --- a/posix/nanosleep.c

> +++ b/posix/nanosleep.c

> @@ -24,10 +24,13 @@ int

>  __nanosleep (const struct timespec *requested_time,

>  	     struct timespec *remaining)

>  {

> -  __set_errno (ENOSYS);

> -  return -1;

> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);


Shouldn't it set flags to TIMER_ABSTIME?

In the current form it causes a regression when the syscall is interrupted
by signal - "remaining" is updated by the kernel but __clock_nanosleep
leaves it unchanged.


-- 
ldv
Adhemerval Zanella Netto Nov. 29, 2019, 11:27 a.m. UTC | #4
On 28/11/2019 20:44, Dmitry V. Levin wrote:
> On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:

>> The generic version is straightforward.  For Hurd, its nanosleep

>> implementation is moved to clock_nanosleep with adjustments from

>> generic unix implementation.

>>

>> The generic clock_nanosleep unix version is also removed since

>> it calls nanosleep.

>>

>> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.

>> ---

>>  include/time.h                            |  3 +

>>  posix/nanosleep.c                         | 13 ++--

>>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------

>>  sysdeps/mach/nanosleep.c                  | 79 -----------------------

>>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-

>>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------

>>  time/clock_nanosleep.c                    |  2 +-

>>  7 files changed, 71 insertions(+), 138 deletions(-)

>>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)

>>  delete mode 100644 sysdeps/mach/nanosleep.c

>>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

>>

>> diff --git a/include/time.h b/include/time.h

>> index 8ac58e891b..b3e635395d 100644

>> --- a/include/time.h

>> +++ b/include/time.h

>> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)

>>  extern __typeof (clock_settime) __clock_settime;

>>  libc_hidden_proto (__clock_settime)

>>  

>> +extern __typeof (clock_nanosleep) __clock_nanosleep;

>> +libc_hidden_proto (__clock_nanosleep);

>> +

>>  #ifdef __linux__

>>  extern __typeof (clock_adjtime) __clock_adjtime;

>>  libc_hidden_proto (__clock_adjtime);

>> diff --git a/posix/nanosleep.c b/posix/nanosleep.c

>> index d8564c7119..ed41c8cce7 100644

>> --- a/posix/nanosleep.c

>> +++ b/posix/nanosleep.c

>> @@ -24,10 +24,13 @@ int

>>  __nanosleep (const struct timespec *requested_time,

>>  	     struct timespec *remaining)

>>  {

>> -  __set_errno (ENOSYS);

>> -  return -1;

>> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);

> 

> Shouldn't it set flags to TIMER_ABSTIME?

> 

> In the current form it causes a regression when the syscall is interrupted

> by signal - "remaining" is updated by the kernel but __clock_nanosleep

> leaves it unchanged.


I am not seeing this regression:

$ cat test.c 
#include <stdio.h>
#include <time.h>
#include <unistd.h>
#include <signal.h>
#include <stdint.h>

void sigalrm_handler (int s)
{
}

int main ()
{
  {
    signal (SIGALRM, SIG_IGN);
    alarm (1);
    struct timespec r = { 0, 0 };
    nanosleep (& (struct timespec) { 2, 0 }, &r);
    printf ("SIG_IGN: %ju.%09ju\n", (uintmax_t) r.tv_sec, (uintmax_t) r.tv_nsec);
  }

  {
    signal (SIGALRM, sigalrm_handler);
    alarm (1);
    struct timespec r = { 0, 0 };
    nanosleep (& (struct timespec) { 2, 0 }, &r);
    printf ("handler: %ju.%09ju\n", (uintmax_t) r.tv_sec, (uintmax_t) r.tv_nsec);
  }

  return 0;
}
$ gcc -Wall test.c -o test -m64
$ ldd test
        linux-vdso.so.1 (0x00007ffcd2bee000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fdf26866000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fdf26e59000)
$ /lib/x86_64-linux-gnu/libc.so.6
GNU C Library (Ubuntu GLIBC 2.27-3ubuntu1) stable release version 2.27.
[...]
$ ./test
SIG_IGN: 0.000000000
handler: 1.000018272
$ ./libc.so
GNU C Library (GNU libc) development release version 2.30.9000.
[..]
$ ./testrun.sh ./test
SIG_IGN: 0.000000000
handler: 1.000043369

Same for i686-linux-gnu running on a 4.15 kernel (which calls clock_gettime)
and 5.3 (which calls clock_gettime_time64).

My understanding is TIMER_ABSTIME *does not* update the timer in case of
a interruption:

kernel/time/posix-timers.c

1208 SYSCALL_DEFINE4(clock_nanosleep, const clockid_t, which_clock, int, flags,
1209                 const struct __kernel_timespec __user *, rqtp,
1210                 struct __kernel_timespec __user *, rmtp)
1211 {
1212         const struct k_clock *kc = clockid_to_kclock(which_clock);
1213         struct timespec64 t;
1214 
1215         if (!kc)
1216                 return -EINVAL;
1217         if (!kc->nsleep)
1218                 return -EOPNOTSUPP;
1219 
1220         if (get_timespec64(&t, rqtp))
1221                 return -EFAULT;
1222 
1223         if (!timespec64_valid(&t))
1224                 return -EINVAL;
1225         if (flags & TIMER_ABSTIME)
1226                 rmtp = NULL;
Dmitry V. Levin Nov. 29, 2019, 3:21 p.m. UTC | #5
On Fri, Nov 29, 2019 at 08:27:06AM -0300, Adhemerval Zanella wrote:
> On 28/11/2019 20:44, Dmitry V. Levin wrote:

> > On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:

> >> The generic version is straightforward.  For Hurd, its nanosleep

> >> implementation is moved to clock_nanosleep with adjustments from

> >> generic unix implementation.

> >>

> >> The generic clock_nanosleep unix version is also removed since

> >> it calls nanosleep.

> >>

> >> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.

> >> ---

> >>  include/time.h                            |  3 +

> >>  posix/nanosleep.c                         | 13 ++--

> >>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------

> >>  sysdeps/mach/nanosleep.c                  | 79 -----------------------

> >>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-

> >>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------

> >>  time/clock_nanosleep.c                    |  2 +-

> >>  7 files changed, 71 insertions(+), 138 deletions(-)

> >>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)

> >>  delete mode 100644 sysdeps/mach/nanosleep.c

> >>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

> >>

> >> diff --git a/include/time.h b/include/time.h

> >> index 8ac58e891b..b3e635395d 100644

> >> --- a/include/time.h

> >> +++ b/include/time.h

> >> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)

> >>  extern __typeof (clock_settime) __clock_settime;

> >>  libc_hidden_proto (__clock_settime)

> >>  

> >> +extern __typeof (clock_nanosleep) __clock_nanosleep;

> >> +libc_hidden_proto (__clock_nanosleep);

> >> +

> >>  #ifdef __linux__

> >>  extern __typeof (clock_adjtime) __clock_adjtime;

> >>  libc_hidden_proto (__clock_adjtime);

> >> diff --git a/posix/nanosleep.c b/posix/nanosleep.c

> >> index d8564c7119..ed41c8cce7 100644

> >> --- a/posix/nanosleep.c

> >> +++ b/posix/nanosleep.c

> >> @@ -24,10 +24,13 @@ int

> >>  __nanosleep (const struct timespec *requested_time,

> >>  	     struct timespec *remaining)

> >>  {

> >> -  __set_errno (ENOSYS);

> >> -  return -1;

> >> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);

> > 

> > Shouldn't it set flags to TIMER_ABSTIME?

> > 

> > In the current form it causes a regression when the syscall is interrupted

> > by signal - "remaining" is updated by the kernel but __clock_nanosleep

> > leaves it unchanged.

> 

> I am not seeing this regression:


The regression has been detected by the strace test suite on Rawhide,
the log is at
https://kojipkgs.fedoraproject.org/work/tasks/1866/39391866/build.log
(look for "FAIL: restart_syscall" there), the test is
https://github.com/strace/strace/blob/master/tests/restart_syscall.c


-- 
ldv
Dmitry V. Levin Nov. 29, 2019, 5:49 p.m. UTC | #6
On Fri, Nov 29, 2019 at 06:21:53PM +0300, Dmitry V. Levin wrote:
> On Fri, Nov 29, 2019 at 08:27:06AM -0300, Adhemerval Zanella wrote:

> > On 28/11/2019 20:44, Dmitry V. Levin wrote:

> > > On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:

> > >> The generic version is straightforward.  For Hurd, its nanosleep

> > >> implementation is moved to clock_nanosleep with adjustments from

> > >> generic unix implementation.

> > >>

> > >> The generic clock_nanosleep unix version is also removed since

> > >> it calls nanosleep.

> > >>

> > >> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.

> > >> ---

> > >>  include/time.h                            |  3 +

> > >>  posix/nanosleep.c                         | 13 ++--

> > >>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------

> > >>  sysdeps/mach/nanosleep.c                  | 79 -----------------------

> > >>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-

> > >>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------

> > >>  time/clock_nanosleep.c                    |  2 +-

> > >>  7 files changed, 71 insertions(+), 138 deletions(-)

> > >>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)

> > >>  delete mode 100644 sysdeps/mach/nanosleep.c

> > >>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

> > >>

> > >> diff --git a/include/time.h b/include/time.h

> > >> index 8ac58e891b..b3e635395d 100644

> > >> --- a/include/time.h

> > >> +++ b/include/time.h

> > >> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)

> > >>  extern __typeof (clock_settime) __clock_settime;

> > >>  libc_hidden_proto (__clock_settime)

> > >>  

> > >> +extern __typeof (clock_nanosleep) __clock_nanosleep;

> > >> +libc_hidden_proto (__clock_nanosleep);

> > >> +

> > >>  #ifdef __linux__

> > >>  extern __typeof (clock_adjtime) __clock_adjtime;

> > >>  libc_hidden_proto (__clock_adjtime);

> > >> diff --git a/posix/nanosleep.c b/posix/nanosleep.c

> > >> index d8564c7119..ed41c8cce7 100644

> > >> --- a/posix/nanosleep.c

> > >> +++ b/posix/nanosleep.c

> > >> @@ -24,10 +24,13 @@ int

> > >>  __nanosleep (const struct timespec *requested_time,

> > >>  	     struct timespec *remaining)

> > >>  {

> > >> -  __set_errno (ENOSYS);

> > >> -  return -1;

> > >> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);

> > > 

> > > Shouldn't it set flags to TIMER_ABSTIME?

> > > 

> > > In the current form it causes a regression when the syscall is interrupted

> > > by signal - "remaining" is updated by the kernel but __clock_nanosleep

> > > leaves it unchanged.

> > 

> > I am not seeing this regression:

> 

> The regression has been detected by the strace test suite on Rawhide,

> the log is at

> https://kojipkgs.fedoraproject.org/work/tasks/1866/39391866/build.log

> (look for "FAIL: restart_syscall" there), the test is

> https://github.com/strace/strace/blob/master/tests/restart_syscall.c


Here is a stripped down example:
$ cat nanorem.c
#include <err.h>
#include <signal.h>
#include <stdint.h>
#include <stdio.h>
#include <sys/time.h>
#include <time.h>

int
main(void)
{
	if (signal(SIGALRM, SIG_IGN))
		err(1, "signal");

	const sigset_t set = {};
	if (sigprocmask(SIG_SETMASK, &set, NULL))
		err(1, "sigprocmask");

	const struct itimerval itv = { .it_value.tv_usec = 111111 };
	const struct timespec req = { .tv_nsec = 222222222 };
	const struct timespec bad = { 0xbadc0ded, 0xbadc0ded };

	struct timespec rem = bad;
	if (setitimer(ITIMER_REAL, &itv, NULL))
		err(1, "setitimer");
	if (nanosleep(&req, &rem))
		err(1, "nanosleep");
	printf("nanosleep: tv_sec=%lld, tv_nsec=%llu\n",
	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

	rem = bad;
	if (setitimer(ITIMER_REAL, &itv, NULL))
		err(1, "setitimer");
	if (clock_nanosleep(CLOCK_REALTIME, 0, &req, &rem))
		err(1, "clock_nanosleep");
	printf("clock_nanosleep: tv_sec=%lld, tv_nsec=%llu\n",
	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

	return 0;
}
$ gcc -Wall -O2 nanorem.c -o nanorem
$ strace -qq -enone -esignal=none ./nanorem
nanosleep: tv_sec=0, tv_nsec=111185098
clock_nanosleep: tv_sec=0, tv_nsec=111173210

Apparently, the output for nanosleep is no longer the same after the
change.


-- 
ldv
Adhemerval Zanella Netto Nov. 29, 2019, 8:33 p.m. UTC | #7
On 29/11/2019 14:49, Dmitry V. Levin wrote:
> On Fri, Nov 29, 2019 at 06:21:53PM +0300, Dmitry V. Levin wrote:

>> On Fri, Nov 29, 2019 at 08:27:06AM -0300, Adhemerval Zanella wrote:

>>> On 28/11/2019 20:44, Dmitry V. Levin wrote:

>>>> On Wed, Nov 06, 2019 at 09:52:44AM -0300, Adhemerval Zanella wrote:

>>>>> The generic version is straightforward.  For Hurd, its nanosleep

>>>>> implementation is moved to clock_nanosleep with adjustments from

>>>>> generic unix implementation.

>>>>>

>>>>> The generic clock_nanosleep unix version is also removed since

>>>>> it calls nanosleep.

>>>>>

>>>>> Checked on x86_64-linux-gnu and powerpc64le-linux-gnu.

>>>>> ---

>>>>>  include/time.h                            |  3 +

>>>>>  posix/nanosleep.c                         | 13 ++--

>>>>>  sysdeps/{unix => mach}/clock_nanosleep.c  | 79 +++++++++++++++++------

>>>>>  sysdeps/mach/nanosleep.c                  | 79 -----------------------

>>>>>  sysdeps/unix/sysv/linux/clock_nanosleep.c |  2 +-

>>>>>  sysdeps/unix/sysv/linux/nanosleep.c       | 31 ---------

>>>>>  time/clock_nanosleep.c                    |  2 +-

>>>>>  7 files changed, 71 insertions(+), 138 deletions(-)

>>>>>  rename sysdeps/{unix => mach}/clock_nanosleep.c (59%)

>>>>>  delete mode 100644 sysdeps/mach/nanosleep.c

>>>>>  delete mode 100644 sysdeps/unix/sysv/linux/nanosleep.c

>>>>>

>>>>> diff --git a/include/time.h b/include/time.h

>>>>> index 8ac58e891b..b3e635395d 100644

>>>>> --- a/include/time.h

>>>>> +++ b/include/time.h

>>>>> @@ -25,6 +25,9 @@ libc_hidden_proto (__clock_gettime)

>>>>>  extern __typeof (clock_settime) __clock_settime;

>>>>>  libc_hidden_proto (__clock_settime)

>>>>>  

>>>>> +extern __typeof (clock_nanosleep) __clock_nanosleep;

>>>>> +libc_hidden_proto (__clock_nanosleep);

>>>>> +

>>>>>  #ifdef __linux__

>>>>>  extern __typeof (clock_adjtime) __clock_adjtime;

>>>>>  libc_hidden_proto (__clock_adjtime);

>>>>> diff --git a/posix/nanosleep.c b/posix/nanosleep.c

>>>>> index d8564c7119..ed41c8cce7 100644

>>>>> --- a/posix/nanosleep.c

>>>>> +++ b/posix/nanosleep.c

>>>>> @@ -24,10 +24,13 @@ int

>>>>>  __nanosleep (const struct timespec *requested_time,

>>>>>  	     struct timespec *remaining)

>>>>>  {

>>>>> -  __set_errno (ENOSYS);

>>>>> -  return -1;

>>>>> +  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);

>>>>

>>>> Shouldn't it set flags to TIMER_ABSTIME?

>>>>

>>>> In the current form it causes a regression when the syscall is interrupted

>>>> by signal - "remaining" is updated by the kernel but __clock_nanosleep

>>>> leaves it unchanged.

>>>

>>> I am not seeing this regression:

>>

>> The regression has been detected by the strace test suite on Rawhide,

>> the log is at

>> https://kojipkgs.fedoraproject.org/work/tasks/1866/39391866/build.log

>> (look for "FAIL: restart_syscall" there), the test is

>> https://github.com/strace/strace/blob/master/tests/restart_syscall.c

> 

> Here is a stripped down example:

> $ cat nanorem.c

> #include <err.h>

> #include <signal.h>

> #include <stdint.h>

> #include <stdio.h>

> #include <sys/time.h>

> #include <time.h>

> 

> int

> main(void)

> {

> 	if (signal(SIGALRM, SIG_IGN))

> 		err(1, "signal");

> 

> 	const sigset_t set = {};

> 	if (sigprocmask(SIG_SETMASK, &set, NULL))

> 		err(1, "sigprocmask");

> 

> 	const struct itimerval itv = { .it_value.tv_usec = 111111 };

> 	const struct timespec req = { .tv_nsec = 222222222 };

> 	const struct timespec bad = { 0xbadc0ded, 0xbadc0ded };

> 

> 	struct timespec rem = bad;

> 	if (setitimer(ITIMER_REAL, &itv, NULL))

> 		err(1, "setitimer");

> 	if (nanosleep(&req, &rem))

> 		err(1, "nanosleep");

> 	printf("nanosleep: tv_sec=%lld, tv_nsec=%llu\n",

> 	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

> 

> 	rem = bad;

> 	if (setitimer(ITIMER_REAL, &itv, NULL))

> 		err(1, "setitimer");

> 	if (clock_nanosleep(CLOCK_REALTIME, 0, &req, &rem))

> 		err(1, "clock_nanosleep");

> 	printf("clock_nanosleep: tv_sec=%lld, tv_nsec=%llu\n",

> 	       (long long) rem.tv_sec, (unsigned long long) rem.tv_nsec);

> 

> 	return 0;

> }

> $ gcc -Wall -O2 nanorem.c -o nanorem

> $ strace -qq -enone -esignal=none ./nanorem

> nanosleep: tv_sec=0, tv_nsec=111185098

> clock_nanosleep: tv_sec=0, tv_nsec=111173210

> 

> Apparently, the output for nanosleep is no longer the same after the

> change.


I am not seeing any change here., using the same environment 
as before (ubuntu 18, glibc 2.27, kernel 4.15, x86_64):

$ strace -qq -enone -esignal=none ./nanorem
nanosleep: tv_sec=0, tv_nsec=111162421
clock_nanosleep: tv_sec=0, tv_nsec=111250560
$ strace -qq -enone -esignal=none ./elf/ld-linux-x86-64.so.2 --library-path .:./math:./elf:./dlfcn:./nss:./nis:./rt:./resolv:./mathvec:./support:./crypt:./nptl ./nanorem
nanosleep: tv_sec=0, tv_nsec=111153506
clock_nanosleep: tv_sec=0, tv_nsec=111153154

Neither on i686-linux-gnu (kernel 4.15 and kernel 5.4).  

My guess is what might be characterized as a regression is although POSIX 
states that nanosleep should be measured as CLOCK_REALTIME [1], Linux in the
other hand uses CLOCK_MONOTONIC:

kernel/time/hrtimer.c
1945 SYSCALL_DEFINE2(nanosleep, struct __kernel_timespec __user *, rqtp,
1946                 struct __kernel_timespec __user *, rmtp)
1947 {
[...]
1958         return hrtimer_nanosleep(&tu, HRTIMER_MODE_REL, CLOCK_MONOTONIC);
1959 }

Not sure if this is worth to add a compat symbol.

[1] https://pubs.opengroup.org/onlinepubs/9699919799/functions/nanosleep.html
diff mbox series

Patch

diff --git a/include/time.h b/include/time.h
index 8ac58e891b..b3e635395d 100644
--- a/include/time.h
+++ b/include/time.h
@@ -25,6 +25,9 @@  libc_hidden_proto (__clock_gettime)
 extern __typeof (clock_settime) __clock_settime;
 libc_hidden_proto (__clock_settime)
 
+extern __typeof (clock_nanosleep) __clock_nanosleep;
+libc_hidden_proto (__clock_nanosleep);
+
 #ifdef __linux__
 extern __typeof (clock_adjtime) __clock_adjtime;
 libc_hidden_proto (__clock_adjtime);
diff --git a/posix/nanosleep.c b/posix/nanosleep.c
index d8564c7119..ed41c8cce7 100644
--- a/posix/nanosleep.c
+++ b/posix/nanosleep.c
@@ -24,10 +24,13 @@  int
 __nanosleep (const struct timespec *requested_time,
 	     struct timespec *remaining)
 {
-  __set_errno (ENOSYS);
-  return -1;
+  int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);
+  if (ret != 0)
+    {
+      __set_errno (ret);
+      return -1;
+    }
+  return 0;
 }
-stub_warning (nanosleep)
-
-hidden_def (__nanosleep)
+libc_hidden_def (__nanosleep)
 weak_alias (__nanosleep, nanosleep)
diff --git a/sysdeps/unix/clock_nanosleep.c b/sysdeps/mach/clock_nanosleep.c
similarity index 59%
rename from sysdeps/unix/clock_nanosleep.c
rename to sysdeps/mach/clock_nanosleep.c
index 8514a439ee..6a0792d684 100644
--- a/sysdeps/unix/clock_nanosleep.c
+++ b/sysdeps/mach/clock_nanosleep.c
@@ -1,5 +1,5 @@ 
-/* High-resolution sleep with the specified clock.
-   Copyright (C) 2000-2019 Free Software Foundation, Inc.
+/* clock_nanosleep - high-resolution sleep with specifiable clock.
+   Copyright (C) 2002-2019 Free Software Foundation, Inc.
    This file is part of the GNU C Library.
 
    The GNU C Library is free software; you can redistribute it and/or
@@ -16,28 +16,70 @@ 
    License along with the GNU C Library; if not, see
    <https://www.gnu.org/licenses/>.  */
 
-#include <assert.h>
 #include <errno.h>
+#include <mach.h>
 #include <time.h>
-#include <sysdep-cancel.h>
+#include <unistd.h>
 #include <shlib-compat.h>
 
-/* This implementation assumes that these is only a `nanosleep' system
-   call.  So we have to remap all other activities.  */
+static struct timespec
+timespec_sub (struct timespec a, struct timespec b)
+{
+  struct timespec result = { a.tv_sec - b.tv_sec,
+			     a.tv_nsec - b.tv_nsec };
+  if (result.tv_nsec < 0)
+    {
+      --result.tv_sec;
+      result.tv_nsec += 1000000000ul;
+    }
+  return result;
+}
+
+static int
+nanosleep_call (const struct timespec *req, struct timespec *rem)
+{
+  mach_port_t recv;
+  struct timespec before;
+  error_t err;
+
+  const mach_msg_timeout_t ms
+    = req->tv_sec * 1000
+    + (req->tv_nsec + 999999) / 1000000;
+
+  recv = __mach_reply_port ();
+
+  if (rem != NULL)
+    __clock_gettime (CLOCK_REALTIME, &before);
+
+  err = __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
+                    0, 0, recv, ms, MACH_PORT_NULL);
+  __mach_port_destroy (mach_task_self (), recv);
+  if (err == EMACH_RCV_INTERRUPTED)
+    {
+      if (rem != NULL)
+	{
+	  struct timespec after, elapsed;
+	  __clock_gettime (CLOCK_REALTIME, &after);
+	  elapsed = timespec_sub (after, before);
+	  *rem = timespec_sub (*req, elapsed);
+	}
+
+      return EINTR;
+    }
+
+  return 0;
+}
+
 int
 __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
 		   struct timespec *rem)
 {
-  struct timespec now;
-
-  if (! valid_nanoseconds (req->tv_nsec))
+  if (clock_id != CLOCK_REALTIME
+      || !valid_nanoseconds (req->tv_nsec)
+      || (flags != 0 || flags != TIMER_ABSTIME))
     return EINVAL;
 
-  if (clock_id == CLOCK_THREAD_CPUTIME_ID)
-    return EINVAL;		/* POSIX specifies EINVAL for this case.  */
-
-  if (clock_id < CLOCK_REALTIME || clock_id > CLOCK_THREAD_CPUTIME_ID)
-    return EINVAL;
+  struct timespec now;
 
   /* If we got an absolute time, remap it.  */
   if (flags == TIMER_ABSTIME)
@@ -68,15 +110,10 @@  __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
       /* Make sure we are not modifying the struct pointed to by REM.  */
       rem = NULL;
     }
-  else if (flags != 0)
-    return EINVAL;
-  else if (clock_id != CLOCK_REALTIME)
-    /* Not supported.  */
-    return ENOTSUP;
 
-  return __nanosleep (req, rem), 0 ? errno : 0;
+  return nanosleep_call (req, rem);
 }
-
+libc_hidden_def (__clock_nanosleep)
 versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17);
 /* clock_nanosleep moved to libc in version 2.17;
    old binaries may expect the symbol version it had in librt.  */
diff --git a/sysdeps/mach/nanosleep.c b/sysdeps/mach/nanosleep.c
deleted file mode 100644
index b60a2179f6..0000000000
--- a/sysdeps/mach/nanosleep.c
+++ /dev/null
@@ -1,79 +0,0 @@ 
-/* nanosleep -- sleep for a period specified with a struct timespec
-   Copyright (C) 2002-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <errno.h>
-#include <mach.h>
-#include <time.h>
-#include <unistd.h>
-
-# define timespec_sub(a, b, result)					      \
-  do {									      \
-    (result)->tv_sec = (a)->tv_sec - (b)->tv_sec;			      \
-    (result)->tv_nsec = (a)->tv_nsec - (b)->tv_nsec;			      \
-    if ((result)->tv_nsec < 0) {					      \
-      --(result)->tv_sec;						      \
-      (result)->tv_nsec += 1000000000;					      \
-    }									      \
-  } while (0)
-
-int
-__libc_nanosleep (const struct timespec *requested_time,
-                  struct timespec *remaining)
-{
-  mach_port_t recv;
-  struct timespec before;
-  error_t err;
-
-  if (requested_time->tv_sec < 0
-      || ! valid_nanoseconds (requested_time->tv_nsec))
-    {
-      errno = EINVAL;
-      return -1;
-    }
-
-  const mach_msg_timeout_t ms
-    = requested_time->tv_sec * 1000
-    + (requested_time->tv_nsec + 999999) / 1000000;
-
-  recv = __mach_reply_port ();
-
-  if (remaining != 0)
-    __clock_gettime (CLOCK_REALTIME, &before);
-
-  err = __mach_msg (NULL, MACH_RCV_MSG|MACH_RCV_TIMEOUT|MACH_RCV_INTERRUPT,
-                    0, 0, recv, ms, MACH_PORT_NULL);
-  __mach_port_destroy (mach_task_self (), recv);
-  if (err == EMACH_RCV_INTERRUPTED)
-    {
-      if (remaining != 0)
-	{
-	  struct timespec after, elapsed;
-	  __clock_gettime (CLOCK_REALTIME, &after);
-	  timespec_sub (&after, &before, &elapsed);
-	  timespec_sub (requested_time, &elapsed, remaining);
-	}
-
-      errno = EINTR;
-      return -1;
-    }
-
-  return 0;
-}
-weak_alias(__libc_nanosleep, __nanosleep)
-libc_hidden_def (__nanosleep)
-weak_alias (__libc_nanosleep, nanosleep)
diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c
index 1f240b8720..f3c6fd2d5f 100644
--- a/sysdeps/unix/sysv/linux/clock_nanosleep.c
+++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c
@@ -42,7 +42,7 @@  __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
   return (INTERNAL_SYSCALL_ERROR_P (r, err)
 	  ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
 }
-
+libc_hidden_def (__clock_nanosleep)
 versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17);
 /* clock_nanosleep moved to libc in version 2.17;
    old binaries may expect the symbol version it had in librt.  */
diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
deleted file mode 100644
index 6787909248..0000000000
--- a/sysdeps/unix/sysv/linux/nanosleep.c
+++ /dev/null
@@ -1,31 +0,0 @@ 
-/* Linux nanosleep syscall implementation.
-   Copyright (C) 2017-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-
-   The GNU C Library is free software; you can redistribute it and/or
-   modify it under the terms of the GNU Lesser General Public
-   License as published by the Free Software Foundation; either
-   version 2.1 of the License, or (at your option) any later version.
-
-   The GNU C Library is distributed in the hope that it will be useful,
-   but WITHOUT ANY WARRANTY; without even the implied warranty of
-   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
-   Lesser General Public License for more details.
-
-   You should have received a copy of the GNU Lesser General Public
-   License along with the GNU C Library; if not, see
-   <https://www.gnu.org/licenses/>.  */
-
-#include <time.h>
-#include <sysdep-cancel.h>
-#include <not-cancel.h>
-
-/* Pause execution for a number of nanoseconds.  */
-int
-__nanosleep (const struct timespec *requested_time,
-	     struct timespec *remaining)
-{
-  return SYSCALL_CANCEL (nanosleep, requested_time, remaining);
-}
-hidden_def (__nanosleep)
-weak_alias (__nanosleep, nanosleep)
diff --git a/time/clock_nanosleep.c b/time/clock_nanosleep.c
index 5952d29195..d4fc16f6ec 100644
--- a/time/clock_nanosleep.c
+++ b/time/clock_nanosleep.c
@@ -33,7 +33,7 @@  __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req,
   /* Not implemented.  */
   return ENOSYS;
 }
-
+libc_hidden_def (__clock_nanosleep)
 versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17);
 /* clock_nanosleep moved to libc in version 2.17;
    old binaries may expect the symbol version it had in librt.  */