[PATCH v3] y2038: Convert aio_suspend to support 64 bit time
Lukasz Majewski
lukma@denx.de
Wed Nov 25 12:57:57 GMT 2020
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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://sourceware.org/pipermail/libc-alpha/attachments/20201125/c6bceda3/attachment.sig>
More information about the Libc-alpha
mailing list