This is the mail archive of the libc-alpha@sourceware.org mailing list for the glibc project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable


On Tue, Oct 29, 2019 at 9:16 PM Adhemerval Zanella
<adhemerval.zanella@linaro.org> wrote:
>
>
>
> On 24/10/2019 20:41, Alistair Francis wrote:
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep.c b/sysdeps/unix/sysv/linux/nanosleep.c
> > index 6787909248f..10a6d57a1de 100644
> > --- a/sysdeps/unix/sysv/linux/nanosleep.c
> > +++ b/sysdeps/unix/sysv/linux/nanosleep.c
> > @@ -17,15 +17,76 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <sysdep-cancel.h>
> >  #include <not-cancel.h>
> >
> >  /* Pause execution for a number of nanoseconds.  */
> >  int
> > +__nanosleep_time64 (const struct __timespec64 *requested_time,
> > +                    struct __timespec64 *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;
> > +    }
> > +
> > +  ts32 = valid_timespec64_to_timespec (*requested_time);
> > +  ret = SYSCALL_CANCEL (nanosleep, &ts32, &tr32);
> > +
> > +  if ((ret == 0 || errno != ENOSYS) && remaining)
> > +    *remaining = valid_timespec_to_timespec64 (tr32);
> > +
> > +  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;
> > +  struct __timespec64 treq64, trem64;
> > +
> > +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> > +  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)
> > +        *remaining = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return r;
> >  }
> > +#endif
> > +
> >  hidden_def (__nanosleep)
> >  weak_alias (__nanosleep, nanosleep)
>
> There is no need to replicate all the syscall logic, nanosleep can be implemented
> with __clock_nanosleep.  You can do:
>
> int
> __nanosleep (const struct timespec *requested_time,
>              struct timespec *remaining)
> {
>   int ret = __clock_nanosleep (CLOCK_REALTIME, 0, requested_time, remaining);
>   if (ret != 0)
>     {
>       __set_errno (-ret);
>       return -1;
>     }
>   return ret;
> }
>
> I think we can even make it the default version, thus removing the linux
> specific file.

Ok, testing now then I'll send the patch.

>
>
> > diff --git a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > index d6442bf4f7f..1a6c6c0a48a 100644
> > --- a/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > +++ b/sysdeps/unix/sysv/linux/nanosleep_nocancel.c
> > @@ -17,13 +17,74 @@
> >     <https://www.gnu.org/licenses/>.  */
> >
> >  #include <time.h>
> > +#include <kernel-features.h>
> >  #include <sysdep-cancel.h>
> >  #include <not-cancel.h>
> >
> > +int
> > +__nanosleep_nocancel_time64 (const struct __timespec64 *requested_time,
> > +                             struct __timespec64 *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;
> > +    }
> > +
> > +  ts32 = valid_timespec64_to_timespec (*requested_time);
> > +  ret = INLINE_SYSCALL_CALL (nanosleep, &ts32, &tr32);
> > +
> > +  if (ret == 0 || errno != ENOSYS)
> > +    *remaining = valid_timespec_to_timespec64 (tr32);
> > +
> > +  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;
> > +  struct __timespec64 treq64, trem64;
> > +
> > +  treq64 = valid_timespec_to_timespec64 (*requested_time);
> > +  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)
> > +        *remaining = valid_timespec64_to_timespec (trem64);
> > +    }
> > +
> > +  return ret;
> >  }
> > +#endif
> > +
> >  hidden_def (__nanosleep_nocancel)
> >
>
> The __nanosleep_nocancel is just used priority mutex for pthread_mutex_timedlock,
> and I am almost sure that we can replace the code path with __lll_clocklock_wait.
> Something like the below.
>
> If you like I can sent it as a cleanup patch, since it would require some more
> work to remove the __nanosleep_nocancel internal definitions as well.

I would prefer that going in seperatley via a cleanup patch.

Alistair

>
> --
>
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index d6f0d9e31a..b9536ddb19 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -25,6 +25,7 @@
>  #include <atomic.h>
>  #include <lowlevellock.h>
>  #include <not-cancel.h>
> +#include <futex-internal.h>
>
>  #include <stap-probe.h>
>
> @@ -377,50 +378,29 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>             int private = (robust
>                            ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
>                            : PTHREAD_MUTEX_PSHARED (mutex));
> -           INTERNAL_SYSCALL_DECL (__err);
> -
> -           int e = INTERNAL_SYSCALL (futex, __err, 4, &mutex->__data.__lock,
> -                                     __lll_private_flag (FUTEX_LOCK_PI,
> -                                                         private), 1,
> -                                     abstime);
> -           if (INTERNAL_SYSCALL_ERROR_P (e, __err))
> +           int e = futex_lock_pi ((unsigned int *) &mutex->__data.__lock, 1,
> +                                  abstime, private);
> +           if (e == ETIMEDOUT)
>               {
> -               if (INTERNAL_SYSCALL_ERRNO (e, __err) == ETIMEDOUT)
> -                 return ETIMEDOUT;
> -
> -               if (INTERNAL_SYSCALL_ERRNO (e, __err) == ESRCH
> -                   || INTERNAL_SYSCALL_ERRNO (e, __err) == EDEADLK)
> +               return ETIMEDOUT;
> +             }
> +           else if (e == ESRCH || e == EDEADLK)
> +             {
> +               assert (e != EDEADLK
> +                       || (kind != PTHREAD_MUTEX_ERRORCHECK_NP
> +                          && kind != PTHREAD_MUTEX_RECURSIVE_NP));
> +               /* ESRCH can happen only for non-robust PI mutexes where
> +                  the owner of the lock died.  */
> +               assert (e != ESRCH || !robust);
> +
> +               /* Delay the thread until the timeout is reached. Then return
> +                  ETIMEDOUT.  */
> +               do
>                   {
> -                   assert (INTERNAL_SYSCALL_ERRNO (e, __err) != EDEADLK
> -                           || (kind != PTHREAD_MUTEX_ERRORCHECK_NP
> -                               && kind != PTHREAD_MUTEX_RECURSIVE_NP));
> -                   /* ESRCH can happen only for non-robust PI mutexes where
> -                      the owner of the lock died.  */
> -                   assert (INTERNAL_SYSCALL_ERRNO (e, __err) != ESRCH
> -                           || !robust);
> -
> -                   /* Delay the thread until the timeout is reached.
> -                      Then return ETIMEDOUT.  */
> -                   struct timespec reltime;
> -                   struct timespec now;
> -
> -                   INTERNAL_SYSCALL (clock_gettime, __err, 2, clockid,
> -                                     &now);
> -                   reltime.tv_sec = abstime->tv_sec - now.tv_sec;
> -                   reltime.tv_nsec = abstime->tv_nsec - now.tv_nsec;
> -                   if (reltime.tv_nsec < 0)
> -                     {
> -                       reltime.tv_nsec += 1000000000;
> -                       --reltime.tv_sec;
> -                     }
> -                   if (reltime.tv_sec >= 0)
> -                     while (__nanosleep_nocancel (&reltime, &reltime) != 0)
> -                       continue;
> -
> -                   return ETIMEDOUT;
> -                 }
> -
> -               return INTERNAL_SYSCALL_ERRNO (e, __err);
> +                   e = __lll_clocklock_wait (&(int){0}, CLOCK_REALTIME,
> +                                             abstime, private);
> +                 } while (e != ETIMEDOUT);
> +               return e;
>               }
>
>             oldval = mutex->__data.__lock;
> diff --git a/sysdeps/unix/sysv/linux/futex-internal.h b/sysdeps/unix/sysv/linux/futex-internal.h
> index 5a4f4ff818..1442df63a1 100644
> --- a/sysdeps/unix/sysv/linux/futex-internal.h
> +++ b/sysdeps/unix/sysv/linux/futex-internal.h
> @@ -252,4 +252,30 @@ futex_wake (unsigned int *futex_word, int processes_to_wake, int private)
>      }
>  }
>
> +static __always_inline int
> +futex_lock_pi (unsigned int *futex_word, unsigned int expected,
> +              const struct timespec *abstime, int private)
> +{
> +  int err = lll_futex_timed_lock_pi (futex_word, expected, abstime, private);
> +  switch (err)
> +    {
> +    case 0:
> +    case -EAGAIN:
> +    case -EINTR:
> +    case -ETIMEDOUT:
> +    case -ESRCH:
> +    case -EDEADLK:
> +      return -err;
> +
> +    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
> +    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
> +                    being normalized.  Must have been caused by a glibc or
> +                    application bug.  */
> +    case -ENOSYS: /* Must have been caused by a glibc bug.  */
> +    /* No other errors are documented at this time.  */
> +    default:
> +      futex_fatal_error ();
> +    }
> +}
> +
>  #endif  /* futex-internal.h */
> diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> index b423673ed4..5730eb2ba2 100644
> --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
> @@ -127,6 +127,11 @@
>                      FUTEX_OP_CLEAR_WAKE_IF_GT_ONE)
>
>  /* Priority Inheritance support.  */
> +#define lll_futex_timed_lock_pi(futexp, val, reltime, private)                 \
> +  lll_futex_syscall (4, futexp,                                                \
> +                    __lll_private_flag (FUTEX_LOCK_PI, private),       \
> +                    val, reltime)
> +
>  #define lll_futex_wait_requeue_pi(futexp, val, mutex, private) \
>    lll_futex_timed_wait_requeue_pi (futexp, val, NULL, 0, mutex, private)
>


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]