[v2,05/10] Use clock_gettime to implement time.

  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.

	* time/time.c (time): No longer a stub implementation.
	Call __clock_gettime.

	* sysdeps/unix/sysv/linux/powerpc/Versions (GLIBC_PRIVATE):
	Remove __vdso_time.
	* sysdeps/unix/sysv/linux/powerpc/init-first.c (__vdso_time): Delete.
	(_libc_vdso_platform_setup): Don't initialize __vdso_time.
	* sysdeps/unix/sysv/linux/x86_64/x32/syscalls.list: Remove entry
	for time.

	* sysdeps/posix/time.c
	* sysdeps/unix/sysv/linux/time.c
	* sysdeps/unix/sysv/linux/i386/time.c
	* sysdeps/unix/sysv/linux/powerpc/time.c
	* sysdeps/unix/sysv/linux/sparc/sparc64/time.c
	* sysdeps/unix/sysv/linux/x86/time.c:
	Delete file.
---
 sysdeps/posix/time.c                          | 40 ---------
 sysdeps/unix/sysv/linux/i386/time.c           | 34 --------
 sysdeps/unix/sysv/linux/powerpc/Versions      |  1 -
 sysdeps/unix/sysv/linux/powerpc/init-first.c  |  5 --
 sysdeps/unix/sysv/linux/powerpc/time.c        | 84 -------------------
 sysdeps/unix/sysv/linux/sparc/sparc64/time.c  |  1 -
 sysdeps/unix/sysv/linux/time.c                | 41 ---------
 sysdeps/unix/sysv/linux/x86/time.c            | 59 -------------
 .../unix/sysv/linux/x86_64/x32/syscalls.list  |  1 -
 time/time.c                                   | 12 ++-
 10 files changed, 5 insertions(+), 273 deletions(-)
 delete mode 100644 sysdeps/posix/time.c
 delete mode 100644 sysdeps/unix/sysv/linux/i386/time.c
 delete mode 100644 sysdeps/unix/sysv/linux/powerpc/time.c
 delete mode 100644 sysdeps/unix/sysv/linux/sparc/sparc64/time.c
 delete mode 100644 sysdeps/unix/sysv/linux/time.c
 delete mode 100644 sysdeps/unix/sysv/linux/x86/time.c
  

* 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
  

* Zack Weinberg:

> 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?

I see 4.9 ns with CLOCK_REALTIME_COARSE, compared to 2.2 ns for time.
It still looks significantly slower to me.

We also have to be careful not to use clocks that don't stay in the vDSO
on all supported kernel/hypervisor combinations.

Benchmark code is below.

Thanks,
Florian

#include <err.h>
#include <stdlib.h>
#include <string.h>
#include <sys/time.h>
#include <time.h>

int
main (int argc, char **argv)
{
  int count;
  if (argc != 3 || (count = atoi (argv[2])) <= 0)
    err (1, "usage: %s METHOD ITERATIONS", argv[0]);

  if (strcmp (argv[1], "time") == 0)
    for (int i = 0; i < count; ++i)
      time (NULL);
  else if (strcmp (argv[1], "gettimeofday") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timeval tv;
        gettimeofday (&tv, NULL);
      }
  else if (strcmp (argv[1], "realtime") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timespec tv;
        clock_gettime (CLOCK_REALTIME, &tv);
      }
  else if (strcmp (argv[1], "realtime_coarse") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timespec tv;
        clock_gettime (CLOCK_REALTIME_COARSE, &tv);
      }
  else if (strcmp (argv[1], "monotonic") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timespec tv;
        clock_gettime (CLOCK_MONOTONIC, &tv);
      }
  else if (strcmp (argv[1], "monotonic_coarse") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timespec tv;
        clock_gettime (CLOCK_MONOTONIC_COARSE, &tv);
      }
  else if (strcmp (argv[1], "monotonic_raw") == 0)
    for (int i = 0; i < count; ++i)
      {
        struct timespec tv;
        clock_gettime (CLOCK_MONOTONIC_RAW, &tv);
      }
  else
    errx (1, "invalid clock: %s", argv[1]);

  return 0;
}
  

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