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

Message ID 20200923122502.28172-1-lukma@denx.de
State Dropped
Delegated to: Lukasz Majewski
Headers
Series [RFC,v1] y2038: nptl: Convert pthread_mutex_{clock|timed}lock to support 64 bit |

Commit Message

Lukasz Majewski Sept. 23, 2020, 12:25 p.m. UTC
  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.

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

Comments

Lukasz Majewski Oct. 4, 2020, 3:21 p.m. UTC | #1
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
  
Andreas Schwab Oct. 5, 2020, 8:08 a.m. UTC | #2
On Sep 23 2020, Lukasz Majewski wrote:

> +  int err = INTERNAL_SYSCALL_CALL (futex_time64, futex, __lll_private_flag
> +                                   (FUTEX_WAIT, private), val, tsp);

Don't split function calls before the paren.

Andreas.
  
Adhemerval Zanella Netto Oct. 5, 2020, 2:41 p.m. UTC | #3
On 23/09/2020 09:25, Lukasz Majewski wrote:
> 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.
> 
> 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

Some comments below.

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

Ok.

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

Ok.

> 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

Ok.

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

Ok.

> @@ -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:

Ok.

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

Ok.

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

The only usage of futex_lock_pi64 is here, so intead to force a cast just
change the futex_lock_pi64 prototype to accept a 'int *' as argument for
the futex.

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

Ok.

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

Ok.

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

Ok.

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

Ok.

> 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.  */

Double space after period.

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

As indicated by Andreas, don't split function calls before the paren.

> +#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);

Ok.                               

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


This is incorrect I am not sure why it hasn't triggered any regression in your
testing.  The 'lll_clocklock' and the '__lll_clocklock' works because they are
macros and expands the the futex *address* on the same TU. Passing the futex
*value* and using the address of the local variable uses the different *address*
than the intended one.

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

I think the idea of the current lll_clocklock macro is to inline the atomic 
fast path and I think we should keep the same strategy to avoid possible 
performance regressions.

So in a short:

  1. Move __futex_clocklock64 implementation to futex-internal.h and change
     its prototype to receive the futex address (similar to __lll_clocklock).

  static inline int
  __futex_clocklock64 (int *futex, clockid_t clockid,
                       const struct __timespec64 *abstime, int private)
  { 
    int err = 0;
    if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq (futex, 1, 0)))
      { 
        while (atomic_exchange_acq (futex, 2) != 0)
          { 
            err = __futex_clocklock_wait64 (futex, 2, clockid, abstime, private);
            if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW)
              break;
          }
      }
    return err;
  }

  2. Call __futex_clocklock64 nptl/pthread_mutex_timedlock.c using the expected
     signature, i.e, by passing the lock address (&mutex->__data.__lock).

  3. Adapt the elision-timed.c implementations from x86, powerpc, and s390 to
     do the same.


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

Same as before, don't split function calls before the paren.

> +#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);
> +    }

Ok.

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

Ok.

> @@ -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 */

As before the function prototype is not really correct.

> 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) \
>
  
Lukasz Majewski Oct. 6, 2020, 9:42 a.m. UTC | #4
Hi Adhemerval,

> On 23/09/2020 09:25, Lukasz Majewski wrote:
> > 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.
> > 
> > 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  
> 
> Some comments below.

Thank you for your feedback.

> 
> > ---
> >  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,  
> 
> Ok.
> 
> > @@ -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,  
> 
> Ok.
> 
> > 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  
> 
> Ok.
> 
> > @@ -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;  
> 
> Ok.
> 
> > @@ -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:  
> 
> Ok.
> 
> > @@ -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 ();  
> 
> Ok.
> 
> > @@ -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)  
> 
> The only usage of futex_lock_pi64 is here, so intead to force a cast
> just change the futex_lock_pi64 prototype to accept a 'int *' as
> argument for the futex.

Ok.

> 
> > @@ -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;
> >  	      }  
> 
> Ok.
> 
> > @@ -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;  
> 
> Ok.
> 
> > @@ -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;  
> 
> Ok.
> 
> > @@ -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)  
> 
> Ok.
> 
> > 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.  */  
> 
> Double space after period.

I've double checked this and there are two spaces between . and *.

> 
> > +      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);  
> 
> As indicated by Andreas, don't split function calls before the paren.

So it shall be:

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);  
> 
> Ok.                               
> 
> > +
> > +      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;  
> 
> 
> This is incorrect I am not sure why it hasn't triggered any
> regression in your testing. 

Please note that this is the RFC patch as I was not sure about the
"elision" related changes.

> The 'lll_clocklock' and the
> '__lll_clocklock' works because they are macros and expands the the
> futex *address* on the same TU.

Yes, you are of course right here. Thanks for spotting it.

> Passing the futex *value* and using
> the address of the local variable uses the different *address* than
> the intended one.

Yes, correct. I've adjusted the code accordingly.

> 
> > +  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;
> > +}  
> 
> I think the idea of the current lll_clocklock macro is to inline the
> atomic fast path and I think we should keep the same strategy to
> avoid possible performance regressions.

Ok.

> 
> So in a short:
> 
>   1. Move __futex_clocklock64 implementation to futex-internal.h and
> change its prototype to receive the futex address (similar to
> __lll_clocklock).
> 
>   static inline int
>   __futex_clocklock64 (int *futex, clockid_t clockid,
>                        const struct __timespec64 *abstime, int
> private) { 
>     int err = 0;
>     if (__glibc_unlikely (atomic_compare_and_exchange_bool_acq
> (futex, 1, 0))) { 
>         while (atomic_exchange_acq (futex, 2) != 0)
>           { 
>             err = __futex_clocklock_wait64 (futex, 2, clockid,
> abstime, private); if (err == EINVAL || err == ETIMEDOUT || err ==
> EOVERFLOW) break;
>           }
>       }
>     return err;
>   }
> 
>   2. Call __futex_clocklock64 nptl/pthread_mutex_timedlock.c using
> the expected signature, i.e, by passing the lock address
> (&mutex->__data.__lock).
> 
>   3. Adapt the elision-timed.c implementations from x86, powerpc, and
> s390 to do the same.
> 

Ok.

> 
> > 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);  
> 
> Same as before, don't split function calls before the paren.
> 
> > +#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);
> > +    }  
> 
> Ok.
> 
> > +#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.
> >    
> 
> Ok.
> 
> > @@ -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 */  
> 
> As before the function prototype is not really correct.

This has been fixed. Thanks for pointing this out.

> 
> > 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
  
Adhemerval Zanella Netto Oct. 6, 2020, 1:54 p.m. UTC | #5
On 06/10/2020 06:42, Lukasz Majewski wrote:
>> As indicated by Andreas, don't split function calls before the paren.
> 
> So it shall be:
> 
> int err = INTERNAL_SYSCALL_CALL (futex_time64, futex,
> 				 __lll_private_flag (FUTEX_WAIT,private),
> 				 val, tsp);

It should have an extra space after comma and prior 'private'. The rest
looks ok.
  

Patch

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