* [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c
@ 2020-11-14 21:45 Lukasz Majewski
2020-11-14 21:45 ` [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time Lukasz Majewski
2020-11-17 14:27 ` [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Adhemerval Zanella
0 siblings, 2 replies; 6+ messages in thread
From: Lukasz Majewski @ 2020-11-14 21:45 UTC (permalink / raw)
To: Joseph Myers, Paul Eggert, Adhemerval Zanella
Cc: Alistair Francis, Arnd Bergmann, Alistair Francis, GNU C Library,
Florian Weimer, Carlos O'Donell, Stepan Golosunov,
Andreas Schwab, Zack Weinberg, Lukasz Majewski
This patch is a preparatory work, to provide 64 bit time support
to aio_suspend.c, which would avoid build breaks on the HURD (i686-gnu)
port, as it is not supporting 64 bit version of
__pthread_cond_timedwait64.
The file sysdeps/unix/sysv/linux/aio_suspend.c is the exact copy of
./sysdeps/pthread/aio_suspend.c
---
Changes for v2:
- Rephase the commit message
---
sysdeps/unix/sysv/linux/aio_suspend.c | 253 ++++++++++++++++++++++++++
1 file changed, 253 insertions(+)
create mode 100644 sysdeps/unix/sysv/linux/aio_suspend.c
diff --git a/sysdeps/unix/sysv/linux/aio_suspend.c b/sysdeps/unix/sysv/linux/aio_suspend.c
new file mode 100644
index 0000000000..ad03f13558
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/aio_suspend.c
@@ -0,0 +1,253 @@
+/* Suspend until termination of a requests.
+ Copyright (C) 1997-2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <https://www.gnu.org/licenses/>. */
+
+
+/* We use an UGLY hack to prevent gcc from finding us cheating. The
+ implementations of aio_suspend and aio_suspend64 are identical and so
+ we want to avoid code duplication by using aliases. But gcc sees
+ the different parameter lists and prints a warning. We define here
+ a function so that aio_suspend64 has no prototype. */
+#define aio_suspend64 XXX
+#include <aio.h>
+/* And undo the hack. */
+#undef aio_suspend64
+
+#include <assert.h>
+#include <errno.h>
+#include <stdbool.h>
+#include <stdlib.h>
+#include <sys/time.h>
+
+#include <libc-lock.h>
+#include <aio_misc.h>
+
+
+struct clparam
+{
+ const struct aiocb *const *list;
+ struct waitlist *waitlist;
+ struct requestlist **requestlist;
+#ifndef DONT_NEED_AIO_MISC_COND
+ pthread_cond_t *cond;
+#endif
+ int nent;
+};
+
+
+static void
+cleanup (void *arg)
+{
+#ifdef DONT_NEED_AIO_MISC_COND
+ /* Acquire the mutex. If pthread_cond_*wait is used this would
+ happen implicitly. */
+ pthread_mutex_lock (&__aio_requests_mutex);
+#endif
+
+ const struct clparam *param = (const struct clparam *) arg;
+
+ /* Now remove the entry in the waiting list for all requests
+ which didn't terminate. */
+ int cnt = param->nent;
+ while (cnt-- > 0)
+ if (param->list[cnt] != NULL
+ && param->list[cnt]->__error_code == EINPROGRESS)
+ {
+ struct waitlist **listp;
+
+ assert (param->requestlist[cnt] != NULL);
+
+ /* There is the chance that we cannot find our entry anymore. This
+ could happen if the request terminated and restarted again. */
+ listp = ¶m->requestlist[cnt]->waiting;
+ while (*listp != NULL && *listp != ¶m->waitlist[cnt])
+ listp = &(*listp)->next;
+
+ if (*listp != NULL)
+ *listp = (*listp)->next;
+ }
+
+#ifndef DONT_NEED_AIO_MISC_COND
+ /* Release the conditional variable. */
+ (void) pthread_cond_destroy (param->cond);
+#endif
+
+ /* Release the mutex. */
+ pthread_mutex_unlock (&__aio_requests_mutex);
+}
+
+#ifdef DONT_NEED_AIO_MISC_COND
+static int
+__attribute__ ((noinline))
+do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
+{
+ int result = 0;
+
+ AIO_MISC_WAIT (result, *cntr, timeout, 1);
+
+ return result;
+}
+#endif
+
+int
+aio_suspend (const struct aiocb *const list[], int nent,
+ const struct timespec *timeout)
+{
+ if (__glibc_unlikely (nent < 0))
+ {
+ __set_errno (EINVAL);
+ return -1;
+ }
+
+ struct waitlist waitlist[nent];
+ struct requestlist *requestlist[nent];
+#ifndef DONT_NEED_AIO_MISC_COND
+ pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
+#endif
+ int cnt;
+ bool any = false;
+ int result = 0;
+ unsigned int cntr = 1;
+
+ /* Request the mutex. */
+ pthread_mutex_lock (&__aio_requests_mutex);
+
+ /* There is not yet a finished request. Signal the request that
+ we are working for it. */
+ for (cnt = 0; cnt < nent; ++cnt)
+ if (list[cnt] != NULL)
+ {
+ if (list[cnt]->__error_code == EINPROGRESS)
+ {
+ requestlist[cnt] = __aio_find_req ((aiocb_union *) list[cnt]);
+
+ if (requestlist[cnt] != NULL)
+ {
+#ifndef DONT_NEED_AIO_MISC_COND
+ waitlist[cnt].cond = &cond;
+#endif
+ waitlist[cnt].result = NULL;
+ waitlist[cnt].next = requestlist[cnt]->waiting;
+ waitlist[cnt].counterp = &cntr;
+ waitlist[cnt].sigevp = NULL;
+ requestlist[cnt]->waiting = &waitlist[cnt];
+ any = true;
+ }
+ else
+ /* We will never suspend. */
+ break;
+ }
+ else
+ /* We will never suspend. */
+ break;
+ }
+
+
+ /* Only if none of the entries is NULL or finished to be wait. */
+ if (cnt == nent && any)
+ {
+ struct clparam clparam =
+ {
+ .list = list,
+ .waitlist = waitlist,
+ .requestlist = requestlist,
+#ifndef DONT_NEED_AIO_MISC_COND
+ .cond = &cond,
+#endif
+ .nent = nent
+ };
+
+ pthread_cleanup_push (cleanup, &clparam);
+
+#ifdef DONT_NEED_AIO_MISC_COND
+ result = do_aio_misc_wait (&cntr, timeout);
+#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);
+ }
+#endif
+
+ pthread_cleanup_pop (0);
+ }
+
+ /* Now remove the entry in the waiting list for all requests
+ which didn't terminate. */
+ while (cnt-- > 0)
+ if (list[cnt] != NULL && list[cnt]->__error_code == EINPROGRESS)
+ {
+ struct waitlist **listp;
+
+ assert (requestlist[cnt] != NULL);
+
+ /* There is the chance that we cannot find our entry anymore. This
+ could happen if the request terminated and restarted again. */
+ listp = &requestlist[cnt]->waiting;
+ while (*listp != NULL && *listp != &waitlist[cnt])
+ listp = &(*listp)->next;
+
+ if (*listp != NULL)
+ *listp = (*listp)->next;
+ }
+
+#ifndef DONT_NEED_AIO_MISC_COND
+ /* Release the conditional variable. */
+ if (__glibc_unlikely (pthread_cond_destroy (&cond) != 0))
+ /* This must never happen. */
+ abort ();
+#endif
+
+ if (result != 0)
+ {
+#ifndef DONT_NEED_AIO_MISC_COND
+ /* An error occurred. Possibly it's ETIMEDOUT. We have to translate
+ the timeout error report of `pthread_cond_timedwait' to the
+ form expected from `aio_suspend'. */
+ if (result == ETIMEDOUT)
+ __set_errno (EAGAIN);
+ else
+#endif
+ __set_errno (result);
+
+ result = -1;
+ }
+
+ /* Release the mutex. */
+ pthread_mutex_unlock (&__aio_requests_mutex);
+
+ return result;
+}
+
+weak_alias (aio_suspend, aio_suspend64)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time
2020-11-14 21:45 [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Lukasz Majewski
@ 2020-11-14 21:45 ` Lukasz Majewski
2020-11-17 14:41 ` Adhemerval Zanella
2020-11-17 14:27 ` [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Adhemerval Zanella
1 sibling, 1 reply; 6+ messages in thread
From: Lukasz Majewski @ 2020-11-14 21:45 UTC (permalink / raw)
To: Joseph Myers, Paul Eggert, Adhemerval Zanella
Cc: Alistair Francis, Arnd Bergmann, Alistair Francis, GNU C Library,
Florian Weimer, Carlos O'Donell, Stepan Golosunov,
Andreas Schwab, Zack Weinberg, Lukasz Majewski
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)
+{
+ 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/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
@@ -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)
--
2.20.1
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c
2020-11-14 21:45 [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Lukasz Majewski
2020-11-14 21:45 ` [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time Lukasz Majewski
@ 2020-11-17 14:27 ` Adhemerval Zanella
2020-11-17 15:58 ` Lukasz Majewski
1 sibling, 1 reply; 6+ messages in thread
From: Adhemerval Zanella @ 2020-11-17 14:27 UTC (permalink / raw)
To: Lukasz Majewski, Joseph Myers, Paul Eggert
Cc: Alistair Francis, Arnd Bergmann, Alistair Francis, GNU C Library,
Florian Weimer, Carlos O'Donell, Stepan Golosunov,
Andreas Schwab, Zack Weinberg
On 14/11/2020 18:45, Lukasz Majewski wrote:
> This patch is a preparatory work, to provide 64 bit time support
> to aio_suspend.c, which would avoid build breaks on the HURD (i686-gnu)
> port, as it is not supporting 64 bit version of
> __pthread_cond_timedwait64.
>
> The file sysdeps/unix/sysv/linux/aio_suspend.c is the exact copy of
> ./sysdeps/pthread/aio_suspend.c
>
> ---
> Changes for v2:
> - Rephase the commit message
> ---
I am not very found of this code duplication and the Linux version does
not need !DONT_NEED_AIO_MISC_COND, since nptl/aio_misc.h will always
define to 1.
I think it would be better to move the sysdeps/pthread/aio_suspend.c
to a parametrized implementation (sysdeps/pthread/aio_suspend_skeleton.c
or something) where the internal types and function should be defined
(similar to the dynamic_array.h) something like:
94 #ifdef DONT_NEED_AIO_MISC_COND
95 static int
96 __attribute__ ((noinline))
97 do_aio_misc_wait (unsigned int *cntr, const TIMESPEC_STRUCT *timeout)
98 {
99 int result = 0;
100
101 AIO_MISC_WAIT_MACRO (result, *cntr, timeout, 1);
102
103 return result;
104 }
105 #endif
106
107 int
108 AIO_SUSPEND (const struct aiocb *const list[], int nent,
109 const TIMESPEC_STRUCT *timeout)
110 {
111 if (__glibc_unlikely (nent < 0))
112 {
113 __set_errno (EINVAL);
114 return -1;
115 }
And then generic sysdeps/pthread/aio_suspend.c will define the types and
include the parametrized implementation:
#define TIMESPEC_STRUCT struct timespec
#define AIO_SUSPEND aio_suspend
#include <sysdeps/pthread/aio_suspend_skeleton.c>
The Linux implementation sysdeps/unix/sysv/linux/aio_suspend.c then can
do:
#if __TIMESIZE != 64
# define TIMESPEC_STRUCT struct __timespec64
# define AIO_SUSPEND __aio_suspend64_time64
# define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT64
#else
# define TIMESPEC_STRUCT struct timespec
# define AIO_SUSPEND aio_suspend
# define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT
#endif
#include <sysdeps/pthread/aio_suspend_skeleton.c>
#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
> sysdeps/unix/sysv/linux/aio_suspend.c | 253 ++++++++++++++++++++++++++
> 1 file changed, 253 insertions(+)
> create mode 100644 sysdeps/unix/sysv/linux/aio_suspend.c
>
> diff --git a/sysdeps/unix/sysv/linux/aio_suspend.c b/sysdeps/unix/sysv/linux/aio_suspend.c
> new file mode 100644
> index 0000000000..ad03f13558
> --- /dev/null
> +++ b/sysdeps/unix/sysv/linux/aio_suspend.c
> @@ -0,0 +1,253 @@
> +/* Suspend until termination of a requests.
> + Copyright (C) 1997-2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> + Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997.
> +
> + The GNU C Library is free software; you can redistribute it and/or
> + modify it under the terms of the GNU Lesser General Public
> + License as published by the Free Software Foundation; either
> + version 2.1 of the License, or (at your option) any later version.
> +
> + The GNU C Library is distributed in the hope that it will be useful,
> + but WITHOUT ANY WARRANTY; without even the implied warranty of
> + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + Lesser General Public License for more details.
> +
> + You should have received a copy of the GNU Lesser General Public
> + License along with the GNU C Library; if not, see
> + <https://www.gnu.org/licenses/>. */
> +
> +
> +/* We use an UGLY hack to prevent gcc from finding us cheating. The
> + implementations of aio_suspend and aio_suspend64 are identical and so
> + we want to avoid code duplication by using aliases. But gcc sees
> + the different parameter lists and prints a warning. We define here
> + a function so that aio_suspend64 has no prototype. */
> +#define aio_suspend64 XXX
> +#include <aio.h>
> +/* And undo the hack. */
> +#undef aio_suspend64
> +
> +#include <assert.h>
> +#include <errno.h>
> +#include <stdbool.h>
> +#include <stdlib.h>
> +#include <sys/time.h>
> +
> +#include <libc-lock.h>
> +#include <aio_misc.h>
> +
> +
> +struct clparam
> +{
> + const struct aiocb *const *list;
> + struct waitlist *waitlist;
> + struct requestlist **requestlist;
> +#ifndef DONT_NEED_AIO_MISC_COND
> + pthread_cond_t *cond;
> +#endif
> + int nent;
> +};
> +
> +
> +static void
> +cleanup (void *arg)
> +{
> +#ifdef DONT_NEED_AIO_MISC_COND
> + /* Acquire the mutex. If pthread_cond_*wait is used this would
> + happen implicitly. */
> + pthread_mutex_lock (&__aio_requests_mutex);
> +#endif
> +
> + const struct clparam *param = (const struct clparam *) arg;
> +
> + /* Now remove the entry in the waiting list for all requests
> + which didn't terminate. */
> + int cnt = param->nent;
> + while (cnt-- > 0)
> + if (param->list[cnt] != NULL
> + && param->list[cnt]->__error_code == EINPROGRESS)
> + {
> + struct waitlist **listp;
> +
> + assert (param->requestlist[cnt] != NULL);
> +
> + /* There is the chance that we cannot find our entry anymore. This
> + could happen if the request terminated and restarted again. */
> + listp = ¶m->requestlist[cnt]->waiting;
> + while (*listp != NULL && *listp != ¶m->waitlist[cnt])
> + listp = &(*listp)->next;
> +
> + if (*listp != NULL)
> + *listp = (*listp)->next;
> + }
> +
> +#ifndef DONT_NEED_AIO_MISC_COND
> + /* Release the conditional variable. */
> + (void) pthread_cond_destroy (param->cond);
> +#endif
> +
> + /* Release the mutex. */
> + pthread_mutex_unlock (&__aio_requests_mutex);
> +}
> +
> +#ifdef DONT_NEED_AIO_MISC_COND
> +static int
> +__attribute__ ((noinline))
> +do_aio_misc_wait (unsigned int *cntr, const struct timespec *timeout)
> +{
> + int result = 0;
> +
> + AIO_MISC_WAIT (result, *cntr, timeout, 1);
> +
> + return result;
> +}
> +#endif
> +
> +int
> +aio_suspend (const struct aiocb *const list[], int nent,
> + const struct timespec *timeout)
> +{
> + if (__glibc_unlikely (nent < 0))
> + {
> + __set_errno (EINVAL);
> + return -1;
> + }
> +
> + struct waitlist waitlist[nent];
> + struct requestlist *requestlist[nent];
> +#ifndef DONT_NEED_AIO_MISC_COND
> + pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> +#endif
> + int cnt;
> + bool any = false;
> + int result = 0;
> + unsigned int cntr = 1;
> +
> + /* Request the mutex. */
> + pthread_mutex_lock (&__aio_requests_mutex);
> +
> + /* There is not yet a finished request. Signal the request that
> + we are working for it. */
> + for (cnt = 0; cnt < nent; ++cnt)
> + if (list[cnt] != NULL)
> + {
> + if (list[cnt]->__error_code == EINPROGRESS)
> + {
> + requestlist[cnt] = __aio_find_req ((aiocb_union *) list[cnt]);
> +
> + if (requestlist[cnt] != NULL)
> + {
> +#ifndef DONT_NEED_AIO_MISC_COND
> + waitlist[cnt].cond = &cond;
> +#endif
> + waitlist[cnt].result = NULL;
> + waitlist[cnt].next = requestlist[cnt]->waiting;
> + waitlist[cnt].counterp = &cntr;
> + waitlist[cnt].sigevp = NULL;
> + requestlist[cnt]->waiting = &waitlist[cnt];
> + any = true;
> + }
> + else
> + /* We will never suspend. */
> + break;
> + }
> + else
> + /* We will never suspend. */
> + break;
> + }
> +
> +
> + /* Only if none of the entries is NULL or finished to be wait. */
> + if (cnt == nent && any)
> + {
> + struct clparam clparam =
> + {
> + .list = list,
> + .waitlist = waitlist,
> + .requestlist = requestlist,
> +#ifndef DONT_NEED_AIO_MISC_COND
> + .cond = &cond,
> +#endif
> + .nent = nent
> + };
> +
> + pthread_cleanup_push (cleanup, &clparam);
> +
> +#ifdef DONT_NEED_AIO_MISC_COND
> + result = do_aio_misc_wait (&cntr, timeout);
> +#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);
> + }
> +#endif
> +
> + pthread_cleanup_pop (0);
> + }
> +
> + /* Now remove the entry in the waiting list for all requests
> + which didn't terminate. */
> + while (cnt-- > 0)
> + if (list[cnt] != NULL && list[cnt]->__error_code == EINPROGRESS)
> + {
> + struct waitlist **listp;
> +
> + assert (requestlist[cnt] != NULL);
> +
> + /* There is the chance that we cannot find our entry anymore. This
> + could happen if the request terminated and restarted again. */
> + listp = &requestlist[cnt]->waiting;
> + while (*listp != NULL && *listp != &waitlist[cnt])
> + listp = &(*listp)->next;
> +
> + if (*listp != NULL)
> + *listp = (*listp)->next;
> + }
> +
> +#ifndef DONT_NEED_AIO_MISC_COND
> + /* Release the conditional variable. */
> + if (__glibc_unlikely (pthread_cond_destroy (&cond) != 0))
> + /* This must never happen. */
> + abort ();
> +#endif
> +
> + if (result != 0)
> + {
> +#ifndef DONT_NEED_AIO_MISC_COND
> + /* An error occurred. Possibly it's ETIMEDOUT. We have to translate
> + the timeout error report of `pthread_cond_timedwait' to the
> + form expected from `aio_suspend'. */
> + if (result == ETIMEDOUT)
> + __set_errno (EAGAIN);
> + else
> +#endif
> + __set_errno (result);
> +
> + result = -1;
> + }
> +
> + /* Release the mutex. */
> + pthread_mutex_unlock (&__aio_requests_mutex);
> +
> + return result;
> +}
> +
> +weak_alias (aio_suspend, aio_suspend64)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time
2020-11-14 21:45 ` [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time Lukasz Majewski
@ 2020-11-17 14:41 ` Adhemerval Zanella
2020-11-17 23:01 ` Lukasz Majewski
0 siblings, 1 reply; 6+ messages in thread
From: Adhemerval Zanella @ 2020-11-17 14:41 UTC (permalink / raw)
To: Lukasz Majewski, Joseph Myers, Paul Eggert
Cc: Alistair Francis, Arnd Bergmann, Alistair Francis, GNU C Library,
Florian Weimer, Carlos O'Donell, Stepan Golosunov,
Andreas Schwab, Zack Weinberg
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 '__'.
> + 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.
> +{
> + 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. 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).
Also, once GAI_MISC_WAIT y2038 supported is added we will probably need
to remove the ununsed futex_reltimed_wait{_cancellable}.
> 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.
> @@ -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)
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c
2020-11-17 14:27 ` [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Adhemerval Zanella
@ 2020-11-17 15:58 ` Lukasz Majewski
0 siblings, 0 replies; 6+ messages in thread
From: Lukasz Majewski @ 2020-11-17 15:58 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: Joseph Myers, Paul Eggert, Alistair Francis, Arnd Bergmann,
Alistair Francis, GNU C Library, Florian Weimer,
Carlos O'Donell, Stepan Golosunov, Andreas Schwab,
Zack Weinberg
[-- Attachment #1: Type: text/plain, Size: 11817 bytes --]
Hi Adhemerval,
> On 14/11/2020 18:45, Lukasz Majewski wrote:
> > This patch is a preparatory work, to provide 64 bit time support
> > to aio_suspend.c, which would avoid build breaks on the HURD
> > (i686-gnu) port, as it is not supporting 64 bit version of
> > __pthread_cond_timedwait64.
> >
> > The file sysdeps/unix/sysv/linux/aio_suspend.c is the exact copy of
> > ./sysdeps/pthread/aio_suspend.c
> >
> > ---
> > Changes for v2:
> > - Rephase the commit message
> > ---
>
> I am not very found of this code duplication and the Linux version
> does not need !DONT_NEED_AIO_MISC_COND, since nptl/aio_misc.h will
> always define to 1.
>
> I think it would be better to move the sysdeps/pthread/aio_suspend.c
> to a parametrized implementation
> (sysdeps/pthread/aio_suspend_skeleton.c or something) where the
> internal types and function should be defined (similar to the
> dynamic_array.h) something like:
>
> 94 #ifdef DONT_NEED_AIO_MISC_COND
> 95 static int
> 96 __attribute__ ((noinline))
> 97 do_aio_misc_wait (unsigned int *cntr, const TIMESPEC_STRUCT
> *timeout) 98 {
> 99 int result = 0;
> 100
> 101 AIO_MISC_WAIT_MACRO (result, *cntr, timeout, 1);
> 102
> 103 return result;
> 104 }
> 105 #endif
> 106
> 107 int
> 108 AIO_SUSPEND (const struct aiocb *const list[], int nent,
> 109 const TIMESPEC_STRUCT *timeout)
> 110 {
> 111 if (__glibc_unlikely (nent < 0))
> 112 {
> 113 __set_errno (EINVAL);
> 114 return -1;
> 115 }
>
> And then generic sysdeps/pthread/aio_suspend.c will define the types
> and include the parametrized implementation:
>
> #define TIMESPEC_STRUCT struct timespec
> #define AIO_SUSPEND aio_suspend
> #include <sysdeps/pthread/aio_suspend_skeleton.c>
>
>
> The Linux implementation sysdeps/unix/sysv/linux/aio_suspend.c then
> can do:
>
>
> #if __TIMESIZE != 64
> # define TIMESPEC_STRUCT struct __timespec64
> # define AIO_SUSPEND __aio_suspend64_time64
> # define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT64
> #else
> # define TIMESPEC_STRUCT struct timespec
> # define AIO_SUSPEND aio_suspend
> # define AIO_MISC_WAIT_MACRO AIO_MISC_WAIT
> #endif
> #include <sysdeps/pthread/aio_suspend_skeleton.c>
>
> #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
>
Ok. This shall help with avoiding code duplication.
>
> > sysdeps/unix/sysv/linux/aio_suspend.c | 253
> > ++++++++++++++++++++++++++ 1 file changed, 253 insertions(+)
> > create mode 100644 sysdeps/unix/sysv/linux/aio_suspend.c
> >
> > diff --git a/sysdeps/unix/sysv/linux/aio_suspend.c
> > b/sysdeps/unix/sysv/linux/aio_suspend.c new file mode 100644
> > index 0000000000..ad03f13558
> > --- /dev/null
> > +++ b/sysdeps/unix/sysv/linux/aio_suspend.c
> > @@ -0,0 +1,253 @@
> > +/* Suspend until termination of a requests.
> > + Copyright (C) 1997-2020 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > + Contributed by Ulrich Drepper <drepper@cygnus.com>, 1997.
> > +
> > + The GNU C Library is free software; you can redistribute it
> > and/or
> > + modify it under the terms of the GNU Lesser General Public
> > + License as published by the Free Software Foundation; either
> > + version 2.1 of the License, or (at your option) any later
> > version. +
> > + The GNU C Library is distributed in the hope that it will be
> > useful,
> > + but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > GNU
> > + Lesser General Public License for more details.
> > +
> > + You should have received a copy of the GNU Lesser General Public
> > + License along with the GNU C Library; if not, see
> > + <https://www.gnu.org/licenses/>. */
> > +
> > +
> > +/* We use an UGLY hack to prevent gcc from finding us cheating.
> > The
> > + implementations of aio_suspend and aio_suspend64 are identical
> > and so
> > + we want to avoid code duplication by using aliases. But gcc
> > sees
> > + the different parameter lists and prints a warning. We define
> > here
> > + a function so that aio_suspend64 has no prototype. */
> > +#define aio_suspend64 XXX
> > +#include <aio.h>
> > +/* And undo the hack. */
> > +#undef aio_suspend64
> > +
> > +#include <assert.h>
> > +#include <errno.h>
> > +#include <stdbool.h>
> > +#include <stdlib.h>
> > +#include <sys/time.h>
> > +
> > +#include <libc-lock.h>
> > +#include <aio_misc.h>
> > +
> > +
> > +struct clparam
> > +{
> > + const struct aiocb *const *list;
> > + struct waitlist *waitlist;
> > + struct requestlist **requestlist;
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + pthread_cond_t *cond;
> > +#endif
> > + int nent;
> > +};
> > +
> > +
> > +static void
> > +cleanup (void *arg)
> > +{
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > + /* Acquire the mutex. If pthread_cond_*wait is used this would
> > + happen implicitly. */
> > + pthread_mutex_lock (&__aio_requests_mutex);
> > +#endif
> > +
> > + const struct clparam *param = (const struct clparam *) arg;
> > +
> > + /* Now remove the entry in the waiting list for all requests
> > + which didn't terminate. */
> > + int cnt = param->nent;
> > + while (cnt-- > 0)
> > + if (param->list[cnt] != NULL
> > + && param->list[cnt]->__error_code == EINPROGRESS)
> > + {
> > + struct waitlist **listp;
> > +
> > + assert (param->requestlist[cnt] != NULL);
> > +
> > + /* There is the chance that we cannot find our entry
> > anymore. This
> > + could happen if the request terminated and restarted
> > again. */
> > + listp = ¶m->requestlist[cnt]->waiting;
> > + while (*listp != NULL && *listp != ¶m->waitlist[cnt])
> > + listp = &(*listp)->next;
> > +
> > + if (*listp != NULL)
> > + *listp = (*listp)->next;
> > + }
> > +
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* Release the conditional variable. */
> > + (void) pthread_cond_destroy (param->cond);
> > +#endif
> > +
> > + /* Release the mutex. */
> > + pthread_mutex_unlock (&__aio_requests_mutex);
> > +}
> > +
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > +static int
> > +__attribute__ ((noinline))
> > +do_aio_misc_wait (unsigned int *cntr, const struct timespec
> > *timeout) +{
> > + int result = 0;
> > +
> > + AIO_MISC_WAIT (result, *cntr, timeout, 1);
> > +
> > + return result;
> > +}
> > +#endif
> > +
> > +int
> > +aio_suspend (const struct aiocb *const list[], int nent,
> > + const struct timespec *timeout)
> > +{
> > + if (__glibc_unlikely (nent < 0))
> > + {
> > + __set_errno (EINVAL);
> > + return -1;
> > + }
> > +
> > + struct waitlist waitlist[nent];
> > + struct requestlist *requestlist[nent];
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + pthread_cond_t cond = PTHREAD_COND_INITIALIZER;
> > +#endif
> > + int cnt;
> > + bool any = false;
> > + int result = 0;
> > + unsigned int cntr = 1;
> > +
> > + /* Request the mutex. */
> > + pthread_mutex_lock (&__aio_requests_mutex);
> > +
> > + /* There is not yet a finished request. Signal the request that
> > + we are working for it. */
> > + for (cnt = 0; cnt < nent; ++cnt)
> > + if (list[cnt] != NULL)
> > + {
> > + if (list[cnt]->__error_code == EINPROGRESS)
> > + {
> > + requestlist[cnt] = __aio_find_req ((aiocb_union *)
> > list[cnt]); +
> > + if (requestlist[cnt] != NULL)
> > + {
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + waitlist[cnt].cond = &cond;
> > +#endif
> > + waitlist[cnt].result = NULL;
> > + waitlist[cnt].next = requestlist[cnt]->waiting;
> > + waitlist[cnt].counterp = &cntr;
> > + waitlist[cnt].sigevp = NULL;
> > + requestlist[cnt]->waiting = &waitlist[cnt];
> > + any = true;
> > + }
> > + else
> > + /* We will never suspend. */
> > + break;
> > + }
> > + else
> > + /* We will never suspend. */
> > + break;
> > + }
> > +
> > +
> > + /* Only if none of the entries is NULL or finished to be wait.
> > */
> > + if (cnt == nent && any)
> > + {
> > + struct clparam clparam =
> > + {
> > + .list = list,
> > + .waitlist = waitlist,
> > + .requestlist = requestlist,
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + .cond = &cond,
> > +#endif
> > + .nent = nent
> > + };
> > +
> > + pthread_cleanup_push (cleanup, &clparam);
> > +
> > +#ifdef DONT_NEED_AIO_MISC_COND
> > + result = do_aio_misc_wait (&cntr, timeout);
> > +#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);
> > + }
> > +#endif
> > +
> > + pthread_cleanup_pop (0);
> > + }
> > +
> > + /* Now remove the entry in the waiting list for all requests
> > + which didn't terminate. */
> > + while (cnt-- > 0)
> > + if (list[cnt] != NULL && list[cnt]->__error_code ==
> > EINPROGRESS)
> > + {
> > + struct waitlist **listp;
> > +
> > + assert (requestlist[cnt] != NULL);
> > +
> > + /* There is the chance that we cannot find our entry
> > anymore. This
> > + could happen if the request terminated and restarted
> > again. */
> > + listp = &requestlist[cnt]->waiting;
> > + while (*listp != NULL && *listp != &waitlist[cnt])
> > + listp = &(*listp)->next;
> > +
> > + if (*listp != NULL)
> > + *listp = (*listp)->next;
> > + }
> > +
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* Release the conditional variable. */
> > + if (__glibc_unlikely (pthread_cond_destroy (&cond) != 0))
> > + /* This must never happen. */
> > + abort ();
> > +#endif
> > +
> > + if (result != 0)
> > + {
> > +#ifndef DONT_NEED_AIO_MISC_COND
> > + /* An error occurred. Possibly it's ETIMEDOUT. We have to
> > translate
> > + the timeout error report of `pthread_cond_timedwait' to
> > the
> > + form expected from `aio_suspend'. */
> > + if (result == ETIMEDOUT)
> > + __set_errno (EAGAIN);
> > + else
> > +#endif
> > + __set_errno (result);
> > +
> > + result = -1;
> > + }
> > +
> > + /* Release the mutex. */
> > + pthread_mutex_unlock (&__aio_requests_mutex);
> > +
> > + return result;
> > +}
> > +
> > +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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time
2020-11-17 14:41 ` Adhemerval Zanella
@ 2020-11-17 23:01 ` Lukasz Majewski
0 siblings, 0 replies; 6+ messages in thread
From: Lukasz Majewski @ 2020-11-17 23:01 UTC (permalink / raw)
To: Adhemerval Zanella
Cc: Joseph Myers, Paul Eggert, Alistair Francis, Arnd Bergmann,
Alistair Francis, GNU C Library, Florian Weimer,
Carlos O'Donell, Stepan Golosunov, Andreas Schwab,
Zack Weinberg
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-11-17 23:01 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-14 21:45 [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Lukasz Majewski
2020-11-14 21:45 ` [PATCH v2 2/2] y2038: Convert aio_suspend to support 64 bit time Lukasz Majewski
2020-11-17 14:41 ` Adhemerval Zanella
2020-11-17 23:01 ` Lukasz Majewski
2020-11-17 14:27 ` [PATCH v2 1/2] aio: Provide Linux specific copy of aio_suspend.c Adhemerval Zanella
2020-11-17 15:58 ` Lukasz Majewski
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).