* [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-11-23 22:32 [PATCH v3 1/2] sysdeps: Add clock_gettime64 vDSO Alistair Francis
@ 2019-11-23 22:32 ` Alistair Francis
2019-12-03 19:02 ` Alistair Francis
2019-12-03 19:29 ` Adhemerval Zanella
0 siblings, 2 replies; 11+ messages in thread
From: Alistair Francis @ 2019-11-23 22:32 UTC (permalink / raw)
To: libc-alpha; +Cc: alistair23, Alistair Francis
With the clock_gettime64 call we prefer to use vDSO. There is no call
to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
doesn't support vDSO.
---
This was 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
v3:
- Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
- Change code style to be more glibc-ish
v2:
- Add commit message
- Change ret_64 to int
include/time.h | 3 ++
sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
2 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/include/time.h b/include/time.h
index d7800eb30f..c19c73ae09 100644
--- a/include/time.h
+++ b/include/time.h
@@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
#if __TIMESIZE == 64
# define __clock_nanosleep_time64 __clock_nanosleep
+# define __clock_gettime64 __clock_gettime
#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)
+extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
+libc_hidden_proto (__clock_gettime64)
#endif
/* Use in the clock_* functions. Size of the field representing the
diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
index ca40983f6c..875c4fe905 100644
--- a/sysdeps/unix/sysv/linux/clock_gettime.c
+++ b/sysdeps/unix/sysv/linux/clock_gettime.c
@@ -17,6 +17,7 @@
<https://www.gnu.org/licenses/>. */
#include <sysdep.h>
+#include <kernel-features.h>
#include <errno.h>
#include <time.h>
#include "kernel-posix-cpu-timers.h"
@@ -30,10 +31,51 @@
/* Get current value of CLOCK and store it in TP. */
int
+__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
+{
+#ifdef __ASSUME_TIME64_SYSCALLS
+# ifndef __NR_clock_gettime64
+# define __NR_clock_gettime64 __NR_clock_gettime
+# define __vdso_clock_gettime64 __vdso_clock_gettime
+# endif
+ return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
+#else
+# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
+ int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
+ if (ret64 == 0 || errno != ENOSYS)
+ return ret64;
+# endif
+ struct timespec tp32;
+ int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
+ if (ret == 0)
+ *tp = valid_timespec_to_timespec64 (tp32);
+ return ret;
+#endif
+}
+
+#if __TIMESIZE != 64
+int
__clock_gettime (clockid_t clock_id, struct timespec *tp)
{
- return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
+ int ret;
+ struct __timespec64 tp64;
+
+ ret = __clock_gettime64 (clock_id, &tp64);
+
+ if (ret == 0)
+ {
+ if (! in_time_t_range (tp64.tv_sec))
+ {
+ __set_errno (EOVERFLOW);
+ return -1;
+ }
+
+ *tp = valid_timespec64_to_timespec (tp64);
+ }
+
+ return ret;
}
+#endif
libc_hidden_def (__clock_gettime)
versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
--
2.24.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v3 1/2] sysdeps: Add clock_gettime64 vDSO
@ 2019-11-23 22:32 Alistair Francis
2019-11-23 22:32 ` [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Alistair Francis
0 siblings, 1 reply; 11+ messages in thread
From: Alistair Francis @ 2019-11-23 22:32 UTC (permalink / raw)
To: libc-alpha; +Cc: alistair23, Alistair Francis, Adhemerval Zanella
Add support for the clock_gettim64 vDSO calls. These are protected by
the HAVE_CLOCK_GETTIME64_VSYSCALL define.
HAVE_CLOCK_GETTIME64_VSYSCALL should be defined for 32-bit platforms
(WORDSIZE == 32) that only run on the 5.1 kernel or later. WORDSIZE ==
64 platforms can use #define __vdso_clock_gettime64 __vdso_clock_gettime
and use the __vdso_clock_gettime syscall as they don't have a
__vdso_clock_gettime64 call.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
---
sysdeps/unix/sysv/linux/init-first.c | 10 ++++++++++
sysdeps/unix/sysv/linux/libc-vdso.h | 4 ++++
2 files changed, 14 insertions(+)
diff --git a/sysdeps/unix/sysv/linux/init-first.c b/sysdeps/unix/sysv/linux/init-first.c
index d90ca820be..d005d13322 100644
--- a/sysdeps/unix/sysv/linux/init-first.c
+++ b/sysdeps/unix/sysv/linux/init-first.c
@@ -24,6 +24,11 @@
int (*VDSO_SYMBOL(clock_gettime)) (clockid_t, struct timespec *)
attribute_hidden;
#endif
+/* vDSO symbol used on clock_gettime64 implementation. */
+#ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
+int (*VDSO_SYMBOL(clock_gettime64)) (clockid_t, struct __timespec64 *)
+ attribute_hidden;
+#endif
/* vDSO symbol used on clock_getres implementation. */
#ifdef HAVE_CLOCK_GETRES_VSYSCALL
int (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *)
@@ -52,6 +57,11 @@ __libc_vdso_platform_setup (void)
= get_vdso_mangle_symbol (HAVE_CLOCK_GETTIME_VSYSCALL);
#endif
+#ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
+ VDSO_SYMBOL(clock_gettime64)
+ = get_vdso_mangle_symbol (HAVE_CLOCK_GETTIME64_VSYSCALL);
+#endif
+
#ifdef HAVE_CLOCK_GETRES_VSYSCALL
VDSO_SYMBOL(clock_getres)
= get_vdso_mangle_symbol (HAVE_CLOCK_GETRES_VSYSCALL);
diff --git a/sysdeps/unix/sysv/linux/libc-vdso.h b/sysdeps/unix/sysv/linux/libc-vdso.h
index 792ac39d85..c6d505bab3 100644
--- a/sysdeps/unix/sysv/linux/libc-vdso.h
+++ b/sysdeps/unix/sysv/linux/libc-vdso.h
@@ -32,6 +32,10 @@
extern int (*VDSO_SYMBOL(clock_gettime)) (clockid_t, struct timespec *)
attribute_hidden;
#endif
+#ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
+extern int (*VDSO_SYMBOL(clock_gettime64)) (clockid_t, struct __timespec64 *)
+ attribute_hidden;
+#endif
#ifdef HAVE_CLOCK_GETRES_VSYSCALL
extern int (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *)
attribute_hidden;
--
2.24.0
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-11-23 22:32 ` [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Alistair Francis
@ 2019-12-03 19:02 ` Alistair Francis
2019-12-03 19:29 ` Adhemerval Zanella
1 sibling, 0 replies; 11+ messages in thread
From: Alistair Francis @ 2019-12-03 19:02 UTC (permalink / raw)
To: Alistair Francis; +Cc: GNU C Library
On Sat, Nov 23, 2019 at 2:32 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> With the clock_gettime64 call we prefer to use vDSO. There is no call
> to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
> doesn't support vDSO.
Ping!
Alistair
> ---
> This was 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
>
> v3:
> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
> - Change code style to be more glibc-ish
> v2:
> - Add commit message
> - Change ret_64 to int
>
> include/time.h | 3 ++
> sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
> 2 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/include/time.h b/include/time.h
> index d7800eb30f..c19c73ae09 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
>
> #if __TIMESIZE == 64
> # define __clock_nanosleep_time64 __clock_nanosleep
> +# define __clock_gettime64 __clock_gettime
> #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)
> +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
> +libc_hidden_proto (__clock_gettime64)
> #endif
>
> /* Use in the clock_* functions. Size of the field representing the
> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
> index ca40983f6c..875c4fe905 100644
> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
> @@ -17,6 +17,7 @@
> <https://www.gnu.org/licenses/>. */
>
> #include <sysdep.h>
> +#include <kernel-features.h>
> #include <errno.h>
> #include <time.h>
> #include "kernel-posix-cpu-timers.h"
> @@ -30,10 +31,51 @@
>
> /* Get current value of CLOCK and store it in TP. */
> int
> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +# ifndef __NR_clock_gettime64
> +# define __NR_clock_gettime64 __NR_clock_gettime
> +# define __vdso_clock_gettime64 __vdso_clock_gettime
> +# endif
> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> +#else
> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> + if (ret64 == 0 || errno != ENOSYS)
> + return ret64;
> +# endif
> + struct timespec tp32;
> + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> + if (ret == 0)
> + *tp = valid_timespec_to_timespec64 (tp32);
> + return ret;
> +#endif
> +}
> +
> +#if __TIMESIZE != 64
> +int
> __clock_gettime (clockid_t clock_id, struct timespec *tp)
> {
> - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> + int ret;
> + struct __timespec64 tp64;
> +
> + ret = __clock_gettime64 (clock_id, &tp64);
> +
> + if (ret == 0)
> + {
> + if (! in_time_t_range (tp64.tv_sec))
> + {
> + __set_errno (EOVERFLOW);
> + return -1;
> + }
> +
> + *tp = valid_timespec64_to_timespec (tp64);
> + }
> +
> + return ret;
> }
> +#endif
> libc_hidden_def (__clock_gettime)
>
> versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
> --
> 2.24.0
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-11-23 22:32 ` [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Alistair Francis
2019-12-03 19:02 ` Alistair Francis
@ 2019-12-03 19:29 ` Adhemerval Zanella
2019-12-03 23:39 ` Alistair Francis
2019-12-04 17:24 ` Adhemerval Zanella
1 sibling, 2 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2019-12-03 19:29 UTC (permalink / raw)
To: libc-alpha
On 23/11/2019 19:27, Alistair Francis wrote:
> With the clock_gettime64 call we prefer to use vDSO. There is no call
> to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
> doesn't support vDSO.
Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
> ---
> This was 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
>
> v3:
> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
> - Change code style to be more glibc-ish
> v2:
> - Add commit message
> - Change ret_64 to int
>
> include/time.h | 3 ++
> sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
> 2 files changed, 46 insertions(+), 1 deletion(-)
>
> diff --git a/include/time.h b/include/time.h
> index d7800eb30f..c19c73ae09 100644
> --- a/include/time.h
> +++ b/include/time.h
> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
>
> #if __TIMESIZE == 64
> # define __clock_nanosleep_time64 __clock_nanosleep
> +# define __clock_gettime64 __clock_gettime
> #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)
> +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
> +libc_hidden_proto (__clock_gettime64)
> #endif
>
> /* Use in the clock_* functions. Size of the field representing the
> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
> index ca40983f6c..875c4fe905 100644
> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
> @@ -17,6 +17,7 @@
> <https://www.gnu.org/licenses/>. */
>
> #include <sysdep.h>
> +#include <kernel-features.h>
> #include <errno.h>
> #include <time.h>
> #include "kernel-posix-cpu-timers.h"
> @@ -30,10 +31,51 @@
>
> /* Get current value of CLOCK and store it in TP. */
> int
> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> +{
> +#ifdef __ASSUME_TIME64_SYSCALLS
> +# ifndef __NR_clock_gettime64
> +# define __NR_clock_gettime64 __NR_clock_gettime
> +# define __vdso_clock_gettime64 __vdso_clock_gettime
> +# endif
> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
__vdso_clock_gettime64/__NR_clock_gettime64 will be called.
> +#else
> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> + if (ret64 == 0 || errno != ENOSYS)
> + return ret64;
> +# endif
This should be:
#if defined __NR_clock_gettime64
int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
[...]
And before sysdep-vdso.h it should be.
#if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined HAVE_CLOCK_GETTIME64_VSYSCALL
# define HAVE_SYSCALL
#endif
If !HAVE_SYSCALL the INLINE_VSYSCALL is defined as INLINE_SYCALL.
It means that if an architecture does provide __NR_clock_gettime64
but not provide HAVE_CLOCK_GETTIME64_VSYSCALL, it will still use
the syscall. This code only uses the syscall if the vDSO is still
present.
I hope I can get my vDSO refactor, which would simplify a bit this
code.
> + struct timespec tp32;
> + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> + if (ret == 0)
> + *tp = valid_timespec_to_timespec64 (tp32);
> + return ret;
> +#endif
> +}
Ok, old fallback to time32 vdso/syscall.
> +
> +#if __TIMESIZE != 64
> +int
> __clock_gettime (clockid_t clock_id, struct timespec *tp)
> {
> - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> + int ret;
> + struct __timespec64 tp64;
> +
> + ret = __clock_gettime64 (clock_id, &tp64);
> +
> + if (ret == 0)
> + {
> + if (! in_time_t_range (tp64.tv_sec))
> + {
> + __set_errno (EOVERFLOW);
> + return -1;
> + }
> +
> + *tp = valid_timespec64_to_timespec (tp64);
> + }
> +
> + return ret;
> }
> +#endif
> libc_hidden_def (__clock_gettime)
>
> versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
>
Ok.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-03 19:29 ` Adhemerval Zanella
@ 2019-12-03 23:39 ` Alistair Francis
2019-12-04 12:55 ` Adhemerval Zanella
2019-12-04 17:24 ` Adhemerval Zanella
1 sibling, 1 reply; 11+ messages in thread
From: Alistair Francis @ 2019-12-03 23:39 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library
isOn Tue, Dec 3, 2019 at 11:29 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 23/11/2019 19:27, Alistair Francis wrote:
> > With the clock_gettime64 call we prefer to use vDSO. There is no call
> > to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
> > doesn't support vDSO.
>
> Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
Thanks for the review.
>
> > ---
> > This was 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
> >
> > v3:
> > - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
> > - Change code style to be more glibc-ish
> > v2:
> > - Add commit message
> > - Change ret_64 to int
> >
> > include/time.h | 3 ++
> > sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
> > 2 files changed, 46 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/time.h b/include/time.h
> > index d7800eb30f..c19c73ae09 100644
> > --- a/include/time.h
> > +++ b/include/time.h
> > @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
> >
> > #if __TIMESIZE == 64
> > # define __clock_nanosleep_time64 __clock_nanosleep
> > +# define __clock_gettime64 __clock_gettime
> > #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)
> > +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
> > +libc_hidden_proto (__clock_gettime64)
> > #endif
> >
> > /* Use in the clock_* functions. Size of the field representing the
> > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
> > index ca40983f6c..875c4fe905 100644
> > --- a/sysdeps/unix/sysv/linux/clock_gettime.c
> > +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
> > @@ -17,6 +17,7 @@
> > <https://www.gnu.org/licenses/>. */
> >
> > #include <sysdep.h>
> > +#include <kernel-features.h>
> > #include <errno.h>
> > #include <time.h>
> > #include "kernel-posix-cpu-timers.h"
> > @@ -30,10 +31,51 @@
> >
> > /* Get current value of CLOCK and store it in TP. */
> > int
> > +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> > +{
> > +#ifdef __ASSUME_TIME64_SYSCALLS
> > +# ifndef __NR_clock_gettime64
> > +# define __NR_clock_gettime64 __NR_clock_gettime
> > +# define __vdso_clock_gettime64 __vdso_clock_gettime
> > +# endif
> > + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
>
> Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
> __vdso_clock_gettime64/__NR_clock_gettime64 will be called.
>
> > +#else
> > +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
> > + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> > + if (ret64 == 0 || errno != ENOSYS)
> > + return ret64;
> > +# endif
> This should be:
>
> #if defined __NR_clock_gettime64
> int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
> [...]
This doesn't work for 32-bit archs. As HAVE_CLOCK_GETTIME_VSYSCALL is
defined, HAVE_VSYSCALL is also defined, but we then use the #if
defined __NR_clock_gettime64 clause and fail to compile as there is no
VDSO for gettime64.
This diff works
diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c
b/sysdeps/unix/sysv/linux/clock_gettime.c
index 5051a87f83..575590475c 100644
--- a/sysdeps/unix/sysv/linux/clock_gettime.c
+++ b/sysdeps/unix/sysv/linux/clock_gettime.c
@@ -22,7 +22,9 @@
#include <time.h>
#include "kernel-posix-cpu-timers.h"
-#if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined
HAVE_CLOCK_GETTIME64_VSYSCALL
+#if defined __ASSUME_TIME64_SYSCALLS
+ || (defined HAVE_CLOCK_GETTIME_VSYSCALL && !defined __NR_clock_gettime64)
+ || (defined HAVE_CLOCK_GETTIME64_VSYSCALL && defined __NR_clock_gettime64)
# define HAVE_VSYSCALL
#endif
#include <sysdep-vdso.h>
Otherwise we can't use #if defined __NR_clock_gettime64
>
> And before sysdep-vdso.h it should be.
>
> #if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined HAVE_CLOCK_GETTIME64_VSYSCALL
> # define HAVE_SYSCALL
I'm assuming you mean # define HAVE_VSYSCALL
Alistair
> #endif
>
> If !HAVE_SYSCALL the INLINE_VSYSCALL is defined as INLINE_SYCALL.
> It means that if an architecture does provide __NR_clock_gettime64
> but not provide HAVE_CLOCK_GETTIME64_VSYSCALL, it will still use
> the syscall. This code only uses the syscall if the vDSO is still
> present.
>
> I hope I can get my vDSO refactor, which would simplify a bit this
> code.
>
> > + struct timespec tp32;
> > + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> > + if (ret == 0)
> > + *tp = valid_timespec_to_timespec64 (tp32);
> > + return ret;
> > +#endif
> > +}
>
> Ok, old fallback to time32 vdso/syscall.
>
> > +
> > +#if __TIMESIZE != 64
> > +int
> > __clock_gettime (clockid_t clock_id, struct timespec *tp)
> > {
> > - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> > + int ret;
> > + struct __timespec64 tp64;
> > +
> > + ret = __clock_gettime64 (clock_id, &tp64);
> > +
> > + if (ret == 0)
> > + {
> > + if (! in_time_t_range (tp64.tv_sec))
> > + {
> > + __set_errno (EOVERFLOW);
> > + return -1;
> > + }
> > +
> > + *tp = valid_timespec64_to_timespec (tp64);
> > + }
> > +
> > + return ret;
> > }
> > +#endif
> > libc_hidden_def (__clock_gettime)
> >
> > versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
> >
>
> Ok.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-03 23:39 ` Alistair Francis
@ 2019-12-04 12:55 ` Adhemerval Zanella
0 siblings, 0 replies; 11+ messages in thread
From: Adhemerval Zanella @ 2019-12-04 12:55 UTC (permalink / raw)
To: Alistair Francis; +Cc: GNU C Library
On 03/12/2019 20:33, Alistair Francis wrote:
> isOn Tue, Dec 3, 2019 at 11:29 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
>>
>>
>>
>> On 23/11/2019 19:27, Alistair Francis wrote:
>>> With the clock_gettime64 call we prefer to use vDSO. There is no call
>>> to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
>>> doesn't support vDSO.
>>
>> Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
>
> Thanks for the review.
>
>>
>>> ---
>>> This was 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
>>>
>>> v3:
>>> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
>>> - Change code style to be more glibc-ish
>>> v2:
>>> - Add commit message
>>> - Change ret_64 to int
>>>
>>> include/time.h | 3 ++
>>> sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
>>> 2 files changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/time.h b/include/time.h
>>> index d7800eb30f..c19c73ae09 100644
>>> --- a/include/time.h
>>> +++ b/include/time.h
>>> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
>>>
>>> #if __TIMESIZE == 64
>>> # define __clock_nanosleep_time64 __clock_nanosleep
>>> +# define __clock_gettime64 __clock_gettime
>>> #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)
>>> +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
>>> +libc_hidden_proto (__clock_gettime64)
>>> #endif
>>>
>>> /* Use in the clock_* functions. Size of the field representing the
>>> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
>>> index ca40983f6c..875c4fe905 100644
>>> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
>>> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
>>> @@ -17,6 +17,7 @@
>>> <https://www.gnu.org/licenses/>. */
>>>
>>> #include <sysdep.h>
>>> +#include <kernel-features.h>
>>> #include <errno.h>
>>> #include <time.h>
>>> #include "kernel-posix-cpu-timers.h"
>>> @@ -30,10 +31,51 @@
>>>
>>> /* Get current value of CLOCK and store it in TP. */
>>> int
>>> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
>>> +{
>>> +#ifdef __ASSUME_TIME64_SYSCALLS
>>> +# ifndef __NR_clock_gettime64
>>> +# define __NR_clock_gettime64 __NR_clock_gettime
>>> +# define __vdso_clock_gettime64 __vdso_clock_gettime
>>> +# endif
>>> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
>>
>> Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
>> __vdso_clock_gettime64/__NR_clock_gettime64 will be called.
>>
>>> +#else
>>> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
>>> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
>>> + if (ret64 == 0 || errno != ENOSYS)
>>> + return ret64;
>>> +# endif
>> This should be:
>>
>> #if defined __NR_clock_gettime64
>> int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
>> [...]
>
> This doesn't work for 32-bit archs. As HAVE_CLOCK_GETTIME_VSYSCALL is
> defined, HAVE_VSYSCALL is also defined, but we then use the #if
> defined __NR_clock_gettime64 clause and fail to compile as there is no
> VDSO for gettime64.
Sign... yes you are correct, the current vDSO mechanism is clunky and does
not allow to just define the usage of an specific vDSO symbol.
>
> This diff works
>
> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c
> b/sysdeps/unix/sysv/linux/clock_gettime.c
> index 5051a87f83..575590475c 100644
> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
> @@ -22,7 +22,9 @@
> #include <time.h>
> #include "kernel-posix-cpu-timers.h"
>
> -#if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined
> HAVE_CLOCK_GETTIME64_VSYSCALL
> +#if defined __ASSUME_TIME64_SYSCALLS
> + || (defined HAVE_CLOCK_GETTIME_VSYSCALL && !defined __NR_clock_gettime64)
> + || (defined HAVE_CLOCK_GETTIME64_VSYSCALL && defined __NR_clock_gettime64)
> # define HAVE_VSYSCALL
> #endif
> #include <sysdep-vdso.h>
>
> Otherwise we can't use #if defined __NR_clock_gettime64
I don't think this is fully correct because __ASSUME_TIME64_SYSCALLS does not
automatically makes the architecture provide a vDSO. I think the original v2
version is simpler and better for an initial approach, and now I see that we
do need to use my refactor to simplify this vDSO mess.
So we current have the following scenarios:
1. Only define a 64-bit __NR_clock_gettime.
- i.e: old 64-bit architectures (alpha, ia64).
2. Only define a 64-bit __NR_clock_gettime and provide a 64-bit vDSO symbol.
- i.e: current 64-bit architectures (aarch64, sparc64, s390x, powerpc64{le},
x86_64, riscv64, and mips64).
3. Only define __NR_clock_gettime64.
- i.e: newer 32-bits ports and s390 on Linux v5.4+.
4. Only define __NR_clock_gettime64 and provide a 64-bit vDSO symbol.
- i.e: newer 32-bits ports.
5. Only define a 32-bit __NR_clock_gettime
- i.e: some 32-bits ports built against kernel headers older than v5.1
(csky, hppa, m68k, microblaze, nios2, and sh).
6. Only define a 32-bit __NR_clock_gettime and provide a 32-bit vDSO symbol.
- i.e: some 32-bits ports built against kernel header older than v5.1
(i386, powerpc32, s390, sparc32, arm, and mips).
7. Define both __NR_clock_gettime64 and __NR_clock_gettime.
- i.e: some 32-bits ports built against kernel headers equal or newer
than v5.1 (csky, hppa, m68k, microblaze, nios2, and sh).
8. Define both __NR_clock_gettime64, provide a 64-bit vDSO, define
__NR_clock_gettime, and provide a 32-bit vDSO.
- i.e: some 32-bits ports built against kernel header equal or newer
then v5.1 (i386, arm, and mips).
9. Define both __NR_clock_gettime64, provide a 64-bit vDSO, and
define __NR_clock_gettime (unlikely, but possible).
10. Define __NR_clock_gettime64 and __NR_clock_gettime and provide
a 32-bit vDSO.
- i.e. sparc32, powerpc32
The original patch cover 1, 2, 3, 4, 5, 6, 7, and 10. The 8 and 9 would
work, but it won't call the 64-bit vDSO.
I think 8th is an important scenario we should support, but we can work this
out after my vDSO refactor. And the 9th scenario seems unlike and we can
also fix it later.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-03 19:29 ` Adhemerval Zanella
2019-12-03 23:39 ` Alistair Francis
@ 2019-12-04 17:24 ` Adhemerval Zanella
2019-12-04 17:48 ` Alistair Francis
1 sibling, 1 reply; 11+ messages in thread
From: Adhemerval Zanella @ 2019-12-04 17:24 UTC (permalink / raw)
To: libc-alpha
On 03/12/2019 16:29, Adhemerval Zanella wrote:
>
>
> On 23/11/2019 19:27, Alistair Francis wrote:
>> With the clock_gettime64 call we prefer to use vDSO. There is no call
>> to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
>> doesn't support vDSO.
>
> Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
As [1] I think we can handle both the cases:
8. Define both __NR_clock_gettime64, provide a 64-bit vDSO, define
__NR_clock_gettime, and provide a 32-bit vDSO.
- i.e: some 32-bits ports built against kernel header equal or newer
then v5.1 (i386, arm, and mips).
9. Define both __NR_clock_gettime64, provide a 64-bit vDSO, and
define __NR_clock_gettime (unlikely, but possible)..
With my vDSO refactor to use relro data structures.
This patch is ok.
Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
[1] https://sourceware.org/ml/libc-alpha/2019-12/msg00142.html
>
>> ---
>> This was 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
>>
>> v3:
>> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
>> - Change code style to be more glibc-ish
>> v2:
>> - Add commit message
>> - Change ret_64 to int
>>
>> include/time.h | 3 ++
>> sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
>> 2 files changed, 46 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/time.h b/include/time.h
>> index d7800eb30f..c19c73ae09 100644
>> --- a/include/time.h
>> +++ b/include/time.h
>> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
>>
>> #if __TIMESIZE == 64
>> # define __clock_nanosleep_time64 __clock_nanosleep
>> +# define __clock_gettime64 __clock_gettime
>> #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)
>> +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
>> +libc_hidden_proto (__clock_gettime64)
>> #endif
>>
>> /* Use in the clock_* functions. Size of the field representing the
>> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
>> index ca40983f6c..875c4fe905 100644
>> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
>> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
>> @@ -17,6 +17,7 @@
>> <https://www.gnu.org/licenses/>. */
>>
>> #include <sysdep.h>
>> +#include <kernel-features.h>
>> #include <errno.h>
>> #include <time.h>
>> #include "kernel-posix-cpu-timers.h"
>> @@ -30,10 +31,51 @@
>>
>> /* Get current value of CLOCK and store it in TP. */
>> int
>> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
>> +{
>> +#ifdef __ASSUME_TIME64_SYSCALLS
>> +# ifndef __NR_clock_gettime64
>> +# define __NR_clock_gettime64 __NR_clock_gettime
>> +# define __vdso_clock_gettime64 __vdso_clock_gettime
>> +# endif
>> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
>
> Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
> __vdso_clock_gettime64/__NR_clock_gettime64 will be called.
>
>> +#else
>> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
>> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
>> + if (ret64 == 0 || errno != ENOSYS)
>> + return ret64;
>> +# endif
> This should be:
>
> #if defined __NR_clock_gettime64
> int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
> [...]
>
> And before sysdep-vdso.h it should be.
>
> #if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined HAVE_CLOCK_GETTIME64_VSYSCALL
> # define HAVE_SYSCALL
> #endif
>
> If !HAVE_SYSCALL the INLINE_VSYSCALL is defined as INLINE_SYCALL.
> It means that if an architecture does provide __NR_clock_gettime64
> but not provide HAVE_CLOCK_GETTIME64_VSYSCALL, it will still use
> the syscall. This code only uses the syscall if the vDSO is still
> present.
>
> I hope I can get my vDSO refactor, which would simplify a bit this
> code.
>
>> + struct timespec tp32;
>> + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
>> + if (ret == 0)
>> + *tp = valid_timespec_to_timespec64 (tp32);
>> + return ret;
>> +#endif
>> +}
>
> Ok, old fallback to time32 vdso/syscall.
>
>> +
>> +#if __TIMESIZE != 64
>> +int
>> __clock_gettime (clockid_t clock_id, struct timespec *tp)
>> {
>> - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
>> + int ret;
>> + struct __timespec64 tp64;
>> +
>> + ret = __clock_gettime64 (clock_id, &tp64);
>> +
>> + if (ret == 0)
>> + {
>> + if (! in_time_t_range (tp64.tv_sec))
>> + {
>> + __set_errno (EOVERFLOW);
>> + return -1;
>> + }
>> +
>> + *tp = valid_timespec64_to_timespec (tp64);
>> + }
>> +
>> + return ret;
>> }
>> +#endif
>> libc_hidden_def (__clock_gettime)
>>
>> versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
>>
>
> Ok.
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-04 17:24 ` Adhemerval Zanella
@ 2019-12-04 17:48 ` Alistair Francis
2019-12-05 10:49 ` Lukasz Majewski
0 siblings, 1 reply; 11+ messages in thread
From: Alistair Francis @ 2019-12-04 17:48 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: GNU C Library
On Wed, Dec 4, 2019 at 9:24 AM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 03/12/2019 16:29, Adhemerval Zanella wrote:
> >
> >
> > On 23/11/2019 19:27, Alistair Francis wrote:
> >> With the clock_gettime64 call we prefer to use vDSO. There is no call
> >> to clock_gettime64 on glibc with older headers and kernel 5.1+ if it
> >> doesn't support vDSO.
> >
> > Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
>
> As [1] I think we can handle both the cases:
>
> 8. Define both __NR_clock_gettime64, provide a 64-bit vDSO, define
> __NR_clock_gettime, and provide a 32-bit vDSO.
> - i.e: some 32-bits ports built against kernel header equal or newer
> then v5.1 (i386, arm, and mips).
>
> 9. Define both __NR_clock_gettime64, provide a 64-bit vDSO, and
> define __NR_clock_gettime (unlikely, but possible)..
>
> With my vDSO refactor to use relro data structures.
>
> This patch is ok.
>
> Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
Thanks :)
I'll apply these two patches later today unless there are any other comments.
Alistair
>
> [1] https://sourceware.org/ml/libc-alpha/2019-12/msg00142.html
>
> >
> >> ---
> >> This was 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
> >>
> >> v3:
> >> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have __NR_clock_gettime64
> >> - Change code style to be more glibc-ish
> >> v2:
> >> - Add commit message
> >> - Change ret_64 to int
> >>
> >> include/time.h | 3 ++
> >> sysdeps/unix/sysv/linux/clock_gettime.c | 44 ++++++++++++++++++++++++-
> >> 2 files changed, 46 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/include/time.h b/include/time.h
> >> index d7800eb30f..c19c73ae09 100644
> >> --- a/include/time.h
> >> +++ b/include/time.h
> >> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1, time_t time0);
> >>
> >> #if __TIMESIZE == 64
> >> # define __clock_nanosleep_time64 __clock_nanosleep
> >> +# define __clock_gettime64 __clock_gettime
> >> #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)
> >> +extern int __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp);
> >> +libc_hidden_proto (__clock_gettime64)
> >> #endif
> >>
> >> /* Use in the clock_* functions. Size of the field representing the
> >> diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
> >> index ca40983f6c..875c4fe905 100644
> >> --- a/sysdeps/unix/sysv/linux/clock_gettime.c
> >> +++ b/sysdeps/unix/sysv/linux/clock_gettime.c
> >> @@ -17,6 +17,7 @@
> >> <https://www.gnu.org/licenses/>. */
> >>
> >> #include <sysdep.h>
> >> +#include <kernel-features.h>
> >> #include <errno.h>
> >> #include <time.h>
> >> #include "kernel-posix-cpu-timers.h"
> >> @@ -30,10 +31,51 @@
> >>
> >> /* Get current value of CLOCK and store it in TP. */
> >> int
> >> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> >> +{
> >> +#ifdef __ASSUME_TIME64_SYSCALLS
> >> +# ifndef __NR_clock_gettime64
> >> +# define __NR_clock_gettime64 __NR_clock_gettime
> >> +# define __vdso_clock_gettime64 __vdso_clock_gettime
> >> +# endif
> >> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> >
> > Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
> > __vdso_clock_gettime64/__NR_clock_gettime64 will be called.
> >
> >> +#else
> >> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
> >> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> >> + if (ret64 == 0 || errno != ENOSYS)
> >> + return ret64;
> >> +# endif
> > This should be:
> >
> > #if defined __NR_clock_gettime64
> > int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
> > [...]
> >
> > And before sysdep-vdso.h it should be.
> >
> > #if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined HAVE_CLOCK_GETTIME64_VSYSCALL
> > # define HAVE_SYSCALL
> > #endif
> >
> > If !HAVE_SYSCALL the INLINE_VSYSCALL is defined as INLINE_SYCALL.
> > It means that if an architecture does provide __NR_clock_gettime64
> > but not provide HAVE_CLOCK_GETTIME64_VSYSCALL, it will still use
> > the syscall. This code only uses the syscall if the vDSO is still
> > present.
> >
> > I hope I can get my vDSO refactor, which would simplify a bit this
> > code.
> >
> >> + struct timespec tp32;
> >> + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> >> + if (ret == 0)
> >> + *tp = valid_timespec_to_timespec64 (tp32);
> >> + return ret;
> >> +#endif
> >> +}
> >
> > Ok, old fallback to time32 vdso/syscall.
> >
> >> +
> >> +#if __TIMESIZE != 64
> >> +int
> >> __clock_gettime (clockid_t clock_id, struct timespec *tp)
> >> {
> >> - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> >> + int ret;
> >> + struct __timespec64 tp64;
> >> +
> >> + ret = __clock_gettime64 (clock_id, &tp64);
> >> +
> >> + if (ret == 0)
> >> + {
> >> + if (! in_time_t_range (tp64.tv_sec))
> >> + {
> >> + __set_errno (EOVERFLOW);
> >> + return -1;
> >> + }
> >> +
> >> + *tp = valid_timespec64_to_timespec (tp64);
> >> + }
> >> +
> >> + return ret;
> >> }
> >> +#endif
> >> libc_hidden_def (__clock_gettime)
> >>
> >> versioned_symbol (libc, __clock_gettime, clock_gettime, GLIBC_2_17);
> >>
> >
> > Ok.
> >
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-04 17:48 ` Alistair Francis
@ 2019-12-05 10:49 ` Lukasz Majewski
2019-12-05 11:11 ` Arnd Bergmann
0 siblings, 1 reply; 11+ messages in thread
From: Lukasz Majewski @ 2019-12-05 10:49 UTC (permalink / raw)
To: Alistair Francis
Cc: Adhemerval Zanella, GNU C Library, Alistair Francis, arnd
[-- Attachment #1.1: Type: text/plain, Size: 7460 bytes --]
Hi Alistair,
> On Wed, Dec 4, 2019 at 9:24 AM Adhemerval Zanella
> <adhemerval.zanella@linaro.org> wrote:
> >
> >
> >
> > On 03/12/2019 16:29, Adhemerval Zanella wrote:
> > >
> > >
> > > On 23/11/2019 19:27, Alistair Francis wrote:
> > >> With the clock_gettime64 call we prefer to use vDSO. There is no
> > >> call to clock_gettime64 on glibc with older headers and kernel
> > >> 5.1+ if it doesn't support vDSO.
> > >
> > > Ok with the fix below regarding HAVE_CLOCK_GETTIME64_VSYSCALL.
> >
> > As [1] I think we can handle both the cases:
> >
> > 8. Define both __NR_clock_gettime64, provide a 64-bit vDSO,
> > define __NR_clock_gettime, and provide a 32-bit vDSO.
> > - i.e: some 32-bits ports built against kernel header equal
> > or newer then v5.1 (i386, arm, and mips).
> >
> > 9. Define both __NR_clock_gettime64, provide a 64-bit vDSO, and
> > define __NR_clock_gettime (unlikely, but possible)..
> >
> > With my vDSO refactor to use relro data structures.
> >
> > This patch is ok.
> >
> > Reviewed-by: Adhemerval Zanella <adhemerval.zanella@linaro.org>
>
> Thanks :)
>
> I'll apply these two patches later today unless there are any other
> comments.
Thank you for preparing and applying this patch.
Just informative note (for the record). For ARM 32 bit systems - after
checking with [1] it has been apparent that those systems doesn't yet
support __vdso_clock_gettime64 [2].
If anybody would need to build glibc with headers from Linux 5.1+ (with
clock_gettime64 support), he/she shall apply attached patch.
This is not a problem for now, as we still have some time until
__ASSUME_TIME64_SYSCALLS is enabled by default in glibc.
Moreover, I do guess that __vdso_clock_gettime64 for ARM 32 bits will
be available by then :-).
Links:
[1] - https://github.com/lmajewski/meta-y2038
[2] -
https://elixir.bootlin.com/linux/latest/ident/__vdso_clock_gettime64
>
> Alistair
>
> >
> > [1] https://sourceware.org/ml/libc-alpha/2019-12/msg00142.html
> >
> > >
> > >> ---
> > >> This was 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
> > >>
> > >> v3:
> > >> - Assume HAVE_CLOCK_GETTIME64_VSYSCALL means we have
> > >> __NR_clock_gettime64
> > >> - Change code style to be more glibc-ish
> > >> v2:
> > >> - Add commit message
> > >> - Change ret_64 to int
> > >>
> > >> include/time.h | 3 ++
> > >> sysdeps/unix/sysv/linux/clock_gettime.c | 44
> > >> ++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 1
> > >> deletion(-)
> > >>
> > >> diff --git a/include/time.h b/include/time.h
> > >> index d7800eb30f..c19c73ae09 100644
> > >> --- a/include/time.h
> > >> +++ b/include/time.h
> > >> @@ -211,11 +211,14 @@ extern double __difftime (time_t time1,
> > >> time_t time0);
> > >>
> > >> #if __TIMESIZE == 64
> > >> # define __clock_nanosleep_time64 __clock_nanosleep
> > >> +# define __clock_gettime64 __clock_gettime
> > >> #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)
> > >> +extern int __clock_gettime64 (clockid_t clock_id, struct
> > >> __timespec64 *tp); +libc_hidden_proto (__clock_gettime64)
> > >> #endif
> > >>
> > >> /* Use in the clock_* functions. Size of the field
> > >> representing the diff --git
> > >> a/sysdeps/unix/sysv/linux/clock_gettime.c
> > >> b/sysdeps/unix/sysv/linux/clock_gettime.c index
> > >> ca40983f6c..875c4fe905 100644 ---
> > >> a/sysdeps/unix/sysv/linux/clock_gettime.c +++
> > >> b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -17,6 +17,7 @@
> > >> <https://www.gnu.org/licenses/>. */
> > >>
> > >> #include <sysdep.h>
> > >> +#include <kernel-features.h>
> > >> #include <errno.h>
> > >> #include <time.h>
> > >> #include "kernel-posix-cpu-timers.h"
> > >> @@ -30,10 +31,51 @@
> > >>
> > >> /* Get current value of CLOCK and store it in TP. */
> > >> int
> > >> +__clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
> > >> +{
> > >> +#ifdef __ASSUME_TIME64_SYSCALLS
> > >> +# ifndef __NR_clock_gettime64
> > >> +# define __NR_clock_gettime64 __NR_clock_gettime
> > >> +# define __vdso_clock_gettime64 __vdso_clock_gettime
> > >> +# endif
> > >> + return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
> > >
> > > Ok, in this case either __vdso_clock_gettime/__NR_clock_gettime or
> > > __vdso_clock_gettime64/__NR_clock_gettime64 will be called.
> > >
> > >> +#else
> > >> +# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
> > >> + int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id,
> > >> tp);
> > >> + if (ret64 == 0 || errno != ENOSYS)
> > >> + return ret64;
> > >> +# endif
> > > This should be:
> > >
> > > #if defined __NR_clock_gettime64
> > > int ret64 = INLINE_VSYSCALL (clock_gettime64, ...)
> > > [...]
> > >
> > > And before sysdep-vdso.h it should be.
> > >
> > > #if defined HAVE_CLOCK_GETTIME_VSYSCALL || defined
> > > HAVE_CLOCK_GETTIME64_VSYSCALL # define HAVE_SYSCALL
> > > #endif
> > >
> > > If !HAVE_SYSCALL the INLINE_VSYSCALL is defined as INLINE_SYCALL.
> > > It means that if an architecture does provide __NR_clock_gettime64
> > > but not provide HAVE_CLOCK_GETTIME64_VSYSCALL, it will still use
> > > the syscall. This code only uses the syscall if the vDSO is still
> > > present.
> > >
> > > I hope I can get my vDSO refactor, which would simplify a bit this
> > > code.
> > >
> > >> + struct timespec tp32;
> > >> + int ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
> > >> + if (ret == 0)
> > >> + *tp = valid_timespec_to_timespec64 (tp32);
> > >> + return ret;
> > >> +#endif
> > >> +}
> > >
> > > Ok, old fallback to time32 vdso/syscall.
> > >
> > >> +
> > >> +#if __TIMESIZE != 64
> > >> +int
> > >> __clock_gettime (clockid_t clock_id, struct timespec *tp)
> > >> {
> > >> - return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp);
> > >> + int ret;
> > >> + struct __timespec64 tp64;
> > >> +
> > >> + ret = __clock_gettime64 (clock_id, &tp64);
> > >> +
> > >> + if (ret == 0)
> > >> + {
> > >> + if (! in_time_t_range (tp64.tv_sec))
> > >> + {
> > >> + __set_errno (EOVERFLOW);
> > >> + return -1;
> > >> + }
> > >> +
> > >> + *tp = valid_timespec64_to_timespec (tp64);
> > >> + }
> > >> +
> > >> + return ret;
> > >> }
> > >> +#endif
> > >> libc_hidden_def (__clock_gettime)
> > >>
> > >> versioned_symbol (libc, __clock_gettime, clock_gettime,
> > >> GLIBC_2_17);
> > >
> > > Ok.
> > >
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1.2: 0001-Y2038-clock_gettime-Use-non-vDSO-syscall-for-clock_g.patch --]
[-- Type: text/x-patch, Size: 1353 bytes --]
From d48860f4f8a8d75e689ee7f91214698598fbbcb8 Mon Sep 17 00:00:00 2001
From: Lukasz Majewski <lukma@denx.de>
Date: Thu, 5 Dec 2019 11:12:48 +0100
Subject: [PATCH] Y2038: clock_gettime: Use non vDSO syscall for
clock_gettime64 on 32 bit ARM
Up till Linux v5.4 [*], there is no 64 bit version of __vdso_clock_gettime64()
available for 32 bit ARM.
For that reason one shall use non VDSO syscall for this purpose.
Link:
[*] - https://elixir.bootlin.com/linux/latest/ident/__vdso_clock_gettime64
---
sysdeps/unix/sysv/linux/clock_gettime.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c
index 875c4fe905..a717d8d88a 100644
--- a/sysdeps/unix/sysv/linux/clock_gettime.c
+++ b/sysdeps/unix/sysv/linux/clock_gettime.c
@@ -38,7 +38,11 @@ __clock_gettime64 (clockid_t clock_id, struct __timespec64 *tp)
# define __NR_clock_gettime64 __NR_clock_gettime
# define __vdso_clock_gettime64 __vdso_clock_gettime
# endif
+# ifdef HAVE_CLOCK_GETTIME64_VSYSCALL
return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
+# else
+ return INLINE_SYSCALL (clock_gettime64, 2, clock_id, tp);
+# endif
#else
# if defined HAVE_CLOCK_GETTIME64_VSYSCALL
int ret64 = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
--
2.20.1
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-05 10:49 ` Lukasz Majewski
@ 2019-12-05 11:11 ` Arnd Bergmann
2019-12-05 13:24 ` Lukasz Majewski
0 siblings, 1 reply; 11+ messages in thread
From: Arnd Bergmann @ 2019-12-05 11:11 UTC (permalink / raw)
To: Lukasz Majewski
Cc: Alistair Francis, Adhemerval Zanella, GNU C Library, Alistair Francis
On Thu, Dec 5, 2019 at 11:48 AM Lukasz Majewski <lukma@denx.de> wrote:
> This is not a problem for now, as we still have some time until
> __ASSUME_TIME64_SYSCALLS is enabled by default in glibc.
> Moreover, I do guess that __vdso_clock_gettime64 for ARM 32 bits will
> be available by then :-).
Have you checked mainline? The generic vdso support including
clock_gettime64 was merged for ARM in v5.5, though it is still
missing on nds32, powerpc32, riscv32, s390 (compat mode only)
and sparc32. No idea if all of them will ever do the change, some
might not care about 32 bit enough any more. Then again, a lot
of architectures don't have support vdso at all, so you could treat
them the same way.
Arnd
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable
2019-12-05 11:11 ` Arnd Bergmann
@ 2019-12-05 13:24 ` Lukasz Majewski
0 siblings, 0 replies; 11+ messages in thread
From: Lukasz Majewski @ 2019-12-05 13:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Alistair Francis, Adhemerval Zanella, GNU C Library, Alistair Francis
[-- Attachment #1: Type: text/plain, Size: 1237 bytes --]
Hi Arnd,
> On Thu, Dec 5, 2019 at 11:48 AM Lukasz Majewski <lukma@denx.de> wrote:
> > This is not a problem for now, as we still have some time until
> > __ASSUME_TIME64_SYSCALLS is enabled by default in glibc.
> > Moreover, I do guess that __vdso_clock_gettime64 for ARM 32 bits
> > will be available by then :-).
>
> Have you checked mainline? The generic vdso support including
> clock_gettime64 was merged for ARM in v5.5,
Indeed - it is now available in linux next-20191205
(SHA1: 74d06efb9c2f99b496eb118b1e941dc4c6404e93).
I've only checked the sources for linux v5.4 tag.
> though it is still
> missing on nds32, powerpc32, riscv32, s390 (compat mode only)
> and sparc32. No idea if all of them will ever do the change, some
> might not care about 32 bit enough any more. Then again, a lot
> of architectures don't have support vdso at all, so you could treat
> them the same way.
Thanks for sharing the information.
>
> Arnd
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2019-12-05 13:24 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-23 22:32 [PATCH v3 1/2] sysdeps: Add clock_gettime64 vDSO Alistair Francis
2019-11-23 22:32 ` [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Alistair Francis
2019-12-03 19:02 ` Alistair Francis
2019-12-03 19:29 ` Adhemerval Zanella
2019-12-03 23:39 ` Alistair Francis
2019-12-04 12:55 ` Adhemerval Zanella
2019-12-04 17:24 ` Adhemerval Zanella
2019-12-04 17:48 ` Alistair Francis
2019-12-05 10:49 ` Lukasz Majewski
2019-12-05 11:11 ` Arnd Bergmann
2019-12-05 13:24 ` Lukasz Majewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).