Re: [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 5537 bytes --]

Hi Adhemerval,

> The __futex_abstimed_wait64 needs also to return EINVAL syscall
> errors.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/pthread_mutex_timedlock.c |  4 +--
>  sysdeps/nptl/futex-internal.c  | 57
> +--------------------------------- sysdeps/nptl/futex-internal.h  |
> 7 ++--- 3 files changed, 5 insertions(+), 63 deletions(-)
> 
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index 0ec47359be..e643eab258 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -391,8 +391,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, /* Delay the thread until the timeout is reached. Then return
>  		   ETIMEDOUT.  */
>  		do
> -		  e = __futex_clocklock_wait64 (&(int){0}, 0,
> clockid, abstime,
> -		                                private);
> +		  e = __futex_abstimed_wait64 (&(unsigned int){0},
> 0, clockid,
> +					       abstime, private);

I think that it would be nice to have more verbose commit message - to
clearly explain why it is possible to replace "*_clocklock_*" with
"*_abstimed_*".

Reviewed-by: Lukasz Majewski <lukma@denx.de>

>  		while (e != ETIMEDOUT);
>  		return ETIMEDOUT;
>  	      }
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index 30c662547f..11031cc46a 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -90,15 +90,13 @@ __futex_abstimed_wait_common64 (unsigned int*
> futex_word, case -EAGAIN:
>      case -EINTR:
>      case -ETIMEDOUT:
> +    case -EINVAL:
>      case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t
> type, but underlying kernel does not support 64 bit time_t futex
>                           syscalls.  */
>        return -err;
>  
>      case -EFAULT: /* Must have been caused by a glibc or application
> bug.  */
> -    case -EINVAL: /* Either due to wrong alignment or due to the
> timeout not
> -		     being normalized.  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:
> @@ -124,56 +122,3 @@ __futex_abstimed_wait_cancelable64 (unsigned
> int* futex_word, return __futex_abstimed_wait_common64 (futex_word,
> expected, clockid, abstime, private, true);
>  }
> -
> -int
> -__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
> -                          const struct __timespec64 *abstime, int
> private) -{
> -  struct __timespec64 ts, *tsp = NULL;
> -
> -  if (abstime != NULL)
> -    {
> -      /* Reject invalid timeouts.  */
> -      if (! valid_nanoseconds (abstime->tv_nsec))
> -        return EINVAL;
> -
> -      /* Get the current time. This can only fail if clockid is not
> valid.  */
> -      if (__glibc_unlikely (__clock_gettime64 (clockid, &ts) != 0))
> -        return EINVAL;
> -
> -      /* Compute relative timeout.  */
> -      ts.tv_sec = abstime->tv_sec - ts.tv_sec;
> -      ts.tv_nsec = abstime->tv_nsec - ts.tv_nsec;
> -      if (ts.tv_nsec < 0)
> -        {
> -	  ts.tv_nsec += 1000000000;
> -	  --ts.tv_sec;
> -        }
> -
> -      if (ts.tv_sec < 0)
> -        return ETIMEDOUT;
> -
> -      tsp = &ts;
> -    }
> -
> -  int err = INTERNAL_SYSCALL_CALL (futex_time64, futex,
> -                                   __lll_private_flag (FUTEX_WAIT,
> private),
> -                                   val, tsp);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> -  if (err == -ENOSYS)
> -    {
> -      if (tsp != NULL && ! in_time_t_range (tsp->tv_sec))
> -        return EOVERFLOW;
> -
> -      struct timespec ts32;
> -      if (tsp != NULL)
> -        ts32 = valid_timespec64_to_timespec (*tsp);
> -
> -      err = INTERNAL_SYSCALL_CALL (futex, futex,
> -                                   __lll_private_flag (FUTEX_WAIT,
> private),
> -                                   val, tsp != NULL ? &ts32 : NULL);
> -    }
> -#endif
> -
> -  return -err;
> -}
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index dcc7958d59..632051278b 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -417,10 +417,6 @@ __futex_abstimed_wait64 (unsigned int*
> futex_word, unsigned int expected, const struct __timespec64* abstime,
>                           int private) attribute_hidden;
>  
> -int
> -__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
> -                          const struct __timespec64 *abstime,
> -                          int private) attribute_hidden;
>  
>  static __always_inline int
>  __futex_clocklock64 (int *futex, clockid_t clockid,
> @@ -432,7 +428,8 @@ __futex_clocklock64 (int *futex, clockid_t
> clockid, {
>        while (atomic_exchange_acq (futex, 2) != 0)
>          {
> -          err = __futex_clocklock_wait64 (futex, 2, clockid,
> abstime, private);
> +          err = __futex_abstimed_wait64 ((unsigned int *) futex, 2,
> clockid,
> +					 abstime, private);
>            if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW)
>              break;
>          }




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]