| Message ID | 20191205114845.54c1cc6f@jawa |
|---|---|
| State | Superseded |
| Headers |
Received: (qmail 24941 invoked by alias); 5 Dec 2019 10:49:01 -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 24927 invoked by uid 89); 5 Dec 2019 10:49:00 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-20.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_LOW autolearn=ham version=3.3.1 spammy=2019-12 X-HELO: mail-out.m-online.net Date: Thu, 5 Dec 2019 11:48:45 +0100 From: Lukasz Majewski <lukma@denx.de> To: Alistair Francis <alistair23@gmail.com> Cc: Adhemerval Zanella <adhemerval.zanella@linaro.org>, GNU C Library <libc-alpha@sourceware.org>, Alistair Francis <alistair.francis@wdc.com>, arnd@arndb.de Subject: Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable Message-ID: <20191205114845.54c1cc6f@jawa> In-Reply-To: <CAKmqyKO8uv=VjWUERJK6E-Dn-+W8tYvXvkzVe0eLzd6e2s6daw@mail.gmail.com> References: <20191123222708.17021-1-alistair.francis@wdc.com> <20191123222708.17021-2-alistair.francis@wdc.com> <621b67c9-028e-e913-8b9a-7978d2d05e04@linaro.org> <51162415-4919-0905-2aa3-4b5d33912f75@linaro.org> <CAKmqyKO8uv=VjWUERJK6E-Dn-+W8tYvXvkzVe0eLzd6e2s6daw@mail.gmail.com> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha512; boundary="Sig_/kfJ=oKpOQiR9d_yYp5UH/Fn"; protocol="application/pgp-signature" |
Commit Message
Lukasz Majewski
Dec. 5, 2019, 10:48 a.m. UTC
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
Comments
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
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
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