Hi Adhemerval, thanks for the respin of this series. Just some minor comments below. On Wed Aug 11 2021, Adhemerval Zanella wrote: > This patch uses the new futex PI operation provided by Linux v5.14 > when it is required. > > The futex_lock_pi64() is moved to futex-internal.c (since it used on > two different places and its code size might be large depending of the > kernel configuration) and clockid is added as an argument. > > Co-authored-by: Kurt Kanzenbach > --- > nptl/futex-internal.c | 63 +++++++++++++++++++++++++++++++ > nptl/pthread_mutex_lock.c | 3 +- > nptl/pthread_mutex_timedlock.c | 3 +- > sysdeps/nptl/futex-internal.h | 54 +------------------------- > sysdeps/nptl/lowlevellock-futex.h | 1 + > 5 files changed, 70 insertions(+), 54 deletions(-) > > diff --git a/nptl/futex-internal.c b/nptl/futex-internal.c > index e74647a9d4..58605b2fca 100644 > --- a/nptl/futex-internal.c > +++ b/nptl/futex-internal.c > @@ -140,3 +140,66 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word, > abstime, private, true); > } > libc_hidden_def (__futex_abstimed_wait_cancelable64) > + > +int > +__futex_lock_pi64 (int *futex_word, clockid_t clockid, > + const struct __timespec64 *abstime, int private) > +{ > + int err; Is the clockid check not needed? if (! lll_futex_supported_clockid (clockid)) return EINVAL; > + > + unsigned int clockbit = clockid == CLOCK_REALTIME > + ? FUTEX_CLOCK_REALTIME : 0; > + int op_pi2 = __lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private); > +#if __ASSUME_FUTEX_LOCK_PI2 > + /* Assume __ASSUME_TIME64_SYSCALLS since FUTEX_LOCK_PI2 was added later. */ > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi2, 0, abstime); > +#else > + /* FUTEX_LOCK_PI does not support clock selection, so for CLOCK_MONOTONIC > + the only option is to use FUTEX_LOCK_PI2. */ > + int op_pi1 = __lll_private_flag (FUTEX_LOCK_PI, private); > + int op_pi = abstime != NULL && clockid != CLOCK_REALTIME ? op_pi2 : op_pi1; > + > +# ifdef __ASSUME_TIME64_SYSCALLS > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime); > +# else > + bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec); > + if (need_time64) > + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime); > + else > + { > + struct timespec ts32, *pts32 = NULL; > + if (abstime != NULL) > + { > + ts32 = valid_timespec64_to_timespec (*abstime); > + pts32 = &ts32; > + } > + err = INTERNAL_SYSCALL_CALL (futex, futex_word, op_pi, 0, pts32); > + } > +# endif /* __ASSUME_TIME64_SYSCALLS */ > + /* FUTEX_LOCK_PI2 is not available on this kernel. */ > + if (err == -ENOSYS) > + err = -EINVAL; > +#endif /* __ASSUME_FUTEX_LOCK_PI2 */ > + > + switch (err) > + { > + case 0: > + case -EAGAIN: > + case -EINTR: > + case -ETIMEDOUT: > + case -ESRCH: > + case -EDEADLK: > + case -EINVAL: /* This indicates either state corruption or that the kernel > + found a waiter on futex address which is waiting via > + FUTEX_WAIT or FUTEX_WAIT_BITSET. This is reported on > + some futex_lock_pi usage (pthread_mutex_timedlock for > + instance). */ > + return -err; > + > + case -EFAULT: /* 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 (); > + } > +} > diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c > index da624f322d..8e2ff2793f 100644 > --- a/nptl/pthread_mutex_lock.c > +++ b/nptl/pthread_mutex_lock.c > @@ -422,7 +422,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex) > int private = (robust > ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) > : PTHREAD_MUTEX_PSHARED (mutex)); > - int e = futex_lock_pi64 (&mutex->__data.__lock, NULL, private); > + int e = __futex_lock_pi64 (&mutex->__data.__lock, 0 /* ununsed */, And then use CLOCK_REALTIME here? > + NULL, private); > if (e == ESRCH || e == EDEADLK) > { > assert (e != EDEADLK > diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c > index 11ad7005d0..ca51da6f6c 100644 > --- a/nptl/pthread_mutex_timedlock.c > +++ b/nptl/pthread_mutex_timedlock.c > @@ -370,7 +370,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex, > int private = (robust > ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex) > : PTHREAD_MUTEX_PSHARED (mutex)); > - int e = futex_lock_pi64 (&mutex->__data.__lock, abstime, private); > + int e = __futex_lock_pi64 (&mutex->__data.__lock, clockid, abstime, > + private); > if (e == ETIMEDOUT) > return ETIMEDOUT; > else if (e == ESRCH || e == EDEADLK) > diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h > index 79a366604d..f0b7f814cc 100644 > --- a/sysdeps/nptl/futex-internal.h > +++ b/sysdeps/nptl/futex-internal.h > @@ -250,58 +250,8 @@ futex_wake (unsigned int* futex_word, int processes_to_wake, int private) > futex. > - ETIMEDOUT if the ABSTIME expires. > */ I think the documentation above should be slightly updated for this function: --- a/sysdeps/nptl/futex-internal.h +++ b/sysdeps/nptl/futex-internal.h @@ -236,8 +236,8 @@ futex_wake (unsigned int* futex_word, in are done in descending priority order. The ABSTIME arguments provides an absolute timeout (measured against the - CLOCK_REALTIME clock). If TIMEOUT is NULL, the operation will block - indefinitely. + CLOCK_REALTIME or CLOCK_MONOTONIC clock). If TIMEOUT is NULL, the operation + will block indefinitely. Returns: @@ -247,61 +247,11 @@ futex_wake (unsigned int* futex_word, in - EDEADLK if the futex is already locked by the caller. - ESRCH if the thread ID int he futex does not exist. - EINVAL is the state is corrupted or if there is a waiter on the - futex. + futex or if the clockid is invalid. - ETIMEDOUT if the ABSTIME expires. */ Thanks, Kurt