From patchwork Tue Dec 3 23:33:12 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Alistair Francis X-Patchwork-Id: 36488 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: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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 Date: Tue, 3 Dec 2019 15:33:12 -0800 Message-ID: Subject: Re: [PATCH v3 2/2] sysdeps/clock_gettime: Use clock_gettime64 if avaliable To: Adhemerval Zanella Cc: GNU C Library isOn Tue, Dec 3, 2019 at 11:29 AM 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. 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 @@ > > . */ > > > > #include > > +#include > > #include > > #include > > #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 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. 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 #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