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

[Mike Crowe]

On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
> 
> 
> 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.

OK. I won't then. Thanks.

> 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;
> -		      }

If this is removed then is there a risk of getting into a busy loop if
someone passes a bogus timespec? (Regardless of the answer, it makes sense
to ensure that is tested somehow.)

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

LGTM.

Thanks.

Mike.