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: Adhemerval Zanella <adhemerval dot zanella at linaro dot org>
- To: libc-alpha at sourceware dot org
- Date: Tue, 29 Oct 2019 17:15:17 -0300
- Subject: Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable
- References: <20191024234106.27081-1-alistair.francis@wdc.com>
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.
> 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)