From patchwork Tue Oct 29 20:15:17 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Adhemerval Zanella Netto X-Patchwork-Id: 35451 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: List-Unsubscribe: List-Subscribe: List-Archive: List-Post: List-Help: , 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: To: libc-alpha@sourceware.org References: <20191024234106.27081-1-alistair.francis@wdc.com> From: Adhemerval Zanella 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> 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 @@ > . */ > > #include > +#include > #include > #include > > /* 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 @@ > . */ > > #include > +#include > #include > #include > > +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 #include #include +#include #include @@ -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)