diff mbox series

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

Message ID 20201118075853.4438-1-lukma@denx.de
State Dropped
Delegated to: Lukasz Majewski
Headers show
Series [v3] y2038: Convert aio_suspend to support 64 bit time | expand

Commit Message

Lukasz Majewski Nov. 18, 2020, 7:58 a.m. UTC
The aio_suspend function has been converted to support 64 bit time.
In the __aio_suspend_common64() function not all occurrences of struct
timespec, clock_gettime() and pthread_cond_timedwait() have been replaced
with their 64 bit counterparts, as i686-gnu port (HURD) don't #define
DONT_NEED_AIO_MISC_COND and requires them (as it is not yet supporting 64
bit time for being Y2038 safe).

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

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

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/pthread/aio_suspend.c | 39 +++++++++++++--
 4 files changed, 139 insertions(+), 5 deletions(-)

Comments

Andreas Schwab Nov. 18, 2020, 9:29 a.m. UTC | #1
On Nov 18 2020, Lukasz Majewski wrote:

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

Why is that not static?

Andreas.
Lukasz Majewski Nov. 18, 2020, 9:35 a.m. UTC | #2
Hi Andreas,

> On Nov 18 2020, Lukasz Majewski wrote:
> 
> > @@ -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)  
> 
> Why is that not static?

Good point - it could be static. I've fixed it.

> 
> Andreas.
> 




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 Nov. 18, 2020, 1:53 p.m. UTC | #3
On 18/11/2020 06:35, Lukasz Majewski wrote:
> Hi Andreas,
> 
>> On Nov 18 2020, Lukasz Majewski wrote:
>>
>>> @@ -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)  
>>
>> Why is that not static?
> 
> Good point - it could be static. I've fixed it.

I am failing to see why you need __aio_suspend_common64, can't you
rename aio_suspend to __aio_suspend_time64?
Lukasz Majewski Nov. 19, 2020, 8:58 p.m. UTC | #4
Hi Adhemerval,

> On 18/11/2020 06:35, Lukasz Majewski wrote:
> > Hi Andreas,
> >   
> >> On Nov 18 2020, Lukasz Majewski wrote:
> >>  
> >>> @@ -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)    
> >>
> >> Why is that not static?  
> > 
> > Good point - it could be static. I've fixed it.  
> 
> I am failing to see why you need __aio_suspend_common64, can't you
> rename aio_suspend to __aio_suspend_time64?

Ok. I will change the name. Do you have any other comments regarding
this patch?


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
Lukasz Majewski Nov. 23, 2020, 8:22 a.m. UTC | #5
Hi Adhemerval,

> On 18/11/2020 06:35, Lukasz Majewski wrote:
> > Hi Andreas,
> >   
> >> On Nov 18 2020, Lukasz Majewski wrote:
> >>  
> >>> @@ -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)    
> >>
> >> Why is that not static?  
> > 
> > Good point - it could be static. I've fixed it.  
> 
> I am failing to see why you need __aio_suspend_common64, can't you
> rename aio_suspend to __aio_suspend_time64?

Do you have any other comments regarding this patch?


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 Nov. 23, 2020, 9:09 p.m. UTC | #6
On 18/11/2020 04:58, Lukasz Majewski wrote:
> The aio_suspend function has been converted to support 64 bit time.
> In the __aio_suspend_common64() function not all occurrences of struct
> timespec, clock_gettime() and pthread_cond_timedwait() have been replaced
> with their 64 bit counterparts, as i686-gnu port (HURD) don't #define
> DONT_NEED_AIO_MISC_COND and requires them (as it is not yet supporting 64
> bit time for being Y2038 safe).
> 
> 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
> 
> 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
> 
> 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/pthread/aio_suspend.c | 39 +++++++++++++--
>  4 files changed, 139 insertions(+), 5 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..b9d226c98d 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;							      \

I don't we need to add futex_reltimed_wait_cancelable64, but rather use
__futex_abstimed_wait_cancelable64 with a timeout adjust prior the
call.  I would make the futex changes not need and it would allow to 
eventually remove the futex_reltimed_wait/futex_reltimed_wait_cancelable on
gai is implemented.

I have add this required changes over my futex refactor [1], but it should
be applicable on master with some adjustment to futex-internal.[c,h].

> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index c27d0cdac8..e351e28d1d 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)
> +{
> +  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)
> +{
> +  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 */
> diff --git a/sysdeps/pthread/aio_suspend.c b/sysdeps/pthread/aio_suspend.c
> index ad03f13558..eebe10536e 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_common64 (const struct aiocb *const list[], int nent,
> +                        const struct __timespec64 *timeout)
>  {
>    if (__glibc_unlikely (nent < 0))
>      {
> @@ -183,6 +183,14 @@ 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.  */
> +	  /* Y2038: The below code is used by ports which do NOT
> +	     #define DONT_NEED_AIO_MISC_COND flag (e.g. i686-gnu)
> +	     Ports which define it use futex syscalls supporting 64 bit
> +	     time. As a result below code don't need to be adjusted to
> +	     support 64 bit time as it is advised to either add support
> +	     for DONT_NEED_AIO_MISC_COND flag or provide
> +	     __pthread_cond_timedwait64() implementation and use struct
> +	     __timespec64 and __clock_gettime64().  */

I don't think this comment adds much, Hurd will need to revise this code
anyway to add y2038.  In fact I think if we calculate the absolute timeout
prior, as required to call __futex_abstimed_wait_cancelable64 as I
suggested below; this whole part could be simplified to just:

  #ifdef DONT_NEED_AIO_MISC_COND
        result = do_aio_misc_wait (&cntr, timeout == NULL ? NULL : &ts);
  #else 
        struct timespec ts32 = valid_timespec64_to_timespec (ts);
        result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
                                         timeout == NULL ? NULL : &ts32);
  #endif

I have implemented this on [1].

>  	  struct timespec now;
>  	  struct timespec abstime;
>  
> @@ -250,4 +258,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,

Space before '('.

> +              const struct timespec *timeout)
> +{
> +  struct __timespec64 ts64;
> +
> +  if (timeout != NULL)
> +    {

I think for one-line statement there is no need to brackets.

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

[1] https://sourceware.org/git/?p=glibc.git;a=commit;h=f92702293d48478348df1bf5c9196f2af9cc63b4
Lukasz Majewski Nov. 25, 2020, 12:57 p.m. UTC | #7
Hi Adhemerval,

> On 18/11/2020 04:58, Lukasz Majewski wrote:
> > The aio_suspend function has been converted to support 64 bit time.
> > In the __aio_suspend_common64() function not all occurrences of
> > struct timespec, clock_gettime() and pthread_cond_timedwait() have
> > been replaced with their 64 bit counterparts, as i686-gnu port
> > (HURD) don't #define DONT_NEED_AIO_MISC_COND and requires them (as
> > it is not yet supporting 64 bit time for being Y2038 safe).
> > 
> > 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
> > 
> > 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
> > 
> > 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/pthread/aio_suspend.c |
> > 39 +++++++++++++-- 4 files changed, 139 insertions(+), 5
> > 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..b9d226c98d 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;
> > 				      \  
> 
> I don't we need to add futex_reltimed_wait_cancelable64, but rather
> use __futex_abstimed_wait_cancelable64 with a timeout adjust prior the
> call.  I would make the futex changes not need and it would allow to 
> eventually remove the
> futex_reltimed_wait/futex_reltimed_wait_cancelable on gai is
> implemented.
> 
> I have add this required changes over my futex refactor [1], but it
> should be applicable on master with some adjustment to
> futex-internal.[c,h].
> 
> > diff --git a/sysdeps/nptl/futex-internal.h
> > b/sysdeps/nptl/futex-internal.h index c27d0cdac8..e351e28d1d 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) +{
> > +  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)
> > +{
> > +  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 */
> > diff --git a/sysdeps/pthread/aio_suspend.c
> > b/sysdeps/pthread/aio_suspend.c index ad03f13558..eebe10536e 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_common64 (const struct aiocb *const list[], int nent,
> > +                        const struct __timespec64 *timeout)
> >  {
> >    if (__glibc_unlikely (nent < 0))
> >      {
> > @@ -183,6 +183,14 @@ 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.  */
> > +	  /* Y2038: The below code is used by ports which do NOT
> > +	     #define DONT_NEED_AIO_MISC_COND flag (e.g. i686-gnu)
> > +	     Ports which define it use futex syscalls supporting
> > 64 bit
> > +	     time. As a result below code don't need to be
> > adjusted to
> > +	     support 64 bit time as it is advised to either add
> > support
> > +	     for DONT_NEED_AIO_MISC_COND flag or provide
> > +	     __pthread_cond_timedwait64() implementation and use
> > struct
> > +	     __timespec64 and __clock_gettime64().  */  
> 
> I don't think this comment adds much, Hurd will need to revise this
> code anyway to add y2038.  In fact I think if we calculate the
> absolute timeout prior, as required to call
> __futex_abstimed_wait_cancelable64 as I suggested below; this whole
> part could be simplified to just:
> 
>   #ifdef DONT_NEED_AIO_MISC_COND
>         result = do_aio_misc_wait (&cntr, timeout == NULL ? NULL :
> &ts); #else 
>         struct timespec ts32 = valid_timespec64_to_timespec (ts);
>         result = pthread_cond_timedwait (&cond, &__aio_requests_mutex,
>                                          timeout == NULL ? NULL :
> &ts32); #endif
> 
> I have implemented this on [1].

I've already tested (build and QEMU) this patch set [1] and I would
prefer if you could also pull aio_suspend (as it is on top of your
rework) at once.

> 
> >  	  struct timespec now;
> >  	  struct timespec abstime;
> >  
> > @@ -250,4 +258,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,  
> 
> Space before '('.
> 
> > +              const struct timespec *timeout)
> > +{
> > +  struct __timespec64 ts64;
> > +
> > +  if (timeout != NULL)
> > +    {  
> 
> I think for one-line statement there is no need to brackets.
> 
> > +      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)
> >   
> 
> [1]
> https://sourceware.org/git/?p=glibc.git;a=commit;h=f92702293d48478348df1bf5c9196f2af9cc63b4




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
diff mbox series

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/sysdeps/nptl/aio_misc.h b/sysdeps/nptl/aio_misc.h
index 3f195f4794..b9d226c98d 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..e351e28d1d 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)
+{
+  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)
+{
+  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 */
diff --git a/sysdeps/pthread/aio_suspend.c b/sysdeps/pthread/aio_suspend.c
index ad03f13558..eebe10536e 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_common64 (const struct aiocb *const list[], int nent,
+                        const struct __timespec64 *timeout)
 {
   if (__glibc_unlikely (nent < 0))
     {
@@ -183,6 +183,14 @@  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.  */
+	  /* Y2038: The below code is used by ports which do NOT
+	     #define DONT_NEED_AIO_MISC_COND flag (e.g. i686-gnu)
+	     Ports which define it use futex syscalls supporting 64 bit
+	     time. As a result below code don't need to be adjusted to
+	     support 64 bit time as it is advised to either add support
+	     for DONT_NEED_AIO_MISC_COND flag or provide
+	     __pthread_cond_timedwait64() implementation and use struct
+	     __timespec64 and __clock_gettime64().  */
 	  struct timespec now;
 	  struct timespec abstime;
 
@@ -250,4 +258,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)