* Zack Weinberg:
> Most ports were using gettimeofday to implement time, or they were> making a direct (v)syscall. Unconditionally switch to using> clock_gettime instead. All sysdeps implementations of time are> removed.
I'm sorry, but this is clearly not advisable because clock_gettime is
almost an order of magnitude slower than time.
A synthetic benchmark with back-to-back time calls takes 2.3 ns on a
Xeon Gold 6126 CPU, but 17.9 ns on your branch.
Given that time has no stringent accuracy requirements, this shouldn't
come as a surprise.
Thanks,
Florian

On Wed, Aug 28, 2019 at 2:16 PM Florian Weimer <fweimer@redhat.com> wrote:
>> * Zack Weinberg:>> > Most ports were using gettimeofday to implement time, or they were> > making a direct (v)syscall. Unconditionally switch to using> > clock_gettime instead. All sysdeps implementations of time are> > removed.>> I'm sorry, but this is clearly not advisable because clock_gettime is> almost an order of magnitude slower than time.
Hmm. How do we reconcile this with the (Linux) kernel maintainers'
stated intentions to provide only clock_gettime in the future? We
could use CLOCK_REALTIME_COARSE when available, I suppose; can you see
whether that recovers the lost performance, or else send me your
benchmark code so I can try it?
zw

Florian Weimer wrote:
> Given that time has no stringent accuracy requirements, this shouldn't> come as a surprise.
In what sense does 'time' have less-stringent accuracy requirements than
clock_gettime (CLOCK_REALTIME) does? Can 'time' return a value that disagrees
with the time_t part of what clock_gettime (CLOCK_REALTIME) returns? Any such
disagreement would cause application bugs that could well be unlikely and hard
to track down; also, if there is disagreement, replacing calls to 'time' with
calls to clock_gettime might not be advisable even aside from performance issues.

* Paul Eggert:
> Florian Weimer wrote:>> Given that time has no stringent accuracy requirements, this shouldn't>> come as a surprise.>> In what sense does 'time' have less-stringent accuracy requirements> than clock_gettime (CLOCK_REALTIME) does? Can 'time' return a value> that disagrees with the time_t part of what clock_gettime> (CLOCK_REALTIME) returns? Any such disagreement would cause> application bugs that could well be unlikely and hard to track down;> also, if there is disagreement, replacing calls to 'time' with calls> to clock_gettime might not be advisable even aside from performance> issues.
Maybe accuracy is the wrong word. But time can definitely return the
value of a variable that is incremented periodically from the timer
interrupt. That's not possible for gettimeofday or clock_gettime with
CLOCK_REALTIME (from a quality-of-implementation perspective).
Thanks,
Florian

Florian Weimer wrote:
> time can definitely return the> value of a variable that is incremented periodically from the timer> interrupt.
Is that variable the one that CLOCK_REALTIME_COARSE uses? If so, and if we're
going to replace calls to 'time' with calls to 'clock_realtime', we can do
either of the following:
* Use CLOCK_REALTIME_COARSE. This takes less CPU time and its behavior better
matches what the current glibc does.
* Use CLOCK_REALTIME. This will lessen bugs due to naive code (quite possibly
some code within glibc!) which assumes that 'time (0)' and clock_gettime
(CLOCK_REALTIME, ...)' use the same clock.
It sounds you're leaning towards (1) and I'm inclined to agree. However,
shouldn't the manual say that 'time' does not necessarily agree with
CLOCK_REALTIME? The current behavior is a trap for the unwary.
To illustrate the problems with naive code, see the attached program. On my
Fedora 30 x86-64 desktop, './a.out' outputs this:
clock_gettime (CLOCK_REALTIME, ...) yielded a seconds count of 1567026298;
then time (0) yielded a seconds count of 1567026297, which was 1 s earlier.
Time warp!
In contrast, './a.out 1' (which uses CLOCK_REALTIME_COARSE) finds no
discrepancies in 2**32 attempts.

* Paul Eggert:
> Florian Weimer wrote:>> time can definitely return the>> value of a variable that is incremented periodically from the timer>> interrupt.>> Is that variable the one that CLOCK_REALTIME_COARSE uses? If so, and> if we're going to replace calls to 'time' with calls to> 'clock_realtime', we can do either of the following:>> * Use CLOCK_REALTIME_COARSE. This takes less CPU time and its behavior> better matches what the current glibc does.>> * Use CLOCK_REALTIME. This will lessen bugs due to naive code (quite> possibly some code within glibc!) which assumes that 'time (0)' and> clock_gettime (CLOCK_REALTIME, ...)' use the same clock.
I think we should keep using the time entry in the vDSO. This
consolidation is just not possible to do for performance reasons.
> It sounds you're leaning towards (1) and I'm inclined to> agree. However, shouldn't the manual say that 'time' does not> necessarily agree with CLOCK_REALTIME? The current behavior is a trap> for the unwary.
Yes, clarifying the manual would make sense. I do not know where the
discrepancy comes from. It could just be that CLOCK_REALTIME performs
rounding, while time performs truncation.
Thanks,
Florian

On Wed, Aug 28, 2019 at 5:39 PM Florian Weimer <fweimer@redhat.com> wrote:
> I think we should keep using the time entry in the vDSO. This> consolidation is just not possible to do for performance reasons.
To be crystal clear about where I'm coming from, I think this
consolidation is *necessary* to clear the way for the y2038 patches.
And I was under the impression that the kernel people wanted to
withdraw support for the time and gettimeofday entry points (at least
for new architectures going forward).
So I'm only looking for ways to mitigate the performance impact at
this point. Before I back off on "henceforth we only call the
kernel's clock_gettime" I would need to hear *both* a really
compelling argument for why we need to keep calling the time or
gettimeofday entry points -- a few ns more expense on a call that only
reports the time to the nearest second isn't going to cut it -- *and*
an explanation of why it won't interfere with the y2038 work and/or
why we won't have to stop using them in the future anyway.
zw

* Zack Weinberg:
> On Wed, Aug 28, 2019 at 5:39 PM Florian Weimer <fweimer@redhat.com> wrote:>> I think we should keep using the time entry in the vDSO. This>> consolidation is just not possible to do for performance reasons.>> To be crystal clear about where I'm coming from, I think this> consolidation is *necessary* to clear the way for the y2038 patches.
But it's not an absolute technical requirement, right?
I mean, if we wanted, we could still add a file like
sysdeps/unix/sysv/linux/x86_64/time.c and do things differently there?
(Although we'd also have to change the vDSO setup code for an efficient
implementation.)
> And I was under the impression that the kernel people wanted to> withdraw support for the time and gettimeofday entry points (at least> for new architectures going forward).
Yes, but that doesn't mean they want performance regressions on x86-64.
I think.
> So I'm only looking for ways to mitigate the performance impact at> this point. Before I back off on "henceforth we only call the> kernel's clock_gettime" I would need to hear *both* a really> compelling argument for why we need to keep calling the time or> gettimeofday entry points -- a few ns more expense on a call that only> reports the time to the nearest second isn't going to cut it -- *and*> an explanation of why it won't interfere with the y2038 work and/or> why we won't have to stop using them in the future anyway.
I expect that people have time calls in (binary) logging code where this
is visible. I think EXPLAIN ANALYZE in PostgreSQL also rather sensitive
to timing performance, but fortunately it already uses
clock_gettimeofday.
I'm not too concerned here (assuming that we *can* still optimize the
time function on select architectures if that proves necessary in a
couple of years). It's just that I really dislike the idea of
performance regressions on 64-bit architectures as a side effect of
Y2038 support on 32-bit architectures.
Thanks,
Florian

On 02/09/2019 10:32, Florian Weimer wrote:
> * Zack Weinberg:> >> On Wed, Aug 28, 2019 at 5:39 PM Florian Weimer <fweimer@redhat.com> wrote:>>> I think we should keep using the time entry in the vDSO. This>>> consolidation is just not possible to do for performance reasons.>>>> To be crystal clear about where I'm coming from, I think this>> consolidation is *necessary* to clear the way for the y2038 patches.> > But it's not an absolute technical requirement, right?> > I mean, if we wanted, we could still add a file like> sysdeps/unix/sysv/linux/x86_64/time.c and do things differently there?> (Although we'd also have to change the vDSO setup code for an efficient> implementation.)> >> And I was under the impression that the kernel people wanted to>> withdraw support for the time and gettimeofday entry points (at least>> for new architectures going forward).> > Yes, but that doesn't mean they want performance regressions on x86-64.> I think.> >> So I'm only looking for ways to mitigate the performance impact at>> this point. Before I back off on "henceforth we only call the>> kernel's clock_gettime" I would need to hear *both* a really>> compelling argument for why we need to keep calling the time or>> gettimeofday entry points -- a few ns more expense on a call that only>> reports the time to the nearest second isn't going to cut it -- *and*>> an explanation of why it won't interfere with the y2038 work and/or>> why we won't have to stop using them in the future anyway.> > I expect that people have time calls in (binary) logging code where this> is visible. I think EXPLAIN ANALYZE in PostgreSQL also rather sensitive> to timing performance, but fortunately it already uses> clock_gettimeofday.> > I'm not too concerned here (assuming that we *can* still optimize the> time function on select architectures if that proves necessary in a> couple of years). It's just that I really dislike the idea of> performance regressions on 64-bit architectures as a side effect of> Y2038 support on 32-bit architectures.
I think the *default* implementation for time should indeed be done through
clock_gettime, as this patch intends, however I also think it is possible to
keep arch-specific code in place so the optimized vDSO implementation are
used. It will also allow each arch maintainer to do the switch if the
consensus would to use the default one.
My view is we can use my proposed patch to refactor time [1], and change
the fallback syscall path to call clock_gettime instead. I can work
towards the modification.
[1] https://sourceware.org/ml/libc-alpha/2019-07/msg00157.html

Patch

diff --git a/sysdeps/posix/time.c b/sysdeps/posix/time.c
deleted file mode 100644
index e1b3bc8d4c..0000000000--- a/sysdeps/posix/time.c+++ /dev/null@@ -1,40 +0,0 @@-/* Copyright (C) 1991-2019 Free Software Foundation, Inc.- This file is part of the GNU C Library.-- The GNU C Library is free software; you can redistribute it and/or- modify it under the terms of the GNU Lesser General Public- License as published by the Free Software Foundation; either- version 2.1 of the License, or (at your option) any later version.-- The GNU C Library is distributed in the hope that it will be useful,- but WITHOUT ANY WARRANTY; without even the implied warranty of- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU- Lesser General Public License for more details.-- You should have received a copy of the GNU Lesser General Public- License along with the GNU C Library; if not, see- <http://www.gnu.org/licenses/>. */--#include <stddef.h> /* For NULL. */-#include <time.h>-#include <sys/time.h>---/* Return the current time as a `time_t' and also put it in *T if T is- not NULL. Time is represented as seconds from Jan 1 00:00:00 1970. */-time_t-time (time_t *t)-{- struct timeval tv;- time_t result;-- if (__gettimeofday (&tv, (struct timezone *) NULL))- result = (time_t) -1;- else- result = (time_t) tv.tv_sec;-- if (t != NULL)- *t = result;- return result;-}-libc_hidden_def (time)diff --git a/sysdeps/unix/sysv/linux/i386/time.c b/sysdeps/unix/sysv/linux/i386/time.c
deleted file mode 100644
index 440e3e6ab4..0000000000--- a/sysdeps/unix/sysv/linux/i386/time.c+++ /dev/null@@ -1,34 +0,0 @@-/* time -- Get number of seconds since Epoch. Linux/i386 version.- Copyright (C) 2015-2019 Free Software Foundation, Inc.- This file is part of the GNU C Library.-- The GNU C Library is free software; you can redistribute it and/or- modify it under the terms of the GNU Lesser General Public- License as published by the Free Software Foundation; either- version 2.1 of the License, or (at your option) any later version.-- The GNU C Library is distributed in the hope that it will be useful,- but WITHOUT ANY WARRANTY; without even the implied warranty of- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU- Lesser General Public License for more details.-- You should have received a copy of the GNU Lesser General Public- License along with the GNU C Library; if not, see- <http://www.gnu.org/licenses/>. */--#ifdef SHARED-# define time __redirect_time-#endif--#include <time.h>--#ifdef SHARED-# undef time-# define time_type __redirect_time--# undef libc_hidden_def-# define libc_hidden_def(name) \- __hidden_ver1 (__time_syscall, __GI_time, __time_syscall);-#endif--#include <sysdeps/unix/sysv/linux/x86/time.c>diff --git a/sysdeps/unix/sysv/linux/powerpc/Versions b/sysdeps/unix/sysv/linux/powerpc/Versionsindex 8ebeea15a1..859e0d7daf 100644--- a/sysdeps/unix/sysv/linux/powerpc/Versions+++ b/sysdeps/unix/sysv/linux/powerpc/Versions@@ -10,7 +10,6 @@ libc {
__vdso_clock_gettime;
__vdso_clock_getres;
__vdso_getcpu;
- __vdso_time;
}
}
libm {
diff --git a/sysdeps/unix/sysv/linux/powerpc/init-first.c b/sysdeps/unix/sysv/linux/powerpc/init-first.cindex 831f910788..4f12b59e76 100644--- a/sysdeps/unix/sysv/linux/powerpc/init-first.c+++ b/sysdeps/unix/sysv/linux/powerpc/init-first.c@@ -25,7 +25,6 @@ int (*VDSO_SYMBOL(clock_gettime)) (clockid_t, struct timespec *);
int (*VDSO_SYMBOL(clock_getres)) (clockid_t, struct timespec *);
unsigned long long (*VDSO_SYMBOL(get_tbfreq)) (void);
int (*VDSO_SYMBOL(getcpu)) (unsigned *, unsigned *);
-time_t (*VDSO_SYMBOL(time)) (time_t *);
#if defined(__PPC64__) || defined(__powerpc64__)
void *VDSO_SYMBOL(sigtramp_rt64);
@@ -59,10 +58,6 @@ _libc_vdso_platform_setup (void)
PTR_MANGLE (p);
VDSO_SYMBOL (getcpu) = p;
- p = _dl_vdso_vsym ("__kernel_time", &linux2615);- PTR_MANGLE (p);- VDSO_SYMBOL (time) = p;-
/* PPC64 uses only one signal trampoline symbol, while PPC32 will use
two depending if SA_SIGINFO is used (__kernel_sigtramp_rt32) or not
(__kernel_sigtramp32).
diff --git a/sysdeps/unix/sysv/linux/powerpc/time.c b/sysdeps/unix/sysv/linux/powerpc/time.c
deleted file mode 100644
index cb3e8b9a73..0000000000--- a/sysdeps/unix/sysv/linux/powerpc/time.c+++ /dev/null@@ -1,84 +0,0 @@-/* time system call for Linux/PowerPC.- Copyright (C) 2013-2019 Free Software Foundation, Inc.- This file is part of the GNU C Library.-- The GNU C Library is free software; you can redistribute it and/or- modify it under the terms of the GNU Lesser General Public- License as published by the Free Software Foundation; either- version 2.1 of the License, or (at your option) any later version.-- The GNU C Library is distributed in the hope that it will be useful,- but WITHOUT ANY WARRANTY; without even the implied warranty of- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU- Lesser General Public License for more details.-- You should have received a copy of the GNU Lesser General Public- License along with the GNU C Library; if not, see- <http://www.gnu.org/licenses/>. */--#ifdef SHARED-# ifndef __powerpc64__-# define time __redirect_time-# else-# define __redirect_time time-# endif--# include <time.h>-# include <sysdep.h>-# include <dl-vdso.h>-# include <libc-vdso.h>-# include <dl-machine.h>--# ifndef __powerpc64__-# undef time--time_t-__time_vsyscall (time_t *t)-{- return INLINE_VSYSCALL (time, 1, t);-}--/* __GI_time is defined as hidden and for ppc32 it enables the- compiler make a local call (symbol@local) for internal GLIBC usage. It- means the PLT won't be used and the ifunc resolver will be called directly.- For ppc64 a call to a function in another translation unit might use a- different toc pointer thus disallowing direct branchess and making internal- ifuncs calls safe. */-# undef libc_hidden_def-# define libc_hidden_def(name) \- __hidden_ver1 (__time_vsyscall, __GI_time, __time_vsyscall);--# endif /* !__powerpc64__ */--static time_t-time_syscall (time_t *t)-{- struct timeval tv;- time_t result;-- if (INLINE_VSYSCALL (gettimeofday, 2, &tv, NULL) < 0)- result = (time_t) -1;- else- result = (time_t) tv.tv_sec;-- if (t != NULL)- *t = result;- return result;-}--# define INIT_ARCH() \- PREPARE_VERSION_KNOWN (linux2615, LINUX_2_6_15); \- void *vdso_time = _dl_vdso_vsym ("__kernel_time", &linux2615);--/* If the vDSO is not available we fall back to the syscall. */-libc_ifunc_hidden (__redirect_time, time,- vdso_time- ? VDSO_IFUNC_RET (vdso_time)- : (void *) time_syscall);-libc_hidden_def (time)--#else--#include <sysdeps/posix/time.c>--#endif /* !SHARED */diff --git a/sysdeps/unix/sysv/linux/sparc/sparc64/time.c b/sysdeps/unix/sysv/linux/sparc/sparc64/time.c
deleted file mode 100644
index 509b580c55..0000000000--- a/sysdeps/unix/sysv/linux/sparc/sparc64/time.c+++ /dev/null@@ -1 +0,0 @@-#include <sysdeps/posix/time.c>diff --git a/sysdeps/unix/sysv/linux/time.c b/sysdeps/unix/sysv/linux/time.c
deleted file mode 100644
index 1978f6d817..0000000000--- a/sysdeps/unix/sysv/linux/time.c+++ /dev/null@@ -1,41 +0,0 @@-/* Copyright (C) 2005-2019 Free Software Foundation, Inc.- This file is part of the GNU C Library.-- The GNU C Library is free software; you can redistribute it and/or- modify it under the terms of the GNU Lesser General Public- License as published by the Free Software Foundation; either- version 2.1 of the License, or (at your option) any later version.-- The GNU C Library is distributed in the hope that it will be useful,- but WITHOUT ANY WARRANTY; without even the implied warranty of- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU- Lesser General Public License for more details.-- You should have received a copy of the GNU Lesser General Public- License along with the GNU C Library; if not, see- <http://www.gnu.org/licenses/>. */--#include <stddef.h>-#include <time.h>--#include <sysdep.h>--#ifdef __NR_time--time_t-time (time_t *t)-{- INTERNAL_SYSCALL_DECL (err);- time_t res = INTERNAL_SYSCALL (time, err, 1, NULL);- /* There cannot be any error. */- if (t != NULL)- *t = res;- return res;-}-libc_hidden_def (time)--#else--# include <sysdeps/posix/time.c>--#endifdiff --git a/sysdeps/unix/sysv/linux/x86/time.c b/sysdeps/unix/sysv/linux/x86/time.c
deleted file mode 100644
index 3d72488500..0000000000--- a/sysdeps/unix/sysv/linux/x86/time.c+++ /dev/null@@ -1,59 +0,0 @@-/* time -- Get number of seconds since Epoch. Linux/x86 version.- Copyright (C) 2015-2019 Free Software Foundation, Inc.- This file is part of the GNU C Library.-- The GNU C Library is free software; you can redistribute it and/or- modify it under the terms of the GNU Lesser General Public- License as published by the Free Software Foundation; either- version 2.1 of the License, or (at your option) any later version.-- The GNU C Library is distributed in the hope that it will be useful,- but WITHOUT ANY WARRANTY; without even the implied warranty of- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU- Lesser General Public License for more details.-- You should have received a copy of the GNU Lesser General Public- License along with the GNU C Library; if not, see- <http://www.gnu.org/licenses/>. */--#include <time.h>--#ifdef SHARED--#include <dl-vdso.h>-#include <errno.h>--static time_t-__time_syscall (time_t *t)-{- INTERNAL_SYSCALL_DECL (err);- return INTERNAL_SYSCALL (time, err, 1, t);-}--# ifndef time_type-/* The i386 time.c includes this file with a defined time_type macro.- For x86_64 we have to define it to time as the internal symbol is the- ifunc'ed one. */-# define time_type time-# endif--#undef INIT_ARCH-#define INIT_ARCH() PREPARE_VERSION_KNOWN (linux26, LINUX_2_6);-/* If the vDSO is not available we fall back on the syscall. */-libc_ifunc_hidden (time_type, time,- (_dl_vdso_vsym ("__vdso_time", &linux26)- ?: &__time_syscall))-libc_hidden_def (time)--#else--# include <sysdep.h>--time_t-time (time_t *t)-{- INTERNAL_SYSCALL_DECL (err);- return INTERNAL_SYSCALL (time, err, 1, t);-}--#endifdiff --git a/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list b/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.listindex b44f6f99e9..c0cfa7b0da 100644--- a/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list+++ b/sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list@@ -3,4 +3,3 @@
gettimeofday - gettimeofday:__vdso_gettimeofday@LINUX_2.6 i:pP __gettimeofday gettimeofday
personality EXTRA personality Ei:i __personality personality
posix_fadvise64 - fadvise64 Vi:iiii posix_fadvise posix_fadvise64
-time - time:__vdso_time@LINUX_2.6 Ei:P timediff --git a/time/time.c b/time/time.cindex 88612d6c76..bae0fd14dd 100644--- a/time/time.c+++ b/time/time.c@@ -15,19 +15,17 @@
License along with the GNU C Library; if not, see
<http://www.gnu.org/licenses/>. */
-#include <errno.h>
#include <time.h>
/* Return the time now, and store it in *TIMER if not NULL. */
time_t
time (time_t *timer)
{
- __set_errno (ENOSYS);+ struct timespec ts;+ __clock_gettime (CLOCK_REALTIME, &ts);- if (timer != NULL)- *timer = (time_t) -1;- return (time_t) -1;+ if (timer)+ *timer = ts.tv_sec;+ return ts.tv_sec;
}
libc_hidden_def (time)
--stub_warning (time)