Message ID | 20201123195256.3336217-3-adhemerval.zanella@linaro.org |
---|---|
State | New |
Headers | show |
Series | [01/13] linux: Remove unused internal futex functions | expand |
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
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. */