Message ID | 4c256164-2eb3-aab4-2a32-9d4e1e1f473e@linaro.org |
---|---|
State | Dropped |
Headers |
Received: (qmail 99835 invoked by alias); 29 Oct 2019 20:15:51 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 79186 invoked by uid 89); 29 Oct 2019 20:15:34 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-21.6 required=5.0 tests=AWL, BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, KAM_SHORT, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=Inheritance, replicate X-HELO: mail-qt1-f195.google.com DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=to:references:from:openpgp:autocrypt:subject:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=RYIV0ghWrYKegjYnfHE1hkEYKc40zFqemXdgkUqQrWw=; b=g8H+Cy7x9DIT0H86+s7Y4LiYMnAje1pDpcOTqmmo1xTbzSb6Roks2zsFv+uc5yWtOD z3Tiz0rqpBNDKBSV0SxI3Mh/et16kKmRQ3XLjPvVX3Dse3+Y/eoPCtNmtvx3+2M6iT8r kJWfH+Aij8L5T90c9ekndC0QhU4PcS0eA5gmvn4QrYeYB+OlRAqrfRSFl9jYYuwwdRsS bjYcnjB55fnSH7QgC+yi+je18Zo4iYbap6/sJ0RhuVrkRkYA30eipIaj/FEojfAzXzXG weJi+tGCPtUibrNCvSXEujrCWahXEiBcX9Pvki+9aE1oUhnphXVpLAJLsGyWvEcSh5Ag vZ8A== Return-Path: <adhemerval.zanella@linaro.org> To: libc-alpha@sourceware.org References: <20191024234106.27081-1-alistair.francis@wdc.com> From: Adhemerval Zanella <adhemerval.zanella@linaro.org> Openpgp: preference=signencrypt Subject: Re: [PATCH v2] sysdeps/nanosleep: Use clock_nanosleep_time64 if avaliable Message-ID: <4c256164-2eb3-aab4-2a32-9d4e1e1f473e@linaro.org> Date: Tue, 29 Oct 2019 17:15:17 -0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: <20191024234106.27081-1-alistair.francis@wdc.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit |
Commit Message
Adhemerval Zanella Netto
Oct. 29, 2019, 8:15 p.m. UTC
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. --
Comments
* Adhemerval Zanella: > 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; > } Why the -ret? I think it should be a plain ret. Thanks, Florian
On 30/10/2019 11:06, Florian Weimer wrote: > * Adhemerval Zanella: > >> 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; >> } > > Why the -ret? I think it should be a plain ret. Indeed, my mistake here.
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) >
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) >
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)