[RFC,v3,05/23] sysdeps/timespec_get: Use clock_gettime64 if avaliable

  This will break other 32-bit targets

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 sysdeps/unix/sysv/linux/timespec_get.c | 37 +++++++++++++++++++++++++-
 1 file changed, 36 insertions(+), 1 deletion(-)
  

* Alistair Francis:

> +#if __WORDSIZE == 32
> +int
> +__timespec_get (struct timespec *ts, int base)
> +{
> +   int ret;
> +
> +#ifdef __NR_clock_gettime64
> +  struct __timespec64 ts64;
> +  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64);
> +
> +  ts->tv_sec = ts64.tv_sec;
> +  ts->tv_nsec = ts64.tv_nsec;
> +
> +  if (! in_time_t_range (ts->tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +#endif
> +
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts);
> +#endif

I don't think this is right.  I think you need to cache clock_gettime64
support somewhere and avoid trying to call the non-existing system call
again and again.  And the second system call will overwrite the results
of the first.

Thanks,
Florian
  

On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Alistair Francis:
>
> > +#if __WORDSIZE == 32
> > +int
> > +__timespec_get (struct timespec *ts, int base)
> > +{
> > +   int ret;
> > +
> > +#ifdef __NR_clock_gettime64
> > +  struct __timespec64 ts64;
> > +  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64);
> > +
> > +  ts->tv_sec = ts64.tv_sec;
> > +  ts->tv_nsec = ts64.tv_nsec;
> > +
> > +  if (! in_time_t_range (ts->tv_sec))
> > +    {
> > +      __set_errno (EOVERFLOW);
> > +      return -1;
> > +    }
> > +#endif
> > +
> > +#ifndef __ASSUME_TIME64_SYSCALLS
> > +  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts);
> > +#endif
>
> I don't think this is right.  I think you need to cache clock_gettime64
> support somewhere and avoid trying to call the non-existing system call
> again and again.

How important is this? It sounds like a micro-optimization for a case that
very few people are going to hit, given that running an application with
64-bit time_t on an old kernel will likely hit other bugs that glibc cannot
deal with.

      Arnd
  

* Arnd Bergmann:

> On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote:
>>
>> * Alistair Francis:
>>
>> > +#if __WORDSIZE == 32
>> > +int
>> > +__timespec_get (struct timespec *ts, int base)
>> > +{
>> > +   int ret;
>> > +
>> > +#ifdef __NR_clock_gettime64
>> > +  struct __timespec64 ts64;
>> > +  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME, &ts64);
>> > +
>> > +  ts->tv_sec = ts64.tv_sec;
>> > +  ts->tv_nsec = ts64.tv_nsec;
>> > +
>> > +  if (! in_time_t_range (ts->tv_sec))
>> > +    {
>> > +      __set_errno (EOVERFLOW);
>> > +      return -1;
>> > +    }
>> > +#endif
>> > +
>> > +#ifndef __ASSUME_TIME64_SYSCALLS
>> > +  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME, ts);
>> > +#endif
>>
>> I don't think this is right.  I think you need to cache clock_gettime64
>> support somewhere and avoid trying to call the non-existing system call
>> again and again.
>
> How important is this? It sounds like a micro-optimization for a case that
> very few people are going to hit, given that running an application with
> 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot
> deal with.

I don't think it's a microoptimization.  On old kernels without
clock_gettime64 in the vDSO, 32-bit timespec_get will always make the
system call, which fails.  Only then the 32-bit clock_gettime in the
vDSO is used.  This effectively negates the performance benefit of the
vDSO, I think.

(Not sure if this will even build on non-RV32 as posted, given that we
don't have a vDSO variable in glibc for clock_gettime64, but I assume
that's going to be fixed if necessary.)

Thanks,
Florian
  

On Wed, Jul 17, 2019 at 10:12 AM Florian Weimer <fweimer@redhat.com> wrote:
> * Arnd Bergmann:
> > On Wed, Jul 17, 2019 at 7:08 AM Florian Weimer <fweimer@redhat.com> wrote:
> >> * Alistair Francis:
> > How important is this? It sounds like a micro-optimization for a case that
> > very few people are going to hit, given that running an application with
> > 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot
> > deal with.
>
> I don't think it's a microoptimization.  On old kernels without
> clock_gettime64 in the vDSO, 32-bit timespec_get will always make the
> system call, which fails.  Only then the 32-bit clock_gettime in the
> vDSO is used.  This effectively negates the performance benefit of the
> vDSO, I think.

I understand that it would be much slower on that particular combination,
I just can't think of anyone who would run into this in practice outside of
validation testing that makes sure glibc does run this way.

If you have any real-world binary built with 64-bit time_t, this will
require all library dependencies other than glibc to be built the same way
and that in turn won't happen unless someone builds a whole distro
for 64-bit time_t, which would be crazy unless they also use a modern
kernel.

I can understand the need to make it work in principle, but does it
have to be efficient?

      Arnd
  

* Arnd Bergmann:

> I understand that it would be much slower on that particular combination,
> I just can't think of anyone who would run into this in practice outside of
> validation testing that makes sure glibc does run this way.

I think it will happen if you run a glibc which has been built against
current kernel headers (and with this patch or something like it) on a
host which has an older kernel.  Due to the way the patch has been
written, it will also impact legacy applications which call the 32-bit
functions, not just 32-bit applications which use the 64-bit interfaces.
(I have not checked the impact on 64-bit kernels yet, hopefully they
will never define __NR_clock_gettime64.)

I could be totally wrong about this, or our disagreement could be about
the relevance of this scenario.  Not sure which.

(I think supporting old host kernels is important to our users.)

Thanks,
Florian
  

On Wed, Jul 17, 2019 at 10:41 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Arnd Bergmann:
>
> > I understand that it would be much slower on that particular combination,
> > I just can't think of anyone who would run into this in practice outside of
> > validation testing that makes sure glibc does run this way.
>
> I think it will happen if you run a glibc which has been built against
> current kernel headers (and with this patch or something like it) on a
> host which has an older kernel.  Due to the way the patch has been
> written, it will also impact legacy applications which call the 32-bit
> functions, not just 32-bit applications which use the 64-bit interfaces.

Ah right, makes sense.

> I could be totally wrong about this, or our disagreement could be about
> the relevance of this scenario.  Not sure which.
>
> (I think supporting old host kernels is important to our users.)

No, I was just wrong and had only thought about applications calling
directly into clock_gettime() while using a 64-bit time_t, but not
the case of glibc-internal functions calling __clock_gettime64()
for convenience, which is something I even advocated for in one of
my other comments.

       Arnd
  

Hi Alistair,

> This will break other 32-bit targets
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  sysdeps/unix/sysv/linux/timespec_get.c | 37
> +++++++++++++++++++++++++- 1 file changed, 36 insertions(+), 1
> deletion(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/timespec_get.c
> b/sysdeps/unix/sysv/linux/timespec_get.c index 52080ddf08..78fa2aba1b
> 100644 --- a/sysdeps/unix/sysv/linux/timespec_get.c
> +++ b/sysdeps/unix/sysv/linux/timespec_get.c
> @@ -24,6 +24,36 @@
>  #endif
>  #include <sysdep-vdso.h>
>  
> +
> +#if __WORDSIZE == 32
> +int
> +__timespec_get (struct timespec *ts, int base)
> +{
> +   int ret;
> +
> +#ifdef __NR_clock_gettime64
> +  struct __timespec64 ts64;
> +  ret = INTERNAL_VSYSCALL (clock_gettime64, err, 2, CLOCK_REALTIME,
> &ts64); +
> +  ts->tv_sec = ts64.tv_sec;
> +  ts->tv_nsec = ts64.tv_nsec;
> +

You may consider using following helper functions (which are the part
of Y2038 support): [1]

> +  if (! in_time_t_range (ts->tv_sec))
> +    {
> +      __set_errno (EOVERFLOW);
> +      return -1;
> +    }
> +#endif
> +
> +#ifndef __ASSUME_TIME64_SYSCALLS
> +  ret = INTERNAL_VSYSCALL (clock_gettime, err, 2, CLOCK_REALTIME,
> ts); +#endif
> +
> +  return ret;
> +}
> +
> +#else
> +
>  /* Set TS to calendar time based in time base BASE.  */
>  int
>  timespec_get (struct timespec *ts, int base)
> @@ -33,9 +63,13 @@ timespec_get (struct timespec *ts, int base)
>        int res;
>        INTERNAL_SYSCALL_DECL (err);
>      case TIME_UTC:
> +#if __WORDSIZE == 32
> +      res = __timespec_get (*ts, base);
> +#else
>        res = INTERNAL_VSYSCALL (clock_gettime, err, 2,
> CLOCK_REALTIME, ts); +#endif
>        if (INTERNAL_SYSCALL_ERROR_P (res, err))
> -	return 0;
> +        return 0;
>        break;
>  
>      default:
> @@ -44,3 +78,4 @@ timespec_get (struct timespec *ts, int base)
>  
>    return base;
>  }
> +#endif

Note:
[1] - https://github.com/lmajewski/y2038_glibc/commit/12ac380db090219aac4ed8b0ef179b9fcc4c296e

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
  

On Wed, 17 Jul 2019, Arnd Bergmann wrote:

> > I don't think this is right.  I think you need to cache clock_gettime64
> > support somewhere and avoid trying to call the non-existing system call
> > again and again.
> 
> How important is this? It sounds like a micro-optimization for a case that
> very few people are going to hit, given that running an application with
> 64-bit time_t on an old kernel will likely hit other bugs that glibc cannot
> deal with.

It's a generic design question for all the time64 patches that might end 
up using one or another syscall at runtime depending on kernel support - 
we'll need to arrive at a consensus on whether such caching (probably 
shared between all relevant syscalls) is desired.  It's not clear to me 
whether the case of _TIME_BITS=64 on an old kernel will end up as a rare 
case or not.  And as per my previous comments, many existing functions 
using 32-bit times should end up as thin wrappers round functions using 
64-bit times, so potentially making two syscalls every time unless there 
is such caching.