| Message ID | 20191108170302.29838-1-alistair.francis@wdc.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v6,1/3] sysdeps/clock_nanosleep: Use clock_nanosleep_time64 if avaliable | expand |
On 08/11/2019 14:03, Alistair Francis wrote: > The clock_nanosleep syscall is not supported on newer 32-bit platforms (such > as RV32). To fix this issue let's use clock_nanosleep_time64 if it is > avaliable. > --- > This patch was runtime tested with RV32 and RV64 > It was build tested using the ./scripts/build-many-glibcs.py script. > > I also ran: > $ make ; make install ; make check > tests on native ARM (32-bit) with the following three confiugrations: > - 4.19 Kernel and 4.19 Headers > - 5.2 Kernel and 4.19 Headers > - 5.2 Kernel and 5.2 Headers > and didn't see any regressions > > v6: > - Split header changes into seperate patches > - Don't check for ENOSYS in the incorrect places > - Don't change indendtation of file > v5: > - Fix clock_nanosleep syscall > - Rebase on master > > v4: > - Rebase on master > - Use __clock_nanosleep to avoid duplicate implementations > - Fix the error handling when a syscall fails > v2: > - Explicitly include `#include <kernel-features.h>` LGTM with a small nit below. > > include/time.h | 8 +++ > sysdeps/unix/sysv/linux/clock_nanosleep.c | 61 +++++++++++++++++++++-- > 2 files changed, 65 insertions(+), 4 deletions(-) > > diff --git a/include/time.h b/include/time.h > index b3e635395db..d7800eb30f8 100644 > --- a/include/time.h > +++ b/include/time.h > @@ -209,6 +209,14 @@ libc_hidden_proto (__difftime64) > > extern double __difftime (time_t time1, time_t time0); > > +#if __TIMESIZE == 64 > +# define __clock_nanosleep_time64 __clock_nanosleep > +#else > +extern int __clock_nanosleep_time64 (clockid_t clock_id, > + int flags, const struct __timespec64 *req, > + struct __timespec64 *rem); > +libc_hidden_proto (__clock_nanosleep_time64) > +#endif > > /* Use in the clock_* functions. Size of the field representing the > actual clock ID. */ Ok. > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c > index f3c6fd2d5f7..903ed58e218 100644 > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > @@ -16,6 +16,7 @@ > <https://www.gnu.org/licenses/>. */ > > #include <time.h> > +#include <kernel-features.h> > #include <errno.h> > > #include <sysdep-cancel.h> > @@ -26,9 +27,11 @@ > /* We can simply use the syscall. The CPU clocks are not supported > with this function. */ > int > -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > - struct timespec *rem) > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, > + struct __timespec64 *rem) > { > + int r; > + > if (clock_id == CLOCK_THREAD_CPUTIME_ID) > return EINVAL; > if (clock_id == CLOCK_PROCESS_CPUTIME_ID) > @@ -37,11 +40,61 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > /* If the call is interrupted by a signal handler or encounters an error, > it returns a positive value similar to errno. */ > INTERNAL_SYSCALL_DECL (err); > - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > - req, rem); > + > +#ifdef __ASSUME_TIME64_SYSCALLS > +# ifndef __NR_clock_nanosleep_time64 > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > +# endif > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > + flags, req, rem); > +#else > +# ifdef __NR_clock_nanosleep_time64 > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > + flags, req, rem); > + > + if (r == 0 || errno != ENOSYS) > + { > + return (INTERNAL_SYSCALL_ERROR_P (r, err) > + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > + } > +# endif /* __NR_clock_nanosleep_time64 */ Ok. > + struct timespec ts32, tr32; > + > + if (! in_time_t_range (req->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > + > + ts32 = valid_timespec64_to_timespec (*req); > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > + &ts32, &tr32); > + > + if (r == 0 && rem) > + *rem = valid_timespec_to_timespec64 (tr32); > +#endif /* __ASSUME_TIME64_SYSCALLS */ > + > return (INTERNAL_SYSCALL_ERROR_P (r, err) > ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > } Ok. > + > +#if __TIMESIZE != 64 > +int > +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > + struct timespec *rem) > +{ > + int r; > + struct __timespec64 treq64, trem64; > + > + treq64 = valid_timespec_to_timespec64 (*req); > + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); > + > + if (r == 0 && rem) > + *rem = valid_timespec64_to_timespec (trem64); No implicit check on rem, use rem != NULL. > + > + return r; > +} > +#endif > libc_hidden_def (__clock_nanosleep) > versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); > /* clock_nanosleep moved to libc in version 2.17; >
On Fri, Nov 8, 2019 at 11:01 AM Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote: > > > > On 08/11/2019 14:03, Alistair Francis wrote: > > The clock_nanosleep syscall is not supported on newer 32-bit platforms (such > > as RV32). To fix this issue let's use clock_nanosleep_time64 if it is > > avaliable. > > --- > > This patch was runtime tested with RV32 and RV64 > > It was build tested using the ./scripts/build-many-glibcs.py script. > > > > I also ran: > > $ make ; make install ; make check > > tests on native ARM (32-bit) with the following three confiugrations: > > - 4.19 Kernel and 4.19 Headers > > - 5.2 Kernel and 4.19 Headers > > - 5.2 Kernel and 5.2 Headers > > and didn't see any regressions > > > > v6: > > - Split header changes into seperate patches > > - Don't check for ENOSYS in the incorrect places > > - Don't change indendtation of file > > v5: > > - Fix clock_nanosleep syscall > > - Rebase on master > > > > v4: > > - Rebase on master > > - Use __clock_nanosleep to avoid duplicate implementations > > - Fix the error handling when a syscall fails > > v2: > > - Explicitly include `#include <kernel-features.h>` > > LGTM with a small nit below. Thanks, I have fixed the nit. Alistair > > > > > include/time.h | 8 +++ > > sysdeps/unix/sysv/linux/clock_nanosleep.c | 61 +++++++++++++++++++++-- > > 2 files changed, 65 insertions(+), 4 deletions(-) > > > > diff --git a/include/time.h b/include/time.h > > index b3e635395db..d7800eb30f8 100644 > > --- a/include/time.h > > +++ b/include/time.h > > @@ -209,6 +209,14 @@ libc_hidden_proto (__difftime64) > > > > extern double __difftime (time_t time1, time_t time0); > > > > +#if __TIMESIZE == 64 > > +# define __clock_nanosleep_time64 __clock_nanosleep > > +#else > > +extern int __clock_nanosleep_time64 (clockid_t clock_id, > > + int flags, const struct __timespec64 *req, > > + struct __timespec64 *rem); > > +libc_hidden_proto (__clock_nanosleep_time64) > > +#endif > > > > /* Use in the clock_* functions. Size of the field representing the > > actual clock ID. */ > > Ok. > > > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c > > index f3c6fd2d5f7..903ed58e218 100644 > > --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c > > +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c > > @@ -16,6 +16,7 @@ > > <https://www.gnu.org/licenses/>. */ > > > > #include <time.h> > > +#include <kernel-features.h> > > #include <errno.h> > > > > #include <sysdep-cancel.h> > > @@ -26,9 +27,11 @@ > > /* We can simply use the syscall. The CPU clocks are not supported > > with this function. */ > > int > > -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > - struct timespec *rem) > > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, > > + struct __timespec64 *rem) > > { > > + int r; > > + > > if (clock_id == CLOCK_THREAD_CPUTIME_ID) > > return EINVAL; > > if (clock_id == CLOCK_PROCESS_CPUTIME_ID) > > @@ -37,11 +40,61 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > /* If the call is interrupted by a signal handler or encounters an error, > > it returns a positive value similar to errno. */ > > INTERNAL_SYSCALL_DECL (err); > > - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > > - req, rem); > > + > > +#ifdef __ASSUME_TIME64_SYSCALLS > > +# ifndef __NR_clock_nanosleep_time64 > > +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep > > +# endif > > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > > + flags, req, rem); > > +#else > > +# ifdef __NR_clock_nanosleep_time64 > > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, > > + flags, req, rem); > > + > > + if (r == 0 || errno != ENOSYS) > > + { > > + return (INTERNAL_SYSCALL_ERROR_P (r, err) > > + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > > + } > > +# endif /* __NR_clock_nanosleep_time64 */ > > Ok. > > > + struct timespec ts32, tr32; > > + > > + if (! in_time_t_range (req->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > + > > + ts32 = valid_timespec64_to_timespec (*req); > > + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, > > + &ts32, &tr32); > > + > > + if (r == 0 && rem) > > + *rem = valid_timespec_to_timespec64 (tr32); > > +#endif /* __ASSUME_TIME64_SYSCALLS */ > > + > > return (INTERNAL_SYSCALL_ERROR_P (r, err) > > ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); > > } > > Ok. > > > + > > +#if __TIMESIZE != 64 > > +int > > +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, > > + struct timespec *rem) > > +{ > > + int r; > > + struct __timespec64 treq64, trem64; > > + > > + treq64 = valid_timespec_to_timespec64 (*req); > > + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); > > + > > + if (r == 0 && rem) > > + *rem = valid_timespec64_to_timespec (trem64); > > No implicit check on rem, use rem != NULL. > > > + > > + return r; > > +} > > +#endif > > libc_hidden_def (__clock_nanosleep) > > versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); > > /* clock_nanosleep moved to libc in version 2.17; > >
This breaks rt/tst-timer on i586 and powerpc and armv7l. Andreas.
On Sun, Nov 10, 2019 at 1:00 AM Andreas Schwab <schwab@linux-m68k.org> wrote: > > This breaks rt/tst-timer on i586 and powerpc and armv7l. I don't understand how. I tested it on ARMv7. Do you have any more details on why it is failing? Is there a way to run just that test so I don't have to run all the others? Also is this on a 5.1+ or earlier kernel/headers? Alistair > > Andreas. > > -- > Andreas Schwab, schwab@linux-m68k.org > GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 > "And now for something completely different."
On Nov 10 2019, Alistair Francis wrote: > On Sun, Nov 10, 2019 at 1:00 AM Andreas Schwab <schwab@linux-m68k.org> wrote: >> >> This breaks rt/tst-timer on i586 and powerpc and armv7l. > > I don't understand how. I tested it on ARMv7. Do you have any more > details on why it is failing? I don't know, it just hangs. > Also is this on a 5.1+ or earlier kernel/headers? All the latest. https://build.opensuse.org/package/show/home:Andreas_Schwab:glibc/glibc Andreas.
On 10/11/2019 14:44, Andreas Schwab wrote: > On Nov 10 2019, Alistair Francis wrote: > >> On Sun, Nov 10, 2019 at 1:00 AM Andreas Schwab <schwab@linux-m68k.org> wrote: >>> >>> This breaks rt/tst-timer on i586 and powerpc and armv7l. >> >> I don't understand how. I tested it on ARMv7. Do you have any more >> details on why it is failing? > > I don't know, it just hangs. > >> Also is this on a 5.1+ or earlier kernel/headers? > > All the latest. > > https://build.opensuse.org/package/show/home:Andreas_Schwab:glibc/glibc > > Andreas. > It is because clock_nanosspe requires to handle EINTR as a possible return value and update the remaining argument accordingly: diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c index 60c61ee277..fc47c58ee7 100644 --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -52,13 +52,10 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, flags, req, rem); - if (r == 0 || errno != ENOSYS) - { - return (INTERNAL_SYSCALL_ERROR_P (r, err) - ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); - } + if (r != -ENOSYS) + return (INTERNAL_SYSCALL_ERROR_P (r, err) + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); # endif /* __NR_clock_nanosleep_time64 */ - struct timespec ts32, tr32; if (! in_time_t_range (req->tv_sec)) { @@ -66,11 +63,12 @@ __clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec return -1; } - ts32 = valid_timespec64_to_timespec (*req); + struct timespec tr32; + struct timespec ts32 = valid_timespec64_to_timespec (*req); r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, &ts32, &tr32); - if (r == 0 && rem != NULL) + if (r == -EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0) *rem = valid_timespec_to_timespec64 (tr32); #endif /* __ASSUME_TIME64_SYSCALLS */ @@ -89,7 +87,7 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, treq64 = valid_timespec_to_timespec64 (*req); r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); - if (r == 0 && rem != NULL) + if (r == EINTR && rem != NULL && (flags & TIMER_ABSTIME) == 0) *rem = valid_timespec64_to_timespec (trem64); return r;
diff --git a/include/time.h b/include/time.h index b3e635395db..d7800eb30f8 100644 --- a/include/time.h +++ b/include/time.h @@ -209,6 +209,14 @@ libc_hidden_proto (__difftime64) extern double __difftime (time_t time1, time_t time0); +#if __TIMESIZE == 64 +# define __clock_nanosleep_time64 __clock_nanosleep +#else +extern int __clock_nanosleep_time64 (clockid_t clock_id, + int flags, const struct __timespec64 *req, + struct __timespec64 *rem); +libc_hidden_proto (__clock_nanosleep_time64) +#endif /* Use in the clock_* functions. Size of the field representing the actual clock ID. */ diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c b/sysdeps/unix/sysv/linux/clock_nanosleep.c index f3c6fd2d5f7..903ed58e218 100644 --- a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++ b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -16,6 +16,7 @@ <https://www.gnu.org/licenses/>. */ #include <time.h> +#include <kernel-features.h> #include <errno.h> #include <sysdep-cancel.h> @@ -26,9 +27,11 @@ /* We can simply use the syscall. The CPU clocks are not supported with this function. */ int -__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, - struct timespec *rem) +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const struct __timespec64 *req, + struct __timespec64 *rem) { + int r; + if (clock_id == CLOCK_THREAD_CPUTIME_ID) return EINVAL; if (clock_id == CLOCK_PROCESS_CPUTIME_ID) @@ -37,11 +40,61 @@ __clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, /* If the call is interrupted by a signal handler or encounters an error, it returns a positive value similar to errno. */ INTERNAL_SYSCALL_DECL (err); - int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, - req, rem); + +#ifdef __ASSUME_TIME64_SYSCALLS +# ifndef __NR_clock_nanosleep_time64 +# define __NR_clock_nanosleep_time64 __NR_clock_nanosleep +# endif + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, + flags, req, rem); +#else +# ifdef __NR_clock_nanosleep_time64 + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err, clock_id, + flags, req, rem); + + if (r == 0 || errno != ENOSYS) + { + return (INTERNAL_SYSCALL_ERROR_P (r, err) + ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); + } +# endif /* __NR_clock_nanosleep_time64 */ + struct timespec ts32, tr32; + + if (! in_time_t_range (req->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } + + ts32 = valid_timespec64_to_timespec (*req); + r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, clock_id, flags, + &ts32, &tr32); + + if (r == 0 && rem) + *rem = valid_timespec_to_timespec64 (tr32); +#endif /* __ASSUME_TIME64_SYSCALLS */ + return (INTERNAL_SYSCALL_ERROR_P (r, err) ? INTERNAL_SYSCALL_ERRNO (r, err) : 0); } + +#if __TIMESIZE != 64 +int +__clock_nanosleep (clockid_t clock_id, int flags, const struct timespec *req, + struct timespec *rem) +{ + int r; + struct __timespec64 treq64, trem64; + + treq64 = valid_timespec_to_timespec64 (*req); + r = __clock_nanosleep_time64 (clock_id, flags, &treq64, &trem64); + + if (r == 0 && rem) + *rem = valid_timespec64_to_timespec (trem64); + + return r; +} +#endif libc_hidden_def (__clock_nanosleep) versioned_symbol (libc, __clock_nanosleep, clock_nanosleep, GLIBC_2_17); /* clock_nanosleep moved to libc in version 2.17;