[v5] y2038: Convert aio_suspend to support 64 bit time

Message ID 20201128215149.31014-1-lukma@denx.de
State Committed
Delegated to: Lukasz Majewski
Headers
Series [v5] y2038: Convert aio_suspend to support 64 bit time |

Commit Message

Lukasz Majewski Nov. 28, 2020, 9:51 p.m. UTC
  The aio_suspend function has been converted to support 64 bit time.

This change uses (in aio_misc.h):
- __futex_abstimed_wait64 (instead of futex_reltimed_wait)
- __futex_abstimed_wait_cancellable64
	(instead of futex_reltimed_wait_cancellable)
    from ./sysdeps/nptl/futex-helpers.h

The aio_suspend() accepts relative timeout, which then is converted to
absolute one.

The i686-gnu port (HURD) do not define DONT_NEED_AIO_MISC_COND and as it
doesn't (yet) support 64 bit time it uses not converted
pthread_cond_timedwait().

The __aio_suspend() is supposed to be run on ports with __TIMESIZE !=64 and
__WORDSIZE==32. It internally utilizes __aio_suspend_time64() and hence the
conversion from 32 bit struct timespec to 64 bit one is required.

For ports supporting 64 bit time the __aio_suspend_time64() will be used
either via alias (to __aio_suspend when __TIMESIZE==64) or redirection
(when -D_TIME_BITS=64 is passed).

Build tests:
./src/scripts/build-many-glibcs.py glibcs

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
---
Changes for v2:
- Add missing -EOVERFLOW error handling for __futex_reltimed_wait64 and
  _futex_reltimed_wait_cancelable64

Changes for v3:
- Remove "__" prefix from futex_reltimed_wait64 and
  futex_reltimed_wait_cancellable64

- Remove some code, as HURD is not defining DONT_NEED_AIO_MISC_COND
  (i.e. Linux ports are defining it) and add in-code explanation why
  the code is NOT converted to support 64 bit time.

- Rewrite the commit message

Changes for v4:
- Re-base on the newest master after refactor of the futex code - e.g. no
  need to provide futex_reltimed_wait_cancelable64()

- Add missing libpthread_hidden_{proto|def}

Changes for v5:
- Properly handle the situation when status = EOVERFLOW
---
 include/aio.h                 |  8 +++++
 nptl/Versions                 |  1 +
 sysdeps/nptl/aio_misc.h       | 11 ++++---
 sysdeps/nptl/futex-internal.c |  2 ++
 sysdeps/nptl/futex-internal.h |  6 ++--
 sysdeps/pthread/aio_suspend.c | 60 ++++++++++++++++++++---------------
 6 files changed, 57 insertions(+), 31 deletions(-)
  

Comments

Adhemerval Zanella Netto Nov. 30, 2020, 11:48 a.m. UTC | #1
On 28/11/2020 18:51, Lukasz Majewski wrote:
> The aio_suspend function has been converted to support 64 bit time.
> 
> This change uses (in aio_misc.h):
> - __futex_abstimed_wait64 (instead of futex_reltimed_wait)
> - __futex_abstimed_wait_cancellable64
> 	(instead of futex_reltimed_wait_cancellable)
>     from ./sysdeps/nptl/futex-helpers.h
> 
> The aio_suspend() accepts relative timeout, which then is converted to
> absolute one.
> 
> The i686-gnu port (HURD) do not define DONT_NEED_AIO_MISC_COND and as it
> doesn't (yet) support 64 bit time it uses not converted
> pthread_cond_timedwait().
> 
> The __aio_suspend() is supposed to be run on ports with __TIMESIZE !=64 and
> __WORDSIZE==32. It internally utilizes __aio_suspend_time64() and hence the
> conversion from 32 bit struct timespec to 64 bit one is required.
> 
> For ports supporting 64 bit time the __aio_suspend_time64() will be used
> either via alias (to __aio_suspend when __TIMESIZE==64) or redirection
> (when -D_TIME_BITS=64 is passed).
> 
> Build tests:
> ./src/scripts/build-many-glibcs.py glibcs
> 
> Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

LGTM, thanks.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>


> ---
> Changes for v2:
> - Add missing -EOVERFLOW error handling for __futex_reltimed_wait64 and
>   _futex_reltimed_wait_cancelable64
> 
> Changes for v3:
> - Remove "__" prefix from futex_reltimed_wait64 and
>   futex_reltimed_wait_cancellable64
> 
> - Remove some code, as HURD is not defining DONT_NEED_AIO_MISC_COND
>   (i.e. Linux ports are defining it) and add in-code explanation why
>   the code is NOT converted to support 64 bit time.
> 
> - Rewrite the commit message
> 
> Changes for v4:
> - Re-base on the newest master after refactor of the futex code - e.g. no
>   need to provide futex_reltimed_wait_cancelable64()
> 
> - Add missing libpthread_hidden_{proto|def}
> 
> Changes for v5:
> - Properly handle the situation when status = EOVERFLOW
> ---
>  include/aio.h                 |  8 +++++
>  nptl/Versions                 |  1 +
>  sysdeps/nptl/aio_misc.h       | 11 ++++---
>  sysdeps/nptl/futex-internal.c |  2 ++
>  sysdeps/nptl/futex-internal.h |  6 ++--
>  sysdeps/pthread/aio_suspend.c | 60 ++++++++++++++++++++---------------
>  6 files changed, 57 insertions(+), 31 deletions(-)
> 
> diff --git a/include/aio.h b/include/aio.h
> index 90c74f9951..c7f4233310 100644
> --- a/include/aio.h
> +++ b/include/aio.h
> @@ -9,6 +9,14 @@ extern void __aio_init (const struct aioinit *__init);
>     lio_listio and we do not issue events for each individual list
>     element.  */
>  #define LIO_NO_INDIVIDUAL_EVENT	128
> +
> +# if __TIMESIZE == 64
> +#  define __aio_suspend_time64 __aio_suspend
> +# else
> +extern int __aio_suspend_time64 (const struct aiocb *const list[], int nent,
> +                                 const struct __timespec64 *timeout);
> +librt_hidden_proto (__aio_suspend_time64)
> +# endif
>  #endif
>  
>  #endif
> diff --git a/nptl/Versions b/nptl/Versions
> index aed118e717..02650fe91c 100644
> --- a/nptl/Versions
> +++ b/nptl/Versions
> @@ -302,6 +302,7 @@ libpthread {
>      __pthread_clock_gettime; __pthread_clock_settime;
>      __pthread_unwind; __pthread_get_minstack;
>      __pthread_barrier_init; __pthread_barrier_wait;
> +    __futex_abstimed_wait64; __futex_abstimed_wait_cancelable64;
>      __shm_directory;
>      __libpthread_freeres;
>    }
> diff --git a/sysdeps/nptl/aio_misc.h b/sysdeps/nptl/aio_misc.h
> index 3f195f4794..effe94efaa 100644
> --- a/sysdeps/nptl/aio_misc.h
> +++ b/sysdeps/nptl/aio_misc.h
> @@ -45,11 +45,12 @@
>  	do								      \
>  	  {								      \
>  	    if (cancel)							      \
> -	      status = futex_reltimed_wait_cancelable (			      \
> -		(unsigned int *) futexaddr, oldval, timeout, FUTEX_PRIVATE);  \
> +	      status = __futex_abstimed_wait_cancelable64 (		      \
> +		(unsigned int *) futexaddr, oldval, CLOCK_MONOTONIC, timeout, \
> +		FUTEX_PRIVATE);						      \
>  	    else							      \
> -	      status = futex_reltimed_wait ((unsigned int *) futexaddr,	      \
> -		oldval, timeout, FUTEX_PRIVATE);	      		      \
> +	      status = __futex_abstimed_wait64 ((unsigned int *) futexaddr,   \
> +		oldval, CLOCK_REALTIME, timeout, FUTEX_PRIVATE); 	      \
>  	    if (status != EAGAIN)					      \
>  	      break;							      \
>  									      \
> @@ -61,6 +62,8 @@
>  	  result = EINTR;						      \
>  	else if (status == ETIMEDOUT)					      \
>  	  result = EAGAIN;						      \
> +	else if (status == EOVERFLOW)					      \
> +	  result = EOVERFLOW;						      \
>  	else								      \
>  	  assert (status == 0 || status == EAGAIN);			      \
>  									      \
> diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
> index 11031cc46a..22ef312c2a 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -112,6 +112,7 @@ __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
>    return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
>                                           abstime, private, false);
>  }
> +libpthread_hidden_def (__futex_abstimed_wait64)
>  
>  int
>  __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> @@ -122,3 +123,4 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>    return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
>                                           abstime, private, true);
>  }
> +libpthread_hidden_def (__futex_abstimed_wait_cancelable64)
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index e67803952f..1640da0ce8 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -405,13 +405,15 @@ int
>  __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
>                                      unsigned int expected, clockid_t clockid,
>                                      const struct __timespec64* abstime,
> -                                    int private) attribute_hidden;
> +                                    int private);
> +libpthread_hidden_proto (__futex_abstimed_wait_cancelable64);
>  
>  int
>  __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
>                           clockid_t clockid,
>                           const struct __timespec64* abstime,
> -                         int private) attribute_hidden;
> +                         int private);
> +libpthread_hidden_proto (__futex_abstimed_wait64);
>  
>  
>  static __always_inline int
> diff --git a/sysdeps/pthread/aio_suspend.c b/sysdeps/pthread/aio_suspend.c
> index ad03f13558..32032a4f44 100644
> --- a/sysdeps/pthread/aio_suspend.c
> +++ b/sysdeps/pthread/aio_suspend.c
> @@ -94,7 +94,7 @@ cleanup (void *arg)
>  #ifdef DONT_NEED_AIO_MISC_COND
>  static int
>  __attribute__ ((noinline))
> -do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
> +do_aio_misc_wait (unsigned int *cntr, const struct __timespec64 *timeout)
>  {
>    int result = 0;
>  
> @@ -105,8 +105,8 @@ do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
>  #endif
>  
>  int
> -aio_suspend (const struct aiocb *const list[], int nent,
> -	     const struct timespec *timeout)
> +__aio_suspend_time64 (const struct aiocb *const list[], int nent,
> +		      const struct __timespec64 *timeout)
>  {
>    if (__glibc_unlikely (nent < 0))
>      {
> @@ -157,6 +157,18 @@ aio_suspend (const struct aiocb *const list[], int nent,
>  	  break;
>        }
>  
> +  struct __timespec64 ts;
> +  if (timeout != NULL)
> +    {
> +      __clock_gettime64 (CLOCK_MONOTONIC, &ts);
> +      ts.tv_sec += timeout->tv_sec;
> +      ts.tv_nsec += timeout->tv_nsec;
> +      if (ts.tv_nsec >= 1000000000)
> +	{
> +	  ts.tv_nsec -= 1000000000;
> +	  ts.tv_sec++;
> +	}
> +    }
>  
>    /* Only if none of the entries is NULL or finished to be wait.  */
>    if (cnt == nent && any)
> @@ -175,29 +187,11 @@ aio_suspend (const struct aiocb *const list[], int nent,
>        pthread_cleanup_push (cleanup, &clparam);
>  
>  #ifdef DONT_NEED_AIO_MISC_COND
> -      result = do_aio_misc_wait (&cntr, timeout);
> +      result = do_aio_misc_wait (&cntr, timeout == NULL ? NULL : &ts);
>  #else
> -      if (timeout == NULL)
> -	result = pthread_cond_wait (&cond, &__aio_requests_mutex);
> -      else
> -	{
> -	  /* We have to convert the relative timeout value into an
> -	     absolute time value with pthread_cond_timedwait expects.  */
> -	  struct timespec now;
> -	  struct timespec abstime;
> -
> -	  __clock_gettime (CLOCK_REALTIME, &now);
> -	  abstime.tv_nsec = timeout->tv_nsec + now.tv_nsec;
> -	  abstime.tv_sec = timeout->tv_sec + now.tv_sec;
> -	  if (abstime.tv_nsec >= 1000000000)
> -	    {
> -	      abstime.tv_nsec -= 1000000000;
> -	      abstime.tv_sec += 1;
> -	    }
> -
> -	  result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
> -					   &abstime);
> -	}
> +      struct timespec ts32 = valid_timespec64_to_timespec (ts);
> +      result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
> +				       timeout == NULL ? NULL : &ts32);
>  #endif
>  
>        pthread_cleanup_pop (0);
> @@ -250,4 +244,20 @@ aio_suspend (const struct aiocb *const list[], int nent,
>    return result;
>  }
>  
> +#if __TIMESIZE != 64
> +librt_hidden_def (__aio_suspend_time64)
> +
> +int
> +__aio_suspend (const struct aiocb *const list[], int nent,
> +               const struct timespec *timeout)
> +{
> +  struct __timespec64 ts64;
> +
> +  if (timeout != NULL)
> +    ts64 = valid_timespec_to_timespec64 (*timeout);
> +
> +  return __aio_suspend_time64 (list, nent, timeout != NULL ? &ts64 : NULL);
> +}
> +#endif
> +weak_alias (__aio_suspend, aio_suspend)
>  weak_alias (aio_suspend, aio_suspend64)
>
  

Patch

diff --git a/include/aio.h b/include/aio.h
index 90c74f9951..c7f4233310 100644
--- a/include/aio.h
+++ b/include/aio.h
@@ -9,6 +9,14 @@  extern void __aio_init (const struct aioinit *__init);
    lio_listio and we do not issue events for each individual list
    element.  */
 #define LIO_NO_INDIVIDUAL_EVENT	128
+
+# if __TIMESIZE == 64
+#  define __aio_suspend_time64 __aio_suspend
+# else
+extern int __aio_suspend_time64 (const struct aiocb *const list[], int nent,
+                                 const struct __timespec64 *timeout);
+librt_hidden_proto (__aio_suspend_time64)
+# endif
 #endif
 
 #endif
diff --git a/nptl/Versions b/nptl/Versions
index aed118e717..02650fe91c 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -302,6 +302,7 @@  libpthread {
     __pthread_clock_gettime; __pthread_clock_settime;
     __pthread_unwind; __pthread_get_minstack;
     __pthread_barrier_init; __pthread_barrier_wait;
+    __futex_abstimed_wait64; __futex_abstimed_wait_cancelable64;
     __shm_directory;
     __libpthread_freeres;
   }
diff --git a/sysdeps/nptl/aio_misc.h b/sysdeps/nptl/aio_misc.h
index 3f195f4794..effe94efaa 100644
--- a/sysdeps/nptl/aio_misc.h
+++ b/sysdeps/nptl/aio_misc.h
@@ -45,11 +45,12 @@ 
 	do								      \
 	  {								      \
 	    if (cancel)							      \
-	      status = futex_reltimed_wait_cancelable (			      \
-		(unsigned int *) futexaddr, oldval, timeout, FUTEX_PRIVATE);  \
+	      status = __futex_abstimed_wait_cancelable64 (		      \
+		(unsigned int *) futexaddr, oldval, CLOCK_MONOTONIC, timeout, \
+		FUTEX_PRIVATE);						      \
 	    else							      \
-	      status = futex_reltimed_wait ((unsigned int *) futexaddr,	      \
-		oldval, timeout, FUTEX_PRIVATE);	      		      \
+	      status = __futex_abstimed_wait64 ((unsigned int *) futexaddr,   \
+		oldval, CLOCK_REALTIME, timeout, FUTEX_PRIVATE); 	      \
 	    if (status != EAGAIN)					      \
 	      break;							      \
 									      \
@@ -61,6 +62,8 @@ 
 	  result = EINTR;						      \
 	else if (status == ETIMEDOUT)					      \
 	  result = EAGAIN;						      \
+	else if (status == EOVERFLOW)					      \
+	  result = EOVERFLOW;						      \
 	else								      \
 	  assert (status == 0 || status == EAGAIN);			      \
 									      \
diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
index 11031cc46a..22ef312c2a 100644
--- a/sysdeps/nptl/futex-internal.c
+++ b/sysdeps/nptl/futex-internal.c
@@ -112,6 +112,7 @@  __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
   return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
                                          abstime, private, false);
 }
+libpthread_hidden_def (__futex_abstimed_wait64)
 
 int
 __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
@@ -122,3 +123,4 @@  __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
   return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
                                          abstime, private, true);
 }
+libpthread_hidden_def (__futex_abstimed_wait_cancelable64)
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index e67803952f..1640da0ce8 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -405,13 +405,15 @@  int
 __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
                                     unsigned int expected, clockid_t clockid,
                                     const struct __timespec64* abstime,
-                                    int private) attribute_hidden;
+                                    int private);
+libpthread_hidden_proto (__futex_abstimed_wait_cancelable64);
 
 int
 __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
                          clockid_t clockid,
                          const struct __timespec64* abstime,
-                         int private) attribute_hidden;
+                         int private);
+libpthread_hidden_proto (__futex_abstimed_wait64);
 
 
 static __always_inline int
diff --git a/sysdeps/pthread/aio_suspend.c b/sysdeps/pthread/aio_suspend.c
index ad03f13558..32032a4f44 100644
--- a/sysdeps/pthread/aio_suspend.c
+++ b/sysdeps/pthread/aio_suspend.c
@@ -94,7 +94,7 @@  cleanup (void *arg)
 #ifdef DONT_NEED_AIO_MISC_COND
 static int
 __attribute__ ((noinline))
-do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
+do_aio_misc_wait (unsigned int *cntr, const struct __timespec64 *timeout)
 {
   int result = 0;
 
@@ -105,8 +105,8 @@  do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
 #endif
 
 int
-aio_suspend (const struct aiocb *const list[], int nent,
-	     const struct timespec *timeout)
+__aio_suspend_time64 (const struct aiocb *const list[], int nent,
+		      const struct __timespec64 *timeout)
 {
   if (__glibc_unlikely (nent < 0))
     {
@@ -157,6 +157,18 @@  aio_suspend (const struct aiocb *const list[], int nent,
 	  break;
       }
 
+  struct __timespec64 ts;
+  if (timeout != NULL)
+    {
+      __clock_gettime64 (CLOCK_MONOTONIC, &ts);
+      ts.tv_sec += timeout->tv_sec;
+      ts.tv_nsec += timeout->tv_nsec;
+      if (ts.tv_nsec >= 1000000000)
+	{
+	  ts.tv_nsec -= 1000000000;
+	  ts.tv_sec++;
+	}
+    }
 
   /* Only if none of the entries is NULL or finished to be wait.  */
   if (cnt == nent && any)
@@ -175,29 +187,11 @@  aio_suspend (const struct aiocb *const list[], int nent,
       pthread_cleanup_push (cleanup, &clparam);
 
 #ifdef DONT_NEED_AIO_MISC_COND
-      result = do_aio_misc_wait (&cntr, timeout);
+      result = do_aio_misc_wait (&cntr, timeout == NULL ? NULL : &ts);
 #else
-      if (timeout == NULL)
-	result = pthread_cond_wait (&cond, &__aio_requests_mutex);
-      else
-	{
-	  /* We have to convert the relative timeout value into an
-	     absolute time value with pthread_cond_timedwait expects.  */
-	  struct timespec now;
-	  struct timespec abstime;
-
-	  __clock_gettime (CLOCK_REALTIME, &now);
-	  abstime.tv_nsec = timeout->tv_nsec + now.tv_nsec;
-	  abstime.tv_sec = timeout->tv_sec + now.tv_sec;
-	  if (abstime.tv_nsec >= 1000000000)
-	    {
-	      abstime.tv_nsec -= 1000000000;
-	      abstime.tv_sec += 1;
-	    }
-
-	  result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
-					   &abstime);
-	}
+      struct timespec ts32 = valid_timespec64_to_timespec (ts);
+      result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
+				       timeout == NULL ? NULL : &ts32);
 #endif
 
       pthread_cleanup_pop (0);
@@ -250,4 +244,20 @@  aio_suspend (const struct aiocb *const list[], int nent,
   return result;
 }
 
+#if __TIMESIZE != 64
+librt_hidden_def (__aio_suspend_time64)
+
+int
+__aio_suspend (const struct aiocb *const list[], int nent,
+               const struct timespec *timeout)
+{
+  struct __timespec64 ts64;
+
+  if (timeout != NULL)
+    ts64 = valid_timespec_to_timespec64 (*timeout);
+
+  return __aio_suspend_time64 (list, nent, timeout != NULL ? &ts64 : NULL);
+}
+#endif
+weak_alias (__aio_suspend, aio_suspend)
 weak_alias (aio_suspend, aio_suspend64)