Message ID | 0d116a8faab58db1952a256c6cb75e7b0f9af444.1563321715.git.alistair.francis@wdc.com |
---|---|
State | New |
Headers | show |
Series | [RFC,v3,01/23] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable | expand |
* Alistair Francis: > +#if __WORDSIZE == 32 > +int > +__timespec_get (struct timespec *ts, int base) > +{ > + int ret; > + > +#ifdef __NR_clock_gettime64 > + struct __timespec64 ts64; > + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64); > + > + ts->tv_sec = ts64.tv_sec; > + ts->tv_nsec = ts64.tv_nsec; > + > + if (! in_time_t_range (ts->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > +#endif > + > +#ifndef __ASSUME_TIME64_SYSCALLS > + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); > +#endif I don't think this is right. I think you need to cache clock_gettime64 support somewhere and avoid trying to call the non-existing system call again and again. And the second system call will overwrite the results of the first. Thanks, Florian
On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Alistair Francis: > > > +#if __WORDSIZE == 32 > > +int > > +__timespec_get (struct timespec *ts, int base) > > +{ > > + int ret; > > + > > +#ifdef __NR_clock_gettime64 > > + struct __timespec64 ts64; > > + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64); > > + > > + ts->tv_sec = ts64.tv_sec; > > + ts->tv_nsec = ts64.tv_nsec; > > + > > + if (! in_time_t_range (ts->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > +#endif > > + > > +#ifndef __ASSUME_TIME64_SYSCALLS > > + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); > > +#endif > > I don't think this is right. I think you need to cache clock_gettime64 > support somewhere and avoid trying to call the non-existing system call > again and again. How important is this? It sounds like a micro-optimization for a case that very few people are going to hit, given that running an application with 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot deal with. Arnd
* Arnd Bergmann: > On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * Alistair Francis: >> >> > +#if __WORDSIZE == 32 >> > +int >> > +__timespec_get (struct timespec *ts, int base) >> > +{ >> > + int ret; >> > + >> > +#ifdef __NR_clock_gettime64 >> > + struct __timespec64 ts64; >> > + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64); >> > + >> > + ts->tv_sec = ts64.tv_sec; >> > + ts->tv_nsec = ts64.tv_nsec; >> > + >> > + if (! in_time_t_range (ts->tv_sec)) >> > + { >> > + __set_errno (EOVERFLOW); >> > + return -1; >> > + } >> > +#endif >> > + >> > +#ifndef __ASSUME_TIME64_SYSCALLS >> > + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); >> > +#endif >> >> I don't think this is right. I think you need to cache clock_gettime64 >> support somewhere and avoid trying to call the non-existing system call >> again and again. > > How important is this? It sounds like a micro-optimization for a case that > very few people are going to hit, given that running an application with > 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot > deal with. I don't think it's a microoptimization. On old kernels without clock_gettime64 in the vDSO, 32-bit timespec_get will always make the system call, which fails. Only then the 32-bit clock_gettime in the vDSO is used. This effectively negates the performance benefit of the vDSO, I think. (Not sure if this will even build on non-RV32 as posted, given that we don't have a vDSO variable in glibc for clock_gettime64, but I assume that's going to be fixed if necessary.) Thanks, Florian
On Wed, Jul 17, 2019 at 10:12 AM Florian Weimer <fweimer@redhat.com> wrote: > * Arnd Bergmann: > > On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote: > >> * Alistair Francis: > > How important is this? It sounds like a micro-optimization for a case that > > very few people are going to hit, given that running an application with > > 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot > > deal with. > > I don't think it's a microoptimization. On old kernels without > clock_gettime64 in the vDSO, 32-bit timespec_get will always make the > system call, which fails. Only then the 32-bit clock_gettime in the > vDSO is used. This effectively negates the performance benefit of the > vDSO, I think. I understand that it would be much slower on that particular combination, I just can't think of anyone who would run into this in practice outside of validation testing that makes sure glibc does run this way. If you have any real-world binary built with 64-bit time_t, this will require all library dependencies other than glibc to be built the same way and that in turn won't happen unless someone builds a whole distro for 64-bit time_t, which would be crazy unless they also use a modern kernel. I can understand the need to make it work in principle, but does it have to be efficient? Arnd
* Arnd Bergmann: > I understand that it would be much slower on that particular combination, > I just can't think of anyone who would run into this in practice outside of > validation testing that makes sure glibc does run this way. I think it will happen if you run a glibc which has been built against current kernel headers (and with this patch or something like it) on a host which has an older kernel. Due to the way the patch has been written, it will also impact legacy applications which call the 32-bit functions, not just 32-bit applications which use the 64-bit interfaces. (I have not checked the impact on 64-bit kernels yet, hopefully they will never define __NR_clock_gettime64.) I could be totally wrong about this, or our disagreement could be about the relevance of this scenario. Not sure which. (I think supporting old host kernels is important to our users.) Thanks, Florian
On Wed, Jul 17, 2019 at 10:41 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Arnd Bergmann: > > > I understand that it would be much slower on that particular combination, > > I just can't think of anyone who would run into this in practice outside of > > validation testing that makes sure glibc does run this way. > > I think it will happen if you run a glibc which has been built against > current kernel headers (and with this patch or something like it) on a > host which has an older kernel. Due to the way the patch has been > written, it will also impact legacy applications which call the 32-bit > functions, not just 32-bit applications which use the 64-bit interfaces. Ah right, makes sense. > I could be totally wrong about this, or our disagreement could be about > the relevance of this scenario. Not sure which. > > (I think supporting old host kernels is important to our users.) No, I was just wrong and had only thought about applications calling directly into clock_gettime() while using a 64-bit time_t, but not the case of glibc-internal functions calling __clock_gettime64() for convenience, which is something I even advocated for in one of my other comments. Arnd
Hi Alistair, > This will break other 32-bit targets > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > sysdeps/unix/sysv/linux/timespec_get.c | 37 > +++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 > deletion(-) > > diff --git a/sysdeps/unix/sysv/linux/timespec_get.c > b/sysdeps/unix/sysv/linux/timespec_get.c index 52080ddf08..78fa2aba1b > 100644 --- a/sysdeps/unix/sysv/linux/timespec_get.c > +++ b/sysdeps/unix/sysv/linux/timespec_get.c > @@ -24,6 +24,36 @@ > #endif > #include <sysdep-vdso.h> > > + > +#if __WORDSIZE == 32 > +int > +__timespec_get (struct timespec *ts, int base) > +{ > + int ret; > + > +#ifdef __NR_clock_gettime64 > + struct __timespec64 ts64; > + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, > &ts64); + > + ts->tv_sec = ts64.tv_sec; > + ts->tv_nsec = ts64.tv_nsec; > + You may consider using following helper functions (which are the part of Y2038 support): [1] > + if (! in_time_t_range (ts->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > +#endif > + > +#ifndef __ASSUME_TIME64_SYSCALLS > + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, > ts); +#endif > + > + return ret; > +} > + > +#else > + > /* Set TS to calendar time based in time base BASE. */ > int > timespec_get (struct timespec *ts, int base) > @@ -33,9 +63,13 @@ timespec_get (struct timespec *ts, int base) > int res; > INTERNAL_SYSCALL_DECL (err); > case TIME_UTC: > +#if __WORDSIZE == 32 > + res = __timespec_get (*ts, base); > +#else > res = INTERNAL_VSYSCALL (clock_gettime, err, 2, > CLOCK_REALTIME, ts); +#endif > if (INTERNAL_SYSCALL_ERROR_P (res, err)) > - return 0; > + return 0; > break; > > default: > @@ -44,3 +78,4 @@ timespec_get (struct timespec *ts, int base) > > return base; > } > +#endif Note: [1] - https://github.com/lmajewski/y2038_glibc/commit/12ac380db090219aac4ed8b0ef179b9fcc4c296e 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
On Wed, 17 Jul 2019, Arnd Bergmann wrote: > > I don't think this is right. I think you need to cache clock_gettime64 > > support somewhere and avoid trying to call the non-existing system call > > again and again. > > How important is this? It sounds like a micro-optimization for a case that > very few people are going to hit, given that running an application with > 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot > deal with. It's a generic design question for all the time64 patches that might end up using one or another syscall at runtime depending on kernel support - we'll need to arrive at a consensus on whether such caching (probably shared between all relevant syscalls) is desired. It's not clear to me whether the case of _TIME_BITS=64 on an old kernel will end up as a rare case or not. And as per my previous comments, many existing functions using 32-bit times should end up as thin wrappers round functions using 64-bit times, so potentially making two syscalls every time unless there is such caching.
diff --git a/sysdeps/unix/sysv/linux/timespec_get.c b/sysdeps/unix/sysv/linux/timespec_get.c index 52080ddf08..78fa2aba1b 100644 --- a/sysdeps/unix/sysv/linux/timespec_get.c +++ b/sysdeps/unix/sysv/linux/timespec_get.c @@ -24,6 +24,36 @@ #endif #include <sysdep-vdso.h> + +#if __WORDSIZE == 32 +int +__timespec_get (struct timespec *ts, int base) +{ + int ret; + +#ifdef __NR_clock_gettime64 + struct __timespec64 ts64; + ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64); + + ts->tv_sec = ts64.tv_sec; + ts->tv_nsec = ts64.tv_nsec; + + if (! in_time_t_range (ts->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } +#endif + +#ifndef __ASSUME_TIME64_SYSCALLS + ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); +#endif + + return ret; +} + +#else + /* Set TS to calendar time based in time base BASE. */ int timespec_get (struct timespec *ts, int base) @@ -33,9 +63,13 @@ timespec_get (struct timespec *ts, int base) int res; INTERNAL_SYSCALL_DECL (err); case TIME_UTC: +#if __WORDSIZE == 32 + res = __timespec_get (*ts, base); +#else res = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts); +#endif if (INTERNAL_SYSCALL_ERROR_P (res, err)) - return 0; + return 0; break; default: @@ -44,3 +78,4 @@ timespec_get (struct timespec *ts, int base) return base; } +#endif
This will break other 32-bit targets Signed-off-by: Alistair Francis <alistair.francis@wdc.com> --- sysdeps/unix/sysv/linux/timespec_get.c | 37 +++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1 deletion(-)