[RFC,v5,05/21] sysdeps/clock_gettime: Use clock_gettime64 if avaliable

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

Commit Message

Alistair Francis Aug. 29, 2019, 4:50 p.m.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>


	* sysdeps/unix/sysv/linux/clock_gettime.c: Use clock_gettime64 if avaliable.
---
 include/time.h                          |  1 +
 sysdeps/unix/sysv/linux/clock_gettime.c | 48 ++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 1 deletion(-)

-- 
2.22.0

Comments

Lukasz Majewski Oct. 29, 2019, noon | #1
Hi Alistair,

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

> 

> 	* sysdeps/unix/sysv/linux/clock_gettime.c: Use

> clock_gettime64 if avaliable. ---

>  include/time.h                          |  1 +

>  sysdeps/unix/sysv/linux/clock_gettime.c | 48

> ++++++++++++++++++++++++- 2 files changed, 48 insertions(+), 1

> deletion(-)

> 

> diff --git a/include/time.h b/include/time.h

> index 6d81f91384b..1e33f34e1f6 100644

> --- a/include/time.h

> +++ b/include/time.h

> @@ -177,6 +177,7 @@ extern double __difftime (time_t time1, time_t

> time0); #define __clock_nanosleep_time64 __clock_nanosleep

>  #define __nanosleep_time64 __nanosleep

>  #define __nanosleep_nocancel_time64 __nanosleep_nocancel

> +#define __clock_gettime64 __clock_gettime

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

> 5fc47fb7dc7..ea98af9bf1a 100644 ---

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

> b/sysdeps/unix/sysv/linux/clock_gettime.c @@ -28,9 +28,55 @@

>  

>  /* Get current value of CLOCK and store it in TP.  */

>  int

> +__clock_gettime64 (clockid_t clock_id, struct timespec *tp)

> +{

> +#ifdef __ASSUME_TIME64_SYSCALLS

> +# ifndef __NR_clock_gettime64

> +#  define __NR_clock_gettime64 __NR_clock_gettime

> +# endif

> +   return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);

> +#else

> +   int ret;

> +# ifdef __NR_clock_gettime64

> +  ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);

> +

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

> +    return ret;

> +# endif /* __NR_clock_gettime64 */

> +  struct timespec tp32;

> +

> +  ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);

> +

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

> +    valid_timespec_to_timespec64(tp32, tp);

> +

> +  return ret;

> +#endif /* __ASSUME_TIME64_SYSCALLS */

> +}

> +

> +#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 || errno != ENOSYS)

> +    {

> +      if (! in_time_t_range (tp64.tv_sec))

> +        {

> +          __set_errno (EOVERFLOW);

> +          return -1;

> +        }

> +

> +      valid_timespec64_to_timespec(&tp64, tp);

> +

> +      return ret;

> +    }

>  }

> +#endif

> +

>  weak_alias (__clock_gettime, clock_gettime)

>  libc_hidden_def (__clock_gettime)


In your github repository - I've noticed the updated version of patch
providing __clock_gettime64 [1] conversion.

Do you plan to re-send this patch soon?


Please find some comments regarding this particular patch:

1. Please add following code instead the line 182 [2] (@include/time.h):

+#if __TIMESIZE == 64
+# define __clock_gettime64 __clock_gettime
+#else
+extern int __clock_gettime64 (clockid_t clock_id,
+                              struct __timespec64 *tp);
+libc_hidden_proto (__clock_gettime64);
+#endif


2. The line 44 (@sysv/linux/clock_gettime.c) is a bit tricky

I've grepped the recent kernel (-master branch) tag: v5.4-rc5 with
git grep -n "vdso_clock_gettime64"

And the __vdso_clock_gettime64 is available for mips, aarch64 and x86
(plain ARM 32 bits is missing though).

Considering the above - the vDSO may be a bit tricky for some archs as
the 'clock_gettime64' "name" argument to INLINE_VSYSCALL () macro is
then extended to __vdso_##name (@ sysdeps/unix/sysv/linux/sysdep-vdso.h)
It seems like one would need a global (i.e. at clock_gettime.c file)
__vdso_clock_gettime64 fallback.


IMHO the condition '&& defined HAVE_CLOCK_GETTIME64_VSYSCALL' in Line
44 [3] could be removed.


3. Line 50 - replace tp32 with ts32 (as we define structure, not
pointer).

4. Line 54 - IMHO the 'if (ret == 0 || errno != ENOSYS)' shall be
replaced with 'if (! ret && tp)' 
(as we reference *tp, which may be NULL). The same situation is in Line
70.


Links:
[1] -
https://github.com/alistair23/glibc/commit/b7682ca51f6d7ea702eeb9066c7710f5a40f6590

[2] -
https://github.com/alistair23/glibc/commit/b7682ca51f6d7ea702eeb9066c7710f5a40f6590#diff-5b9f1c6457e0e10079f657f283c19861R182

[3] -
https://github.com/alistair23/glibc/commit/b7682ca51f6d7ea702eeb9066c7710f5a40f6590#diff-cb7e59ab5b305ba007394bee79cdd6d6R44



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

Patch

diff --git a/include/time.h b/include/time.h
index 6d81f91384b..1e33f34e1f6 100644
--- a/include/time.h
+++ b/include/time.h
@@ -177,6 +177,7 @@  extern double __difftime (time_t time1, time_t time0);
 #define __clock_nanosleep_time64 __clock_nanosleep
 #define __nanosleep_time64 __nanosleep
 #define __nanosleep_nocancel_time64 __nanosleep_nocancel
+#define __clock_gettime64 __clock_gettime
 #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 5fc47fb7dc7..ea98af9bf1a 100644
--- a/sysdeps/unix/sysv/linux/clock_gettime.c
+++ b/sysdeps/unix/sysv/linux/clock_gettime.c
@@ -28,9 +28,55 @@ 
 
 /* Get current value of CLOCK and store it in TP.  */
 int
+__clock_gettime64 (clockid_t clock_id, struct timespec *tp)
+{
+#ifdef __ASSUME_TIME64_SYSCALLS
+# ifndef __NR_clock_gettime64
+#  define __NR_clock_gettime64 __NR_clock_gettime
+# endif
+   return INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
+#else
+   int ret;
+# ifdef __NR_clock_gettime64
+  ret = INLINE_VSYSCALL (clock_gettime64, 2, clock_id, tp);
+
+  if (ret == 0 || errno != ENOSYS)
+    return ret;
+# endif /* __NR_clock_gettime64 */
+  struct timespec tp32;
+
+  ret = INLINE_VSYSCALL (clock_gettime, 2, clock_id, &tp32);
+
+  if (ret == 0 || errno != ENOSYS)
+    valid_timespec_to_timespec64(tp32, tp);
+
+  return ret;
+#endif /* __ASSUME_TIME64_SYSCALLS */
+}
+
+#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 || errno != ENOSYS)
+    {
+      if (! in_time_t_range (tp64.tv_sec))
+        {
+          __set_errno (EOVERFLOW);
+          return -1;
+        }
+
+      valid_timespec64_to_timespec(&tp64, tp);
+
+      return ret;
+    }
 }
+#endif
+
 weak_alias (__clock_gettime, clock_gettime)
 libc_hidden_def (__clock_gettime)