Message ID | CAKmqyKP1DYPVNfHSJ83F1_c+1QPJ1i8zNrSTFbMhy05gcTjoaw@mail.gmail.com |
---|---|
State | Superseded |
Headers |
Received: (qmail 14907 invoked by alias); 3 Dec 2019 23:39:19 -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 14899 invoked by uid 89); 3 Dec 2019 23:39:18 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-19.9 required=5.0 tests=AWL, BAYES_00, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=ison X-HELO: mail-lj1-f194.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=4GODAPmXDQWBKIGVDNK0BHYoK+JUTJXZc2UCFMlkvhk=; b=MsBU7WHM4rtn/j2F5awhKMdmkvQ0PEZEdCayERzGuYYHLntIAIyAvviFaYR/i1/FDT m+RMY5sxfa+IzHf6kzjIVXzknHbgGRFR4Q1DB2RgXfITU3In2zicvuj6cS8k6ZrC1qAj 9/M6IbD8o+4I2hfy2Bks5humvzkay5jRfOHLg35FmZb7eEadbBL9UE5qZ9cEdq1lZ2vi Obhv6eIH+taHDi3FYkJrXgTXbmoUU67m+/kFmxaP/dRM7nKdX3Wwrh6oHRG/vF8JENty PkBWeQqMH+4dO4Vn1jwI3Iyu3Vj01rRz/baT1lS7LNq9P8klOxCYXeXvWWMbnS84tMm0 N8SA== MIME-Version: 1.0 References: <20191123222708.17021-1-alistair.francis@wdc.com> <20191123222708.17021-2-alistair.francis@wdc.com> <621b67c9-028e-e913-8b9a-7978d2d05e04@linaro.org> In-Reply-To: <621b67c9-028e-e913-8b9a-7978d2d05e04@linaro.org> From: Alistair Francis <alistair23@gmail.com> Date: Tue, 3 Dec 2019 15:33:12 -0800 Message-ID: <CAKmqyKP1DYPVNfHSJ83F1_c+1QPJ1i8zNrSTFbMhy05gcTjoaw@mail.gmail.com> Subject: Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable To: Adhemerval Zanella <adhemerval.zanella@linaro.org> Cc: GNU C Library <libc-alpha@sourceware.org> Content-Type: text/plain; charset="UTF-8" |
Commit Message
Alistair Francis
Dec. 3, 2019, 11:33 p.m. UTC
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 #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.
Comments
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.
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