This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
- From: Alistair Francis <alistair23 at gmail dot com>
- To: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- Cc: GNU C Library <libc-alpha at sourceware dot org>
- Date: Mon, 4 Nov 2019 10:16:14 -0800
- Subject: Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
- References: <20191024234106.27081-1-alistair.francis@wdc.com> <4c256164-2eb3-aab4-2a32-9d4e1e1f473e@linaro.org>
On Tue, Oct 29, 2019 at 1: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;
> }
This doesn't work as __clock_nanosleep() isn't avaliable in nptl so it
fails to compile. My v3 patch attempted to fix this, but that also
doesn't work and ends up squishing some patches together.
Alistair
>
> I think we can even make it the default version, thus removing the linux
> specific file.
>
>
> > 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.
>
> --
>
> 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)
>