Re: [RFC v5 02/21] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable

[Alistair Francis <alistair23@gmail.com>]

On Tue, Oct 15, 2019 at 8:55 AM Lukasz Majewski <lukma@denx.de> wrote:
>
> Hi Alistair,
>
> > On Mon, Oct 14, 2019 at 7:00 AM Lukasz Majewski <lukma@denx.de> wrote:
> > >
> > > Hi Alistair,
> > >
> > > > The nanosleep syscall is not supported on newer 32-bit platforms
> > > > (such as RV32). To fix this issue let's use
> > > > clock_nanosleep_time64 if it is avaliable.
> > > >
> > > > Let's use CLOCK_REALTIME when calling clock_nanosleep_time64 as
> > > > the Linux specification says:
> > > >   "POSIX.1 specifies that nanosleep() should measure time against
> > > > the CLOCK_REALTIME clock. However, Linux measures the time using
> > > > the CLOCK_MONOTONIC clock. This probably does not matter, since
> > > > the POSIX.1 specification for clock_settime(2) says that
> > > > discontinuous changes in CLOCK_REALTIME should not affect
> > > > nanosleep()"
> > > >
> > > > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > > >
> > > >       * nptl/thrd_sleep.c: Use clock_nanosleep_time64 instead of
> > > > nanosleep.
> > > >       * sysdeps/unix/sysv/linux/nanosleep.c: Likewise.
> > > >       * sysdeps/unix/sysv/linux/clock_nanosleep.c: Likewise.
> > > >       * sysdeps/unix/sysv/linux/nanosleep_nocancel.c: Likewise.
> > > > ---
> > > >  include/time.h                               |  6 ++
> > > >  nptl/thrd_sleep.c                            | 69
> > > > ++++++++++++++++-- sysdeps/unix/sysv/linux/clock_nanosleep.c    |
> > > > 74 ++++++++++++++++++-- sysdeps/unix/sysv/linux/nanosleep.c
> > > >    | 65 ++++++++++++++++-
> > > > sysdeps/unix/sysv/linux/nanosleep_nocancel.c | 64
> > > > ++++++++++++++++- 5 files changed, 264 insertions(+), 14
> > > > deletions(-)
> > > >
> > > > diff --git a/include/time.h b/include/time.h
> > > > index 95b31429212..6d81f91384b 100644
> > > > --- a/include/time.h
> > > > +++ b/include/time.h
> > > > @@ -172,6 +172,12 @@ libc_hidden_proto (__difftime64)
> > > >
> > > >  extern double __difftime (time_t time1, time_t time0);
> > > >
> > > > +#if __TIMESIZE == 64
> > > > +#define __thrd_sleep_time64 thrd_sleep
> > > > +#define __clock_nanosleep_time64 __clock_nanosleep
> > > > +#define __nanosleep_time64 __nanosleep
> > > > +#define __nanosleep_nocancel_time64 __nanosleep_nocancel
> > > > +#endif
> > > >
> > > >  /* Use in the clock_* functions.  Size of the field representing
> > > > the actual clock ID.  */
> > > > diff --git a/nptl/thrd_sleep.c b/nptl/thrd_sleep.c
> > > > index 07a51808df2..e85da561c68 100644
> > > > --- a/nptl/thrd_sleep.c
> > > > +++ b/nptl/thrd_sleep.c
> > > > @@ -22,18 +22,79 @@
> > > >  #include "thrd_priv.h"
> > > >
> > > >  int
> > > > -thrd_sleep (const struct timespec* time_point, struct timespec*
> > > > remaining) +__thrd_sleep_time64 (const struct timespec*
> > > > time_point, struct timespec* remaining) {
> > > >    INTERNAL_SYSCALL_DECL (err);
> > > > -  int ret = INTERNAL_SYSCALL_CANCEL (nanosleep, err, time_point,
> > > > remaining);
> > > > +  int ret = -1;
> > > > +
> > > > +#ifdef __ASSUME_TIME64_SYSCALLS
> > > > +# ifndef __NR_clock_nanosleep_time64
> > > > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > > > +# endif
> > > > +  ret = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > > > CLOCK_REALTIME,
> > > > +                                 0, time_point, remaining);
> > > > +#else
> > > > +# ifdef __NR_clock_nanosleep_time64
> > > > +  long int ret_64;
> > > > +
> > > > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > > > CLOCK_REALTIME,
> > > > +                                    0, time_point, remaining);
> > > > +
> > > > +  if (ret_64 == 0 || errno != ENOSYS)
> > > > +    ret = ret_64;
> > > > +# endif /* __NR_clock_nanosleep_time64 */
> > > > +  if (ret < 0)
> > > > +    {
> > > > +      struct timespec tp32, tr32;
> > > > +
> > > > +      if (! in_time_t_range (time_point->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      valid_timespec64_to_timespec (time_point, &tp32);
> > > > +      ret =  INTERNAL_SYSCALL_CANCEL (nanosleep, err, &tp32,
> > > > &tr32); +
> > > > +      if ((ret == 0 || errno != ENOSYS) && remaining)
> > > > +        valid_timespec_to_timespec64(&tr32, remaining);
> > > > +    }
> > > > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > > > +
> > > >    if (INTERNAL_SYSCALL_ERROR_P (ret, err))
> > > >      {
> > > >        /* C11 states thrd_sleep function returns -1 if it has been
> > > > interrupted
> > > > -      by a signal, or a negative value if it fails.  */
> > > > +         by a signal, or a negative value if it fails.  */
> > > >        ret = INTERNAL_SYSCALL_ERRNO (ret, err);
> > > >        if (ret == EINTR)
> > > > -     return -1;
> > > > +        return -1;
> > > >        return -2;
> > > >      }
> > > >    return 0;
> > > >  }
> > > > +
> > > > +#if __TIMESIZE != 64
> > > > +int
> > > > +thrd_sleep (const struct timespec* time_point, struct timespec*
> > > > remaining) +{
> > > > +  int ret;
> > > > +  timespec64 tp64, tr64;
> > > > +
> > > > +  valid_timespec_to_timespec64(time_point, &tp64);
> > > > +  ret = __thrd_sleep_time64 (&tp64, &tr64);
> > > > +
> > > > +  if (ret == 0 || errno != ENOSYS)
> > > > +    {
> > > > +      if (! in_time_t_range (tr64->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      if (remaining)
> > > > +        valid_timespec64_to_timespec(&tr32, remaining);
> > > > +    }
> > > > +
> > > > +  return ret;
> > > > +}
> > > > +#endif
> > >
> > > This code seems to follow the pattern introduced in clock_settime()
> > > conversion (already in the -master branch).
> >
> > I hope so :)
> >
> > >
> > > Do you plan to send this and the following patches:
> > >
> > > https://patchwork.ozlabs.org/patch/1155396/    --> clock_gettime
> > > https://patchwork.ozlabs.org/patch/1155398/    --> timespec_get
> >
> > Yes
> >
> > >
> > > As a separate patch series so we can all benefit from having them in
> > > the -master glibc branch?
> > > Those patches seems to be orthogonal to ones adding support for
> > > RISC-V.
> >
> > Yep! My plan is to send them seperatley. I have been working through
> > the RV32 tree sending patches in small batches to get them merged. I
> > have a few more batches before I get to these time64_t conversion
> > patches though. I am unfortunately seeing failures with my time64_t
> > conversion patches which I haven't had a chance to look into.
> >
> > >
> > >
> > > The gettimeofday() syscall handling code seems to being now
> > > converted to clock_gettime() in the glibc (or at least there is
> > > some ongoing effort):
> > >
> > > https://sourceware.org/git/?p=glibc.git;a=commit;h=b30d257ea8f557bdadca09f5b112538e7b807eb9
> > >
> >
> > Yep, I have rebased all of my work on top of this.
> >
> > >
> > > Similar situation is with stat and statfs (which are now under the
> > > discussion/review):
> > >
> > > https://patchwork.ozlabs.org/patch/1155399/    --> stat
> > > https://patchwork.ozlabs.org/patch/1155401/    --> statfs
> >
> > Yep, I'm working on the next version right now.
> >
> > If you want you are welcome to split out any patches and get them
> > upstreaed. Even just fixing failing test cases would speed things up.
> > My branch is available here:
> > https://github.com/alistair23/glibc/commits/alistair/rv32.next
> >
>
> Please find some comments and my future work plan:
>
> 1. The "sysdeps/clock_getres: Use inline syscall if required" :
> https://github.com/alistair23/glibc/commit/830707a4a3f662b1a97860afe658f6bbd9731d13
> commit does not convert this syscall.
>
> However, I can convert and post to the glibc alpha the __clock_getres64
> (as I already have some older code available). I'm now working on
> preparing proper patch.

Go for it! :)

>
>
> 2. Potential compilation error for SoCs with TIMESIZE != 64:
> https://github.com/alistair23/glibc/commit/4309573e10479516fcdf75c353309497f8017c90#diff-85643011757f36e12ca60b37c3e8ec0dR75

What is the error?

>
>
> 3. Would you be so kind and post (to glibc alpha) the already converted
> (in your -next tree) __clock_nanosleep64:
> https://github.com/alistair23/glibc/commit/9e3d95ad1f70909a787dd11fbc735bcafa02050e#diff-5b9f1c6457e0e10079f657f283c19861R177

This patch is causing build failures, so I need to investigate that.

>
> Just one remark - please in include/time.h use the following paradigm:
> https://github.com/lmajewski/y2038_glibc/commit/7ff615859f9e111c0c4cf8a6c085201c7ed82813#diff-5b9f1c6457e0e10079f657f283c19861R128

Ok, I'll add this.

Alistair

>
> for redirection.
>
>
> > Alistair
> >
> > >
> > > > diff --git a/sysdeps/unix/sysv/linux/clock_nanosleep.c
> > > > b/sysdeps/unix/sysv/linux/clock_nanosleep.c index
> > > > 0cb6614dc92..64302fbcc69 100644 ---
> > > > a/sysdeps/unix/sysv/linux/clock_nanosleep.c +++
> > > > b/sysdeps/unix/sysv/linux/clock_nanosleep.c @@ -21,13 +21,14 @@
> > > >  #include <sysdep-cancel.h>
> > > >  #include "kernel-posix-cpu-timers.h"
> > > >
> > > > -
> > > >  /* We can simply use the syscall.  The CPU clocks are not
> > > > supported with this function.  */
> > > >  int
> > > > -__clock_nanosleep (clockid_t clock_id, int flags, const struct
> > > > timespec *req,
> > > > -                struct timespec *rem)
> > > > +__clock_nanosleep_time64 (clockid_t clock_id, int flags, const
> > > > struct timespec *req,
> > > > +                          struct timespec *rem)
> > > >  {
> > > > +  int r = -1;
> > > > +
> > > >    if (clock_id == CLOCK_THREAD_CPUTIME_ID)
> > > >      return EINVAL;
> > > >    if (clock_id == CLOCK_PROCESS_CPUTIME_ID)
> > > > @@ -36,9 +37,70 @@ __clock_nanosleep (clockid_t clock_id, int
> > > > flags, const struct timespec *req, /* If the call is interrupted
> > > > by a signal handler or encounters an error, it returns a positive
> > > > value similar to errno.  */ INTERNAL_SYSCALL_DECL (err);
> > > > -  int r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err,
> > > > clock_id, flags,
> > > > -                                req, rem);
> > > > +
> > > > +#ifdef __ASSUME_TIME64_SYSCALLS
> > > > +# ifndef __NR_clock_nanosleep_time64
> > > > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > > > +# endif
> > > > +  r = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > > > clock_id,
> > > > +                               flags, req, rem);
> > > > +#else
> > > > +# ifdef __NR_clock_nanosleep_time64
> > > > +  long int ret_64;
> > > > +
> > > > +  ret_64 = INTERNAL_SYSCALL_CANCEL (clock_nanosleep_time64, err,
> > > > clock_id,
> > > > +                                    flags, req, rem);
> > > > +
> > > > +  if (ret_64 == 0 || errno != ENOSYS)
> > > > +    r = ret_64;
> > > > +# endif /* __NR_clock_nanosleep_time64 */
> > > > +  if (r < 0)
> > > > +    {
> > > > +      struct timespec ts32, tr32;
> > > > +
> > > > +      if (! in_time_t_range (req->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      valid_timespec64_to_timespec (req, &ts32);
> > > > +      r =  INTERNAL_SYSCALL_CANCEL (clock_nanosleep, err, &ts32,
> > > > &tr32); +
> > > > +      if ((r == 0 || errno != ENOSYS) && rem)
> > > > +        valid_timespec_to_timespec64(&tr32, rem);
> > > > +    }
> > > > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > > > +
> > > >    return (INTERNAL_SYSCALL_ERROR_P (r, err)
> > > > -       ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> > > > +          ? INTERNAL_SYSCALL_ERRNO (r, err) : 0);
> > > >  }
> > > > +
> > > > +#if __TIMESIZE != 64
> > > > +int
> > > > +__clock_nanosleep (clockid_t clock_id, int flags, const struct
> > > > timespec *req,
> > > > +                   struct timespec *rem)
> > > > +{
> > > > +  int r;
> > > > +  timespec64 treq64, trem64;
> > > > +
> > > > +  valid_timespec_to_timespec64(req, &treq64)
> > > > +  r = __clock_nanosleep_time64 (clock_id, flags, &treq64,
> > > > &trem64); +
> > > > +  if (r == 0 || errno != ENOSYS)
> > > > +    {
> > > > +      if (! in_time_t_range (trem64->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      if (remaining)
> > > > +        valid_timespec64_to_timespec(&tr32, remaining);
> > > > +    }
> > > > +
> > > > +  return r;
> > > > +}
> > > > +#endif
> > > > +
> > > >  weak_alias (__clock_nanosleep, clock_nanosleep)
> > > > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c
> > > > b/sysdeps/unix/sysv/linux/nanosleep.c index
> > > > f14ae565af5..9a8dd5a4b82 100644 ---
> > > > a/sysdeps/unix/sysv/linux/nanosleep.c +++
> > > > b/sysdeps/unix/sysv/linux/nanosleep.c @@ -22,10 +22,71 @@
> > > >
> > > >  /* Pause execution for a number of nanoseconds.  */
> > > >  int
> > > > +__nanosleep_time64 (const struct timespec *requested_time,
> > > > +                    struct timespec *remaining)
> > > > +{
> > > > +#if defined(__ASSUME_TIME64_SYSCALLS)
> > > > +# ifndef __NR_clock_nanosleep_time64
> > > > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > > > +# endif
> > > > +  return SYSCALL_CANCEL (clock_nanosleep_time64, CLOCK_REALTIME,
> > > > 0,
> > > > +                         requested_time, remaining);
> > > > +#else
> > > > +# ifdef __NR_clock_nanosleep_time64
> > > > +  long int ret_64;
> > > > +
> > > > +  ret_64 = SYSCALL_CANCEL (clock_nanosleep_time64,
> > > > CLOCK_REALTIME, 0,
> > > > +                           requested_time, remaining);
> > > > +
> > > > +  if (ret_64 == 0 || errno != ENOSYS)
> > > > +    return ret_64;
> > > > +# endif /* __NR_clock_nanosleep_time64 */
> > > > +  int ret;
> > > > +  struct timespec ts32, tr32;
> > > > +
> > > > +  if (! in_time_t_range (requested_time->tv_sec))
> > > > +    {
> > > > +      __set_errno (EOVERFLOW);
> > > > +      return -1;
> > > > +    }
> > > > +
> > > > +  valid_timespec64_to_timespec (requested_time, &ts32);
> > > > +  ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32);
> > > > +
> > > > +  if ((ret == 0 || errno != ENOSYS) && remaining)
> > > > +    valid_timespec_to_timespec64(&tr32, remaining);
> > > > +
> > > > +  return ret;
> > > > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > > > +}
> > > > +
> > > > +#if __TIMESIZE != 64
> > > > +int
> > > >  __nanosleep (const struct timespec *requested_time,
> > > > -          struct timespec *remaining)
> > > > +             struct timespec *remaining)
> > > >  {
> > > > -  return SYSCALL_CANCEL (nanosleep, requested_time, remaining);
> > > > +  int r;
> > > > +  timespec64 treq64, trem64;
> > > > +
> > > > +
> > > > +  valid_timespec_to_timespec64(req, &treq64);
> > > > +  r = __nanosleep_time64 (&treq64, &trem64);
> > > > +
> > > > +  if (r == 0 || errno != ENOSYS)
> > > > +    {
> > > > +      if (! in_time_t_range (trem64->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      if (remaining)
> > > > +        valid_timespec64_to_timespec(&tr32, remaining);
> > > > +    }
> > > > +
> > > > +  return r;
> > > >  }
> > > > +#endif
> > > > +
> > > >  hidden_def (__nanosleep)
> > > >  weak_alias (__nanosleep, nanosleep)
> > > > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > > > b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c index
> > > > 122ba627ff3..03f5be2ff9e 100644 ---
> > > > a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c +++
> > > > b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c @@ -20,10 +20,70 @@
> > > >  #include <sysdep-cancel.h>
> > > >  #include <not-cancel.h>
> > > >
> > > > +int
> > > > +__nanosleep_nocancel_time64 (const struct timespec
> > > > *requested_time,
> > > > +                             struct timespec *remaining)
> > > > +{
> > > > +#ifdef __ASSUME_TIME64_SYSCALLS
> > > > +# ifndef __NR_clock_nanosleep_time64
> > > > +#  define __NR_clock_nanosleep_time64 __NR_clock_nanosleep
> > > > +# endif
> > > > +  return INLINE_SYSCALL_CALL (clock_nanosleep_time64,
> > > > CLOCK_REALTIME, 0,
> > > > +                              requested_time, remaining);
> > > > +#else
> > > > +# ifdef __NR_clock_nanosleep_time64
> > > > +  long int ret_64;
> > > > +
> > > > +  ret_64 = INLINE_SYSCALL_CALL (clock_nanosleep_time64,
> > > > CLOCK_REALTIME, 0,
> > > > +                                requested_time, remaining);
> > > > +
> > > > +  if (ret_64 == 0 || errno != ENOSYS)
> > > > +    return ret_64;
> > > > +# endif /* __NR_clock_nanosleep_time64 */
> > > > +  int ret;
> > > > +  struct timespec ts32, tr32;
> > > > +
> > > > +  if (! in_time_t_range (requested_time->tv_sec))
> > > > +    {
> > > > +      __set_errno (EOVERFLOW);
> > > > +      return -1;
> > > > +    }
> > > > +
> > > > +  valid_timespec64_to_timespec (requested_time, &ts32);
> > > > +  ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32);
> > > > +
> > > > +  if (ret == 0 || errno != ENOSYS)
> > > > +    valid_timespec_to_timespec64(&tr32, remaining);
> > > > +
> > > > +  return ret;
> > > > +#endif /* __ASSUME_TIME64_SYSCALLS */
> > > > +}
> > > > +
> > > > +#if __TIMESIZE != 64
> > > >  int
> > > >  __nanosleep_nocancel (const struct timespec *requested_time,
> > > > -                   struct timespec *remaining)
> > > > +                      struct timespec *remaining)
> > > >  {
> > > > -  return INLINE_SYSCALL_CALL (nanosleep, requested_time,
> > > > remaining);
> > > > +  int ret;
> > > > +  timespec64 treq64, trem64;
> > > > +
> > > > +  valid_timespec_to_timespec64(req, &treq64)
> > > > +  ret = __nanosleep_nocancel_time64 (&treq64, &trem64);
> > > > +
> > > > +  if (ret == 0 || errno != ENOSYS)
> > > > +    {
> > > > +      if (! in_time_t_range (trem64->tv_sec))
> > > > +        {
> > > > +          __set_errno (EOVERFLOW);
> > > > +          return -1;
> > > > +        }
> > > > +
> > > > +      if (remaining)
> > > > +        valid_timespec64_to_timespec(&tr32, remaining);
> > > > +    }
> > > > +
> > > > +  return ret;
> > > >  }
> > > > +#endif
> > > > +
> > > >  hidden_def (__nanosleep_nocancel)
> > >
> > >
> > >
> > >
> > > 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