[RFC,v3,02/23] sysdeps/gettimeofday: Use clock_gettime64 if avaliable

Message ID d570d35acb1d9027cf7558ba47bf0fb3f5272860.1563321715.git.alistair.francis@wdc.com
State New
Headers show
Series
  • RISC-V glibc port for the 32-bit
Related show

Commit Message

Alistair Francis July 17, 2019, 12:08 a.m.
Not all architectures support the obsolete gettimeofday so use the
newer clock_gettime64 syscall if it is avaliable. This fixes RV32
build issues.

Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

---
 ChangeLog                              |  1 +
 sysdeps/unix/sysv/linux/gettimeofday.c | 28 ++++++++++++++++++++++++++
 2 files changed, 29 insertions(+)

-- 
2.22.0

Comments

Florian Weimer July 17, 2019, 7:09 a.m. | #1
* Alistair Francis:

> diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c

> index a74f03825a..151b1e606c 100644

> --- a/sysdeps/unix/sysv/linux/gettimeofday.c

> +++ b/sysdeps/unix/sysv/linux/gettimeofday.c

> @@ -32,7 +32,35 @@

>  int

>  __gettimeofday (struct timeval *tv, struct timezone *tz)

>  {

> +#ifdef __ASSUME_TIME64_SYSCALLS

> +  int ret;

> +  struct __timespec64 now;

> +

> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> +                         &now);

> +

> +  /* Convert from timespec to timeval */

> +  tv->tv_sec = now.tv_sec;

> +  tv->tv_usec = now.tv_nsec / 1000;

> +

> +  return ret;

> +#else

> +# ifdef __NR_clock_gettime64

> +  long int ret;

> +  struct __timespec64 now;

> +

> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> +                         &now);

> +

> +  /* Convert from timespec to timeval */

> +  tv->tv_sec = now.tv_sec;

> +  tv->tv_usec = now.tv_nsec / 1000;

> +

> +  if (ret == 0 || errno != ENOSYS)

> +    return ret;

> +# endif

>    return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);

> +#endif

>  }


This loses vDSO acceleration if glibc is compiled with kernel headers
which define __NR_clock_gettime64, but the run-time kernel does not have
clock_gettime64 in the vDSO.

And the kernel folks really want us to call clock_gettime when the user
calls the 32-bit function, for tracability of legacy processes.

Thanks,
Florian
Lukasz Majewski July 17, 2019, 12:43 p.m. | #2
Hi Alistair,

> Not all architectures support the obsolete gettimeofday so use the

> newer clock_gettime64 syscall if it is avaliable. This fixes RV32

> build issues.

> 

> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

> ---

>  ChangeLog                              |  1 +

>  sysdeps/unix/sysv/linux/gettimeofday.c | 28

> ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)

> 

> diff --git a/ChangeLog b/ChangeLog

> index 477b9b49b3..9ca390a9c3 100644

> --- a/ChangeLog

> +++ b/ChangeLog

> @@ -1028,6 +1028,7 @@

>  	* nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of

> nanosleep.

>  	* sysdeps/unix/sysv/linux/nanosleep.c: Likewise.

>  	* sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.

> +	* sysdeps/unix/sysv/linux/gettimeofday.c: Use

> clock_gettime64 syscall for gettimeofday. 

>  2019-06-20  Dmitry V. Levin  <ldv@altlinux.org>

>  	    Florian Weimer  <fweimer@redhat.com>

> diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c

> b/sysdeps/unix/sysv/linux/gettimeofday.c index a74f03825a..151b1e606c

> 100644 --- a/sysdeps/unix/sysv/linux/gettimeofday.c

> +++ b/sysdeps/unix/sysv/linux/gettimeofday.c

> @@ -32,7 +32,35 @@

>  int

>  __gettimeofday (struct timeval *tv, struct timezone *tz)

>  {

> +#ifdef __ASSUME_TIME64_SYSCALLS


I'm not the glibc expert but according to [1], the
__ASSUME_TIME64_SYSCALLS will be defined also for __WORDSIZE = 64 archs.

This means that this code will be executed on x86_64 and return with an
ENOTSUPP error as those archs shall not define clock_gettime64 and
will just use the clock_settime.

Please consider re-using pattern from clock_settime conversion [2].

> +  int ret;

> +  struct __timespec64 now;

> +

> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> +                         &now);

> +

> +  /* Convert from timespec to timeval */

> +  tv->tv_sec = now.tv_sec;

> +  tv->tv_usec = now.tv_nsec / 1000;

> +

> +  return ret;

> +#else

> +# ifdef __NR_clock_gettime64

> +  long int ret;

> +  struct __timespec64 now;

> +

> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> +                         &now);

> +

> +  /* Convert from timespec to timeval */

> +  tv->tv_sec = now.tv_sec;

> +  tv->tv_usec = now.tv_nsec / 1000;

> +

> +  if (ret == 0 || errno != ENOSYS)

> +    return ret;

> +# endif

>    return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);

> +#endif

>  }

>  libc_hidden_def (__gettimeofday)

>  weak_alias (__gettimeofday, gettimeofday)


Note:

[1] - 
https://github.com/lmajewski/y2038_glibc/commit/1fdbc6002101a78a8a6a076bbb642b3082c2225d

[2] -
https://github.com/lmajewski/y2038_glibc/commit/69f842a8519ca13ed11fab0ff1bcc6fa1a524192

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
Lukasz Majewski July 17, 2019, 12:48 p.m. | #3
Hi,

> Hi Alistair,

> 

> > Not all architectures support the obsolete gettimeofday so use the

> > newer clock_gettime64 syscall if it is avaliable. This fixes RV32

> > build issues.

> > 

> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

> > ---

> >  ChangeLog                              |  1 +

> >  sysdeps/unix/sysv/linux/gettimeofday.c | 28

> > ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)

> > 

> > diff --git a/ChangeLog b/ChangeLog

> > index 477b9b49b3..9ca390a9c3 100644

> > --- a/ChangeLog

> > +++ b/ChangeLog

> > @@ -1028,6 +1028,7 @@

> >  	* nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of

> > nanosleep.

> >  	* sysdeps/unix/sysv/linux/nanosleep.c: Likewise.

> >  	* sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.

> > +	* sysdeps/unix/sysv/linux/gettimeofday.c: Use

> > clock_gettime64 syscall for gettimeofday. 

> >  2019-06-20  Dmitry V. Levin  <ldv@altlinux.org>

> >  	    Florian Weimer  <fweimer@redhat.com>

> > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c

> > b/sysdeps/unix/sysv/linux/gettimeofday.c index

> > a74f03825a..151b1e606c 100644 ---

> > a/sysdeps/unix/sysv/linux/gettimeofday.c +++

> > b/sysdeps/unix/sysv/linux/gettimeofday.c @@ -32,7 +32,35 @@

> >  int

> >  __gettimeofday (struct timeval *tv, struct timezone *tz)

> >  {

> > +#ifdef __ASSUME_TIME64_SYSCALLS  

> 

> I'm not the glibc expert but according to [1], the

> __ASSUME_TIME64_SYSCALLS will be defined also for __WORDSIZE = 64

> archs.

> 

> This means that this code will be executed on x86_64 and return with

> an ENOTSUPP error as those archs shall not define clock_gettime64 and

     ^^^^^^^ - sorry s/ENOTSUPP/ENOSYS/g

> will just use the clock_settime.

> 

> Please consider re-using pattern from clock_settime conversion [2].

> 

> > +  int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  return ret;

> > +#else

> > +# ifdef __NR_clock_gettime64

> > +  long int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  if (ret == 0 || errno != ENOSYS)

> > +    return ret;

> > +# endif

> >    return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);

> > +#endif

> >  }

> >  libc_hidden_def (__gettimeofday)

> >  weak_alias (__gettimeofday, gettimeofday)  

> 

> Note:

> 

> [1] - 

> https://github.com/lmajewski/y2038_glibc/commit/1fdbc6002101a78a8a6a076bbb642b3082c2225d

> 

> [2] -

> https://github.com/lmajewski/y2038_glibc/commit/69f842a8519ca13ed11fab0ff1bcc6fa1a524192

> 

> 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





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
Alistair Francis July 19, 2019, 10:26 p.m. | #4
On Wed, Jul 17, 2019 at 5:43 AM Lukasz Majewski <lukma@denx.de> wrote:
>

> Hi Alistair,

>

> > Not all architectures support the obsolete gettimeofday so use the

> > newer clock_gettime64 syscall if it is avaliable. This fixes RV32

> > build issues.

> >

> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>

> > ---

> >  ChangeLog                              |  1 +

> >  sysdeps/unix/sysv/linux/gettimeofday.c | 28

> > ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)

> >

> > diff --git a/ChangeLog b/ChangeLog

> > index 477b9b49b3..9ca390a9c3 100644

> > --- a/ChangeLog

> > +++ b/ChangeLog

> > @@ -1028,6 +1028,7 @@

> >       * nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of

> > nanosleep.

> >       * sysdeps/unix/sysv/linux/nanosleep.c: Likewise.

> >       * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.

> > +     * sysdeps/unix/sysv/linux/gettimeofday.c: Use

> > clock_gettime64 syscall for gettimeofday.

> >  2019-06-20  Dmitry V. Levin  <ldv@altlinux.org>

> >           Florian Weimer  <fweimer@redhat.com>

> > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c

> > b/sysdeps/unix/sysv/linux/gettimeofday.c index a74f03825a..151b1e606c

> > 100644 --- a/sysdeps/unix/sysv/linux/gettimeofday.c

> > +++ b/sysdeps/unix/sysv/linux/gettimeofday.c

> > @@ -32,7 +32,35 @@

> >  int

> >  __gettimeofday (struct timeval *tv, struct timezone *tz)

> >  {

> > +#ifdef __ASSUME_TIME64_SYSCALLS

>

> I'm not the glibc expert but according to [1], the

> __ASSUME_TIME64_SYSCALLS will be defined also for __WORDSIZE = 64 archs.

>

> This means that this code will be executed on x86_64 and return with an

> ENOTSUPP error as those archs shall not define clock_gettime64 and

> will just use the clock_settime.

>

> Please consider re-using pattern from clock_settime conversion [2].


Yep, good point. I have updated this to better match what is being
discussed on the list and your patches.

Alistair

>

> > +  int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  return ret;

> > +#else

> > +# ifdef __NR_clock_gettime64

> > +  long int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  if (ret == 0 || errno != ENOSYS)

> > +    return ret;

> > +# endif

> >    return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);

> > +#endif

> >  }

> >  libc_hidden_def (__gettimeofday)

> >  weak_alias (__gettimeofday, gettimeofday)

>

> Note:

>

> [1] -

> https://github.com/lmajewski/y2038_glibc/commit/1fdbc6002101a78a8a6a076bbb642b3082c2225d

>

> [2] -

> https://github.com/lmajewski/y2038_glibc/commit/69f842a8519ca13ed11fab0ff1bcc6fa1a524192

>

> 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
Rich Felker July 20, 2019, 3:20 a.m. | #5
On Wed, Jul 17, 2019 at 09:09:08AM +0200, Florian Weimer wrote:
> * Alistair Francis:

> 

> > diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c

> > index a74f03825a..151b1e606c 100644

> > --- a/sysdeps/unix/sysv/linux/gettimeofday.c

> > +++ b/sysdeps/unix/sysv/linux/gettimeofday.c

> > @@ -32,7 +32,35 @@

> >  int

> >  __gettimeofday (struct timeval *tv, struct timezone *tz)

> >  {

> > +#ifdef __ASSUME_TIME64_SYSCALLS

> > +  int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  return ret;

> > +#else

> > +# ifdef __NR_clock_gettime64

> > +  long int ret;

> > +  struct __timespec64 now;

> > +

> > +  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,

> > +                         &now);

> > +

> > +  /* Convert from timespec to timeval */

> > +  tv->tv_sec = now.tv_sec;

> > +  tv->tv_usec = now.tv_nsec / 1000;

> > +

> > +  if (ret == 0 || errno != ENOSYS)

> > +    return ret;

> > +# endif

> >    return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);

> > +#endif

> >  }

> 

> This loses vDSO acceleration if glibc is compiled with kernel headers

> which define __NR_clock_gettime64, but the run-time kernel does not have

> clock_gettime64 in the vDSO.

> 

> And the kernel folks really want us to call clock_gettime when the user

> calls the 32-bit function, for tracability of legacy processes.


May or may not be relevant to glibc plans, but I want to chime in to
mention that this (kernel tracability of legacy processes) is not
going to work with musl, except possibly static-linked programs with
old libc. Our legacy compat functions are just going to be thin,
implementation-agnostic wrappers around the new functions, so even if
a program is still calling the old clock_gettime symbol with 32-bit
timespec, it's going to be executing the new one as its backend, and
thereby calling the 64-bit syscall or vdso if available.

In the case of new libc with old application on old kernel, there will
actually be two conversions: 32->64 in the core (new) function after
the 64-bit syscall fails with ENOSYS and we fallback to the old
syscall, and 64->32 in the old-ABI-compat wrapper. This is a
consequence of not duplicating functionality and ending up with two
versions of everything to maintain, and also avoids incentivizing
continued use of the old ABI for users with old kernels (as a way to
bypass conversion costs).

In any case, just grepping dynsym tables for references to legacy
symbols seems like a more effective way of finding problems than
grepping for syslog spam...

Rich
Joseph Myers July 25, 2019, 8:54 p.m. | #6
On Fri, 19 Jul 2019, Rich Felker wrote:

> > And the kernel folks really want us to call clock_gettime when the user

> > calls the 32-bit function, for tracability of legacy processes.

> 

> May or may not be relevant to glibc plans, but I want to chime in to

> mention that this (kernel tracability of legacy processes) is not

> going to work with musl, except possibly static-linked programs with

> old libc. Our legacy compat functions are just going to be thin,

> implementation-agnostic wrappers around the new functions, so even if

> a program is still calling the old clock_gettime symbol with 32-bit

> timespec, it's going to be executing the new one as its backend, and

> thereby calling the 64-bit syscall or vdso if available.


And as far as I'm concerned glibc should do likewise (I have a sustained 
objection to duplication of nontrivial function implementations at either 
the source and binary level for time_t variants when thin wrappers would 
do the job just as well, given the principle that the particular choice of 
syscalls for a given function is not a stable interface).  (Where a 
function is defined entirely in syscalls.list, it might continue to use 
the old syscall, subject to working out the best way to set up function 
aliasing for that case.)

> In any case, just grepping dynsym tables for references to legacy

> symbols seems like a more effective way of finding problems than

> grepping for syslog spam...


Yes.

-- 
Joseph S. Myers
joseph@codesourcery.com

Patch

diff --git a/ChangeLog b/ChangeLog
index 477b9b49b3..9ca390a9c3 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1028,6 +1028,7 @@ 
 	* nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of nanosleep.
 	* sysdeps/unix/sysv/linux/nanosleep.c: Likewise.
 	* sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.
+	* sysdeps/unix/sysv/linux/gettimeofday.c: Use clock_gettime64 syscall for gettimeofday.
 
 2019-06-20  Dmitry V. Levin  <ldv@altlinux.org>
 	    Florian Weimer  <fweimer@redhat.com>
diff --git a/sysdeps/unix/sysv/linux/gettimeofday.c b/sysdeps/unix/sysv/linux/gettimeofday.c
index a74f03825a..151b1e606c 100644
--- a/sysdeps/unix/sysv/linux/gettimeofday.c
+++ b/sysdeps/unix/sysv/linux/gettimeofday.c
@@ -32,7 +32,35 @@ 
 int
 __gettimeofday (struct timeval *tv, struct timezone *tz)
 {
+#ifdef __ASSUME_TIME64_SYSCALLS
+  int ret;
+  struct __timespec64 now;
+
+  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,
+                         &now);
+
+  /* Convert from timespec to timeval */
+  tv->tv_sec = now.tv_sec;
+  tv->tv_usec = now.tv_nsec / 1000;
+
+  return ret;
+#else
+# ifdef __NR_clock_gettime64
+  long int ret;
+  struct __timespec64 now;
+
+  ret = INLINE_VSYSCALL (clock_gettime64, 2, CLOCK_REALTIME,
+                         &now);
+
+  /* Convert from timespec to timeval */
+  tv->tv_sec = now.tv_sec;
+  tv->tv_usec = now.tv_nsec / 1000;
+
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
+# endif
   return INLINE_VSYSCALL (gettimeofday, 2, tv, tz);
+#endif
 }
 libc_hidden_def (__gettimeofday)
 weak_alias (__gettimeofday, gettimeofday)