Message ID | 0d116a8faab58db1952a256c6cb75e7b0f9af444.1563321715.git.alistair.francis@wdc.com |
---|---|
State | New, archived |
Headers |
Received: (qmail 16039 invoked by alias); 17 Jul 2019 00:11:49 -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 15961 invoked by uid 89); 17 Jul 2019 00:11:49 -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=HContent-Transfer-Encoding:8bit X-HELO: esa6.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=1563322307; x=1594858307; h=from:to:cc:subject:date:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=2F0gXE8d/IJZUfdsPOVd/1Mkf2NF7LjAcRIoRsZ/x+8=; b=PnCnSSSMA89n2gZqVZftTxg9E+HTBm5hxuVpt1jFE4m1hEm/MTC+Z1en HMpj0A84frpUY/ZJatNsq47oH42svYwzB/0+Jy3bYl+DeaPW+25pCv9Gr aJe1gaDykH7gig/iDsdwIUboynkiH/ihqghx0eUTBuIGWKkNY9ltt/mV6 DlJ97kmjdfCl4WcxeJzap/abrNZhmwZSs7gFCt9od09N5MZyS7vdrJ0lH dyVKtB0SETIaojNN8wQM5HgbdQS6TM4sgrwQk6coH1WKKMgvFCWANd1fF 3gTQYiwSgjgUmtb2eGr4MyqPINrP2qpLXHF6awJ77imBgLUsmKjDibj/a g==; IronPort-SDR: KLwANiBuvC9iVQQd4PqrFchgck1F2OsfYuU609qnnofLAyEuDgJ1oXc0CkpvI8MKvcTZ08sdvp RRUpsLP9SMeGWgXyZ965DPGpksPDUzyt+ryfFiYH6ozyz/O6vnedTSWf8bb9GJdlAzVkLMysM0 Q4/kont825l/47yrggG2Y+fj3DlvhjdRC9w4C9pGtcEbKldH7BgoLJJcSdwtnGK5NmXqsZ1zp5 j3kWP6ye+QqU1t9oUBTl4ZKx02qsktf7PJj1hoJZAnJDhyetD59HSVt2qC8/B4A6XJ67Uai40Z +9M= IronPort-SDR: FKFhDpTqdT0/BeGNwjzI7heC1fQKjAc4dISQS3xLCnH04b3Xy+oI334fgLaKyiLIV6ILjunj9f nCFHtiCqi/GnAL7T1tCb4/9gSdCk180khI/lhzPHd17Xj00JeqMX7JSAGTZD6Pxbk06aTcpkjS WrIQ7sYl/Q77e4O5Hhei6GHmkgaYLNNcabvP/8ZaZ+OgnglKlPpJH34tWOdt4pN+TkvvCr2niw h9S07JAeLod8iyns2n2Jt52svZgT/0rgtdifhQhiVm9A3s6IcsmaAl337Bk+H11CJOjn558C4T OOitvPBUEOqhoxh8kBpTa9/I IronPort-SDR: JsvQtufBrxEKnc1ZyF2B66/N4hCh3eNTYSmcwZ53RiV/FgZBSs/wEnSzx2Vnh4Q881GYyNV1eB PtAvN7WL7a9wK7ixOvOszQHuhM1bL6Wpel/wSjQDs4FpaU4nDP68xwOaRLmo0p6EKgLX1CgCa2 9mM/ijThfGF3IgWjuoVEo92RpEgMD/kl7xF+qjNt6+nrFnfeofEBDe16PUF8hKqIFp9QTq0ZSn dxShnVMaE7Fgb50NFVpCJOTY5vSs0dTBvRSjmUN7HIGiCulpJ89Am2TLlbQMMGXs10RytCygRG NGo= 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 05/23] sysdeps/timespec_get: Use clock_gettime64 if avaliable Date: Tue, 16 Jul 2019 17:08:54 -0700 Message-Id: <0d116a8faab58db1952a256c6cb75e7b0f9af444.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
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(-)
Comments
* 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