[PATCH 2/4] nptl: Use FUTEX_LOCK_PI2 when available
Adhemerval Zanella
adhemerval.zanella@linaro.org
Thu Aug 12 12:40:58 GMT 2021
On 12/08/2021 03:42, Kurt Kanzenbach wrote:
> 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 <kurt@linutronix.de>
>> ---
>> 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;
The ___pthread_mutex_clocklock64() already checks it (as other functions that uses
a clockid_t). I also tested for invalid clocks on tst-mutexpi10.c.
And I think that the check on __futex_abstimed_wait_common() is superfluous..
>
>> +
>> + 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?
If timeout is not define the clock is ignore, isn't?
>
>> + 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:
Thanks, I updated it locally.
>
> --- 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
>
More information about the Libc-alpha
mailing list