This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH 2/3] Refactor nanosleep in terms of clock_nanosleep
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