[PATCH v3] y2038: nptl: Convert pthread_{clock|timed}join_np to support 64 bit time
Adhemerval Zanella
adhemerval.zanella@linaro.org
Wed Jul 29 20:17:29 GMT 2020
On 24/07/2020 18:11, Lukasz Majewski wrote:
> The pthread_clockjoin_np and pthread_timedjoin_np have been converted to
> support 64 bit time.
>
> This change introduces new futex_timed_wait_cancel64 function in
> ./sysdeps/nptl/futex-internal.h, which uses futex_time64 where possible
> and tries to replace low-level preprocessor macros from
> lowlevellock-futex.h
> The pthread_{timed|clock}join_np only accept absolute time. Moreover,
> there is no need to check for NULL passed as *abstime pointer as
> clockwait_tid() always passes struct __timespec64.
>
> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to __pthread_{clock|timed}join_np64 will provide support
> for 64 bit time
>
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
>
> Run-time tests:
> - Run specific tests on ARM/x86 32bit systems (qemu):
> https://github.com/lmajewski/meta-y2038 and run tests:
> https://github.com/lmajewski/y2038-tests/commits/master
>
> Above tests were performed with Y2038 redirection applied as well as without
> to test the proper usage of both __pthread_{timed|clock}join_np64 and
> __pthread_{timed|clock}join_np.
Some comments below, I think it still need some adjustments,
>
> ---
> Changes for v2:
> - Update the commit message
>
> Changes for v3:
> - Use const qualifier for futex_timed_wait_cancel64() timeout parameter
> - Use INTERNAL_SYSCALL_CALL macro instead of INTERNAL_SYSCALL
> - Use LIBC_CANCEL_{RESET|ASYNC} macro instead of CANCEL_{RESET|ASYNC}
> - Add switch clause with listing possible error values returned by
> futex{_time64} syscall
> ---
> nptl/pthreadP.h | 16 ++++++++++-
> nptl/pthread_clockjoin.c | 19 ++++++++++--
> nptl/pthread_join_common.c | 19 ++++++------
> nptl/pthread_timedjoin.c | 18 ++++++++++--
> sysdeps/nptl/futex-internal.h | 54 +++++++++++++++++++++++++++++++++++
> 5 files changed, 112 insertions(+), 14 deletions(-)
>
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 6f94d6be31..99713c8447 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -458,6 +458,20 @@ extern int __pthread_cond_init (pthread_cond_t *cond,
> libc_hidden_proto (__pthread_cond_init)
> extern int __pthread_cond_signal (pthread_cond_t *cond);
> extern int __pthread_cond_wait (pthread_cond_t *cond, pthread_mutex_t *mutex);
> +
> +#if __TIMESIZE == 64
> +# define __pthread_clockjoin_np64 __pthread_clockjoin_np
> +# define __pthread_timedjoin_np64 __pthread_timedjoin_np
> +#else
> +extern int __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> + clockid_t clockid,
> + const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_clockjoin_np64)
> +extern int __pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> + const struct __timespec64 *abstime);
> +libc_hidden_proto (__pthread_timedjoin_np64)
> +#endif
> +
> extern int __pthread_cond_timedwait (pthread_cond_t *cond,
> pthread_mutex_t *mutex,
> const struct timespec *abstime);
> @@ -488,7 +502,7 @@ extern int __pthread_enable_asynccancel (void) attribute_hidden;
> extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
> extern void __pthread_testcancel (void);
> extern int __pthread_clockjoin_ex (pthread_t, void **, clockid_t,
> - const struct timespec *, bool)
> + const struct __timespec64 *, bool)
> attribute_hidden;
> extern int __pthread_sigmask (int, const sigset_t *, sigset_t *);
> libc_hidden_proto (__pthread_sigmask);
Ok.
> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index a3e7f37e3b..3cd780f688 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -16,14 +16,27 @@
> License along with the GNU C Library; if not, see
> <http://www.gnu.org/licenses/>. */
>
> +#include <time.h>
> #include "pthreadP.h"
>
> int
> -__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> - clockid_t clockid,
> - const struct timespec *abstime)
> +__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> + clockid_t clockid, const struct __timespec64 *abstime)
> {
> return __pthread_clockjoin_ex (threadid, thread_return,
> clockid, abstime, true);
> }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_clockjoin_np64)
> +
> +int
> +__pthread_clockjoin_np (pthread_t threadid, void **thread_return,
> + clockid_t clockid, const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_clockjoin_np64 (threadid, thread_return, clockid, &ts64);
> +}
> +#endif
> weak_alias (__pthread_clockjoin_np, pthread_clockjoin_np)
Ok.
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index a96ceafde4..67d8e2b780 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -20,6 +20,7 @@
> #include <atomic.h>
> #include <stap-probe.h>
> #include <time.h>
> +#include <futex-internal.h>
>
> static void
> cleanup (void *arg)
> @@ -37,9 +38,11 @@ cleanup (void *arg)
> afterwards. The kernel up to version 3.16.3 does not use the private futex
> operations for futex wake-up when the clone terminates. */
> static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
> +clockwait_tid (pid_t *tidp, clockid_t clockid,
> + const struct __timespec64 *abstime)
> {
> pid_t tid;
> + int ret;
>
> if (! valid_nanoseconds (abstime->tv_nsec))
> return EINVAL;
> @@ -47,11 +50,11 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
> /* Repeat until thread terminated. */
> while ((tid = *tidp) != 0)
> {
> - struct timespec rt;
> + struct __timespec64 rt;
>
> /* Get the current time. This can only fail if clockid is
> invalid. */
> - if (__glibc_unlikely (__clock_gettime (clockid, &rt)))
> + if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
> return EINVAL;
>
> /* Compute relative timeout. */
Ok.
> @@ -70,9 +73,9 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
> /* If *tidp == tid, wait until thread terminates or the wait times out.
> The kernel up to version 3.16.3 does not use the private futex
> operations for futex wake-up when the clone terminates. */
> - if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)
> - == -ETIMEDOUT)
> - return ETIMEDOUT;
> + ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> + if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> + return -ret;
> }
>
> return 0;
Ok.
> @@ -80,8 +83,8 @@ clockwait_tid (pid_t *tidp, clockid_t clockid, const struct timespec *abstime)
>
> int
> __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> - clockid_t clockid,
> - const struct timespec *abstime, bool block)
> + clockid_t clockid,
> + const struct __timespec64 *abstime, bool block)
> {
> struct pthread *pd = (struct pthread *) threadid;
>
Ok.
> diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> index dd7038dcf7..6164ae7060 100644
> --- a/nptl/pthread_timedjoin.c
> +++ b/nptl/pthread_timedjoin.c
> @@ -16,13 +16,27 @@
> License along with the GNU C Library; if not, see
> <https://www.gnu.org/licenses/>. */
>
> +#include <time.h>
> #include "pthreadP.h"
>
> int
> -__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> - const struct timespec *abstime)
> +__pthread_timedjoin_np64 (pthread_t threadid, void **thread_return,
> + const struct __timespec64 *abstime)
> {
> return __pthread_clockjoin_ex (threadid, thread_return,
> CLOCK_REALTIME, abstime, true);
> }
> +
> +#if __TIMESIZE != 64
> +libc_hidden_def (__pthread_timedjoin_np64)
> +
> +int
> +__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> + const struct timespec *abstime)
> +{
> + struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> + return __pthread_timedjoin_np64 (threadid, thread_return, &ts64);
> +}
> +#endif
Ok.
> weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index d622122ddc..7e75fcadef 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -467,4 +467,58 @@ futex_unlock_pi (unsigned int *futex_word, int private)
> }
> }
>
> +#ifndef __NR_futex_time64
> +# define __NR_futex_time64 __NR_futex
> +#endif
> +
> +static __always_inline int
> +futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> + const struct __timespec64 *timeout, int private)
> +{
> + int oldtype = LIBC_CANCEL_ASYNC ();
> + int err = INTERNAL_SYSCALL_CALL (futex_time64, tidp,
> + __lll_private_flag
> + (FUTEX_WAIT, private), tid, timeout);
> + LIBC_CANCEL_RESET (oldtype);
I think on my last review I forgot to add that we have INTERNAL_SYSCALL_CANCEL
for such cases, and I meant that I didn't have a preference over
INTERNAL_SYSCALL_CALL plus LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET.
> +#ifndef __ASSUME_TIME64_SYSCALLS
> + if (err == -ENOSYS)
> + {
> + struct timespec ts32;
> + if (! in_time_t_range (timeout->tv_sec))
> + {
> + err = -EOVERFLOW;
> + goto out;
> + }
> + ts32 = valid_timespec64_to_timespec (*timeout);
> +
> + err = INTERNAL_SYSCALL_CALL (futex, tidp,
> + __lll_private_flag (FUTEX_WAIT, private),
> + tid, &ts32);
You also need to enable/disable async cancellation on this futex call
(either with LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET or INTERNAL_SYSCALL_CALL).
I think the LIBC_CANCEL_ASYNC/LIBC_CANCEL_RESET is also slight preferable
here since the single thread optimization (which does not call the
enable/disable async cancel) won't be really used.
> + }
> +out:
> +#endif
> + switch (err)
> + {
> + case 0:
> + case -EAGAIN:
> + case -EINTR:
> + case -ETIMEDOUT:
> + case -EDEADLK:
> + case -ENOSYS:
> + case -EOVERFLOW: /* Passed absolute timeout uses 64 bit time_t type, but
> + glibc setup only supports 32 bit value. */
I would make it explicit is not that glibc does not support it, but rather
that the underlying kernel does not have have support for 64-bit time_t
futex syscalls.
> + case -EPERM: /* The caller is not allowed to attach itself to the futex.
> + Used to check if PI futexes are supported by the
> + kernel. */
> + return -err;
> +
> + 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 -EFAULT: /* Must have been caused by a glibc or application bug. */
> + /* No other errors are documented at this time. */
> + default:
> + futex_fatal_error ();
> + }
> +}
> #endif /* futex-internal.h */
>
More information about the Libc-alpha
mailing list