Re: [RFC PATCH v1] y2038: nptl: Convert pthread_mutex_{clock|timed}lock to support 64 bit

[Lukasz Majewski <lukma@denx.de>]

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

Dear Community,

> The pthread_mutex_clocklock and pthread_mutex_timedlock have been
> converted to support 64 bit time.
> 
> This change uses:
> - New __futex_clocklock64 function (instead of lll_clocklock)
> - New __futex_clocklock_wait64 (instead of lll_timedwait)
> 
> from ./sysdeps/nptl/futex-helpers.c and
> 
> - New futex_lock_pi64 defined in sysdeps/nptl/futex-internal.h
> 
> The pthread_mutex_{clock|timed}lock only accepts absolute time.
> Moreover, there is no need to check for NULL passed as *abstime
> pointer to the syscalls as those calls have exported symbols marked
> with __nonull attribute for abstime.
> 
> Some architectures - namely x86, powerpc and s390 - do support lock
> elision. For those - adjustments have been made in arch specific
> elision-*.c files to use __futex_clocklock64 instead of lll_clocklock.
> The __lll_lock_elision (aliased to __lll_clocklock_elision in e.g.
> sysdeps/unix/sysv/linux/s390/elision-timed.c) just uses, in this patch
> provided, __futex_clocklock64.
> 

I would be very grateful for comments regarding this patch.

Thanks in advance.

> For systems with __TIMESIZE != 64 && __WORDSIZE == 32:
> - Conversions between 64 bit time to 32 bit are necessary
> - Redirection to pthread_mutex_{clock|timed}lock will provide support
> for 64 bit time
> 
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
> ---
>  nptl/pthreadP.h                               |  9 +++
>  nptl/pthread_mutex_timedlock.c                | 67 +++++++++++------
>  sysdeps/nptl/futex-internal.c                 | 71
> +++++++++++++++++++ sysdeps/nptl/futex-internal.h                 |
> 52 ++++++++++++++ .../unix/sysv/linux/powerpc/elision-timed.c   |  5
> +- .../unix/sysv/linux/powerpc/lowlevellock.h    |  2 +-
>  sysdeps/unix/sysv/linux/s390/elision-timed.c  |  5 +-
>  sysdeps/unix/sysv/linux/s390/lowlevellock.h   |  2 +-
>  sysdeps/unix/sysv/linux/x86/elision-timed.c   |  5 +-
>  sysdeps/unix/sysv/linux/x86/lowlevellock.h    |  2 +-
>  10 files changed, 191 insertions(+), 29 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 5bcc8a2db5..710b21e890 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -468,6 +468,8 @@ extern int __pthread_cond_wait (pthread_cond_t
> *cond, pthread_mutex_t *mutex); # define
> __pthread_rwlock_clockwrlock64 __pthread_rwlock_clockwrlock # define
> __pthread_rwlock_timedrdlock64 __pthread_rwlock_timedrdlock # define
> __pthread_rwlock_timedwrlock64 __pthread_rwlock_timedwrlock +# define
> __pthread_mutex_clocklock64 __pthread_mutex_clocklock +# define
> __pthread_mutex_timedlock64 __pthread_mutex_timedlock #else
>  extern int __pthread_clockjoin_np64 (pthread_t threadid, void
> **thread_return, clockid_t clockid,
> @@ -499,6 +501,13 @@ libpthread_hidden_proto
> (__pthread_rwlock_timedrdlock64) extern int
> __pthread_rwlock_timedwrlock64 (pthread_rwlock_t *rwlock, const
> struct __timespec64 *abstime); libpthread_hidden_proto
> (__pthread_rwlock_timedwrlock64) +extern int
> __pthread_mutex_clocklock64 (pthread_mutex_t *mutex,
> +                                        clockid_t clockid,
> +                                        const struct __timespec64
> *abstime); +libpthread_hidden_proto (__pthread_mutex_clocklock64)
> +extern int __pthread_mutex_timedlock64 (pthread_mutex_t *mutex,
> +                                        const struct __timespec64
> *abstime); +libpthread_hidden_proto (__pthread_mutex_timedlock64)
>  #endif
>  
>  extern int __pthread_cond_timedwait (pthread_cond_t *cond,
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index 8ae814b984..424dff3cae 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -31,7 +31,7 @@
>  
>  #ifndef lll_clocklock_elision
>  #define lll_clocklock_elision(futex, adapt_count, clockid, abstime,
> private) \
> -  lll_clocklock (futex, clockid, abstime, private)
> +  __futex_clocklock64 (futex, clockid, abstime, private)
>  #endif
>  
>  #ifndef lll_trylock_elision
> @@ -45,7 +45,7 @@
>  int
>  __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>  				  clockid_t clockid,
> -				  const struct timespec *abstime)
> +				  const struct __timespec64 *abstime)
>  {
>    int oldval;
>    pid_t id = THREAD_GETMEM (THREAD_SELF, tid);
> @@ -76,8 +76,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, }
>  
>        /* We have to get the mutex.  */
> -      result = lll_clocklock (mutex->__data.__lock, clockid, abstime,
> -			      PTHREAD_MUTEX_PSHARED (mutex));
> +      result = __futex_clocklock64 (mutex->__data.__lock, clockid,
> abstime,
> +                                    PTHREAD_MUTEX_PSHARED (mutex));
>  
>        if (result != 0)
>  	goto out;
> @@ -99,8 +99,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, FORCE_ELISION (mutex, goto elision);
>      simple:
>        /* Normal mutex.  */
> -      result = lll_clocklock (mutex->__data.__lock, clockid, abstime,
> -			      PTHREAD_MUTEX_PSHARED (mutex));
> +      result = __futex_clocklock64 (mutex->__data.__lock, clockid,
> abstime,
> +                                    PTHREAD_MUTEX_PSHARED (mutex));
>        break;
>  
>      case PTHREAD_MUTEX_TIMED_ELISION_NP:
> @@ -125,9 +125,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, {
>  	      if (cnt++ >= max_cnt)
>  		{
> -		  result = lll_clocklock (mutex->__data.__lock,
> -					  clockid, abstime,
> -					  PTHREAD_MUTEX_PSHARED
> (mutex));
> +		  result = __futex_clocklock64 (mutex->__data.__lock,
> +		                                clockid, abstime,
> +
> PTHREAD_MUTEX_PSHARED (mutex)); break;
>  		}
>  	      atomic_spin_nop ();
> @@ -378,8 +378,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_pi ((unsigned int *)
> &mutex->__data.__lock,
> -				   abstime, private);
> +	    int e = futex_lock_pi64 ((unsigned int *)
> &mutex->__data.__lock,
> +				     abstime, private);
>  	    if (e == ETIMEDOUT)
>  	      return ETIMEDOUT;
>  	    else if (e == ESRCH || e == EDEADLK)
> @@ -394,8 +394,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, /* Delay the thread until the timeout is reached. Then return
>  		   ETIMEDOUT.  */
>  		do
> -		  e = lll_timedwait (&(int){0}, 0, clockid, abstime,
> -				     private);
> +		  e = __futex_clocklock_wait64 (&(int){0}, 0,
> clockid, abstime,
> +		                                private);
>  		while (e != ETIMEDOUT);
>  		return ETIMEDOUT;
>  	      }
> @@ -543,10 +543,10 @@ __pthread_mutex_clocklock_common
> (pthread_mutex_t *mutex, goto failpp;
>  		      }
>  
> -		    struct timespec rt;
> +		    struct __timespec64 rt;
>  
>  		    /* Get the current time.  */
> -		    __clock_gettime (CLOCK_REALTIME, &rt);
> +		    __clock_gettime64 (CLOCK_REALTIME, &rt);
>  
>  		    /* Compute relative timeout.  */
>  		    rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> @@ -599,9 +599,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, }
>  
>  int
> -__pthread_mutex_clocklock (pthread_mutex_t *mutex,
> -			   clockid_t clockid,
> -			   const struct timespec *abstime)
> +__pthread_mutex_clocklock64 (pthread_mutex_t *mutex,
> +			     clockid_t clockid,
> +			     const struct __timespec64 *abstime)
>  {
>    if (__glibc_unlikely (!lll_futex_supported_clockid (clockid)))
>      return EINVAL;
> @@ -609,13 +609,40 @@ __pthread_mutex_clocklock (pthread_mutex_t
> *mutex, LIBC_PROBE (mutex_clocklock_entry, 3, mutex, clockid,
> abstime); return __pthread_mutex_clocklock_common (mutex, clockid,
> abstime); }
> -weak_alias (__pthread_mutex_clocklock, pthread_mutex_clocklock)
> +
> +#if __TIMESIZE != 64
> +libpthread_hidden_def (__pthread_mutex_clocklock64)
>  
>  int
> -__pthread_mutex_timedlock (pthread_mutex_t *mutex,
> +__pthread_mutex_clocklock (pthread_mutex_t *mutex,
> +			   clockid_t clockid,
>  			   const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_mutex_clocklock64 (mutex, clockid, &ts64);
> +}
> +#endif
> +weak_alias (__pthread_mutex_clocklock, pthread_mutex_clocklock)
> +
> +int
> +__pthread_mutex_timedlock64 (pthread_mutex_t *mutex,
> +			     const struct __timespec64 *abstime)
>  {
>    LIBC_PROBE (mutex_timedlock_entry, 2, mutex, abstime);
>    return __pthread_mutex_clocklock_common (mutex, CLOCK_REALTIME,
> abstime); }
> +
> +#if __TIMESIZE != 64
> +libpthread_hidden_def (__pthread_mutex_timedlock64)
> +
> +int
> +__pthread_mutex_timedlock (pthread_mutex_t *mutex,
> +			   const struct timespec *abstime)
> +{
> +  struct __timespec64 ts64 = valid_timespec_to_timespec64 (*abstime);
> +
> +  return __pthread_mutex_timedlock64 (mutex, &ts64);
> +}
> +#endif
>  weak_alias (__pthread_mutex_timedlock, pthread_mutex_timedlock)
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index 3211b4c94f..2e1df42e98 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -167,3 +167,74 @@ __futex_abstimed_wait64 (unsigned int*
> futex_word, unsigned int expected, futex_fatal_error ();
>      }
>  }
> +
> +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;
> +}
> +
> +int
> +__futex_clocklock64 (int futex, clockid_t clockid,
> +                     const struct __timespec64 *abstime, int private)
> +{
> +  int *futexp = &futex;
> +  int err = 0;
> +
> +  if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq
> (futexp, 1, 0)))
> +    {
> +      while (atomic_exchange_acq (futexp, 2) != 0)
> +        {
> +          err = __futex_clocklock_wait64 (futexp, 2, clockid,
> abstime, private);
> +          if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW)
> +            break;
> +        }
> +    }
> +  return err;
> +}
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 1ba0d61938..e84024591d 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -437,6 +437,51 @@ futex_lock_pi (unsigned int *futex_word, const
> struct timespec *abstime, }
>  }
>  
> +static __always_inline int
> +futex_lock_pi64 (unsigned int *futex_word, const struct __timespec64
> *abstime,
> +                 int private)
> +{
> +  int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
> +                                   __lll_private_flag
> +                                   (FUTEX_LOCK_PI, private), 0,
> abstime); +#ifndef __ASSUME_TIME64_SYSCALLS
> +  if (err == -ENOSYS)
> +    {
> +      if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> +        return EOVERFLOW;
> +
> +      struct timespec ts32;
> +      if (abstime != NULL)
> +        ts32 = valid_timespec64_to_timespec (*abstime);
> +
> +      err = INTERNAL_SYSCALL_CALL (futex, futex_word,
> __lll_private_flag
> +                                   (FUTEX_LOCK_PI, private), 0,
> +                                   abstime != NULL ? &ts32 : NULL);
> +    }
> +#endif
> +  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 ();
> +    }
> +}
> +
>  /* Wakes the top priority waiter that called a futex_lock_pi
> operation on the futex.
>  
> @@ -535,4 +580,11 @@ __futex_abstimed_wait64 (unsigned int*
> futex_word, unsigned int expected, const struct __timespec64* abstime,
>                           int private) attribute_hidden;
>  
> +int
> +__futex_clocklock64 (int futex, clockid_t clockid,
> +                     const struct __timespec64 *abstime, int
> private); +
> +int
> +__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
> +                          const struct __timespec64 *abstime, int
> private); #endif  /* futex-internal.h */
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-timed.c
> b/sysdeps/unix/sysv/linux/powerpc/elision-timed.c index
> 5c2b500f1d..bab826dcba 100644 ---
> a/sysdeps/unix/sysv/linux/powerpc/elision-timed.c +++
> b/sysdeps/unix/sysv/linux/powerpc/elision-timed.c @@ -19,10 +19,11 @@
>  #include <time.h>
>  #include <elision-conf.h>
>  #include "lowlevellock.h"
> +#include "sysdeps/nptl/futex-internal.h"
>  
>  #define __lll_lock_elision __lll_clocklock_elision
> -#define EXTRAARG clockid_t clockid, const struct timespec *t,
> +#define EXTRAARG clockid_t clockid, const struct __timespec64 *t,
>  #undef LLL_LOCK
> -#define LLL_LOCK(a, b) lll_clocklock(a, clockid, t, b)
> +#define LLL_LOCK(a, b) __futex_clocklock64 (a, clockid, t, b)
>  
>  #include "elision-lock.c"
> diff --git a/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h
> b/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h index
> 53ada4a04b..fe7a5d2da5 100644 ---
> a/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h +++
> b/sysdeps/unix/sysv/linux/powerpc/lowlevellock.h @@ -24,7 +24,7 @@
>  /* Transactional lock elision definitions.  */
>  extern int __lll_clocklock_elision
>    (int *futex, short *adapt_count,
> -   clockid_t clockid, const struct timespec *timeout, int private)
> +   clockid_t clockid, const struct __timespec64 *timeout, int
> private) attribute_hidden;
>  
>  #define lll_clocklock_elision(futex, adapt_count, clockid, timeout,
> private) \ diff --git a/sysdeps/unix/sysv/linux/s390/elision-timed.c
> b/sysdeps/unix/sysv/linux/s390/elision-timed.c index
> 83d6a83d8d..4f8174c5a9 100644 ---
> a/sysdeps/unix/sysv/linux/s390/elision-timed.c +++
> b/sysdeps/unix/sysv/linux/s390/elision-timed.c @@ -19,8 +19,9 @@
>  #include <time.h>
>  #include <elision-conf.h>
>  #include <lowlevellock.h>
> +#include "sysdeps/nptl/futex-internal.h"
>  #define __lll_lock_elision __lll_clocklock_elision
> -#define EXTRAARG clockid_t clockid, const struct timespec *t,
> +#define EXTRAARG clockid_t clockid, const struct __timespec64 *t,
>  #undef LLL_LOCK
> -#define LLL_LOCK(a, b) lll_clocklock(a, clockid, t, b)
> +#define LLL_LOCK(a, b) __futex_clocklock64 (a, clockid, t, b)
>  #include "elision-lock.c"
> diff --git a/sysdeps/unix/sysv/linux/s390/lowlevellock.h
> b/sysdeps/unix/sysv/linux/s390/lowlevellock.h index
> c790077a79..27bc23ff24 100644 ---
> a/sysdeps/unix/sysv/linux/s390/lowlevellock.h +++
> b/sysdeps/unix/sysv/linux/s390/lowlevellock.h @@ -24,7 +24,7 @@
>  /* Transactional lock elision definitions.  */
>  extern int __lll_clocklock_elision
>    (int *futex, short *adapt_count,
> -   clockid_t clockid, const struct timespec *timeout, int private)
> +   clockid_t clockid, const struct __timespec64 *timeout, int
> private) attribute_hidden;
>  
>  #  define lll_clocklock_elision(futex, adapt_count, clockid,
> timeout, private) \ diff --git
> a/sysdeps/unix/sysv/linux/x86/elision-timed.c
> b/sysdeps/unix/sysv/linux/x86/elision-timed.c index
> 87e5c788c6..03212d8cd9 100644 ---
> a/sysdeps/unix/sysv/linux/x86/elision-timed.c +++
> b/sysdeps/unix/sysv/linux/x86/elision-timed.c @@ -19,8 +19,9 @@
> #include <time.h> #include <elision-conf.h>
>  #include "lowlevellock.h"
> +#include "sysdeps/nptl/futex-internal.h"
>  #define __lll_lock_elision __lll_clocklock_elision
> -#define EXTRAARG clockid_t clockid, const struct timespec *t,
> +#define EXTRAARG clockid_t clockid, const struct __timespec64 *t,
>  #undef LLL_LOCK
> -#define LLL_LOCK(a, b) lll_clocklock (a, clockid, t, b)
> +#define LLL_LOCK(a, b) __futex_clocklock64 (a, clockid, t, b)
>  #include "elision-lock.c"
> diff --git a/sysdeps/unix/sysv/linux/x86/lowlevellock.h
> b/sysdeps/unix/sysv/linux/x86/lowlevellock.h index
> 27d62c9301..d0ea71b105 100644 ---
> a/sysdeps/unix/sysv/linux/x86/lowlevellock.h +++
> b/sysdeps/unix/sysv/linux/x86/lowlevellock.h @@ -84,7 +84,7 @@
> __lll_cas_lock (int *futex) 
>  extern int __lll_clocklock_elision (int *futex, short *adapt_count,
>                                      clockid_t clockid,
> -				    const struct timespec *timeout,
> +				    const struct __timespec64
> *timeout, int private) attribute_hidden;
>  
>  #define lll_clocklock_elision(futex, adapt_count, clockid, timeout,
> private) \




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