[03/13] nptl: Remove clockwait_tid
Commit Message
It can be replaced with a __futex_abstimed_wait_cancelable64 call,
with the advantage that there is no need to further clock adjustments
to create a absolute timeout. It allows to remove the now ununsed
futex_timed_wait_cancel64 internal function.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_join_common.c | 70 ++++++-----------------------------
sysdeps/nptl/futex-internal.h | 49 ------------------------
2 files changed, 12 insertions(+), 107 deletions(-)
Comments
On Mon, 23 Nov 2020 16:52:46 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:
> It can be replaced with a __futex_abstimed_wait_cancelable64 call,
> with the advantage that there is no need to further clock adjustments
> to create a absolute timeout. It allows to remove the now ununsed
> futex_timed_wait_cancel64 internal function.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_join_common.c | 70
> ++++++----------------------------- sysdeps/nptl/futex-internal.h |
> 49 ------------------------ 2 files changed, 12 insertions(+), 107
> deletions(-)
>
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 67d8e2b780..8a95c58ff3 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -32,55 +32,6 @@ cleanup (void *arg)
> atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
> }
>
> -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
> futex
> - wake-up when the clone terminates. The memory location contains
> the
> - thread ID while the clone is running and is reset to zero by the
> kernel
> - 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 __timespec64 *abstime)
> -{
> - pid_t tid;
> - int ret;
> -
> - if (! valid_nanoseconds (abstime->tv_nsec))
> - return EINVAL;
> -
> - /* Repeat until thread terminated. */
> - while ((tid = *tidp) != 0)
> - {
> - struct __timespec64 rt;
> -
> - /* Get the current time. This can only fail if clockid is
> - invalid. */
> - if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
> - return EINVAL;
> -
> - /* Compute relative timeout. */
> - rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> - if (rt.tv_nsec < 0)
> - {
> - rt.tv_nsec += 1000000000;
> - --rt.tv_sec;
> - }
> -
> - /* Already timed out? */
> - if (rt.tv_sec < 0)
> - return ETIMEDOUT;
> -
> - /* 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. */
> - ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> - if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> - return -ret;
> - }
> -
> - return 0;
> -}
> -
> int
> __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> clockid_t clockid,
> @@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void
> **thread_return, un-wait-ed for again. */
> pthread_cleanup_push (cleanup, &pd->joinid);
>
> - if (abstime != NULL)
> - result = clockwait_tid (&pd->tid, clockid, abstime);
> - else
> - {
> - pid_t tid;
> - /* We need acquire MO here so that we synchronize with the
> - kernel's store to 0 when the clone terminates. (see
> above) */
> - while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> - lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
> + /* We need acquire MO here so that we synchronize with the
> + kernel's store to 0 when the clone terminates. (see above)
> */
> + pid_t tid;
> + while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> + {
> + int ret = __futex_abstimed_wait_cancelable64 (
> + (unsigned int *) &pd->tid, tid, clockid, abstime,
> LLL_SHARED);
> + if (ret == ETIMEDOUT || ret == EOVERFLOW)
> + {
> + result = ret;
> + break;
> + }
> }
>
> pthread_cleanup_pop (0);
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 96d1318091..d5f13d15fb 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int
> private) }
> }
>
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> - const struct __timespec64 *timeout, int
> private) -{
> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> - __lll_private_flag (FUTEX_WAIT,
> private),
> - tid, timeout);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> - if (err == -ENOSYS)
> - {
> - if (in_time_t_range (timeout->tv_sec))
> - {
> - struct timespec ts32 = valid_timespec64_to_timespec
> (*timeout); -
> - err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> - __lll_private_flag
> (FUTEX_WAIT,
> -
> private),
> - tid, &ts32);
> - }
> - else
> - err = -EOVERFLOW;
> - }
> -#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
> - underlying kernel does not support 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 ();
> - }
> -}
> -
> /* The futex_abstimed_wait_cancelable64 has been moved to a separate
> file to avoid problems with exhausting available registers on some
> architectures
> - e.g. on m68k architecture. */
Reviewed-by: Lukasz Majewski <lukma@denx.de>
Best regards,
Lukasz Majewski
--
DENX Software Engineering GmbH, Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
* Adhemerval Zanella via Libc-alpha:
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> - const struct __timespec64 *timeout, int private)
> -{
> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> - __lll_private_flag (FUTEX_WAIT, private),
> - tid, timeout);
This uses FUTEX_WAIT. But the replacement,
__futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET. I do not think
this is correct because the kernel will use FUTEX_WAKE internally for
the pd->tid wakeup relied upon by pthread_join.
This seems to cause pthread_join regressions on some kernel versions.
We need to audit all callers of __futex_abstimed_wait64 if they are
actually compatible with FUTEX_WAIT_BITSET.
Thanks,
Florian
On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
> This uses FUTEX_WAIT. But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
FUTEX_WAIT is exactly equivalent to
FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
Andreas.
On 14/12/2020 09:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> -static __always_inline int
>> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
>> - const struct __timespec64 *timeout, int private)
>> -{
>> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
>> - __lll_private_flag (FUTEX_WAIT, private),
>> - tid, timeout);
>
> This uses FUTEX_WAIT. But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET. I do not think
> this is correct because the kernel will use FUTEX_WAKE internally for
> the pd->tid wakeup relied upon by pthread_join.
>
> This seems to cause pthread_join regressions on some kernel versions.
>
> We need to audit all callers of __futex_abstimed_wait64 if they are
> actually compatible with FUTEX_WAIT_BITSET.
I don't think it should matter, as Andreas has put FUTEX_WAIT is exactly
as FUTEX_WAIT_BITSET plus FUTEX_BITSET_MATCH_ANY. On a recent kernel:
SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
u32, val3)
{
[...]
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
}
long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
u32 __user *uaddr2, u32 val2, u32 val3)
{
[...]
case FUTEX_WAIT:
val3 = FUTEX_BITSET_MATCH_ANY;
fallthrough;
case FUTEX_WAIT_BITSET:
return futex_wait(uaddr, flags, val, timeout, val3);
[...]
* Andreas Schwab:
> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>
>> This uses FUTEX_WAIT. But the replacement,
>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>
> FUTEX_WAIT is exactly equivalent to
> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
Hmm. I will continue debugging. I encounter a missed wakeup in
pthread_join after making some changes that only should affect timing
(intl/tst-gettext6 is among the failures).
Reverting this patch here seems to help.
Thanks,
Florian
* Florian Weimer:
> * Andreas Schwab:
>
>> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>>
>>> This uses FUTEX_WAIT. But the replacement,
>>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>>
>> FUTEX_WAIT is exactly equivalent to
>> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
>
> Hmm. I will continue debugging. I encounter a missed wakeup in
> pthread_join after making some changes that only should affect timing
> (intl/tst-gettext6 is among the failures).
>
> Reverting this patch here seems to help.
It's an accident, due to my changes and:
/* In libc.so or ld.so all futexes are private. */
in lowlevellock-futex.h. So the patch posted here should be okay.
Thanks,
Florian
@@ -32,55 +32,6 @@ cleanup (void *arg)
atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
}
-/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex
- wake-up when the clone terminates. The memory location contains the
- thread ID while the clone is running and is reset to zero by the kernel
- 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 __timespec64 *abstime)
-{
- pid_t tid;
- int ret;
-
- if (! valid_nanoseconds (abstime->tv_nsec))
- return EINVAL;
-
- /* Repeat until thread terminated. */
- while ((tid = *tidp) != 0)
- {
- struct __timespec64 rt;
-
- /* Get the current time. This can only fail if clockid is
- invalid. */
- if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
- return EINVAL;
-
- /* Compute relative timeout. */
- rt.tv_sec = abstime->tv_sec - rt.tv_sec;
- rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
- if (rt.tv_nsec < 0)
- {
- rt.tv_nsec += 1000000000;
- --rt.tv_sec;
- }
-
- /* Already timed out? */
- if (rt.tv_sec < 0)
- return ETIMEDOUT;
-
- /* 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. */
- ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
- if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
- return -ret;
- }
-
- return 0;
-}
-
int
__pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
clockid_t clockid,
@@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
un-wait-ed for again. */
pthread_cleanup_push (cleanup, &pd->joinid);
- if (abstime != NULL)
- result = clockwait_tid (&pd->tid, clockid, abstime);
- else
- {
- pid_t tid;
- /* We need acquire MO here so that we synchronize with the
- kernel's store to 0 when the clone terminates. (see above) */
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
+ /* We need acquire MO here so that we synchronize with the
+ kernel's store to 0 when the clone terminates. (see above) */
+ pid_t tid;
+ while ((tid = atomic_load_acquire (&pd->tid)) != 0)
+ {
+ int ret = __futex_abstimed_wait_cancelable64 (
+ (unsigned int *) &pd->tid, tid, clockid, abstime, LLL_SHARED);
+ if (ret == ETIMEDOUT || ret == EOVERFLOW)
+ {
+ result = ret;
+ break;
+ }
}
pthread_cleanup_pop (0);
@@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int private)
}
}
-static __always_inline int
-futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
- const struct __timespec64 *timeout, int private)
-{
- int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
- __lll_private_flag (FUTEX_WAIT, private),
- tid, timeout);
-#ifndef __ASSUME_TIME64_SYSCALLS
- if (err == -ENOSYS)
- {
- if (in_time_t_range (timeout->tv_sec))
- {
- struct timespec ts32 = valid_timespec64_to_timespec (*timeout);
-
- err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
- __lll_private_flag (FUTEX_WAIT,
- private),
- tid, &ts32);
- }
- else
- err = -EOVERFLOW;
- }
-#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
- underlying kernel does not support 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 ();
- }
-}
-
/* The futex_abstimed_wait_cancelable64 has been moved to a separate file
to avoid problems with exhausting available registers on some architectures
- e.g. on m68k architecture. */