Re: [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time

[Lukasz Majewski <lukma@denx.de>]

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

Hi Adhemerval,

> On 14/11/2020 18:45, Lukasz Majewski wrote:
> > The aio_suspend function has been converted to support 64 bit time.
> > It was also necessary to provide Linux specific copy of
> > aio_suspend.c to avoid problems on i686-gnu (i.e. HURD) port, which
> > is not providing pthread_cond_timedwait() supporting 64 bit time.
> > 
> > This change uses:
> > - New __futex_reltimed_wait64 (instead of futex_reltimed_wait)
> > - New __futex_reltimed_wait_cancellable64
> > 	(instead of futex_reltimed_wait_cancellable)
> >     from ./sysdeps/nptl/futex-helpers.h
> > 
> > The aio_suspend() accepts relative timeout.
> > 
> > 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
> > 
> > ---
> > Changes for v2:
> > - Add missing -EOVERFLOW error handling for __futex_reltimed_wait64
> > and _futex_reltimed_wait_cancelable64
> > 
> > Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  include/aio.h                         |  8 +++
> >  sysdeps/nptl/aio_misc.h               |  4 +-
> >  sysdeps/nptl/futex-internal.h         | 93
> > +++++++++++++++++++++++++++ sysdeps/unix/sysv/linux/aio_suspend.c |
> > 41 +++++++++--- 4 files changed, 136 insertions(+), 10 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/sysdeps/nptl/aio_misc.h b/sysdeps/nptl/aio_misc.h
> > index 3f195f4794..5c553cce56 100644
> > --- a/sysdeps/nptl/aio_misc.h
> > +++ b/sysdeps/nptl/aio_misc.h
> > @@ -45,10 +45,10 @@
> >  	do
> > 	      \ {
> > 		      \ if (cancel)
> > 			      \
> > -	      status = futex_reltimed_wait_cancelable (
> > 		      \
> > +	      status = __futex_reltimed_wait_cancelable64 (
> > 	      \ (unsigned int *) futexaddr, oldval, timeout,
> > FUTEX_PRIVATE);  \ else
> > 		      \
> > -	      status = futex_reltimed_wait ((unsigned int *)
> > futexaddr,	      \
> > +	      status = __futex_reltimed_wait64 ((unsigned int *)
> > futexaddr,   \ oldval, timeout, FUTEX_PRIVATE);
> > 		      \ if (status != EAGAIN)
> > 			      \ break;
> > 				      \ diff --git
> > a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> > index c27d0cdac8..1134ecfe88 100644 ---
> > a/sysdeps/nptl/futex-internal.h +++ b/sysdeps/nptl/futex-internal.h
> > @@ -609,4 +609,97 @@ __futex_clock_wait_bitset64 (int *futexp, int
> > val, clockid_t clockid, const struct __timespec64 *abstime,
> >                               int private) attribute_hidden;
> >  
> > +static __always_inline int
> > +__futex_reltimed_wait64 (unsigned int* futex_word, unsigned int
> > expected,
> > +                         const struct __timespec64* reltime, int
> > private) +{  
> 
> The static inline function does not need to use double '__'. 

Ok.
 
> 
> > +  int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
> > +                                   __lll_private_flag (FUTEX_WAIT,
> > private),
> > +                                   expected, reltime);
> > +#ifndef __ASSUME_TIME64_SYSCALLS
> > +  if (err == -ENOSYS)
> > +    {
> > +      if (in_time_t_range (reltime->tv_sec))
> > +        {
> > +          struct timespec ts32 = valid_timespec64_to_timespec
> > (*reltime); +
> > +          err = INTERNAL_SYSCALL_CALL (futex, futex_word,
> > +                                         __lll_private_flag
> > (FUTEX_WAIT,
> > +
> > private),
> > +                                       expected, &ts32);
> > +        }
> > +      else
> > +        err = -EOVERFLOW;
> > +    }
> > +#endif
> > +  switch (err)
> > +    {
> > +    case 0:
> > +    case -EAGAIN:
> > +    case -EINTR:
> > +    case -ETIMEDOUT:
> > +    case -EOVERFLOW:  /* Passed relative timeout uses 64 bit
> > time_t type, but
> > +                         underlying kernel does not support 64 bit
> > time_t futex
> > +                         syscalls.  */
> > +      return -err;
> > +
> > +    case -EFAULT: /* Must have been caused by a glibc or
> > application bug.  */
> > +    case -EINVAL: /* Either due to wrong alignment or due to the
> > timeout not
> > +		     being normalized.  Must have been caused by a
> > glibc or
> > +		     application bug.  */
> > +    case -ENOSYS: /* Must have been caused by a glibc bug.  */
> > +    /* No other errors are documented at this time.  */
> > +    default:
> > +      futex_fatal_error ();
> > +    }
> > +}
> > +
> > +/* Like futex_reltimed_wait but is a POSIX cancellation point.  */
> > +static __always_inline int
> > +__futex_reltimed_wait_cancelable64 (unsigned int* futex_word,
> > +                                    unsigned int expected,
> > +                                    const struct __timespec64*
> > reltime,
> > +                                    int private)  
> 
> Same as before.

Ok.

> 
> > +{
> > +  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word,
> > +                                     __lll_private_flag
> > (FUTEX_WAIT, private),
> > +                                     expected, reltime);
> > +#ifndef __ASSUME_TIME64_SYSCALLS
> > +  if (err == -ENOSYS)
> > +    {
> > +      if (in_time_t_range (reltime->tv_sec))
> > +        {
> > +          struct timespec ts32 = valid_timespec64_to_timespec
> > (*reltime); +
> > +          err = INTERNAL_SYSCALL_CANCEL (futex, futex_word,
> > +                                         __lll_private_flag
> > (FUTEX_WAIT,
> > +
> > private),
> > +                                         expected, &ts32);
> > +        }
> > +      else
> > +        err = -EOVERFLOW;
> > +    }
> > +#endif
> > +  switch (err)
> > +    {
> > +    case 0:
> > +    case -EAGAIN:
> > +    case -EINTR:
> > +    case -ETIMEDOUT:
> > +    case -EOVERFLOW:  /* Passed relative timeout uses 64 bit
> > time_t type, but
> > +                         underlying kernel does not support 64 bit
> > time_t futex
> > +                         syscalls.  */
> > +      return -err;
> > +
> > +    case -EFAULT: /* Must have been caused by a glibc or
> > application bug.  */
> > +    case -EINVAL: /* Either due to wrong alignment or due to the
> > timeout not
> > +		     being normalized.  Must have been caused by a
> > glibc or
> > +		     application bug.  */
> > +    case -ENOSYS: /* Must have been caused by a glibc bug.  */
> > +    /* No other errors are documented at this time.  */
> > +    default:
> > +      futex_fatal_error ();
> > +    }
> > +}
> > +
> >  #endif  /* futex-internal.h */  
> 
> We might revise this large inline functions later, specially for
> !__ASSUME_TIME64_SYSCALLS. 

Ok.

> These would be used on GAI_MISC_WAIT, so
> it might be better to either export it to libpthread GLIBC_PRIVATE
> namespace or build futex-internal.c on librt (which will pull extra
> unused implementation).

Hmm... Another place which would require Y2038 tunning...

> 
> Also, once GAI_MISC_WAIT y2038 supported is added we will probably
> need to remove the ununsed futex_reltimed_wait{_cancellable}.

Also the C preprocessor macro code from
sysdeps/nptl/lowlevellock-futex.h could be removed.

> 
> > diff --git a/sysdeps/unix/sysv/linux/aio_suspend.c
> > b/sysdeps/unix/sysv/linux/aio_suspend.c index
> > ad03f13558..8ce7bfedb0 100644 ---
> > a/sysdeps/unix/sysv/linux/aio_suspend.c +++
> > b/sysdeps/unix/sysv/linux/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_common64 (const struct aiocb *const list[], int nent,
> > +                        const struct __timespec64 *timeout)
> >  {
> >    if (__glibc_unlikely (nent < 0))
> >      {
> > @@ -183,10 +183,10 @@ aio_suspend (const struct aiocb *const
> > list[], int nent, {
> >  	  /* We have to convert the relative timeout value into an
> >  	     absolute time value with pthread_cond_timedwait
> > expects.  */
> > -	  struct timespec now;
> > -	  struct timespec abstime;
> > +	  struct __timespec64 now;
> > +	  struct __timespec64 abstime;
> >  
> > -	  __clock_gettime (CLOCK_REALTIME, &now);
> > +	  __clock_gettime64 (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)
> > @@ -195,8 +195,8 @@ aio_suspend (const struct aiocb *const list[],
> > int nent, abstime.tv_sec += 1;
> >  	    }
> >  
> > -	  result = pthread_cond_timedwait (&cond,
> > &__aio_requests_mutex,
> > -					   &abstime);
> > +	  result = __pthread_cond_timedwait64 (&cond,
> > &__aio_requests_mutex,
> > +					       &abstime);
> >  	}
> >  #endif
> >    
> 
> The pthread_cond fallback is not needed on Linux, it defines
> DONT_NEED_AIO_MISC_COND which would use the futex calls directly.

The usage of DONT_NEED_AIO_MISC_COND seems to be a bit convoluted. It
mixes #include_next with some headers' dependencies.

However, It looks like I can leave the current implementation as is - I
don't need to adjust aio_suspend() (__aio_suspend_common64) when the
DONT_NEED_AIO_MISC_COND flag is NOT defined - "Linux" ports do define it.

I'm now testing such patch and will post it if it is OK.

> 
> > @@ -250,4 +250,29 @@ aio_suspend (const struct aiocb *const list[],
> > int nent, return result;
> >  }
> >  
> > +int
> > +__aio_suspend_time64 (const struct aiocb *const list[], int nent,
> > +		      const struct __timespec64 *timeout)
> > +{
> > +  return __aio_suspend_common64 (list, nent, timeout);
> > +}
> > +
> > +#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)
> >   




Best regards,

Lukasz Majewski

--

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

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