Message ID | 1f589e5a3fcaa4c103bc83169fffcdea9e1a6b2d.1563321715.git.alistair.francis@wdc.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 15570 invoked by alias); 17 Jul 2019 00:11:45 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 15494 invoked by uid 89); 17 Jul 2019 00:11:45 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3 autolearn=ham version=3.3.1 spammy=Delay X-HELO: esa4.hgst.iphmx.com DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=wdc.com; i=@wdc.com; q=dns/txt; s=dkim.wdc.com; t=1563322304; x=1594858304; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=Ja+hgOQfjiA3Bk/oGNBYwkRBNPtZqBH6L2JpULdsFbY=; b=UhC89DK47958lUnY15AHON0B21k5e8HIRNrmwHDYy0r3GWodGeqXj7ZW t6GbR3vFhMN6ZyIVSWgnXmWcvvfdLDcc0nCg2vbVoggdyAAYxM/hLvJUs s7y9vycEADZDWo6dNvxoZf2AuKsJ5WaiGRD4mwET6zv61jDIjmO6A6kIZ nZAz+a49NOtC4NorVYJEiGkSgsHpoczO+dFplOzBGGEQ0qzWYRnkq9R79 rdBalcdLytO9NfFahfGwX/IujhG0fvv8rVo5ydnyfUOm6mq0nL4QGHOtR tdBaRu88tOD8Q6TXJ6L7pzpksCDhWlWmZ8VaKw1dLnjl5WNDYzLgO8lPY w==; IronPort-SDR: AEmiGnllEmj0W/yQ0md8/Lh/aV7tmMvQEnCiSx6znzVPGxhE4lNNOmvC45VQHh9K7Jdr9vEz5w Di+p2tOtfXV4Kvvo8Aitu/H8XPqSVzi1LmDsRAEO8NFJeZ8WQDtNqmryjANF5HHEubSd0dT1xB gQxYiF9iMZNcCgJz+TkTH644oMjQiGV9ZmI9TsPr/M6OcJcidxwL1THZZ9ErSOKTUCc/6pJNWc /f3i97NVrHlB73X+m4F9Rfx56BjEuJj2dgcU3QL2FURX7f7zlg5wQJfzItVxYo0mBNyBPrKrSs RV0= IronPort-SDR: Gl6mXgwMymifJdbyb7ZkkosaoKtS8SR5Eygh1J1/yLhk0B7E4nCtwAW/mR61jWvujPqkW466JE qFJNjzlaRKT4lmorhaw2GIR9tllHCuwaHBVFeS9KR354ge4crGmT8MK/1o64UUxMYyZsMG4rmq 1sdH4aDBRf84WwnAZcXLkoqaJGrLjdVhzCGGcvC9uvlhwf/9wIqVyANhg1p/rvxLKl3wrdtw/y 1ozo9K+g4QXw0Vy0WZm6a2v+qYGmi+CsHjAZDVEA9/T4IUcUVb7fazQO+wgktpLwZBhAscfo2Q Xdxz1g2TRRdaxepK1WhaPMAN IronPort-SDR: 9DZs/PNUDNHwl/8E8QB7KTSXg3p1dPJQGzBLborV10Z7e9QDpv3bN/50E5WI3gwIF6JvTGlwMQ dbxl5pY0qujBG+iGvlK4VG9xHUb9mdgBae7stXy04DotdFzPf4DOswv4xSRz+1vFqT1CJYNnp1 Owdz0j7XXrRme5h2eQyvS0Tykr/Gw1Ryr3896PAnyHYtLOcrZa+mtN4E+Si1hluAasCCNYWNFX rMNfjqnIgKdVlzKNsDhDrnBoVljmgZ7Sx7HX/Dp0OI2/1viqwrYp1QIIsUOBLP0ZFBSvCbTHFU Hso= From: Alistair Francis <alistair.francis@wdc.com> To: libc-alpha@sourceware.org Cc: arnd@arndb.de, adhemerval.zanella@linaro.org, fweimer@redhat.com, palmer@sifive.com, macro@wdc.com, zongbox@gmail.com, alistair.francis@wdc.com, alistair23@gmail.com Subject: [RFC v3 04/23] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Date: Tue, 16 Jul 2019 17:08:51 -0700 Message-Id: <1f589e5a3fcaa4c103bc83169fffcdea9e1a6b2d.1563321715.git.alistair.francis@wdc.com> In-Reply-To: <cover.1563321715.git.alistair.francis@wdc.com> References: <cover.1563321715.git.alistair.francis@wdc.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit |
Commit Message
Alistair Francis
July 17, 2019, 12:08 a.m. UTC
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
nptl/pthread_mutex_timedlock.c | 7 ++++++
sysdeps/unix/sysv/linux/clock_gettime.c | 30 +++++++++++++++++++++++++
2 files changed, 37 insertions(+)
Comments
* Alistair Francis: > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index 52c258e33d..8d9cae7d87 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -402,10 +402,17 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > /* Delay the thread until the timeout is reached. > Then return ETIMEDOUT. */ > struct timespec reltime; > +#ifdef __NR_clock_gettime64 > + struct __timespec64 now; > + > + INTERNAL_SYSCALL (clock_gettime64, __err, 2, CLOCK_REALTIME, > + &now); > +#else > struct timespec now; > > INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, > &now); > +#endif > reltime.tv_sec = abstime->tv_sec - now.tv_sec; > reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > if (reltime.tv_nsec < 0) I believe this needs to be updated for correctness (truncation of tv_sec) if ever ported to architectures where __nanosleep_nocancel takes a 32-bit time_t argument. I don't know what our plans are regarding to that. If you had #define __NR_clock_gettime64 __NR_clock_gettime in <sysdep.h>, you wouldn't need this change. > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c > index 5fc47fb7dc..4832069c34 100644 > --- a/sysdeps/unix/sysv/linux/clock_gettime.c > +++ b/sysdeps/unix/sysv/linux/clock_gettime.c > @@ -27,10 +27,40 @@ > #include <sysdep-vdso.h> > > /* Get current value of CLOCK and store it in TP. */ > + > +#if __WORDSIZE == 32 > +int > +__clock_gettime (clockid_t clock_id, struct timespec *tp) > +{ > + int ret; > + > +#ifdef __NR_clock_gettime64 > + struct __timespec64 tp64; > + ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, &tp64); > + > + tp->tv_sec = tp64.tv_sec; > + tp->tv_nsec = tp64.tv_nsec; > + > + if (! in_time_t_range (tp->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > +#endif > + > +#ifndef __ASSUME_TIME64_SYSCALLS > + ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); > +#endif > + > + return ret; > +} I think this has the same problems as the timespec_get patch. Thanks, Florian
On Jul 16 2019, Alistair Francis <alistair.francis@wdc.com> wrote: > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c > index 5fc47fb7dc..4832069c34 100644 > --- a/sysdeps/unix/sysv/linux/clock_gettime.c > +++ b/sysdeps/unix/sysv/linux/clock_gettime.c > @@ -27,10 +27,40 @@ > #include <sysdep-vdso.h> > > /* Get current value of CLOCK and store it in TP. */ > + > +#if __WORDSIZE == 32 > +int > +__clock_gettime (clockid_t clock_id, struct timespec *tp) > +{ > + int ret; > + > +#ifdef __NR_clock_gettime64 > + struct __timespec64 tp64; > + ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, &tp64); > + > + tp->tv_sec = tp64.tv_sec; > + tp->tv_nsec = tp64.tv_nsec; This needs to check if clock_gettime64 was successful. Andreas.
On Wed, Jul 17, 2019 at 7:39 AM Florian Weimer <fweimer@redhat.com> wrote: > > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > > index 52c258e33d..8d9cae7d87 100644 > > --- a/nptl/pthread_mutex_timedlock.c > > +++ b/nptl/pthread_mutex_timedlock.c > > @@ -402,10 +402,17 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > > /* Delay the thread until the timeout is reached. > > Then return ETIMEDOUT. */ > > struct timespec reltime; > > +#ifdef __NR_clock_gettime64 > > + struct __timespec64 now; > > + > > + INTERNAL_SYSCALL (clock_gettime64, __err, 2, CLOCK_REALTIME, > > + &now); > > +#else > > struct timespec now; > > > > INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, > > &now); > > +#endif > > reltime.tv_sec = abstime->tv_sec - now.tv_sec; > > reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > > if (reltime.tv_nsec < 0) > > I believe this needs to be updated for correctness (truncation of > tv_sec) if ever ported to architectures where __nanosleep_nocancel takes > a 32-bit time_t argument. I don't know what our plans are regarding to > that. > > If you had > > #define __NR_clock_gettime64 __NR_clock_gettime > > in <sysdep.h>, you wouldn't need this change. Could it be changed to just call internal __clock_gettime64() and __nanosleep_nocancel_time64() functions that will then take care of the architecture specifics like syscall number, vdso and truncation? Arnd
* Arnd Bergmann: >> I believe this needs to be updated for correctness (truncation of >> tv_sec) if ever ported to architectures where __nanosleep_nocancel takes >> a 32-bit time_t argument. I don't know what our plans are regarding to >> that. >> >> If you had >> >> #define __NR_clock_gettime64 __NR_clock_gettime >> >> in <sysdep.h>, you wouldn't need this change. > > Could it be changed to just call internal __clock_gettime64() and > __nanosleep_nocancel_time64() functions that will then take care > of the architecture specifics like syscall number, vdso and > truncation? It depends on how these functions behave. I think we need to figure out if/how we maintain vDSO acceleration for clock_gettime and clock_gettime64 first. Thanks, Florian
On Wed, Jul 17, 2019 at 10:44 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Arnd Bergmann: > > >> I believe this needs to be updated for correctness (truncation of > >> tv_sec) if ever ported to architectures where __nanosleep_nocancel takes > >> a 32-bit time_t argument. I don't know what our plans are regarding to > >> that. > >> > >> If you had > >> > >> #define __NR_clock_gettime64 __NR_clock_gettime > >> > >> in <sysdep.h>, you wouldn't need this change. > > > > Could it be changed to just call internal __clock_gettime64() and > > __nanosleep_nocancel_time64() functions that will then take care > > of the architecture specifics like syscall number, vdso and > > truncation? > > It depends on how these functions behave. I think we need to figure out > if/how we maintain vDSO acceleration for clock_gettime and > clock_gettime64 first. Agreed. In the kernel, the generic clock_gettime64() support only just landed for 5.3, and so far only arm32 and x86-32 have it, while at least mips32, nds32, ppc32, s390 and sparc32 seem to have a the old clock_gettime() and gettimeofday() vdso support. For the internal __clock_gettime64() implementation, we clearly want to call __vdso_clock_gettime64() whenever that is available, but there does not seem to be a good fallback path from there if it is not: - if we try syscall(__NR_clock_gettime64) next, that is a performance regression in systems that don't care about 64-bit time_t - if we try __vdso_clock_gettime() before __NR_clock_gettime64, that can result in incorrect behavior if user space actually relies on being able to work past 2038. Arnd
Hi Alistair, > Signed-off-by: Alistair Francis <alistair.francis@wdc.com> > --- > nptl/pthread_mutex_timedlock.c | 7 ++++++ > sysdeps/unix/sysv/linux/clock_gettime.c | 30 > +++++++++++++++++++++++++ 2 files changed, 37 insertions(+) > > diff --git a/nptl/pthread_mutex_timedlock.c > b/nptl/pthread_mutex_timedlock.c index 52c258e33d..8d9cae7d87 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -402,10 +402,17 @@ __pthread_mutex_clocklock_common > (pthread_mutex_t *mutex, /* Delay the thread until the timeout is > reached. Then return ETIMEDOUT. */ > struct timespec reltime; > +#ifdef __NR_clock_gettime64 > + struct __timespec64 now; > + > + INTERNAL_SYSCALL (clock_gettime64, __err, 2, > CLOCK_REALTIME, > + &now); > +#else > struct timespec now; > > INTERNAL_SYSCALL (clock_gettime, __err, 2, > clockid, &now); > +#endif > reltime.tv_sec = abstime->tv_sec - now.tv_sec; > reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > if (reltime.tv_nsec < 0) > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c > b/sysdeps/unix/sysv/linux/clock_gettime.c index > 5fc47fb7dc..4832069c34 100644 --- > a/sysdeps/unix/sysv/linux/clock_gettime.c +++ > b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -27,10 +27,40 @@ > #include <sysdep-vdso.h> > > /* Get current value of CLOCK and store it in TP. */ > + > +#if __WORDSIZE == 32 > +int > +__clock_gettime (clockid_t clock_id, struct timespec *tp) > +{ > + int ret; > + > +#ifdef __NR_clock_gettime64 > + struct __timespec64 tp64; > + ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, &tp64); > + > + tp->tv_sec = tp64.tv_sec; > + tp->tv_nsec = tp64.tv_nsec; > + > + if (! in_time_t_range (tp->tv_sec)) > + { > + __set_errno (EOVERFLOW); > + return -1; > + } > +#endif > + > +#ifndef __ASSUME_TIME64_SYSCALLS I think that the context of using the __ASSUME_TIME64_SYSCALLS doesn't comply with the semantics which was proposed in its introduction patch [1]. In short: This define means that the system is supporting 64 bit time, not that it has (can use) introduced in Linux 5.1 64 bit time related syscalls (i.e. clock_gettime64, clock_settime64, clock_nanosleep64). Maybe you could consider using the "conversion" approach as proposed for __clock_settime64 conversion [2]? The version in [2] is more "developer/validator" friendly (as suggested by Arnd) as it uses for __TIMESIZE != 64 archs the 32 bit clock_settime. The version from [3] is the one recommended by Joseph (which does the 32 to 64 bit conversion and calls clock_settime64). The full code for a reference with clock_settime conversion and Y2038 support [4]. > + ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); > +#endif > + > + return ret; > +} > +#else > int > __clock_gettime (clockid_t clock_id, struct timespec *tp) > { > return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); > } > +#endif > + > weak_alias (__clock_gettime, clock_gettime) > libc_hidden_def (__clock_gettime) Note: [1] - https://github.com/lmajewski/y2038_glibc/commit/1fdbc6002101a78a8a6a076bbb642b3082c2225d [2] - https://github.com/lmajewski/y2038_glibc/commit/69f842a8519ca13ed11fab0ff1bcc6fa1a524192 [3] - https://github.com/lmajewski/y2038_glibc/commit/fa0f5ff6c942beca383daeff3d48829829ace5b1 [4] - https://github.com/lmajewski/y2038_glibc/commits/Y2038-2.29-glibc-__clock-internal-struct-timespec-v6 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
* Arnd Bergmann: > On Wed, Jul 17, 2019 at 10:44 AM Florian Weimer <fweimer@redhat.com> wrote: >> >> * Arnd Bergmann: >> >> >> I believe this needs to be updated for correctness (truncation of >> >> tv_sec) if ever ported to architectures where __nanosleep_nocancel takes >> >> a 32-bit time_t argument. I don't know what our plans are regarding to >> >> that. >> >> >> >> If you had >> >> >> >> #define __NR_clock_gettime64 __NR_clock_gettime >> >> >> >> in <sysdep.h>, you wouldn't need this change. >> > >> > Could it be changed to just call internal __clock_gettime64() and >> > __nanosleep_nocancel_time64() functions that will then take care >> > of the architecture specifics like syscall number, vdso and >> > truncation? >> >> It depends on how these functions behave. I think we need to figure out >> if/how we maintain vDSO acceleration for clock_gettime and >> clock_gettime64 first. > > Agreed. In the kernel, the generic clock_gettime64() support only just > landed for 5.3, and so far only arm32 and x86-32 have it, while at > least mips32, nds32, ppc32, s390 and sparc32 seem to have a the > old clock_gettime() and gettimeofday() vdso support. > > For the internal __clock_gettime64() implementation, we clearly > want to call __vdso_clock_gettime64() whenever that is available, > but there does not seem to be a good fallback path from there > if it is not: > > - if we try syscall(__NR_clock_gettime64) next, that is a performance > regression in systems that don't care about 64-bit time_t > - if we try __vdso_clock_gettime() before __NR_clock_gettime64, > that can result in incorrect behavior if user space actually relies > on being able to work past 2038. vDSO parsing happens ahead of time, during initialization, so the rules are slightly different than with numbered syscall fallback. System call wrappers with both INLINE_VSYSCALL and fallback are really suspicious and probably not a good idea in general. The real question is whether vDSO and its constituent functions are optional from a userspace ABI perspective. If they are mandatory, we can go straight to the 32-bit vDSO, and, possibly after that, the 32-bit system call. Instead of the vDSO implementation, we would install our own emulation in the function pointer. If the 64-bit vDSO is optional, we would likely have to probe the 64-bit system call once, to keep things sane and simple, and switch between the system call wrapper and the fallback implementation (same as for the mandatory vDSO case). Thanks, Florian
On Wed, Jul 17, 2019 at 5:16 PM Florian Weimer <fweimer@redhat.com> wrote: > > The real question is whether vDSO and its constituent functions are > optional from a userspace ABI perspective. If they are mandatory, we > can go straight to the 32-bit vDSO, and, possibly after that, the 32-bit > system call. Instead of the vDSO implementation, we would install our > own emulation in the function pointer. If the 64-bit vDSO is optional, > we would likely have to probe the 64-bit system call once, to keep > things sane and simple, and switch between the system call wrapper and > the fallback implementation (same as for the mandatory vDSO case). Good question. I assumed that they would be optional because: - Half the Linux architectures don't have any vdso support at all - arm64 traditionally has no support for 32-bit vdso (added in linux-5.3), and still only contains it if an arm32 toolchain is available at kernel build time and we use gcc rather than clang (to be fixed in the future). - I assumed that all libc implementations already need a fallback to plain syscalls to run on older kernels even for architectures that have it today. - The recent rewrite of the vdso implementation in the kernel was not ready for 5.1, so no architecture has clock_gettime64() so far, but a lot of them clock_gettime() I agree that assuming a vdso clock_gettime64() to be present on all architectures would make this much easier for you, but I'd still hope we can find a way to avoid truncating the times if you do that. I have two ideas for how that could be done: - When building for a minimum kernel version of 5.1, don't fall back to __vdso_clock_gettime() or syscall(__NR_clock_gettime) but use the slow path for __clock_gettime64() if the vdso doesn't work. - if __vdso_clock_gettime64() is unavailable and __vdso_clock_gettime() returns negative seconds, fall back to syscall(__NR_clock_gettime). Would either of those meet your requirements? Arnd
* Arnd Bergmann: > I have two ideas for how that could be done: > > - When building for a minimum kernel version of 5.1, don't > fall back to __vdso_clock_gettime() or syscall(__NR_clock_gettime) > but use the slow path for __clock_gettime64() if the vdso doesn't > work. Assuming that clock_gettime64 support is available, yes. > - if __vdso_clock_gettime64() is unavailable and __vdso_clock_gettime() > returns negative seconds, fall back to syscall(__NR_clock_gettime). I don't want to do anything like this. I expect that some of us will eventually use time namespaces to keep programs running (with incorrect time). If we make glibc itself time-sensitive, then things will get horribly complex. > Would either of those meet your requirements? I don't have requirements. I just want something that has limited impact on 64-bit architectures. I don't think probing at startup is too bad, actually. Thanks, Florian
On Thu, Jul 18, 2019 at 10:19 AM Florian Weimer <fweimer@redhat.com> wrote: > > * Arnd Bergmann: > > > I have two ideas for how that could be done: > > > > - When building for a minimum kernel version of 5.1, don't > > fall back to __vdso_clock_gettime() or syscall(__NR_clock_gettime) > > but use the slow path for __clock_gettime64() if the vdso doesn't > > work. > > Assuming that clock_gettime64 support is available, yes. All 32-bit architectures have the clock_gettime64() syscall in 5.1. > > Would either of those meet your requirements? > > I don't have requirements. I just want something that has limited > impact on 64-bit architectures. I don't think probing at startup is too > bad, actually. Ok, fair enough. Arnd
On 18/07/2019 05:18, Florian Weimer wrote: > * Arnd Bergmann: > >> I have two ideas for how that could be done: >> >> - When building for a minimum kernel version of 5.1, don't >> fall back to __vdso_clock_gettime() or syscall(__NR_clock_gettime) >> but use the slow path for __clock_gettime64() if the vdso doesn't >> work. > > Assuming that clock_gettime64 support is available, yes. > >> - if __vdso_clock_gettime64() is unavailable and __vdso_clock_gettime() >> returns negative seconds, fall back to syscall(__NR_clock_gettime). > > I don't want to do anything like this. I expect that some of us will > eventually use time namespaces to keep programs running (with incorrect > time). If we make glibc itself time-sensitive, then things will get > horribly complex. > >> Would either of those meet your requirements? > > I don't have requirements. I just want something that has limited > impact on 64-bit architectures. I don't think probing at startup is too > bad, actually. I hope that my vDSO refactor patchset [1] may simplify the support. For the patchset it just a matter of define the expected vDSO symbol name (HAVE_CLOCK_GETTIME_VSYSCALL for time32), and then {INLINE,INTERNAL}_VSYSCALL will check, demangle, call, and fallback to syscall. I would expect that for time64_t it would be a matter to just add a new define, HAVE_CLOCK_GETTIME64_VSYSCALL, where each architecture would define if were the case. [1] https://sourceware.org/ml/libc-alpha/2019-06/msg00344.html
On Tue, Jul 16, 2019 at 10:39 PM Florian Weimer <fweimer@redhat.com> wrote: > > * Alistair Francis: > > > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > > index 52c258e33d..8d9cae7d87 100644 > > --- a/nptl/pthread_mutex_timedlock.c > > +++ b/nptl/pthread_mutex_timedlock.c > > @@ -402,10 +402,17 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > > /* Delay the thread until the timeout is reached. > > Then return ETIMEDOUT. */ > > struct timespec reltime; > > +#ifdef __NR_clock_gettime64 > > + struct __timespec64 now; > > + > > + INTERNAL_SYSCALL (clock_gettime64, __err, 2, CLOCK_REALTIME, > > + &now); > > +#else > > struct timespec now; > > > > INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, > > &now); > > +#endif > > reltime.tv_sec = abstime->tv_sec - now.tv_sec; > > reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; > > if (reltime.tv_nsec < 0) > > I believe this needs to be updated for correctness (truncation of > tv_sec) if ever ported to architectures where __nanosleep_nocancel takes > a 32-bit time_t argument. I don't know what our plans are regarding to > that. > > If you had > > #define __NR_clock_gettime64 __NR_clock_gettime > > in <sysdep.h>, you wouldn't need this change. Yep, you are right. I have used the #define and removed this change. > > > diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c > > index 5fc47fb7dc..4832069c34 100644 > > --- a/sysdeps/unix/sysv/linux/clock_gettime.c > > +++ b/sysdeps/unix/sysv/linux/clock_gettime.c > > @@ -27,10 +27,40 @@ > > #include <sysdep-vdso.h> > > > > /* Get current value of CLOCK and store it in TP. */ > > + > > +#if __WORDSIZE == 32 > > +int > > +__clock_gettime (clockid_t clock_id, struct timespec *tp) > > +{ > > + int ret; > > + > > +#ifdef __NR_clock_gettime64 > > + struct __timespec64 tp64; > > + ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, &tp64); > > + > > + tp->tv_sec = tp64.tv_sec; > > + tp->tv_nsec = tp64.tv_nsec; > > + > > + if (! in_time_t_range (tp->tv_sec)) > > + { > > + __set_errno (EOVERFLOW); > > + return -1; > > + } > > +#endif > > + > > +#ifndef __ASSUME_TIME64_SYSCALLS > > + ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); > > +#endif > > + > > + return ret; > > +} > > I think this has the same problems as the timespec_get patch. I'm still unclea what that problem is. Lukasz pointed out these don't match the other time64_t conversions so I'll update it to use that style anyway. Alistair > > Thanks, > Florian
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c index 52c258e33d..8d9cae7d87 100644 --- a/nptl/pthread_mutex_timedlock.c +++ b/nptl/pthread_mutex_timedlock.c @@ -402,10 +402,17 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, /* Delay the thread until the timeout is reached. Then return ETIMEDOUT. */ struct timespec reltime; +#ifdef __NR_clock_gettime64 + struct __timespec64 now; + + INTERNAL_SYSCALL (clock_gettime64, __err, 2, CLOCK_REALTIME, + &now); +#else struct timespec now; INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid, &now); +#endif reltime.tv_sec = abstime->tv_sec - now.tv_sec; reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec; if (reltime.tv_nsec < 0) diff --git a/sysdeps/unix/sysv/linux/clock_gettime.c b/sysdeps/unix/sysv/linux/clock_gettime.c index 5fc47fb7dc..4832069c34 100644 --- a/sysdeps/unix/sysv/linux/clock_gettime.c +++ b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -27,10 +27,40 @@ #include <sysdep-vdso.h> /* Get current value of CLOCK and store it in TP. */ + +#if __WORDSIZE == 32 +int +__clock_gettime (clockid_t clock_id, struct timespec *tp) +{ + int ret; + +#ifdef __NR_clock_gettime64 + struct __timespec64 tp64; + ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, &tp64); + + tp->tv_sec = tp64.tv_sec; + tp->tv_nsec = tp64.tv_nsec; + + if (! in_time_t_range (tp->tv_sec)) + { + __set_errno (EOVERFLOW); + return -1; + } +#endif + +#ifndef __ASSUME_TIME64_SYSCALLS + ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); +#endif + + return ret; +} +#else int __clock_gettime (clockid_t clock_id, struct timespec *tp) { return INLINE_VSYSCALL (clock_gettime, 2, clock_id, tp); } +#endif + weak_alias (__clock_gettime, clock_gettime) libc_hidden_def (__clock_gettime)