Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 25/11/2020 12:32, Mike Crowe wrote:
> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>> The idea is to make NPTL implementation to use on the functions
>> provided by futex-internal.h.
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>> ---
>>  nptl/lowlevellock.c                 | 6 +++---
>>  nptl/pthread_mutex_lock.c           | 9 +++++----
>>  nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>  nptl/pthread_mutex_timedlock.c      | 6 +++---
>>  4 files changed, 14 insertions(+), 12 deletions(-)
> 
> [snip]
> 
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index e643eab258..343acf6107 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>  			goto failpp;
>>  		      }
>>  
>> -		    lll_futex_timed_wait (&mutex->__data.__lock,
>> -					  ceilval | 2, &rt,
>> -					  PTHREAD_MUTEX_PSHARED (mutex));
>> +		    __futex_abstimed_wait64 (
>> +		      (unsigned int *) &mutex->__data.__lock, clockid,
>> +		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> 
> I think you've replaced the lll_futex_timed_wait call that expects a
> relative timeout with a __futex_abstimed_wait64 call that expects an
> absolute timeout, yet you still appear to be passing the relative timeout.
> 
> However, it turns out that the implementation for the
> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> completely broken with clockid != CLOCK_REALTIME ever since I added it in
> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> time this was a less obvious __gettimeofday call.)
> 
> I'll work on writing some test cases for the those types of mutex in the
> hope of catching both flaws before fixing them.

Indeed, there is no need to calculate the relative timeout anymore. I think
the fix below should pass the absolute timeout directly.   I will check
a possible regression tests as well.


diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index aaaafa21ce..86c5f4446e 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 	    if (__pthread_current_priority () > ceiling)
 	      {
 		result = EINVAL;
-	      failpp:
 		if (oldprio != -1)
 		  __pthread_tpp_change_priority (oldprio, -1);
 		return result;
@@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
 
 		if (oldval != ceilval)
 		  {
-		    /* Reject invalid timeouts.  */
-		    if (! valid_nanoseconds (abstime->tv_nsec))
-		      {
-			result = EINVAL;
-			goto failpp;
-		      }
-
-		    struct __timespec64 rt;
-
-		    /* Get the current time.  */
-		    __clock_gettime64 (CLOCK_REALTIME, &rt);
-
-		    /* 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)
-		      {
-			result = ETIMEDOUT;
-			goto failpp;
-		      }
-
 		    __futex_abstimed_wait64 (
 		      (unsigned int *) &mutex->__data.__lock, clockid,
-		      ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
+		      ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
 		  }
 	      }
 	    while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,