[2/4] nptl: Use FUTEX_LOCK_PI2 when available

  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(-)
  

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;

> +
> +  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
  

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
>
  

On Thu Aug 12 2021, Adhemerval Zanella wrote:
> 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..

OK, I see.

Thanks,
Kurt