* [PATCH 02/13] nptl: Remove futex_wait_cancelable
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 18:01 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 03/13] nptl: Remove clockwait_tid Adhemerval Zanella
` (11 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
It is used solely on __pthread_cond_wait_common and the call can be
replaced by a __futex_abstimed_wait_cancelable64 one.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_cond_wait.c | 22 ++--------------------
sysdeps/nptl/futex-internal.h | 29 -----------------------------
2 files changed, 2 insertions(+), 49 deletions(-)
diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
index 7d158d553f..685dbca32f 100644
--- a/nptl/pthread_cond_wait.c
+++ b/nptl/pthread_cond_wait.c
@@ -501,26 +501,8 @@ __pthread_cond_wait_common (pthread_cond_t *cond, pthread_mutex_t *mutex,
cbuffer.private = private;
__pthread_cleanup_push (&buffer, __condvar_cleanup_waiting, &cbuffer);
- if (abstime == NULL)
- {
- /* Block without a timeout. */
- err = futex_wait_cancelable (
- cond->__data.__g_signals + g, 0, private);
- }
- else
- {
- /* Block, but with a timeout.
- Work around the fact that the kernel rejects negative timeout
- values despite them being valid. */
- if (__glibc_unlikely (abstime->tv_sec < 0))
- err = ETIMEDOUT;
- else
- {
- err = __futex_abstimed_wait_cancelable64
- (cond->__data.__g_signals + g, 0, clockid, abstime,
- private);
- }
- }
+ err = __futex_abstimed_wait_cancelable64 (
+ cond->__data.__g_signals + g, 0, clockid, abstime, private);
__pthread_cleanup_pop (&buffer, 0);
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 21e3b74be6..96d1318091 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -177,35 +177,6 @@ futex_wait_simple (unsigned int *futex_word, unsigned int expected,
ignore_value (futex_wait (futex_word, expected, private));
}
-
-/* Like futex_wait but is a POSIX cancellation point. */
-static __always_inline int
-futex_wait_cancelable (unsigned int *futex_word, unsigned int expected,
- int private)
-{
- int oldtype;
- oldtype = __pthread_enable_asynccancel ();
- int err = lll_futex_timed_wait (futex_word, expected, NULL, private);
- __pthread_disable_asynccancel (oldtype);
- switch (err)
- {
- case 0:
- case -EAGAIN:
- case -EINTR:
- return -err;
-
- case -ETIMEDOUT: /* Cannot have happened as we provided no timeout. */
- 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_wait, but will eventually time out (i.e., stop being
blocked) after the duration of time provided (i.e., RELTIME) has
passed. The caller must provide a normalized RELTIME. RELTIME can also
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 02/13] nptl: Remove futex_wait_cancelable
2020-11-23 19:52 ` [PATCH 02/13] nptl: Remove futex_wait_cancelable Adhemerval Zanella
@ 2020-11-24 18:01 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 18:01 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 3677 bytes --]
Hi Adhemerval,
> It is used solely on __pthread_cond_wait_common and the call can be
> replaced by a __futex_abstimed_wait_cancelable64 one.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_cond_wait.c | 22 ++--------------------
> sysdeps/nptl/futex-internal.h | 29 -----------------------------
> 2 files changed, 2 insertions(+), 49 deletions(-)
>
> diff --git a/nptl/pthread_cond_wait.c b/nptl/pthread_cond_wait.c
> index 7d158d553f..685dbca32f 100644
> --- a/nptl/pthread_cond_wait.c
> +++ b/nptl/pthread_cond_wait.c
> @@ -501,26 +501,8 @@ __pthread_cond_wait_common (pthread_cond_t
> *cond, pthread_mutex_t *mutex, cbuffer.private = private;
> __pthread_cleanup_push (&buffer,
> __condvar_cleanup_waiting, &cbuffer);
> - if (abstime == NULL)
> - {
> - /* Block without a timeout. */
> - err = futex_wait_cancelable (
> - cond->__data.__g_signals + g, 0, private);
> - }
> - else
> - {
> - /* Block, but with a timeout.
> - Work around the fact that the kernel rejects
> negative timeout
> - values despite them being valid. */
> - if (__glibc_unlikely (abstime->tv_sec < 0))
> - err = ETIMEDOUT;
> - else
> - {
> - err = __futex_abstimed_wait_cancelable64
> - (cond->__data.__g_signals + g, 0, clockid,
> abstime,
> - private);
> - }
> - }
> + err = __futex_abstimed_wait_cancelable64 (
> + cond->__data.__g_signals + g, 0, clockid, abstime,
> private);
The __futex_abstimed_wait_cancelable64 can handle NULL abstime. Thanks
for cleaning this up :-).
> __pthread_cleanup_pop (&buffer, 0);
>
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 21e3b74be6..96d1318091 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -177,35 +177,6 @@ futex_wait_simple (unsigned int *futex_word,
> unsigned int expected, ignore_value (futex_wait (futex_word,
> expected, private)); }
>
> -
> -/* Like futex_wait but is a POSIX cancellation point. */
> -static __always_inline int
> -futex_wait_cancelable (unsigned int *futex_word, unsigned int
> expected,
> - int private)
> -{
> - int oldtype;
> - oldtype = __pthread_enable_asynccancel ();
> - int err = lll_futex_timed_wait (futex_word, expected, NULL,
> private);
> - __pthread_disable_asynccancel (oldtype);
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - return -err;
> -
> - case -ETIMEDOUT: /* Cannot have happened as we provided no
> timeout. */
> - 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_wait, but will eventually time out (i.e., stop being
> blocked) after the duration of time provided (i.e., RELTIME) has
> passed. The caller must provide a normalized RELTIME. RELTIME
> can also
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 03/13] nptl: Remove clockwait_tid
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
2020-11-23 19:52 ` [PATCH 02/13] nptl: Remove futex_wait_cancelable Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 18:13 ` Lukasz Majewski
2020-12-14 12:16 ` Florian Weimer
2020-11-23 19:52 ` [PATCH 04/13] linux: Extend __futex_abstimed_wait_cancelable64 comment Adhemerval Zanella
` (10 subsequent siblings)
12 siblings, 2 replies; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
It can be replaced with a __futex_abstimed_wait_cancelable64 call,
with the advantage that there is no need to further clock adjustments
to create a absolute timeout. It allows to remove the now ununsed
futex_timed_wait_cancel64 internal function.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_join_common.c | 70 ++++++-----------------------------
sysdeps/nptl/futex-internal.h | 49 ------------------------
2 files changed, 12 insertions(+), 107 deletions(-)
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 67d8e2b780..8a95c58ff3 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -32,55 +32,6 @@ cleanup (void *arg)
atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
}
-/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex
- wake-up when the clone terminates. The memory location contains the
- thread ID while the clone is running and is reset to zero by the kernel
- afterwards. The kernel up to version 3.16.3 does not use the private futex
- operations for futex wake-up when the clone terminates. */
-static int
-clockwait_tid (pid_t *tidp, clockid_t clockid,
- const struct __timespec64 *abstime)
-{
- pid_t tid;
- int ret;
-
- if (! valid_nanoseconds (abstime->tv_nsec))
- return EINVAL;
-
- /* Repeat until thread terminated. */
- while ((tid = *tidp) != 0)
- {
- struct __timespec64 rt;
-
- /* Get the current time. This can only fail if clockid is
- invalid. */
- if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
- return EINVAL;
-
- /* Compute relative timeout. */
- rt.tv_sec = abstime->tv_sec - rt.tv_sec;
- rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
- if (rt.tv_nsec < 0)
- {
- rt.tv_nsec += 1000000000;
- --rt.tv_sec;
- }
-
- /* Already timed out? */
- if (rt.tv_sec < 0)
- return ETIMEDOUT;
-
- /* If *tidp == tid, wait until thread terminates or the wait times out.
- The kernel up to version 3.16.3 does not use the private futex
- operations for futex wake-up when the clone terminates. */
- ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
- if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
- return -ret;
- }
-
- return 0;
-}
-
int
__pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
clockid_t clockid,
@@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
un-wait-ed for again. */
pthread_cleanup_push (cleanup, &pd->joinid);
- if (abstime != NULL)
- result = clockwait_tid (&pd->tid, clockid, abstime);
- else
- {
- pid_t tid;
- /* We need acquire MO here so that we synchronize with the
- kernel's store to 0 when the clone terminates. (see above) */
- while ((tid = atomic_load_acquire (&pd->tid)) != 0)
- lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
+ /* We need acquire MO here so that we synchronize with the
+ kernel's store to 0 when the clone terminates. (see above) */
+ pid_t tid;
+ while ((tid = atomic_load_acquire (&pd->tid)) != 0)
+ {
+ int ret = __futex_abstimed_wait_cancelable64 (
+ (unsigned int *) &pd->tid, tid, clockid, abstime, LLL_SHARED);
+ if (ret == ETIMEDOUT || ret == EOVERFLOW)
+ {
+ result = ret;
+ break;
+ }
}
pthread_cleanup_pop (0);
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 96d1318091..d5f13d15fb 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int private)
}
}
-static __always_inline int
-futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
- const struct __timespec64 *timeout, int private)
-{
- int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
- __lll_private_flag (FUTEX_WAIT, private),
- tid, timeout);
-#ifndef __ASSUME_TIME64_SYSCALLS
- if (err == -ENOSYS)
- {
- if (in_time_t_range (timeout->tv_sec))
- {
- struct timespec ts32 = valid_timespec64_to_timespec (*timeout);
-
- err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
- __lll_private_flag (FUTEX_WAIT,
- private),
- tid, &ts32);
- }
- else
- err = -EOVERFLOW;
- }
-#endif
- switch (err)
- {
- case 0:
- case -EAGAIN:
- case -EINTR:
- case -ETIMEDOUT:
- case -EDEADLK:
- case -ENOSYS:
- case -EOVERFLOW: /* Passed absolute timeout uses 64 bit time_t type, but
- underlying kernel does not support 64 bit time_t futex
- syscalls. */
- case -EPERM: /* The caller is not allowed to attach itself to the futex.
- Used to check if PI futexes are supported by the
- kernel. */
- return -err;
-
- 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 -EFAULT: /* Must have been caused by a glibc or application bug. */
- /* No other errors are documented at this time. */
- default:
- futex_fatal_error ();
- }
-}
-
/* The futex_abstimed_wait_cancelable64 has been moved to a separate file
to avoid problems with exhausting available registers on some architectures
- e.g. on m68k architecture. */
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-11-23 19:52 ` [PATCH 03/13] nptl: Remove clockwait_tid Adhemerval Zanella
@ 2020-11-24 18:13 ` Lukasz Majewski
2020-12-14 12:16 ` Florian Weimer
1 sibling, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 18:13 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 6587 bytes --]
On Mon, 23 Nov 2020 16:52:46 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:
> It can be replaced with a __futex_abstimed_wait_cancelable64 call,
> with the advantage that there is no need to further clock adjustments
> to create a absolute timeout. It allows to remove the now ununsed
> futex_timed_wait_cancel64 internal function.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_join_common.c | 70
> ++++++----------------------------- sysdeps/nptl/futex-internal.h |
> 49 ------------------------ 2 files changed, 12 insertions(+), 107
> deletions(-)
>
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 67d8e2b780..8a95c58ff3 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -32,55 +32,6 @@ cleanup (void *arg)
> atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
> }
>
> -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
> futex
> - wake-up when the clone terminates. The memory location contains
> the
> - thread ID while the clone is running and is reset to zero by the
> kernel
> - afterwards. The kernel up to version 3.16.3 does not use the
> private futex
> - operations for futex wake-up when the clone terminates. */
> -static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid,
> - const struct __timespec64 *abstime)
> -{
> - pid_t tid;
> - int ret;
> -
> - if (! valid_nanoseconds (abstime->tv_nsec))
> - return EINVAL;
> -
> - /* Repeat until thread terminated. */
> - while ((tid = *tidp) != 0)
> - {
> - struct __timespec64 rt;
> -
> - /* Get the current time. This can only fail if clockid is
> - invalid. */
> - if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
> - return EINVAL;
> -
> - /* Compute relative timeout. */
> - rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> - if (rt.tv_nsec < 0)
> - {
> - rt.tv_nsec += 1000000000;
> - --rt.tv_sec;
> - }
> -
> - /* Already timed out? */
> - if (rt.tv_sec < 0)
> - return ETIMEDOUT;
> -
> - /* If *tidp == tid, wait until thread terminates or the wait
> times out.
> - The kernel up to version 3.16.3 does not use the private
> futex
> - operations for futex wake-up when the clone terminates. */
> - ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> - if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> - return -ret;
> - }
> -
> - return 0;
> -}
> -
> int
> __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
> clockid_t clockid,
> @@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void
> **thread_return, un-wait-ed for again. */
> pthread_cleanup_push (cleanup, &pd->joinid);
>
> - if (abstime != NULL)
> - result = clockwait_tid (&pd->tid, clockid, abstime);
> - else
> - {
> - pid_t tid;
> - /* We need acquire MO here so that we synchronize with the
> - kernel's store to 0 when the clone terminates. (see
> above) */
> - while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> - lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
> + /* We need acquire MO here so that we synchronize with the
> + kernel's store to 0 when the clone terminates. (see above)
> */
> + pid_t tid;
> + while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> + {
> + int ret = __futex_abstimed_wait_cancelable64 (
> + (unsigned int *) &pd->tid, tid, clockid, abstime,
> LLL_SHARED);
> + if (ret == ETIMEDOUT || ret == EOVERFLOW)
> + {
> + result = ret;
> + break;
> + }
> }
>
> pthread_cleanup_pop (0);
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 96d1318091..d5f13d15fb 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int
> private) }
> }
>
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> - const struct __timespec64 *timeout, int
> private) -{
> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> - __lll_private_flag (FUTEX_WAIT,
> private),
> - tid, timeout);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> - if (err == -ENOSYS)
> - {
> - if (in_time_t_range (timeout->tv_sec))
> - {
> - struct timespec ts32 = valid_timespec64_to_timespec
> (*timeout); -
> - err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> - __lll_private_flag
> (FUTEX_WAIT,
> -
> private),
> - tid, &ts32);
> - }
> - else
> - err = -EOVERFLOW;
> - }
> -#endif
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - case -ETIMEDOUT:
> - case -EDEADLK:
> - case -ENOSYS:
> - case -EOVERFLOW: /* Passed absolute timeout uses 64 bit time_t
> type, but
> - underlying kernel does not support 64 bit
> time_t futex
> - syscalls. */
> - case -EPERM: /* The caller is not allowed to attach itself to
> the futex.
> - Used to check if PI futexes are supported by
> the
> - kernel. */
> - return -err;
> -
> - 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 -EFAULT: /* Must have been caused by a glibc or application
> bug. */
> - /* No other errors are documented at this time. */
> - default:
> - futex_fatal_error ();
> - }
> -}
> -
> /* The futex_abstimed_wait_cancelable64 has been moved to a separate
> file to avoid problems with exhausting available registers on some
> architectures
> - e.g. on m68k architecture. */
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-11-23 19:52 ` [PATCH 03/13] nptl: Remove clockwait_tid Adhemerval Zanella
2020-11-24 18:13 ` Lukasz Majewski
@ 2020-12-14 12:16 ` Florian Weimer
2020-12-14 12:47 ` Andreas Schwab
2020-12-14 12:52 ` Adhemerval Zanella
1 sibling, 2 replies; 40+ messages in thread
From: Florian Weimer @ 2020-12-14 12:16 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha
Cc: Adhemerval Zanella, Michael Kerrisk, Lukasz Majewski
* Adhemerval Zanella via Libc-alpha:
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
> - const struct __timespec64 *timeout, int private)
> -{
> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> - __lll_private_flag (FUTEX_WAIT, private),
> - tid, timeout);
This uses FUTEX_WAIT. But the replacement,
__futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET. I do not think
this is correct because the kernel will use FUTEX_WAKE internally for
the pd->tid wakeup relied upon by pthread_join.
This seems to cause pthread_join regressions on some kernel versions.
We need to audit all callers of __futex_abstimed_wait64 if they are
actually compatible with FUTEX_WAIT_BITSET.
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-12-14 12:16 ` Florian Weimer
@ 2020-12-14 12:47 ` Andreas Schwab
2020-12-14 13:11 ` Florian Weimer
2020-12-14 12:52 ` Adhemerval Zanella
1 sibling, 1 reply; 40+ messages in thread
From: Andreas Schwab @ 2020-12-14 12:47 UTC (permalink / raw)
To: Florian Weimer via Libc-alpha; +Cc: Florian Weimer, Michael Kerrisk
On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
> This uses FUTEX_WAIT. But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
FUTEX_WAIT is exactly equivalent to
FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510 2552 DF73 E780 A9DA AEC1
"And now for something completely different."
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-12-14 12:47 ` Andreas Schwab
@ 2020-12-14 13:11 ` Florian Weimer
2020-12-14 14:02 ` Florian Weimer
0 siblings, 1 reply; 40+ messages in thread
From: Florian Weimer @ 2020-12-14 13:11 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Michael Kerrisk
* Andreas Schwab:
> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>
>> This uses FUTEX_WAIT. But the replacement,
>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>
> FUTEX_WAIT is exactly equivalent to
> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
Hmm. I will continue debugging. I encounter a missed wakeup in
pthread_join after making some changes that only should affect timing
(intl/tst-gettext6 is among the failures).
Reverting this patch here seems to help.
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-12-14 13:11 ` Florian Weimer
@ 2020-12-14 14:02 ` Florian Weimer
0 siblings, 0 replies; 40+ messages in thread
From: Florian Weimer @ 2020-12-14 14:02 UTC (permalink / raw)
To: Andreas Schwab; +Cc: Florian Weimer via Libc-alpha, Michael Kerrisk
* Florian Weimer:
> * Andreas Schwab:
>
>> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>>
>>> This uses FUTEX_WAIT. But the replacement,
>>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>>
>> FUTEX_WAIT is exactly equivalent to
>> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
>
> Hmm. I will continue debugging. I encounter a missed wakeup in
> pthread_join after making some changes that only should affect timing
> (intl/tst-gettext6 is among the failures).
>
> Reverting this patch here seems to help.
It's an accident, due to my changes and:
/* In libc.so or ld.so all futexes are private. */
in lowlevellock-futex.h. So the patch posted here should be okay.
Thanks,
Florian
--
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
^ permalink raw reply [flat|nested] 40+ messages in thread
* Re: [PATCH 03/13] nptl: Remove clockwait_tid
2020-12-14 12:16 ` Florian Weimer
2020-12-14 12:47 ` Andreas Schwab
@ 2020-12-14 12:52 ` Adhemerval Zanella
1 sibling, 0 replies; 40+ messages in thread
From: Adhemerval Zanella @ 2020-12-14 12:52 UTC (permalink / raw)
To: Florian Weimer, Adhemerval Zanella via Libc-alpha
Cc: Michael Kerrisk, Lukasz Majewski
On 14/12/2020 09:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
>
>> -static __always_inline int
>> -futex_timed_wait_cancel64 (pid_t *tidp, pid_t tid,
>> - const struct __timespec64 *timeout, int private)
>> -{
>> - int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
>> - __lll_private_flag (FUTEX_WAIT, private),
>> - tid, timeout);
>
> This uses FUTEX_WAIT. But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET. I do not think
> this is correct because the kernel will use FUTEX_WAKE internally for
> the pd->tid wakeup relied upon by pthread_join.
>
> This seems to cause pthread_join regressions on some kernel versions.
>
> We need to audit all callers of __futex_abstimed_wait64 if they are
> actually compatible with FUTEX_WAIT_BITSET.
I don't think it should matter, as Andreas has put FUTEX_WAIT is exactly
as FUTEX_WAIT_BITSET plus FUTEX_BITSET_MATCH_ANY. On a recent kernel:
SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
u32, val3)
{
[...]
return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
}
long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
u32 __user *uaddr2, u32 val2, u32 val3)
{
[...]
case FUTEX_WAIT:
val3 = FUTEX_BITSET_MATCH_ANY;
fallthrough;
case FUTEX_WAIT_BITSET:
return futex_wait(uaddr, flags, val, timeout, val3);
[...]
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 04/13] linux: Extend __futex_abstimed_wait_cancelable64 comment
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
2020-11-23 19:52 ` [PATCH 02/13] nptl: Remove futex_wait_cancelable Adhemerval Zanella
2020-11-23 19:52 ` [PATCH 03/13] nptl: Remove clockwait_tid Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 18:16 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 05/13] linux: nptl: Consolidate __futex_abstimed_wait_{cancelable}64 Adhemerval Zanella
` (9 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
And add a small optimization to avoid setting the operation for the
32-bit time fallback operation.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
sysdeps/nptl/futex-internal.c | 8 ++------
sysdeps/nptl/futex-internal.h | 18 +++++++++++++++---
2 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
index 457cd3cd69..e4a14b477c 100644
--- a/sysdeps/nptl/futex-internal.c
+++ b/sysdeps/nptl/futex-internal.c
@@ -25,7 +25,7 @@
#ifndef __ASSUME_TIME64_SYSCALLS
static int
__futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
- unsigned int expected, clockid_t clockid,
+ unsigned int expected, int op,
const struct __timespec64* abstime,
int private)
{
@@ -39,10 +39,6 @@ __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
pts32 = &ts32;
}
- unsigned int clockbit = (clockid == CLOCK_REALTIME)
- ? FUTEX_CLOCK_REALTIME : 0;
- int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
-
return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
pts32, NULL /* Unused. */,
FUTEX_BITSET_MATCH_ANY);
@@ -119,7 +115,7 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
#ifndef __ASSUME_TIME64_SYSCALLS
if (err == -ENOSYS)
err = __futex_abstimed_wait_cancelable32 (futex_word, expected,
- clockid, abstime, private);
+ op, abstime, private);
#endif
switch (err)
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index d5f13d15fb..cefab74301 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -390,9 +390,21 @@ futex_unlock_pi (unsigned int *futex_word, int private)
}
}
-/* The futex_abstimed_wait_cancelable64 has been moved to a separate file
- to avoid problems with exhausting available registers on some architectures
- - e.g. on m68k architecture. */
+/* Like futex_wait, but will eventually time out (i.e., stop being blocked)
+ after the duration of time provided (i.e., ABSTIME) has passed using the
+ clock specified by CLOCKID (currently only CLOCK_REALTIME and
+ CLOCK_MONOTONIC, the ones support by lll_futex_supported_clockid). ABSTIME
+ can also equal NULL, in which case this function behaves equivalent to
+ futex_wait.
+
+ Returns the same values as futex_wait under those same conditions;
+ additionally, returns ETIMEDOUT if the timeout expired.
+
+ The call acts a cancellation entrypoint.
+
+ (The implementation has been moved to a separate file to avoid problems
+ with exhausting available registers on some architectures - e.g. on
+ m68k). */
int
__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
unsigned int expected, clockid_t clockid,
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 04/13] linux: Extend __futex_abstimed_wait_cancelable64 comment
2020-11-23 19:52 ` [PATCH 04/13] linux: Extend __futex_abstimed_wait_cancelable64 comment Adhemerval Zanella
@ 2020-11-24 18:16 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 18:16 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 3601 bytes --]
Hi Adhemerval,
> And add a small optimization to avoid setting the operation for the
> 32-bit time fallback operation.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> sysdeps/nptl/futex-internal.c | 8 ++------
> sysdeps/nptl/futex-internal.h | 18 +++++++++++++++---
> 2 files changed, 17 insertions(+), 9 deletions(-)
>
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index 457cd3cd69..e4a14b477c 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -25,7 +25,7 @@
> #ifndef __ASSUME_TIME64_SYSCALLS
> static int
> __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
> - unsigned int expected, clockid_t
> clockid,
> + unsigned int expected, int op,
> const struct __timespec64*
> abstime, int private)
> {
> @@ -39,10 +39,6 @@ __futex_abstimed_wait_cancelable32 (unsigned int*
> futex_word, pts32 = &ts32;
> }
>
> - unsigned int clockbit = (clockid == CLOCK_REALTIME)
> - ? FUTEX_CLOCK_REALTIME : 0;
> - int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> private); -
> return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
> pts32, NULL /* Unused. */,
> FUTEX_BITSET_MATCH_ANY);
> @@ -119,7 +115,7 @@ __futex_abstimed_wait_cancelable64 (unsigned int*
> futex_word, #ifndef __ASSUME_TIME64_SYSCALLS
> if (err == -ENOSYS)
> err = __futex_abstimed_wait_cancelable32 (futex_word, expected,
> - clockid, abstime,
> private);
> + op, abstime, private);
> #endif
>
> switch (err)
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index d5f13d15fb..cefab74301 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -390,9 +390,21 @@ futex_unlock_pi (unsigned int *futex_word, int
> private) }
> }
>
> -/* The futex_abstimed_wait_cancelable64 has been moved to a separate
> file
> - to avoid problems with exhausting available registers on some
> architectures
> - - e.g. on m68k architecture. */
> +/* Like futex_wait, but will eventually time out (i.e., stop being
> blocked)
> + after the duration of time provided (i.e., ABSTIME) has passed
> using the
> + clock specified by CLOCKID (currently only CLOCK_REALTIME and
> + CLOCK_MONOTONIC, the ones support by
> lll_futex_supported_clockid). ABSTIME
> + can also equal NULL, in which case this function behaves
> equivalent to
> + futex_wait.
> +
> + Returns the same values as futex_wait under those same conditions;
> + additionally, returns ETIMEDOUT if the timeout expired.
> +
> + The call acts a cancellation entrypoint.
> +
> + (The implementation has been moved to a separate file to avoid
> problems
> + with exhausting available registers on some architectures - e.g.
> on
> + m68k). */
> int
> __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> unsigned int expected, clockid_t
> clockid,
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 05/13] linux: nptl: Consolidate __futex_abstimed_wait_{cancelable}64
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (2 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 04/13] linux: Extend __futex_abstimed_wait_cancelable64 comment Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 18:19 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 06/13] linux: nptl: Remove _futex_clock_wait_bitset64 Adhemerval Zanella
` (8 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The only different is how to issue the syscall.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
sysdeps/nptl/futex-internal.c | 111 +++++++++++-----------------------
1 file changed, 35 insertions(+), 76 deletions(-)
diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
index e4a14b477c..f9a2e28ee6 100644
--- a/sysdeps/nptl/futex-internal.c
+++ b/sysdeps/nptl/futex-internal.c
@@ -24,10 +24,10 @@
#ifndef __ASSUME_TIME64_SYSCALLS
static int
-__futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
- unsigned int expected, int op,
- const struct __timespec64* abstime,
- int private)
+__futex_abstimed_wait_common32 (unsigned int* futex_word,
+ unsigned int expected, int op,
+ const struct __timespec64* abstime,
+ int private, bool cancel)
{
struct timespec ts32, *pts32 = NULL;
if (abstime != NULL)
@@ -39,34 +39,16 @@ __futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
pts32 = &ts32;
}
- return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
+ if (cancel)
+ return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
+ pts32, NULL /* Unused. */,
+ FUTEX_BITSET_MATCH_ANY);
+ else
+ return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
pts32, NULL /* Unused. */,
FUTEX_BITSET_MATCH_ANY);
}
-static int
-__futex_abstimed_wait32 (unsigned int* futex_word,
- unsigned int expected, clockid_t clockid,
- const struct __timespec64* abstime,
- int private)
-{
- struct timespec ts32;
-
- if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
- return -EOVERFLOW;
-
- unsigned int clockbit = (clockid == CLOCK_REALTIME) ?
- FUTEX_CLOCK_REALTIME : 0;
- int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
-
- if (abstime != NULL)
- ts32 = valid_timespec64_to_timespec (*abstime);
-
- return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
- abstime != NULL ? &ts32 : NULL,
- NULL /* Unused. */, FUTEX_BITSET_MATCH_ANY);
-}
-
static int
__futex_clock_wait_bitset32 (int *futexp, int val, clockid_t clockid,
const struct __timespec64 *abstime, int private)
@@ -89,11 +71,11 @@ __futex_clock_wait_bitset32 (int *futexp, int val, clockid_t clockid,
}
#endif /* ! __ASSUME_TIME64_SYSCALLS */
-int
-__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
- unsigned int expected, clockid_t clockid,
- const struct __timespec64* abstime,
- int private)
+static int
+__futex_abstimed_wait_common64 (unsigned int* futex_word,
+ unsigned int expected, clockid_t clockid,
+ const struct __timespec64* abstime,
+ int private, bool cancel)
{
unsigned int clockbit;
int err;
@@ -109,13 +91,18 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
- err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word, op, expected,
- abstime, NULL /* Unused. */,
+ if (cancel)
+ err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word, op, expected,
+ abstime, NULL /* Unused. */,
+ FUTEX_BITSET_MATCH_ANY);
+ else
+ err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, expected,
+ abstime, NULL /* Ununsed. */,
FUTEX_BITSET_MATCH_ANY);
#ifndef __ASSUME_TIME64_SYSCALLS
if (err == -ENOSYS)
- err = __futex_abstimed_wait_cancelable32 (futex_word, expected,
- op, abstime, private);
+ err = __futex_abstimed_wait_common32 (futex_word, expected, op, abstime,
+ private, cancel);
#endif
switch (err)
@@ -145,46 +132,18 @@ __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
clockid_t clockid,
const struct __timespec64* abstime, int private)
{
- unsigned int clockbit;
- int err;
-
- /* Work around the fact that the kernel rejects negative timeout values
- despite them being valid. */
- if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
- return ETIMEDOUT;
-
- if (! lll_futex_supported_clockid (clockid))
- return EINVAL;
-
- clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
- int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
-
- err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, expected,
- abstime, NULL /* Unused. */,
- FUTEX_BITSET_MATCH_ANY);
-#ifndef __ASSUME_TIME64_SYSCALLS
- if (err == -ENOSYS)
- err = __futex_abstimed_wait32 (futex_word, expected,
- clockid, abstime, private);
-#endif
- switch (err)
- {
- case 0:
- case -EAGAIN:
- case -EINTR:
- case -ETIMEDOUT:
- return -err;
+ return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
+ abstime, private, false);
+}
- case -EFAULT: /* Must have been caused by a glibc or application bug. */
- case -EINVAL: /* Either due to wrong alignment, unsupported
- clockid 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 ();
- }
+int
+__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
+ unsigned int expected, clockid_t clockid,
+ const struct __timespec64* abstime,
+ int private)
+{
+ return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
+ abstime, private, true);
}
int
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 05/13] linux: nptl: Consolidate __futex_abstimed_wait_{cancelable}64
2020-11-23 19:52 ` [PATCH 05/13] linux: nptl: Consolidate __futex_abstimed_wait_{cancelable}64 Adhemerval Zanella
@ 2020-11-24 18:19 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 18:19 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 7605 bytes --]
Hi Adhemerval,
> The only different is how to issue the syscall.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> sysdeps/nptl/futex-internal.c | 111
> +++++++++++----------------------- 1 file changed, 35 insertions(+),
> 76 deletions(-)
>
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index e4a14b477c..f9a2e28ee6 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -24,10 +24,10 @@
>
> #ifndef __ASSUME_TIME64_SYSCALLS
> static int
> -__futex_abstimed_wait_cancelable32 (unsigned int* futex_word,
> - unsigned int expected, int op,
> - const struct __timespec64*
> abstime,
> - int private)
> +__futex_abstimed_wait_common32 (unsigned int* futex_word,
> + unsigned int expected, int op,
> + const struct __timespec64* abstime,
> + int private, bool cancel)
> {
> struct timespec ts32, *pts32 = NULL;
> if (abstime != NULL)
> @@ -39,34 +39,16 @@ __futex_abstimed_wait_cancelable32 (unsigned int*
> futex_word, pts32 = &ts32;
> }
>
> - return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
> + if (cancel)
> + return INTERNAL_SYSCALL_CANCEL (futex, futex_word, op, expected,
> + pts32, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
> + else
> + return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> pts32, NULL /* Unused. */,
> FUTEX_BITSET_MATCH_ANY);
> }
>
> -static int
> -__futex_abstimed_wait32 (unsigned int* futex_word,
> - unsigned int expected, clockid_t clockid,
> - const struct __timespec64* abstime,
> - int private)
> -{
> - struct timespec ts32;
> -
> - if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> - return -EOVERFLOW;
> -
> - unsigned int clockbit = (clockid == CLOCK_REALTIME) ?
> - FUTEX_CLOCK_REALTIME : 0;
> - int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> private); -
> - if (abstime != NULL)
> - ts32 = valid_timespec64_to_timespec (*abstime);
> -
> - return INTERNAL_SYSCALL_CALL (futex, futex_word, op, expected,
> - abstime != NULL ? &ts32 : NULL,
> - NULL /* Unused. */,
> FUTEX_BITSET_MATCH_ANY); -}
> -
> static int
> __futex_clock_wait_bitset32 (int *futexp, int val, clockid_t clockid,
> const struct __timespec64 *abstime, int
> private) @@ -89,11 +71,11 @@ __futex_clock_wait_bitset32 (int
> *futexp, int val, clockid_t clockid, }
> #endif /* ! __ASSUME_TIME64_SYSCALLS */
>
> -int
> -__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> - unsigned int expected, clockid_t
> clockid,
> - const struct __timespec64*
> abstime,
> - int private)
> +static int
> +__futex_abstimed_wait_common64 (unsigned int* futex_word,
> + unsigned int expected, clockid_t
> clockid,
> + const struct __timespec64* abstime,
> + int private, bool cancel)
> {
> unsigned int clockbit;
> int err;
> @@ -109,13 +91,18 @@ __futex_abstimed_wait_cancelable64 (unsigned
> int* futex_word, clockbit = (clockid == CLOCK_REALTIME) ?
> FUTEX_CLOCK_REALTIME : 0; int op = __lll_private_flag
> (FUTEX_WAIT_BITSET | clockbit, private);
> - err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word, op,
> expected,
> - abstime, NULL /* Unused. */,
> + if (cancel)
> + err = INTERNAL_SYSCALL_CANCEL (futex_time64, futex_word, op,
> expected,
> + abstime, NULL /* Unused. */,
> + FUTEX_BITSET_MATCH_ANY);
> + else
> + err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op,
> expected,
> + abstime, NULL /* Ununsed. */,
> FUTEX_BITSET_MATCH_ANY);
> #ifndef __ASSUME_TIME64_SYSCALLS
> if (err == -ENOSYS)
> - err = __futex_abstimed_wait_cancelable32 (futex_word, expected,
> - op, abstime, private);
> + err = __futex_abstimed_wait_common32 (futex_word, expected, op,
> abstime,
> + private, cancel);
> #endif
>
> switch (err)
> @@ -145,46 +132,18 @@ __futex_abstimed_wait64 (unsigned int*
> futex_word, unsigned int expected, clockid_t clockid,
> const struct __timespec64* abstime, int
> private) {
> - unsigned int clockbit;
> - int err;
> -
> - /* Work around the fact that the kernel rejects negative timeout
> values
> - despite them being valid. */
> - if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
> - return ETIMEDOUT;
> -
> - if (! lll_futex_supported_clockid (clockid))
> - return EINVAL;
> -
> - clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> - int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> private); -
> - err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op,
> expected,
> - abstime, NULL /* Unused. */,
> - FUTEX_BITSET_MATCH_ANY);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> - if (err == -ENOSYS)
> - err = __futex_abstimed_wait32 (futex_word, expected,
> - clockid, abstime, private);
> -#endif
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - case -ETIMEDOUT:
> - return -err;
> + return __futex_abstimed_wait_common64 (futex_word, expected,
> clockid,
> + abstime, private, false);
> +}
>
> - case -EFAULT: /* Must have been caused by a glibc or application
> bug. */
> - case -EINVAL: /* Either due to wrong alignment, unsupported
> - clockid 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 ();
> - }
> +int
> +__futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
> + unsigned int expected, clockid_t
> clockid,
> + const struct __timespec64*
> abstime,
> + int private)
> +{
> + return __futex_abstimed_wait_common64 (futex_word, expected,
> clockid,
> + abstime, private, true);
> }
>
> int
Thanks for making this consolidation. Indeed there is now only
difference in issuing the syscall.
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 06/13] linux: nptl: Remove _futex_clock_wait_bitset64
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (3 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 05/13] linux: nptl: Consolidate __futex_abstimed_wait_{cancelable}64 Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 18:26 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64 Adhemerval Zanella
` (7 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
It can be replaced with a __futex_abstimed_wait64 call.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_mutex_timedlock.c | 7 +++---
sysdeps/nptl/futex-internal.c | 46 ----------------------------------
sysdeps/nptl/futex-internal.h | 5 ----
3 files changed, 4 insertions(+), 54 deletions(-)
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index de88e9fc25..0ec47359be 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -265,12 +265,13 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
assume_other_futex_waiters |= FUTEX_WAITERS;
/* Block using the futex. */
- int err = __futex_clock_wait_bitset64 (&mutex->__data.__lock,
+ int err = __futex_abstimed_wait64 (
+ (unsigned int *) &mutex->__data.__lock,
oldval, clockid, abstime,
PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
/* The futex call timed out. */
- if (err == -ETIMEDOUT)
- return -err;
+ if (err == ETIMEDOUT)
+ return err;
/* Reload current lock value. */
oldval = mutex->__data.__lock;
}
diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
index f9a2e28ee6..30c662547f 100644
--- a/sysdeps/nptl/futex-internal.c
+++ b/sysdeps/nptl/futex-internal.c
@@ -48,27 +48,6 @@ __futex_abstimed_wait_common32 (unsigned int* futex_word,
pts32, NULL /* Unused. */,
FUTEX_BITSET_MATCH_ANY);
}
-
-static int
-__futex_clock_wait_bitset32 (int *futexp, int val, clockid_t clockid,
- const struct __timespec64 *abstime, int private)
-{
- struct timespec ts32;
-
- if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
- return -EOVERFLOW;
-
- const unsigned int clockbit =
- (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
- const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
-
- if (abstime != NULL)
- ts32 = valid_timespec64_to_timespec (*abstime);
-
- return INTERNAL_SYSCALL_CALL (futex, futexp, op, val,
- abstime != NULL ? &ts32 : NULL,
- NULL /* Unused. */, FUTEX_BITSET_MATCH_ANY);
-}
#endif /* ! __ASSUME_TIME64_SYSCALLS */
static int
@@ -198,28 +177,3 @@ __futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
return -err;
}
-
-int
-__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t clockid,
- const struct __timespec64 *abstime,
- int private)
-{
- int ret;
- if (! lll_futex_supported_clockid (clockid))
- {
- return -EINVAL;
- }
-
- const unsigned int clockbit =
- (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
- const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit, private);
-
- ret = INTERNAL_SYSCALL_CALL (futex_time64, futexp, op, val,
- abstime, NULL /* Unused. */,
- FUTEX_BITSET_MATCH_ANY);
-#ifndef __ASSUME_TIME64_SYSCALLS
- if (ret == -ENOSYS)
- ret = __futex_clock_wait_bitset32 (futexp, val, clockid, abstime, private);
-#endif
- return ret;
-}
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index cefab74301..dcc7958d59 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -440,9 +440,4 @@ __futex_clocklock64 (int *futex, clockid_t clockid,
return err;
}
-int
-__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t clockid,
- const struct __timespec64 *abstime,
- int private) attribute_hidden;
-
#endif /* futex-internal.h */
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 06/13] linux: nptl: Remove _futex_clock_wait_bitset64
2020-11-23 19:52 ` [PATCH 06/13] linux: nptl: Remove _futex_clock_wait_bitset64 Adhemerval Zanella
@ 2020-11-24 18:26 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 18:26 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 4320 bytes --]
Hi Adhemerval,
> It can be replaced with a __futex_abstimed_wait64 call.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_mutex_timedlock.c | 7 +++---
> sysdeps/nptl/futex-internal.c | 46
> ---------------------------------- sysdeps/nptl/futex-internal.h |
> 5 ---- 3 files changed, 4 insertions(+), 54 deletions(-)
>
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index de88e9fc25..0ec47359be 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -265,12 +265,13 @@ __pthread_mutex_clocklock_common
> (pthread_mutex_t *mutex, assume_other_futex_waiters |= FUTEX_WAITERS;
>
> /* Block using the futex. */
> - int err = __futex_clock_wait_bitset64
> (&mutex->__data.__lock,
> + int err = __futex_abstimed_wait64 (
> + (unsigned int *) &mutex->__data.__lock,
> oldval, clockid, abstime,
> PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
> /* The futex call timed out. */
> - if (err == -ETIMEDOUT)
> - return -err;
> + if (err == ETIMEDOUT)
> + return err;
> /* Reload current lock value. */
> oldval = mutex->__data.__lock;
> }
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index f9a2e28ee6..30c662547f 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -48,27 +48,6 @@ __futex_abstimed_wait_common32 (unsigned int*
> futex_word, pts32, NULL /* Unused. */,
> FUTEX_BITSET_MATCH_ANY);
> }
> -
> -static int
> -__futex_clock_wait_bitset32 (int *futexp, int val, clockid_t clockid,
> - const struct __timespec64 *abstime, int
> private) -{
> - struct timespec ts32;
> -
> - if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
> - return -EOVERFLOW;
> -
> - const unsigned int clockbit =
> - (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> - const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> private); -
> - if (abstime != NULL)
> - ts32 = valid_timespec64_to_timespec (*abstime);
> -
> - return INTERNAL_SYSCALL_CALL (futex, futexp, op, val,
> - abstime != NULL ? &ts32 : NULL,
> - NULL /* Unused. */,
> FUTEX_BITSET_MATCH_ANY); -}
> #endif /* ! __ASSUME_TIME64_SYSCALLS */
>
> static int
> @@ -198,28 +177,3 @@ __futex_clocklock_wait64 (int *futex, int val,
> clockid_t clockid,
> return -err;
> }
> -
> -int
> -__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t clockid,
> - const struct __timespec64 *abstime,
> - int private)
> -{
> - int ret;
> - if (! lll_futex_supported_clockid (clockid))
> - {
> - return -EINVAL;
> - }
> -
> - const unsigned int clockbit =
> - (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
> - const int op = __lll_private_flag (FUTEX_WAIT_BITSET | clockbit,
> private); -
> - ret = INTERNAL_SYSCALL_CALL (futex_time64, futexp, op, val,
> - abstime, NULL /* Unused. */,
> - FUTEX_BITSET_MATCH_ANY);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> - if (ret == -ENOSYS)
> - ret = __futex_clock_wait_bitset32 (futexp, val, clockid,
> abstime, private); -#endif
> - return ret;
> -}
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index cefab74301..dcc7958d59 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -440,9 +440,4 @@ __futex_clocklock64 (int *futex, clockid_t
> clockid, return err;
> }
>
> -int
> -__futex_clock_wait_bitset64 (int *futexp, int val, clockid_t clockid,
> - const struct __timespec64 *abstime,
> - int private) attribute_hidden;
> -
> #endif /* futex-internal.h */
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (4 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 06/13] linux: nptl: Remove _futex_clock_wait_bitset64 Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:28 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 08/13] linux: nptl: Replace lll_timedwait " Adhemerval Zanella
` (6 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The __futex_abstimed_wait64 needs also to return EINVAL syscall
errors.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_mutex_timedlock.c | 4 +--
sysdeps/nptl/futex-internal.c | 57 +---------------------------------
sysdeps/nptl/futex-internal.h | 7 ++---
3 files changed, 5 insertions(+), 63 deletions(-)
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 0ec47359be..e643eab258 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -391,8 +391,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
/* Delay the thread until the timeout is reached. Then return
ETIMEDOUT. */
do
- e = __futex_clocklock_wait64 (&(int){0}, 0, clockid, abstime,
- private);
+ e = __futex_abstimed_wait64 (&(unsigned int){0}, 0, clockid,
+ abstime, private);
while (e != ETIMEDOUT);
return ETIMEDOUT;
}
diff --git a/sysdeps/nptl/futex-internal.c b/sysdeps/nptl/futex-internal.c
index 30c662547f..11031cc46a 100644
--- a/sysdeps/nptl/futex-internal.c
+++ b/sysdeps/nptl/futex-internal.c
@@ -90,15 +90,13 @@ __futex_abstimed_wait_common64 (unsigned int* futex_word,
case -EAGAIN:
case -EINTR:
case -ETIMEDOUT:
+ case -EINVAL:
case -EOVERFLOW: /* Passed absolute 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:
@@ -124,56 +122,3 @@ __futex_abstimed_wait_cancelable64 (unsigned int* futex_word,
return __futex_abstimed_wait_common64 (futex_word, expected, clockid,
abstime, private, true);
}
-
-int
-__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
- const struct __timespec64 *abstime, int private)
-{
- struct __timespec64 ts, *tsp = NULL;
-
- if (abstime != NULL)
- {
- /* Reject invalid timeouts. */
- if (! valid_nanoseconds (abstime->tv_nsec))
- return EINVAL;
-
- /* Get the current time. This can only fail if clockid is not valid. */
- if (__glibc_unlikely (__clock_gettime64 (clockid, &ts) != 0))
- return EINVAL;
-
- /* Compute relative timeout. */
- ts.tv_sec = abstime->tv_sec - ts.tv_sec;
- ts.tv_nsec = abstime->tv_nsec - ts.tv_nsec;
- if (ts.tv_nsec < 0)
- {
- ts.tv_nsec += 1000000000;
- --ts.tv_sec;
- }
-
- if (ts.tv_sec < 0)
- return ETIMEDOUT;
-
- tsp = &ts;
- }
-
- int err = INTERNAL_SYSCALL_CALL (futex_time64, futex,
- __lll_private_flag (FUTEX_WAIT, private),
- val, tsp);
-#ifndef __ASSUME_TIME64_SYSCALLS
- if (err == -ENOSYS)
- {
- if (tsp != NULL && ! in_time_t_range (tsp->tv_sec))
- return EOVERFLOW;
-
- struct timespec ts32;
- if (tsp != NULL)
- ts32 = valid_timespec64_to_timespec (*tsp);
-
- err = INTERNAL_SYSCALL_CALL (futex, futex,
- __lll_private_flag (FUTEX_WAIT, private),
- val, tsp != NULL ? &ts32 : NULL);
- }
-#endif
-
- return -err;
-}
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index dcc7958d59..632051278b 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -417,10 +417,6 @@ __futex_abstimed_wait64 (unsigned int* futex_word, unsigned int expected,
const struct __timespec64* abstime,
int private) attribute_hidden;
-int
-__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
- const struct __timespec64 *abstime,
- int private) attribute_hidden;
static __always_inline int
__futex_clocklock64 (int *futex, clockid_t clockid,
@@ -432,7 +428,8 @@ __futex_clocklock64 (int *futex, clockid_t clockid,
{
while (atomic_exchange_acq (futex, 2) != 0)
{
- err = __futex_clocklock_wait64 (futex, 2, clockid, abstime, private);
+ err = __futex_abstimed_wait64 ((unsigned int *) futex, 2, clockid,
+ abstime, private);
if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW)
break;
}
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64
2020-11-23 19:52 ` [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64 Adhemerval Zanella
@ 2020-11-24 21:28 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:28 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 5537 bytes --]
Hi Adhemerval,
> The __futex_abstimed_wait64 needs also to return EINVAL syscall
> errors.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_mutex_timedlock.c | 4 +--
> sysdeps/nptl/futex-internal.c | 57
> +--------------------------------- sysdeps/nptl/futex-internal.h |
> 7 ++--- 3 files changed, 5 insertions(+), 63 deletions(-)
>
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index 0ec47359be..e643eab258 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -391,8 +391,8 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, /* Delay the thread until the timeout is reached. Then return
> ETIMEDOUT. */
> do
> - e = __futex_clocklock_wait64 (&(int){0}, 0,
> clockid, abstime,
> - private);
> + e = __futex_abstimed_wait64 (&(unsigned int){0},
> 0, clockid,
> + abstime, private);
I think that it would be nice to have more verbose commit message - to
clearly explain why it is possible to replace "*_clocklock_*" with
"*_abstimed_*".
Reviewed-by: Lukasz Majewski <lukma@denx.de>
> while (e != ETIMEDOUT);
> return ETIMEDOUT;
> }
> diff --git a/sysdeps/nptl/futex-internal.c
> b/sysdeps/nptl/futex-internal.c index 30c662547f..11031cc46a 100644
> --- a/sysdeps/nptl/futex-internal.c
> +++ b/sysdeps/nptl/futex-internal.c
> @@ -90,15 +90,13 @@ __futex_abstimed_wait_common64 (unsigned int*
> futex_word, case -EAGAIN:
> case -EINTR:
> case -ETIMEDOUT:
> + case -EINVAL:
> case -EOVERFLOW: /* Passed absolute 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:
> @@ -124,56 +122,3 @@ __futex_abstimed_wait_cancelable64 (unsigned
> int* futex_word, return __futex_abstimed_wait_common64 (futex_word,
> expected, clockid, abstime, private, true);
> }
> -
> -int
> -__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
> - const struct __timespec64 *abstime, int
> private) -{
> - struct __timespec64 ts, *tsp = NULL;
> -
> - if (abstime != NULL)
> - {
> - /* Reject invalid timeouts. */
> - if (! valid_nanoseconds (abstime->tv_nsec))
> - return EINVAL;
> -
> - /* Get the current time. This can only fail if clockid is not
> valid. */
> - if (__glibc_unlikely (__clock_gettime64 (clockid, &ts) != 0))
> - return EINVAL;
> -
> - /* Compute relative timeout. */
> - ts.tv_sec = abstime->tv_sec - ts.tv_sec;
> - ts.tv_nsec = abstime->tv_nsec - ts.tv_nsec;
> - if (ts.tv_nsec < 0)
> - {
> - ts.tv_nsec += 1000000000;
> - --ts.tv_sec;
> - }
> -
> - if (ts.tv_sec < 0)
> - return ETIMEDOUT;
> -
> - tsp = &ts;
> - }
> -
> - int err = INTERNAL_SYSCALL_CALL (futex_time64, futex,
> - __lll_private_flag (FUTEX_WAIT,
> private),
> - val, tsp);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> - if (err == -ENOSYS)
> - {
> - if (tsp != NULL && ! in_time_t_range (tsp->tv_sec))
> - return EOVERFLOW;
> -
> - struct timespec ts32;
> - if (tsp != NULL)
> - ts32 = valid_timespec64_to_timespec (*tsp);
> -
> - err = INTERNAL_SYSCALL_CALL (futex, futex,
> - __lll_private_flag (FUTEX_WAIT,
> private),
> - val, tsp != NULL ? &ts32 : NULL);
> - }
> -#endif
> -
> - return -err;
> -}
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index dcc7958d59..632051278b 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -417,10 +417,6 @@ __futex_abstimed_wait64 (unsigned int*
> futex_word, unsigned int expected, const struct __timespec64* abstime,
> int private) attribute_hidden;
>
> -int
> -__futex_clocklock_wait64 (int *futex, int val, clockid_t clockid,
> - const struct __timespec64 *abstime,
> - int private) attribute_hidden;
>
> static __always_inline int
> __futex_clocklock64 (int *futex, clockid_t clockid,
> @@ -432,7 +428,8 @@ __futex_clocklock64 (int *futex, clockid_t
> clockid, {
> while (atomic_exchange_acq (futex, 2) != 0)
> {
> - err = __futex_clocklock_wait64 (futex, 2, clockid,
> abstime, private);
> + err = __futex_abstimed_wait64 ((unsigned int *) futex, 2,
> clockid,
> + abstime, private);
> if (err == EINVAL || err == ETIMEDOUT || err == EOVERFLOW)
> break;
> }
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] 40+ messages in thread
* [PATCH 08/13] linux: nptl: Replace lll_timedwait with __futex_abstimed_wait64
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (5 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 07/13] linux: nptl: Replace __futex_clocklock_wait64 with __futex_abstimed_wait64 Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:29 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Adhemerval Zanella
` (5 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
Checked with x86_64-linux-gnu and i686-linux-gnu.
---
nptl/Makefile | 1 -
nptl/lll_timedlock_wait.c | 62 -------------------------------------
nptl/pthread_mutex_lock.c | 4 +--
sysdeps/nptl/lowlevellock.h | 32 -------------------
4 files changed, 2 insertions(+), 97 deletions(-)
delete mode 100644 nptl/lll_timedlock_wait.c
diff --git a/nptl/Makefile b/nptl/Makefile
index 74ab758c12..968768d33b 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -147,7 +147,6 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \
pt-longjmp pt-cleanup\
cancellation \
lowlevellock \
- lll_timedlock_wait \
pt-fork pt-fcntl \
$(pthread-compat-wrappers) \
pt-raise pt-system \
diff --git a/nptl/lll_timedlock_wait.c b/nptl/lll_timedlock_wait.c
deleted file mode 100644
index eabdca70c8..0000000000
--- a/nptl/lll_timedlock_wait.c
+++ /dev/null
@@ -1,62 +0,0 @@
-/* Timed low level locking for pthread library. Generic futex-using version.
- Copyright (C) 2003-2020 Free Software Foundation, Inc.
- This file is part of the GNU C Library.
- Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003.
-
- 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/>. */
-
-#include <atomic.h>
-#include <errno.h>
-#include <lowlevellock.h>
-#include <sys/time.h>
-#include <time.h>
-
-
-int
-__lll_clocklock_wait (int *futex, int val, clockid_t clockid,
- const struct timespec *abstime, int private)
-{
- struct timespec ts, *tsp = NULL;
-
- if (abstime != NULL)
- {
- /* Reject invalid timeouts. */
- if (! valid_nanoseconds (abstime->tv_nsec))
- return EINVAL;
-
- /* Get the current time. This can only fail if clockid is not valid. */
- if (__glibc_unlikely (__clock_gettime (clockid, &ts) != 0))
- return EINVAL;
-
- /* Compute relative timeout. */
- ts.tv_sec = abstime->tv_sec - ts.tv_sec;
- ts.tv_nsec = abstime->tv_nsec - ts.tv_nsec;
- if (ts.tv_nsec < 0)
- {
- ts.tv_nsec += 1000000000;
- --ts.tv_sec;
- }
-
- if (ts.tv_sec < 0)
- return ETIMEDOUT;
-
- tsp = &ts;
- }
-
- /* If *futex == val, wait until woken or timeout. */
- lll_futex_timed_wait (futex, val, tsp, private);
-
- return 0;
-}
diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index 0439002454..1f137f6201 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -425,8 +425,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
/* Delay the thread indefinitely. */
while (1)
- lll_timedwait (&(int){0}, 0, 0 /* ignored */, NULL,
- private);
+ __futex_abstimed_wait64 (&(unsigned int){0}, 0,
+ 0 /* ignored */, NULL, private);
}
oldval = mutex->__data.__lock;
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 68b3be8819..3de87d31a9 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -122,38 +122,6 @@ extern void __lll_lock_wait (int *futex, int private) attribute_hidden;
#define lll_cond_lock(futex, private) __lll_cond_lock (&(futex), private)
-extern int __lll_clocklock_wait (int *futex, int val, clockid_t,
- const struct timespec *,
- int private) attribute_hidden;
-
-#define lll_timedwait(futex, val, clockid, abstime, private) \
- __lll_clocklock_wait (futex, val, clockid, abstime, private)
-
-/* As __lll_lock, but with an absolute timeout measured against the clock
- specified in CLOCKID. If the timeout occurs then return ETIMEDOUT. If
- ABSTIME is invalid, return EINVAL. */
-#define __lll_clocklock(futex, clockid, abstime, private) \
- ({ \
- int *__futex = (futex); \
- int __val = 0; \
- \
- if (__glibc_unlikely \
- (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \
- { \
- while (atomic_exchange_acq (futex, 2) != 0) \
- { \
- __val = __lll_clocklock_wait (__futex, 2, clockid, \
- abstime, private); \
- if (__val == EINVAL || __val == ETIMEDOUT) \
- break; \
- } \
- } \
- __val; \
- })
-#define lll_clocklock(futex, clockid, abstime, private) \
- __lll_clocklock (&(futex), clockid, abstime, private)
-
-
/* This is an expression rather than a statement even though its value is
void, so that it can be used in a comma expression or as an expression
that's cast to void. */
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 08/13] linux: nptl: Replace lll_timedwait with __futex_abstimed_wait64
2020-11-23 19:52 ` [PATCH 08/13] linux: nptl: Replace lll_timedwait " Adhemerval Zanella
@ 2020-11-24 21:29 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:29 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 6147 bytes --]
Hi Adhemerval,
> Checked with x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/Makefile | 1 -
> nptl/lll_timedlock_wait.c | 62
> ------------------------------------- nptl/pthread_mutex_lock.c |
> 4 +-- sysdeps/nptl/lowlevellock.h | 32 -------------------
> 4 files changed, 2 insertions(+), 97 deletions(-)
> delete mode 100644 nptl/lll_timedlock_wait.c
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 74ab758c12..968768d33b 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -147,7 +147,6 @@ libpthread-routines = nptl-init nptlfreeres vars
> events version pt-interp \ pt-longjmp pt-cleanup\
> cancellation \
> lowlevellock \
> - lll_timedlock_wait \
> pt-fork pt-fcntl \
> $(pthread-compat-wrappers) \
> pt-raise pt-system \
> diff --git a/nptl/lll_timedlock_wait.c b/nptl/lll_timedlock_wait.c
> deleted file mode 100644
> index eabdca70c8..0000000000
> --- a/nptl/lll_timedlock_wait.c
> +++ /dev/null
> @@ -1,62 +0,0 @@
> -/* Timed low level locking for pthread library. Generic futex-using
> version.
> - Copyright (C) 2003-2020 Free Software Foundation, Inc.
> - This file is part of the GNU C Library.
> - Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003.
> -
> - 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/>. */
> -
> -#include <atomic.h>
> -#include <errno.h>
> -#include <lowlevellock.h>
> -#include <sys/time.h>
> -#include <time.h>
> -
> -
> -int
> -__lll_clocklock_wait (int *futex, int val, clockid_t clockid,
> - const struct timespec *abstime, int private)
> -{
> - struct timespec ts, *tsp = NULL;
> -
> - if (abstime != NULL)
> - {
> - /* Reject invalid timeouts. */
> - if (! valid_nanoseconds (abstime->tv_nsec))
> - return EINVAL;
> -
> - /* Get the current time. This can only fail if clockid is not
> valid. */
> - if (__glibc_unlikely (__clock_gettime (clockid, &ts) != 0))
> - return EINVAL;
> -
> - /* Compute relative timeout. */
> - ts.tv_sec = abstime->tv_sec - ts.tv_sec;
> - ts.tv_nsec = abstime->tv_nsec - ts.tv_nsec;
> - if (ts.tv_nsec < 0)
> - {
> - ts.tv_nsec += 1000000000;
> - --ts.tv_sec;
> - }
> -
> - if (ts.tv_sec < 0)
> - return ETIMEDOUT;
> -
> - tsp = &ts;
> - }
> -
> - /* If *futex == val, wait until woken or timeout. */
> - lll_futex_timed_wait (futex, val, tsp, private);
> -
> - return 0;
> -}
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index 0439002454..1f137f6201 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -425,8 +425,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
>
> /* Delay the thread indefinitely. */
> while (1)
> - lll_timedwait (&(int){0}, 0, 0 /* ignored */, NULL,
> - private);
> + __futex_abstimed_wait64 (&(unsigned int){0}, 0,
> + 0 /* ignored */, NULL,
> private); }
>
> oldval = mutex->__data.__lock;
> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
> index 68b3be8819..3de87d31a9 100644
> --- a/sysdeps/nptl/lowlevellock.h
> +++ b/sysdeps/nptl/lowlevellock.h
> @@ -122,38 +122,6 @@ extern void __lll_lock_wait (int *futex, int
> private) attribute_hidden; #define lll_cond_lock(futex, private)
> __lll_cond_lock (&(futex), private)
>
> -extern int __lll_clocklock_wait (int *futex, int val, clockid_t,
> - const struct timespec *,
> - int private) attribute_hidden;
> -
> -#define lll_timedwait(futex, val, clockid, abstime, private)
> \
> - __lll_clocklock_wait (futex, val, clockid, abstime, private)
> -
> -/* As __lll_lock, but with an absolute timeout measured against the
> clock
> - specified in CLOCKID. If the timeout occurs then return
> ETIMEDOUT. If
> - ABSTIME is invalid, return EINVAL. */
> -#define __lll_clocklock(futex, clockid, abstime, private) \
> - ({ \
> - int *__futex = (futex); \
> - int __val = 0; \
> - \
> - if (__glibc_unlikely \
> - (atomic_compare_and_exchange_bool_acq (__futex, 1, 0))) \
> - {
> \
> - while (atomic_exchange_acq (futex, 2) != 0) \
> - { \
> - __val = __lll_clocklock_wait (__futex, 2, clockid,
> \
> - abstime, private); \
> - if (__val == EINVAL || __val == ETIMEDOUT)
> \
> - break; \
> - } \
> - }
> \
> - __val; \
> - })
> -#define lll_clocklock(futex, clockid, abstime, private) \
> - __lll_clocklock (&(futex), clockid, abstime, private)
> -
> -
> /* This is an expression rather than a statement even though its
> value is void, so that it can be used in a comma expression or as an
> expression that's cast to void. */
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (6 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 08/13] linux: nptl: Replace lll_timedwait " Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:35 ` Lukasz Majewski
2020-11-25 15:32 ` Mike Crowe
2020-11-23 19:52 ` [PATCH 10/13] linux: nptl: Replace lll_futex_supported_clockid with futex-internal.h Adhemerval Zanella
` (4 subsequent siblings)
12 siblings, 2 replies; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The idea is to make NPTL implementation to use on the functions
provided by futex-internal.h.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/lowlevellock.c | 6 +++---
nptl/pthread_mutex_lock.c | 9 +++++----
nptl/pthread_mutex_setprioceiling.c | 5 +++--
nptl/pthread_mutex_timedlock.c | 6 +++---
4 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
index f69547a235..973df4d03a 100644
--- a/nptl/lowlevellock.c
+++ b/nptl/lowlevellock.c
@@ -18,7 +18,7 @@
<https://www.gnu.org/licenses/>. */
#include <sysdep.h>
-#include <lowlevellock.h>
+#include <futex-internal.h>
#include <atomic.h>
#include <stap-probe.h>
@@ -32,7 +32,7 @@ __lll_lock_wait_private (int *futex)
{
futex:
LIBC_PROBE (lll_lock_wait_private, 1, futex);
- lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */
+ futex_wait ((unsigned int *) futex, 2, LLL_PRIVATE); /* Wait if *futex == 2. */
}
}
@@ -49,7 +49,7 @@ __lll_lock_wait (int *futex, int private)
{
futex:
LIBC_PROBE (lll_lock_wait, 1, futex);
- lll_futex_wait (futex, 2, private); /* Wait if *futex == 2. */
+ futex_wait ((unsigned int *) futex, 2, private); /* Wait if *futex == 2. */
}
}
#endif
diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
index 1f137f6201..965c5a3f4a 100644
--- a/nptl/pthread_mutex_lock.c
+++ b/nptl/pthread_mutex_lock.c
@@ -307,8 +307,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
assume_other_futex_waiters |= FUTEX_WAITERS;
/* Block using the futex and reload current lock value. */
- lll_futex_wait (&mutex->__data.__lock, oldval,
- PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
+ futex_wait ((unsigned int *) &mutex->__data.__lock, oldval,
+ PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
oldval = mutex->__data.__lock;
}
@@ -568,8 +568,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
break;
if (oldval != ceilval)
- lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
- PTHREAD_MUTEX_PSHARED (mutex));
+ futex_wait ((unsigned int * ) &mutex->__data.__lock,
+ ceilval | 2,
+ PTHREAD_MUTEX_PSHARED (mutex));
}
while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
ceilval | 2, ceilval)
diff --git a/nptl/pthread_mutex_setprioceiling.c b/nptl/pthread_mutex_setprioceiling.c
index 521da72622..cbef202579 100644
--- a/nptl/pthread_mutex_setprioceiling.c
+++ b/nptl/pthread_mutex_setprioceiling.c
@@ -21,6 +21,7 @@
#include <errno.h>
#include <pthreadP.h>
#include <atomic.h>
+#include <futex-internal.h>
int
@@ -84,8 +85,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t *mutex, int prioceiling,
break;
if (oldval != ceilval)
- lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
- PTHREAD_MUTEX_PSHARED (mutex));
+ futex_wait ((unsigned int *) &mutex->__data.__lock, ceilval | 2,
+ PTHREAD_MUTEX_PSHARED (mutex));
}
while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
ceilval | 2, ceilval)
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index e643eab258..343acf6107 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
goto failpp;
}
- lll_futex_timed_wait (&mutex->__data.__lock,
- ceilval | 2, &rt,
- PTHREAD_MUTEX_PSHARED (mutex));
+ __futex_abstimed_wait64 (
+ (unsigned int *) &mutex->__data.__lock, clockid,
+ ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
}
}
while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-23 19:52 ` [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Adhemerval Zanella
@ 2020-11-24 21:35 ` Lukasz Majewski
2020-11-25 15:32 ` Mike Crowe
1 sibling, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:35 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 4430 bytes --]
Hi Adhemerval,
> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/lowlevellock.c | 6 +++---
> nptl/pthread_mutex_lock.c | 9 +++++----
> nptl/pthread_mutex_setprioceiling.c | 5 +++--
> nptl/pthread_mutex_timedlock.c | 6 +++---
> 4 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/nptl/lowlevellock.c b/nptl/lowlevellock.c
> index f69547a235..973df4d03a 100644
> --- a/nptl/lowlevellock.c
> +++ b/nptl/lowlevellock.c
> @@ -18,7 +18,7 @@
> <https://www.gnu.org/licenses/>. */
>
> #include <sysdep.h>
> -#include <lowlevellock.h>
> +#include <futex-internal.h>
> #include <atomic.h>
> #include <stap-probe.h>
>
> @@ -32,7 +32,7 @@ __lll_lock_wait_private (int *futex)
> {
> futex:
> LIBC_PROBE (lll_lock_wait_private, 1, futex);
> - lll_futex_wait (futex, 2, LLL_PRIVATE); /* Wait if *futex ==
> 2. */
> + futex_wait ((unsigned int *) futex, 2, LLL_PRIVATE); /* Wait
> if *futex == 2. */ }
> }
>
> @@ -49,7 +49,7 @@ __lll_lock_wait (int *futex, int private)
> {
> futex:
> LIBC_PROBE (lll_lock_wait, 1, futex);
> - lll_futex_wait (futex, 2, private); /* Wait if *futex == 2. */
> + futex_wait ((unsigned int *) futex, 2, private); /* Wait if
> *futex == 2. */ }
> }
> #endif
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index 1f137f6201..965c5a3f4a 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -307,8 +307,8 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
> assume_other_futex_waiters |= FUTEX_WAITERS;
>
> /* Block using the futex and reload current lock value. */
> - lll_futex_wait (&mutex->__data.__lock, oldval,
> - PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
> + futex_wait ((unsigned int *) &mutex->__data.__lock, oldval,
> + PTHREAD_ROBUST_MUTEX_PSHARED (mutex));
> oldval = mutex->__data.__lock;
> }
>
> @@ -568,8 +568,9 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
> break;
>
> if (oldval != ceilval)
> - lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
> - PTHREAD_MUTEX_PSHARED (mutex));
> + futex_wait ((unsigned int * )
> &mutex->__data.__lock,
> + ceilval | 2,
> + PTHREAD_MUTEX_PSHARED (mutex));
> }
> while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock, ceilval | 2, ceilval)
> diff --git a/nptl/pthread_mutex_setprioceiling.c
> b/nptl/pthread_mutex_setprioceiling.c index 521da72622..cbef202579
> 100644 --- a/nptl/pthread_mutex_setprioceiling.c
> +++ b/nptl/pthread_mutex_setprioceiling.c
> @@ -21,6 +21,7 @@
> #include <errno.h>
> #include <pthreadP.h>
> #include <atomic.h>
> +#include <futex-internal.h>
>
>
> int
> @@ -84,8 +85,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t
> *mutex, int prioceiling, break;
>
> if (oldval != ceilval)
> - lll_futex_wait (&mutex->__data.__lock, ceilval | 2,
> - PTHREAD_MUTEX_PSHARED (mutex));
> + futex_wait ((unsigned int *) &mutex->__data.__lock,
> ceilval | 2,
> + PTHREAD_MUTEX_PSHARED (mutex));
> }
> while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock, ceilval | 2, ceilval)
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index e643eab258..343acf6107 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t
> *mutex, goto failpp;
> }
>
> - lll_futex_timed_wait (&mutex->__data.__lock,
> - ceilval | 2, &rt,
> - PTHREAD_MUTEX_PSHARED
> (mutex));
> + __futex_abstimed_wait64 (
> + (unsigned int *) &mutex->__data.__lock,
> clockid,
> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED
> (mutex)); }
> }
> while (atomic_compare_and_exchange_val_acq
> (&mutex->__data.__lock,
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-23 19:52 ` [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Adhemerval Zanella
2020-11-24 21:35 ` Lukasz Majewski
@ 2020-11-25 15:32 ` Mike Crowe
2020-11-25 15:40 ` Adhemerval Zanella
1 sibling, 1 reply; 40+ messages in thread
From: Mike Crowe @ 2020-11-25 15:32 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Michael Kerrisk
On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/lowlevellock.c | 6 +++---
> nptl/pthread_mutex_lock.c | 9 +++++----
> nptl/pthread_mutex_setprioceiling.c | 5 +++--
> nptl/pthread_mutex_timedlock.c | 6 +++---
> 4 files changed, 14 insertions(+), 12 deletions(-)
[snip]
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index e643eab258..343acf6107 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> goto failpp;
> }
>
> - lll_futex_timed_wait (&mutex->__data.__lock,
> - ceilval | 2, &rt,
> - PTHREAD_MUTEX_PSHARED (mutex));
> + __futex_abstimed_wait64 (
> + (unsigned int *) &mutex->__data.__lock, clockid,
> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
I think you've replaced the lll_futex_timed_wait call that expects a
relative timeout with a __futex_abstimed_wait64 call that expects an
absolute timeout, yet you still appear to be passing the relative timeout.
However, it turns out that the implementation for the
PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
completely broken with clockid != CLOCK_REALTIME ever since I added it in
9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
time this was a less obvious __gettimeofday call.)
I'll work on writing some test cases for the those types of mutex in the
hope of catching both flaws before fixing them.
Mike.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-25 15:32 ` Mike Crowe
@ 2020-11-25 15:40 ` Adhemerval Zanella
2020-11-25 15:46 ` Mike Crowe
0 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-25 15:40 UTC (permalink / raw)
To: Mike Crowe; +Cc: libc-alpha, Michael Kerrisk
On 25/11/2020 12:32, Mike Crowe wrote:
> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>> The idea is to make NPTL implementation to use on the functions
>> provided by futex-internal.h.
>>
>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>> ---
>> nptl/lowlevellock.c | 6 +++---
>> nptl/pthread_mutex_lock.c | 9 +++++----
>> nptl/pthread_mutex_setprioceiling.c | 5 +++--
>> nptl/pthread_mutex_timedlock.c | 6 +++---
>> 4 files changed, 14 insertions(+), 12 deletions(-)
>
> [snip]
>
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index e643eab258..343acf6107 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>> goto failpp;
>> }
>>
>> - lll_futex_timed_wait (&mutex->__data.__lock,
>> - ceilval | 2, &rt,
>> - PTHREAD_MUTEX_PSHARED (mutex));
>> + __futex_abstimed_wait64 (
>> + (unsigned int *) &mutex->__data.__lock, clockid,
>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>
> I think you've replaced the lll_futex_timed_wait call that expects a
> relative timeout with a __futex_abstimed_wait64 call that expects an
> absolute timeout, yet you still appear to be passing the relative timeout.
>
> However, it turns out that the implementation for the
> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> completely broken with clockid != CLOCK_REALTIME ever since I added it in
> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> time this was a less obvious __gettimeofday call.)
>
> I'll work on writing some test cases for the those types of mutex in the
> hope of catching both flaws before fixing them.
Indeed, there is no need to calculate the relative timeout anymore. I think
the fix below should pass the absolute timeout directly. I will check
a possible regression tests as well.
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index aaaafa21ce..86c5f4446e 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
if (__pthread_current_priority () > ceiling)
{
result = EINVAL;
- failpp:
if (oldprio != -1)
__pthread_tpp_change_priority (oldprio, -1);
return result;
@@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
if (oldval != ceilval)
{
- /* Reject invalid timeouts. */
- if (! valid_nanoseconds (abstime->tv_nsec))
- {
- result = EINVAL;
- goto failpp;
- }
-
- struct __timespec64 rt;
-
- /* Get the current time. */
- __clock_gettime64 (CLOCK_REALTIME, &rt);
-
- /* Compute relative timeout. */
- rt.tv_sec = abstime->tv_sec - rt.tv_sec;
- rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
- if (rt.tv_nsec < 0)
- {
- rt.tv_nsec += 1000000000;
- --rt.tv_sec;
- }
-
- /* Already timed out? */
- if (rt.tv_sec < 0)
- {
- result = ETIMEDOUT;
- goto failpp;
- }
-
__futex_abstimed_wait64 (
(unsigned int *) &mutex->__data.__lock, clockid,
- ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
+ ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
}
}
while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-25 15:40 ` Adhemerval Zanella
@ 2020-11-25 15:46 ` Mike Crowe
2020-11-25 17:19 ` Adhemerval Zanella
0 siblings, 1 reply; 40+ messages in thread
From: Mike Crowe @ 2020-11-25 15:46 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Michael Kerrisk
On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
>
>
> On 25/11/2020 12:32, Mike Crowe wrote:
> > On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> >> The idea is to make NPTL implementation to use on the functions
> >> provided by futex-internal.h.
> >>
> >> Checked on x86_64-linux-gnu and i686-linux-gnu.
> >> ---
> >> nptl/lowlevellock.c | 6 +++---
> >> nptl/pthread_mutex_lock.c | 9 +++++----
> >> nptl/pthread_mutex_setprioceiling.c | 5 +++--
> >> nptl/pthread_mutex_timedlock.c | 6 +++---
> >> 4 files changed, 14 insertions(+), 12 deletions(-)
> >
> > [snip]
> >
> >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >> index e643eab258..343acf6107 100644
> >> --- a/nptl/pthread_mutex_timedlock.c
> >> +++ b/nptl/pthread_mutex_timedlock.c
> >> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >> goto failpp;
> >> }
> >>
> >> - lll_futex_timed_wait (&mutex->__data.__lock,
> >> - ceilval | 2, &rt,
> >> - PTHREAD_MUTEX_PSHARED (mutex));
> >> + __futex_abstimed_wait64 (
> >> + (unsigned int *) &mutex->__data.__lock, clockid,
> >> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> >
> > I think you've replaced the lll_futex_timed_wait call that expects a
> > relative timeout with a __futex_abstimed_wait64 call that expects an
> > absolute timeout, yet you still appear to be passing the relative timeout.
> >
> > However, it turns out that the implementation for the
> > PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> > completely broken with clockid != CLOCK_REALTIME ever since I added it in
> > 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> > is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> > time this was a less obvious __gettimeofday call.)
> >
> > I'll work on writing some test cases for the those types of mutex in the
> > hope of catching both flaws before fixing them.
>
> Indeed, there is no need to calculate the relative timeout anymore. I think
> the fix below should pass the absolute timeout directly. I will check
> a possible regression tests as well.
OK. I won't then. Thanks.
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index aaaafa21ce..86c5f4446e 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> if (__pthread_current_priority () > ceiling)
> {
> result = EINVAL;
> - failpp:
> if (oldprio != -1)
> __pthread_tpp_change_priority (oldprio, -1);
> return result;
> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>
> if (oldval != ceilval)
> {
> - /* Reject invalid timeouts. */
> - if (! valid_nanoseconds (abstime->tv_nsec))
> - {
> - result = EINVAL;
> - goto failpp;
> - }
If this is removed then is there a risk of getting into a busy loop if
someone passes a bogus timespec? (Regardless of the answer, it makes sense
to ensure that is tested somehow.)
> -
> - struct __timespec64 rt;
> -
> - /* Get the current time. */
> - __clock_gettime64 (CLOCK_REALTIME, &rt);
> -
> - /* Compute relative timeout. */
> - rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> - rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> - if (rt.tv_nsec < 0)
> - {
> - rt.tv_nsec += 1000000000;
> - --rt.tv_sec;
> - }
> -
> - /* Already timed out? */
> - if (rt.tv_sec < 0)
> - {
> - result = ETIMEDOUT;
> - goto failpp;
> - }
> -
> __futex_abstimed_wait64 (
> (unsigned int *) &mutex->__data.__lock, clockid,
> - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
> }
> }
> while (atomic_compare_and_exchange_val_acq (&mutex->__data.__lock,
LGTM.
Thanks.
Mike.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-25 15:46 ` Mike Crowe
@ 2020-11-25 17:19 ` Adhemerval Zanella
2020-11-25 17:37 ` Mike Crowe
0 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-25 17:19 UTC (permalink / raw)
To: Mike Crowe; +Cc: libc-alpha, Michael Kerrisk
On 25/11/2020 12:46, Mike Crowe wrote:
> On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
>>
>>
>> On 25/11/2020 12:32, Mike Crowe wrote:
>>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>>>> The idea is to make NPTL implementation to use on the functions
>>>> provided by futex-internal.h.
>>>>
>>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>>>> ---
>>>> nptl/lowlevellock.c | 6 +++---
>>>> nptl/pthread_mutex_lock.c | 9 +++++----
>>>> nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>>> nptl/pthread_mutex_timedlock.c | 6 +++---
>>>> 4 files changed, 14 insertions(+), 12 deletions(-)
>>>
>>> [snip]
>>>
>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>> index e643eab258..343acf6107 100644
>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>> goto failpp;
>>>> }
>>>>
>>>> - lll_futex_timed_wait (&mutex->__data.__lock,
>>>> - ceilval | 2, &rt,
>>>> - PTHREAD_MUTEX_PSHARED (mutex));
>>>> + __futex_abstimed_wait64 (
>>>> + (unsigned int *) &mutex->__data.__lock, clockid,
>>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>>>
>>> I think you've replaced the lll_futex_timed_wait call that expects a
>>> relative timeout with a __futex_abstimed_wait64 call that expects an
>>> absolute timeout, yet you still appear to be passing the relative timeout.
>>>
>>> However, it turns out that the implementation for the
>>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
>>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
>>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
>>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
>>> time this was a less obvious __gettimeofday call.)
>>>
>>> I'll work on writing some test cases for the those types of mutex in the
>>> hope of catching both flaws before fixing them.
>>
>> Indeed, there is no need to calculate the relative timeout anymore. I think
>> the fix below should pass the absolute timeout directly. I will check
>> a possible regression tests as well.
>
> OK. I won't then. Thanks.
>
>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>> index aaaafa21ce..86c5f4446e 100644
>> --- a/nptl/pthread_mutex_timedlock.c
>> +++ b/nptl/pthread_mutex_timedlock.c
>> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>> if (__pthread_current_priority () > ceiling)
>> {
>> result = EINVAL;
>> - failpp:
>> if (oldprio != -1)
>> __pthread_tpp_change_priority (oldprio, -1);
>> return result;
>> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>
>> if (oldval != ceilval)
>> {
>> - /* Reject invalid timeouts. */
>> - if (! valid_nanoseconds (abstime->tv_nsec))
>> - {
>> - result = EINVAL;
>> - goto failpp;
>> - }
>
> If this is removed then is there a risk of getting into a busy loop if
> someone passes a bogus timespec? (Regardless of the answer, it makes sense
> to ensure that is tested somehow.)
The minimum supported kernel already does the same check on the futex call
(source for Linux 3.2):
2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
2691 struct timespec __user *, utime, u32 __user *, uaddr2,
2692 u32, val3)
2693 {
2694 struct timespec ts;
2695 ktime_t t, *tp = NULL;
2696 u32 val2 = 0;
2697 int cmd = op & FUTEX_CMD_MASK;
2698
2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
2700 cmd == FUTEX_WAIT_BITSET ||
2701 cmd == FUTEX_WAIT_REQUEUE_PI)) {
2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
2703 return -EFAULT;
2704 if (!timespec_valid(&ts))
2705 return -EINVAL;
2706
2707 t = timespec_to_ktime(ts);
2708 if (cmd == FUTEX_WAIT)
2709 t = ktime_add_safe(ktime_get(), t);
2710 tp = &t;
2711 }
113 #define timespec_valid(ts) \
114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))
So it will return EINVAL for bogus timespec.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-25 17:19 ` Adhemerval Zanella
@ 2020-11-25 17:37 ` Mike Crowe
2020-11-25 17:56 ` Adhemerval Zanella
0 siblings, 1 reply; 40+ messages in thread
From: Mike Crowe @ 2020-11-25 17:37 UTC (permalink / raw)
To: Adhemerval Zanella; +Cc: libc-alpha, Michael Kerrisk
On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote:
> On 25/11/2020 12:46, Mike Crowe wrote:
> > On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
> >>
> >>
> >> On 25/11/2020 12:32, Mike Crowe wrote:
> >>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
> >>>> The idea is to make NPTL implementation to use on the functions
> >>>> provided by futex-internal.h.
> >>>>
> >>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
> >>>> ---
> >>>> nptl/lowlevellock.c | 6 +++---
> >>>> nptl/pthread_mutex_lock.c | 9 +++++----
> >>>> nptl/pthread_mutex_setprioceiling.c | 5 +++--
> >>>> nptl/pthread_mutex_timedlock.c | 6 +++---
> >>>> 4 files changed, 14 insertions(+), 12 deletions(-)
> >>>
> >>> [snip]
> >>>
> >>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >>>> index e643eab258..343acf6107 100644
> >>>> --- a/nptl/pthread_mutex_timedlock.c
> >>>> +++ b/nptl/pthread_mutex_timedlock.c
> >>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>>> goto failpp;
> >>>> }
> >>>>
> >>>> - lll_futex_timed_wait (&mutex->__data.__lock,
> >>>> - ceilval | 2, &rt,
> >>>> - PTHREAD_MUTEX_PSHARED (mutex));
> >>>> + __futex_abstimed_wait64 (
> >>>> + (unsigned int *) &mutex->__data.__lock, clockid,
> >>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> >>>
> >>> I think you've replaced the lll_futex_timed_wait call that expects a
> >>> relative timeout with a __futex_abstimed_wait64 call that expects an
> >>> absolute timeout, yet you still appear to be passing the relative timeout.
> >>>
> >>> However, it turns out that the implementation for the
> >>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
> >>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
> >>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
> >>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
> >>> time this was a less obvious __gettimeofday call.)
> >>>
> >>> I'll work on writing some test cases for the those types of mutex in the
> >>> hope of catching both flaws before fixing them.
> >>
> >> Indeed, there is no need to calculate the relative timeout anymore. I think
> >> the fix below should pass the absolute timeout directly. I will check
> >> a possible regression tests as well.
> >
> > OK. I won't then. Thanks.
> >
> >> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> >> index aaaafa21ce..86c5f4446e 100644
> >> --- a/nptl/pthread_mutex_timedlock.c
> >> +++ b/nptl/pthread_mutex_timedlock.c
> >> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >> if (__pthread_current_priority () > ceiling)
> >> {
> >> result = EINVAL;
> >> - failpp:
> >> if (oldprio != -1)
> >> __pthread_tpp_change_priority (oldprio, -1);
> >> return result;
> >> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
> >>
> >> if (oldval != ceilval)
> >> {
> >> - /* Reject invalid timeouts. */
> >> - if (! valid_nanoseconds (abstime->tv_nsec))
> >> - {
> >> - result = EINVAL;
> >> - goto failpp;
> >> - }
> >
> > If this is removed then is there a risk of getting into a busy loop if
> > someone passes a bogus timespec? (Regardless of the answer, it makes sense
> > to ensure that is tested somehow.)
>
> The minimum supported kernel already does the same check on the futex call
> (source for Linux 3.2):
>
> 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
> 2691 struct timespec __user *, utime, u32 __user *, uaddr2,
> 2692 u32, val3)
> 2693 {
> 2694 struct timespec ts;
> 2695 ktime_t t, *tp = NULL;
> 2696 u32 val2 = 0;
> 2697 int cmd = op & FUTEX_CMD_MASK;
> 2698
> 2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
> 2700 cmd == FUTEX_WAIT_BITSET ||
> 2701 cmd == FUTEX_WAIT_REQUEUE_PI)) {
> 2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
> 2703 return -EFAULT;
> 2704 if (!timespec_valid(&ts))
> 2705 return -EINVAL;
> 2706
> 2707 t = timespec_to_ktime(ts);
> 2708 if (cmd == FUTEX_WAIT)
> 2709 t = ktime_add_safe(ktime_get(), t);
> 2710 tp = &t;
> 2711 }
>
> 113 #define timespec_valid(ts) \
> 114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))
>
> So it will return EINVAL for bogus timespec.
Yes, but here:
> __futex_abstimed_wait64 (
> (unsigned int *) &mutex->__data.__lock, clockid,
> - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
> + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
the return value of __futex_abstimed_wait64 is not checked, so the loop
might just spin around busily until the timeout expires. Perhaps the return
value needs checking too?
Thanks.
Mike.
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h
2020-11-25 17:37 ` Mike Crowe
@ 2020-11-25 17:56 ` Adhemerval Zanella
0 siblings, 0 replies; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-25 17:56 UTC (permalink / raw)
To: Mike Crowe; +Cc: libc-alpha, Michael Kerrisk
On 25/11/2020 14:37, Mike Crowe wrote:
> On Wednesday 25 November 2020 at 14:19:37 -0300, Adhemerval Zanella wrote:
>> On 25/11/2020 12:46, Mike Crowe wrote:
>>> On Wednesday 25 November 2020 at 12:40:46 -0300, Adhemerval Zanella wrote:
>>>>
>>>>
>>>> On 25/11/2020 12:32, Mike Crowe wrote:
>>>>> On Monday 23 November 2020 at 16:52:52 -0300, Adhemerval Zanella wrote:
>>>>>> The idea is to make NPTL implementation to use on the functions
>>>>>> provided by futex-internal.h.
>>>>>>
>>>>>> Checked on x86_64-linux-gnu and i686-linux-gnu.
>>>>>> ---
>>>>>> nptl/lowlevellock.c | 6 +++---
>>>>>> nptl/pthread_mutex_lock.c | 9 +++++----
>>>>>> nptl/pthread_mutex_setprioceiling.c | 5 +++--
>>>>>> nptl/pthread_mutex_timedlock.c | 6 +++---
>>>>>> 4 files changed, 14 insertions(+), 12 deletions(-)
>>>>>
>>>>> [snip]
>>>>>
>>>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>>>> index e643eab258..343acf6107 100644
>>>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>>>> @@ -561,9 +561,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>>> goto failpp;
>>>>>> }
>>>>>>
>>>>>> - lll_futex_timed_wait (&mutex->__data.__lock,
>>>>>> - ceilval | 2, &rt,
>>>>>> - PTHREAD_MUTEX_PSHARED (mutex));
>>>>>> + __futex_abstimed_wait64 (
>>>>>> + (unsigned int *) &mutex->__data.__lock, clockid,
>>>>>> + ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>>>>>
>>>>> I think you've replaced the lll_futex_timed_wait call that expects a
>>>>> relative timeout with a __futex_abstimed_wait64 call that expects an
>>>>> absolute timeout, yet you still appear to be passing the relative timeout.
>>>>>
>>>>> However, it turns out that the implementation for the
>>>>> PTHREAD_MUTEX_PP_RECURSIVE_NP and friends case appears to be have been
>>>>> completely broken with clockid != CLOCK_REALTIME ever since I added it in
>>>>> 9d20e22e46d891b929a72b0f35586e079eb083fd anyway since the relative timeout
>>>>> is calculated by calling __clock_gettime64(CLOCK_REALTIME) (although at the
>>>>> time this was a less obvious __gettimeofday call.)
>>>>>
>>>>> I'll work on writing some test cases for the those types of mutex in the
>>>>> hope of catching both flaws before fixing them.
>>>>
>>>> Indeed, there is no need to calculate the relative timeout anymore. I think
>>>> the fix below should pass the absolute timeout directly. I will check
>>>> a possible regression tests as well.
>>>
>>> OK. I won't then. Thanks.
>>>
>>>> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
>>>> index aaaafa21ce..86c5f4446e 100644
>>>> --- a/nptl/pthread_mutex_timedlock.c
>>>> +++ b/nptl/pthread_mutex_timedlock.c
>>>> @@ -508,7 +508,6 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>> if (__pthread_current_priority () > ceiling)
>>>> {
>>>> result = EINVAL;
>>>> - failpp:
>>>> if (oldprio != -1)
>>>> __pthread_tpp_change_priority (oldprio, -1);
>>>> return result;
>>>> @@ -540,37 +539,9 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
>>>>
>>>> if (oldval != ceilval)
>>>> {
>>>> - /* Reject invalid timeouts. */
>>>> - if (! valid_nanoseconds (abstime->tv_nsec))
>>>> - {
>>>> - result = EINVAL;
>>>> - goto failpp;
>>>> - }
>>>
>>> If this is removed then is there a risk of getting into a busy loop if
>>> someone passes a bogus timespec? (Regardless of the answer, it makes sense
>>> to ensure that is tested somehow.)
>>
>> The minimum supported kernel already does the same check on the futex call
>> (source for Linux 3.2):
>>
>> 2690 SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
>> 2691 struct timespec __user *, utime, u32 __user *, uaddr2,
>> 2692 u32, val3)
>> 2693 {
>> 2694 struct timespec ts;
>> 2695 ktime_t t, *tp = NULL;
>> 2696 u32 val2 = 0;
>> 2697 int cmd = op & FUTEX_CMD_MASK;
>> 2698
>> 2699 if (utime && (cmd == FUTEX_WAIT || cmd == FUTEX_LOCK_PI ||
>> 2700 cmd == FUTEX_WAIT_BITSET ||
>> 2701 cmd == FUTEX_WAIT_REQUEUE_PI)) {
>> 2702 if (copy_from_user(&ts, utime, sizeof(ts)) != 0)
>> 2703 return -EFAULT;
>> 2704 if (!timespec_valid(&ts))
>> 2705 return -EINVAL;
>> 2706
>> 2707 t = timespec_to_ktime(ts);
>> 2708 if (cmd == FUTEX_WAIT)
>> 2709 t = ktime_add_safe(ktime_get(), t);
>> 2710 tp = &t;
>> 2711 }
>>
>> 113 #define timespec_valid(ts) \
>> 114 (((ts)->tv_sec >= 0) && (((unsigned long) (ts)->tv_nsec) < NSEC_PER_SEC))
>>
>> So it will return EINVAL for bogus timespec.
>
> Yes, but here:
>
>> __futex_abstimed_wait64 (
>> (unsigned int *) &mutex->__data.__lock, clockid,
>> - ceilval | 2, &rt, PTHREAD_MUTEX_PSHARED (mutex));
>> + ceilval | 2, abstime, PTHREAD_MUTEX_PSHARED (mutex));
>
> the return value of __futex_abstimed_wait64 is not checked, so the loop
> might just spin around busily until the timeout expires. Perhaps the return
> value needs checking too?
Indeed, we need to check for ETIMEDOUT/EOVERFLOW.
^ permalink raw reply [flat|nested] 40+ messages in thread
* [PATCH 10/13] linux: nptl: Replace lll_futex_supported_clockid with futex-internal.h
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (7 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 09/13] linux: nptl: Replace lll_futex_{timed_}wait by futex-internal.h Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:36 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 11/13] nptl: Replace lll_futex_wake " Adhemerval Zanella
` (3 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The idea is to make NPTL implementation to use on the functions
provided by futex-internal.h.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_mutex_timedlock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 343acf6107..b42862193a 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -600,7 +600,7 @@ __pthread_mutex_clocklock64 (pthread_mutex_t *mutex,
clockid_t clockid,
const struct __timespec64 *abstime)
{
- if (__glibc_unlikely (!lll_futex_supported_clockid (clockid)))
+ if (__glibc_unlikely (!futex_abstimed_supported_clockid (clockid)))
return EINVAL;
LIBC_PROBE (mutex_clocklock_entry, 3, mutex, clockid, abstime);
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 10/13] linux: nptl: Replace lll_futex_supported_clockid with futex-internal.h
2020-11-23 19:52 ` [PATCH 10/13] linux: nptl: Replace lll_futex_supported_clockid with futex-internal.h Adhemerval Zanella
@ 2020-11-24 21:36 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:36 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 1268 bytes --]
On Mon, 23 Nov 2020 16:52:53 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:
> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_mutex_timedlock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index 343acf6107..b42862193a 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -600,7 +600,7 @@ __pthread_mutex_clocklock64 (pthread_mutex_t
> *mutex, clockid_t clockid,
> const struct __timespec64 *abstime)
> {
> - if (__glibc_unlikely (!lll_futex_supported_clockid (clockid)))
> + if (__glibc_unlikely (!futex_abstimed_supported_clockid (clockid)))
> return EINVAL;
>
> LIBC_PROBE (mutex_clocklock_entry, 3, mutex, clockid, abstime);
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 11/13] nptl: Replace lll_futex_wake with futex-internal.h
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (8 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 10/13] linux: nptl: Replace lll_futex_supported_clockid with futex-internal.h Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:38 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801] Adhemerval Zanella
` (2 subsequent siblings)
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The idea is to make NPTL implementation to use on the functions
provided by futex-internal.h.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/pthread_mutex_setprioceiling.c | 4 ++--
nptl/pthread_mutex_unlock.c | 6 +++---
nptl/sem_post.c | 9 ++-------
3 files changed, 7 insertions(+), 12 deletions(-)
diff --git a/nptl/pthread_mutex_setprioceiling.c b/nptl/pthread_mutex_setprioceiling.c
index cbef202579..8f1d6e1326 100644
--- a/nptl/pthread_mutex_setprioceiling.c
+++ b/nptl/pthread_mutex_setprioceiling.c
@@ -116,8 +116,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t *mutex, int prioceiling,
| (prioceiling << PTHREAD_MUTEX_PRIO_CEILING_SHIFT);
atomic_full_barrier ();
- lll_futex_wake (&mutex->__data.__lock, INT_MAX,
- PTHREAD_MUTEX_PSHARED (mutex));
+ futex_wake ((unsigned int *)&mutex->__data.__lock, INT_MAX,
+ PTHREAD_MUTEX_PSHARED (mutex));
return 0;
}
diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
index 2b4abb8ebe..56f1732e6d 100644
--- a/nptl/pthread_mutex_unlock.c
+++ b/nptl/pthread_mutex_unlock.c
@@ -162,7 +162,7 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
private = PTHREAD_ROBUST_MUTEX_PSHARED (mutex);
if (__glibc_unlikely ((atomic_exchange_rel (&mutex->__data.__lock, 0)
& FUTEX_WAITERS) != 0))
- lll_futex_wake (&mutex->__data.__lock, 1, private);
+ futex_wake ((unsigned int *) &mutex->__data.__lock, 1, private);
/* We must clear op_pending after we release the mutex.
FIXME However, this violates the mutex destruction requirements
@@ -332,8 +332,8 @@ __pthread_mutex_unlock_full (pthread_mutex_t *mutex, int decr)
&oldval, newval));
if ((oldval & ~PTHREAD_MUTEX_PRIO_CEILING_MASK) > 1)
- lll_futex_wake (&mutex->__data.__lock, 1,
- PTHREAD_MUTEX_PSHARED (mutex));
+ futex_wake ((unsigned int *)&mutex->__data.__lock, 1,
+ PTHREAD_MUTEX_PSHARED (mutex));
int oldprio = newval >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
diff --git a/nptl/sem_post.c b/nptl/sem_post.c
index 88cfc24b30..5dbfb3a214 100644
--- a/nptl/sem_post.c
+++ b/nptl/sem_post.c
@@ -84,19 +84,14 @@ int
attribute_compat_text_section
__old_sem_post (sem_t *sem)
{
- int *futex = (int *) sem;
+ unsigned int *futex = (unsigned int *) sem;
/* We must need to synchronize with consumers of this token, so the atomic
increment must have release MO semantics. */
atomic_write_barrier ();
(void) atomic_increment_val (futex);
/* We always have to assume it is a shared semaphore. */
- int err = lll_futex_wake (futex, 1, LLL_SHARED);
- if (__builtin_expect (err, 0) < 0)
- {
- __set_errno (-err);
- return -1;
- }
+ futex_wake (futex, 1, LLL_SHARED);
return 0;
}
compat_symbol (libpthread, __old_sem_post, sem_post, GLIBC_2_0);
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 11/13] nptl: Replace lll_futex_wake with futex-internal.h
2020-11-23 19:52 ` [PATCH 11/13] nptl: Replace lll_futex_wake " Adhemerval Zanella
@ 2020-11-24 21:38 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:38 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 3411 bytes --]
Hi Adhemerval,
> The idea is to make NPTL implementation to use on the functions
> provided by futex-internal.h.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_mutex_setprioceiling.c | 4 ++--
> nptl/pthread_mutex_unlock.c | 6 +++---
> nptl/sem_post.c | 9 ++-------
> 3 files changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/nptl/pthread_mutex_setprioceiling.c
> b/nptl/pthread_mutex_setprioceiling.c index cbef202579..8f1d6e1326
> 100644 --- a/nptl/pthread_mutex_setprioceiling.c
> +++ b/nptl/pthread_mutex_setprioceiling.c
> @@ -116,8 +116,8 @@ pthread_mutex_setprioceiling (pthread_mutex_t
> *mutex, int prioceiling, | (prioceiling <<
> PTHREAD_MUTEX_PRIO_CEILING_SHIFT); atomic_full_barrier ();
>
> - lll_futex_wake (&mutex->__data.__lock, INT_MAX,
> - PTHREAD_MUTEX_PSHARED (mutex));
> + futex_wake ((unsigned int *)&mutex->__data.__lock, INT_MAX,
> + PTHREAD_MUTEX_PSHARED (mutex));
>
> return 0;
> }
> diff --git a/nptl/pthread_mutex_unlock.c b/nptl/pthread_mutex_unlock.c
> index 2b4abb8ebe..56f1732e6d 100644
> --- a/nptl/pthread_mutex_unlock.c
> +++ b/nptl/pthread_mutex_unlock.c
> @@ -162,7 +162,7 @@ __pthread_mutex_unlock_full (pthread_mutex_t
> *mutex, int decr) private = PTHREAD_ROBUST_MUTEX_PSHARED (mutex);
> if (__glibc_unlikely ((atomic_exchange_rel
> (&mutex->__data.__lock, 0) & FUTEX_WAITERS) != 0))
> - lll_futex_wake (&mutex->__data.__lock, 1, private);
> + futex_wake ((unsigned int *) &mutex->__data.__lock, 1,
> private);
> /* We must clear op_pending after we release the mutex.
> FIXME However, this violates the mutex destruction
> requirements @@ -332,8 +332,8 @@ __pthread_mutex_unlock_full
> (pthread_mutex_t *mutex, int decr) &oldval, newval));
>
> if ((oldval & ~PTHREAD_MUTEX_PRIO_CEILING_MASK) > 1)
> - lll_futex_wake (&mutex->__data.__lock, 1,
> - PTHREAD_MUTEX_PSHARED (mutex));
> + futex_wake ((unsigned int *)&mutex->__data.__lock, 1,
> + PTHREAD_MUTEX_PSHARED (mutex));
>
> int oldprio = newval >> PTHREAD_MUTEX_PRIO_CEILING_SHIFT;
>
> diff --git a/nptl/sem_post.c b/nptl/sem_post.c
> index 88cfc24b30..5dbfb3a214 100644
> --- a/nptl/sem_post.c
> +++ b/nptl/sem_post.c
> @@ -84,19 +84,14 @@ int
> attribute_compat_text_section
> __old_sem_post (sem_t *sem)
> {
> - int *futex = (int *) sem;
> + unsigned int *futex = (unsigned int *) sem;
>
> /* We must need to synchronize with consumers of this token, so
> the atomic increment must have release MO semantics. */
> atomic_write_barrier ();
> (void) atomic_increment_val (futex);
> /* We always have to assume it is a shared semaphore. */
> - int err = lll_futex_wake (futex, 1, LLL_SHARED);
> - if (__builtin_expect (err, 0) < 0)
> - {
> - __set_errno (-err);
> - return -1;
> - }
> + futex_wake (futex, 1, LLL_SHARED);
> return 0;
> }
> compat_symbol (libpthread, __old_sem_post, sem_post, GLIBC_2_0);
Another little step to remove lll_* macros.
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801]
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (9 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 11/13] nptl: Replace lll_futex_wake " Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:43 ` Lukasz Majewski
2020-11-24 21:49 ` Lukasz Majewski
2020-11-23 19:52 ` [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np Adhemerval Zanella
2020-11-24 17:51 ` [PATCH 01/13] linux: Remove unused internal futex functions Lukasz Majewski
12 siblings, 2 replies; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
Changes from previous version:
- Replace ENOTSUP by EINVAL
--
Linux futex FUTEX_LOCK_PI operation only supports CLOCK_REALTIME,
so pthread_mutex_clocklock operation with priority aware mutexes
may fail depending of the input timeout.
Also, it is not possible to convert a CLOCK_MONOTONIC to a
CLOCK_REALTIME due the possible wall clock time change which might
invalid the requested timeout.
Checked on x86_64-linux-gnu and i686-linux-gnu.
---
nptl/Makefile | 2 +-
nptl/pthread_mutex_timedlock.c | 7 ++++
nptl/tst-mutexpi10.c | 68 ++++++++++++++++++++++++++++++++++
sysdeps/pthread/tst-mutex5.c | 2 +
sysdeps/pthread/tst-mutex9.c | 2 +
5 files changed, 80 insertions(+), 1 deletion(-)
create mode 100644 nptl/tst-mutexpi10.c
diff --git a/nptl/Makefile b/nptl/Makefile
index 968768d33b..a48426a396 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -265,7 +265,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
tst-mutex5a tst-mutex7a \
tst-mutexpi1 tst-mutexpi2 tst-mutexpi3 tst-mutexpi4 \
tst-mutexpi5 tst-mutexpi5a tst-mutexpi6 tst-mutexpi7 tst-mutexpi7a \
- tst-mutexpi9 \
+ tst-mutexpi9 tst-mutexpi10 \
tst-cond22 tst-cond26 \
tst-robustpi1 tst-robustpi2 tst-robustpi3 tst-robustpi4 tst-robustpi5 \
tst-robustpi6 tst-robustpi7 tst-robustpi9 \
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index b42862193a..aaaafa21ce 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -313,6 +313,13 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
case PTHREAD_MUTEX_PI_ROBUST_NORMAL_NP:
case PTHREAD_MUTEX_PI_ROBUST_ADAPTIVE_NP:
{
+ /* Currently futex FUTEX_LOCK_PI operation only provides support for
+ CLOCK_REALTIME and trying to emulate by converting a
+ CLOCK_MONOTONIC to CLOCK_REALTIME will take in account possible
+ changes to the wall clock. */
+ if (__glibc_unlikely (clockid != CLOCK_REALTIME))
+ return EINVAL;
+
int kind, robust;
{
/* See concurrency notes regarding __kind in struct __pthread_mutex_s
diff --git a/nptl/tst-mutexpi10.c b/nptl/tst-mutexpi10.c
new file mode 100644
index 0000000000..84ba1dfa97
--- /dev/null
+++ b/nptl/tst-mutexpi10.c
@@ -0,0 +1,68 @@
+/* Check if pthread_mutex_clocklock with PRIO_INHERIT fails with clock
+ different than CLOCK_REALTIME.
+ Copyright (C) 2015-2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ 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/>. */
+
+#include <pthread.h>
+#include <errno.h>
+#include <array_length.h>
+
+#include <support/check.h>
+#include <support/xthread.h>
+#include <support/timespec.h>
+
+static int
+do_test (void)
+{
+ const int types[] = {
+ PTHREAD_MUTEX_NORMAL,
+ PTHREAD_MUTEX_ERRORCHECK,
+ PTHREAD_MUTEX_RECURSIVE,
+ PTHREAD_MUTEX_ADAPTIVE_NP
+ };
+ const int robust[] = {
+ PTHREAD_MUTEX_STALLED,
+ PTHREAD_MUTEX_ROBUST
+ };
+
+
+ for (int t = 0; t < array_length (types); t++)
+ for (int r = 0; r < array_length (robust); r++)
+ {
+ pthread_mutexattr_t attr;
+
+ xpthread_mutexattr_init (&attr);
+ xpthread_mutexattr_setprotocol (&attr, PTHREAD_PRIO_INHERIT);
+ xpthread_mutexattr_settype (&attr, types[t]);
+ xpthread_mutexattr_setrobust (&attr, robust[r]);
+
+ pthread_mutex_t mtx;
+ xpthread_mutex_init (&mtx, &attr);
+
+ struct timespec tmo = timespec_add (xclock_now (CLOCK_MONOTONIC),
+ make_timespec (0, 100000000));
+
+ TEST_COMPARE (pthread_mutex_clocklock (&mtx, CLOCK_MONOTONIC, &tmo),
+ EINVAL);
+
+ xpthread_mutex_destroy (&mtx);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
diff --git a/sysdeps/pthread/tst-mutex5.c b/sysdeps/pthread/tst-mutex5.c
index 14490768c3..bfe1a79fa4 100644
--- a/sysdeps/pthread/tst-mutex5.c
+++ b/sysdeps/pthread/tst-mutex5.c
@@ -112,7 +112,9 @@ static int do_test (void)
{
do_test_clock (CLOCK_USE_TIMEDLOCK, "timedlock");
do_test_clock (CLOCK_REALTIME, "clocklock(realtime)");
+#ifndef ENABLE_PI
do_test_clock (CLOCK_MONOTONIC, "clocklock(monotonic)");
+#endif
return 0;
}
diff --git a/sysdeps/pthread/tst-mutex9.c b/sysdeps/pthread/tst-mutex9.c
index 2d7927b7c2..bfc01f8c75 100644
--- a/sysdeps/pthread/tst-mutex9.c
+++ b/sysdeps/pthread/tst-mutex9.c
@@ -133,7 +133,9 @@ do_test (void)
{
do_test_clock (CLOCK_USE_TIMEDLOCK);
do_test_clock (CLOCK_REALTIME);
+#ifndef ENABLE_PI
do_test_clock (CLOCK_MONOTONIC);
+#endif
return 0;
}
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801]
2020-11-23 19:52 ` [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801] Adhemerval Zanella
@ 2020-11-24 21:43 ` Lukasz Majewski
2020-11-24 21:49 ` Lukasz Majewski
1 sibling, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:43 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 5922 bytes --]
Hi Adhemerval,
> Changes from previous version:
>
> - Replace ENOTSUP by EINVAL
>
> --
>
> Linux futex FUTEX_LOCK_PI operation only supports CLOCK_REALTIME,
> so pthread_mutex_clocklock operation with priority aware mutexes
> may fail depending of the input timeout.
>
> Also, it is not possible to convert a CLOCK_MONOTONIC to a
> CLOCK_REALTIME due the possible wall clock time change which might
> invalid the requested timeout.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/Makefile | 2 +-
> nptl/pthread_mutex_timedlock.c | 7 ++++
> nptl/tst-mutexpi10.c | 68
> ++++++++++++++++++++++++++++++++++ sysdeps/pthread/tst-mutex5.c |
> 2 + sysdeps/pthread/tst-mutex9.c | 2 +
> 5 files changed, 80 insertions(+), 1 deletion(-)
> create mode 100644 nptl/tst-mutexpi10.c
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 968768d33b..a48426a396 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -265,7 +265,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
> tst-mutex5a tst-mutex7a \
> tst-mutexpi1 tst-mutexpi2 tst-mutexpi3 tst-mutexpi4 \
> tst-mutexpi5 tst-mutexpi5a tst-mutexpi6 tst-mutexpi7
> tst-mutexpi7a \
> - tst-mutexpi9 \
> + tst-mutexpi9 tst-mutexpi10 \
> tst-cond22 tst-cond26 \
> tst-robustpi1 tst-robustpi2 tst-robustpi3 tst-robustpi4
> tst-robustpi5 \ tst-robustpi6 tst-robustpi7 tst-robustpi9 \
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index b42862193a..aaaafa21ce 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -313,6 +313,13 @@ __pthread_mutex_clocklock_common
> (pthread_mutex_t *mutex, case PTHREAD_MUTEX_PI_ROBUST_NORMAL_NP:
> case PTHREAD_MUTEX_PI_ROBUST_ADAPTIVE_NP:
> {
> + /* Currently futex FUTEX_LOCK_PI operation only provides
> support for
> + CLOCK_REALTIME and trying to emulate by converting a
> + CLOCK_MONOTONIC to CLOCK_REALTIME will take in account
> possible
> + changes to the wall clock. */
> + if (__glibc_unlikely (clockid != CLOCK_REALTIME))
> + return EINVAL;
> +
> int kind, robust;
> {
> /* See concurrency notes regarding __kind in struct
> __pthread_mutex_s diff --git a/nptl/tst-mutexpi10.c
> b/nptl/tst-mutexpi10.c new file mode 100644
> index 0000000000..84ba1dfa97
> --- /dev/null
> +++ b/nptl/tst-mutexpi10.c
> @@ -0,0 +1,68 @@
> +/* Check if pthread_mutex_clocklock with PRIO_INHERIT fails with
> clock
> + different than CLOCK_REALTIME.
> + Copyright (C) 2015-2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + 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/>. */
> +
> +#include <pthread.h>
> +#include <errno.h>
> +#include <array_length.h>
> +
> +#include <support/check.h>
> +#include <support/xthread.h>
> +#include <support/timespec.h>
> +
> +static int
> +do_test (void)
> +{
> + const int types[] = {
> + PTHREAD_MUTEX_NORMAL,
> + PTHREAD_MUTEX_ERRORCHECK,
> + PTHREAD_MUTEX_RECURSIVE,
> + PTHREAD_MUTEX_ADAPTIVE_NP
> + };
> + const int robust[] = {
> + PTHREAD_MUTEX_STALLED,
> + PTHREAD_MUTEX_ROBUST
> + };
> +
> +
> + for (int t = 0; t < array_length (types); t++)
> + for (int r = 0; r < array_length (robust); r++)
> + {
> + pthread_mutexattr_t attr;
> +
> + xpthread_mutexattr_init (&attr);
> + xpthread_mutexattr_setprotocol (&attr, PTHREAD_PRIO_INHERIT);
> + xpthread_mutexattr_settype (&attr, types[t]);
> + xpthread_mutexattr_setrobust (&attr, robust[r]);
> +
> + pthread_mutex_t mtx;
> + xpthread_mutex_init (&mtx, &attr);
> +
> + struct timespec tmo = timespec_add (xclock_now
> (CLOCK_MONOTONIC),
> + make_timespec (0,
> 100000000)); +
> + TEST_COMPARE (pthread_mutex_clocklock (&mtx,
> CLOCK_MONOTONIC, &tmo),
> + EINVAL);
> +
> + xpthread_mutex_destroy (&mtx);
> + }
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/pthread/tst-mutex5.c
> b/sysdeps/pthread/tst-mutex5.c index 14490768c3..bfe1a79fa4 100644
> --- a/sysdeps/pthread/tst-mutex5.c
> +++ b/sysdeps/pthread/tst-mutex5.c
> @@ -112,7 +112,9 @@ static int do_test (void)
> {
> do_test_clock (CLOCK_USE_TIMEDLOCK, "timedlock");
> do_test_clock (CLOCK_REALTIME, "clocklock(realtime)");
> +#ifndef ENABLE_PI
> do_test_clock (CLOCK_MONOTONIC, "clocklock(monotonic)");
> +#endif
> return 0;
> }
>
> diff --git a/sysdeps/pthread/tst-mutex9.c
> b/sysdeps/pthread/tst-mutex9.c index 2d7927b7c2..bfc01f8c75 100644
> --- a/sysdeps/pthread/tst-mutex9.c
> +++ b/sysdeps/pthread/tst-mutex9.c
> @@ -133,7 +133,9 @@ do_test (void)
> {
> do_test_clock (CLOCK_USE_TIMEDLOCK);
> do_test_clock (CLOCK_REALTIME);
> +#ifndef ENABLE_PI
> do_test_clock (CLOCK_MONOTONIC);
> +#endif
> return 0;
> }
>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread* Re: [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801]
2020-11-23 19:52 ` [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801] Adhemerval Zanella
2020-11-24 21:43 ` Lukasz Majewski
@ 2020-11-24 21:49 ` Lukasz Majewski
2020-11-27 17:39 ` Adhemerval Zanella
1 sibling, 1 reply; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:49 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 6036 bytes --]
On Mon, 23 Nov 2020 16:52:55 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:
> Changes from previous version:
>
> - Replace ENOTSUP by EINVAL
>
> --
>
> Linux futex FUTEX_LOCK_PI operation only supports CLOCK_REALTIME,
> so pthread_mutex_clocklock operation with priority aware mutexes
> may fail depending of the input timeout.
>
> Also, it is not possible to convert a CLOCK_MONOTONIC to a
> CLOCK_REALTIME due the possible wall clock time change which might
> invalid the requested timeout.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/Makefile | 2 +-
> nptl/pthread_mutex_timedlock.c | 7 ++++
> nptl/tst-mutexpi10.c | 68
> ++++++++++++++++++++++++++++++++++ sysdeps/pthread/tst-mutex5.c |
> 2 + sysdeps/pthread/tst-mutex9.c | 2 +
> 5 files changed, 80 insertions(+), 1 deletion(-)
> create mode 100644 nptl/tst-mutexpi10.c
>
> diff --git a/nptl/Makefile b/nptl/Makefile
> index 968768d33b..a48426a396 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -265,7 +265,7 @@ tests = tst-attr2 tst-attr3 tst-default-attr \
> tst-mutex5a tst-mutex7a \
> tst-mutexpi1 tst-mutexpi2 tst-mutexpi3 tst-mutexpi4 \
> tst-mutexpi5 tst-mutexpi5a tst-mutexpi6 tst-mutexpi7
> tst-mutexpi7a \
> - tst-mutexpi9 \
> + tst-mutexpi9 tst-mutexpi10 \
> tst-cond22 tst-cond26 \
> tst-robustpi1 tst-robustpi2 tst-robustpi3 tst-robustpi4
> tst-robustpi5 \ tst-robustpi6 tst-robustpi7 tst-robustpi9 \
> diff --git a/nptl/pthread_mutex_timedlock.c
> b/nptl/pthread_mutex_timedlock.c index b42862193a..aaaafa21ce 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -313,6 +313,13 @@ __pthread_mutex_clocklock_common
> (pthread_mutex_t *mutex, case PTHREAD_MUTEX_PI_ROBUST_NORMAL_NP:
> case PTHREAD_MUTEX_PI_ROBUST_ADAPTIVE_NP:
> {
> + /* Currently futex FUTEX_LOCK_PI operation only provides
> support for
> + CLOCK_REALTIME and trying to emulate by converting a
> + CLOCK_MONOTONIC to CLOCK_REALTIME will take in account
> possible
> + changes to the wall clock. */
> + if (__glibc_unlikely (clockid != CLOCK_REALTIME))
> + return EINVAL;
> +
> int kind, robust;
> {
> /* See concurrency notes regarding __kind in struct
> __pthread_mutex_s diff --git a/nptl/tst-mutexpi10.c
> b/nptl/tst-mutexpi10.c new file mode 100644
> index 0000000000..84ba1dfa97
> --- /dev/null
> +++ b/nptl/tst-mutexpi10.c
> @@ -0,0 +1,68 @@
> +/* Check if pthread_mutex_clocklock with PRIO_INHERIT fails with
> clock
> + different than CLOCK_REALTIME.
> + Copyright (C) 2015-2020 Free Software Foundation, Inc.
Minor -> 2020 only
> + This file is part of the GNU C Library.
> +
> + 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/>. */
> +
> +#include <pthread.h>
> +#include <errno.h>
> +#include <array_length.h>
> +
> +#include <support/check.h>
> +#include <support/xthread.h>
> +#include <support/timespec.h>
> +
> +static int
> +do_test (void)
> +{
> + const int types[] = {
> + PTHREAD_MUTEX_NORMAL,
> + PTHREAD_MUTEX_ERRORCHECK,
> + PTHREAD_MUTEX_RECURSIVE,
> + PTHREAD_MUTEX_ADAPTIVE_NP
> + };
> + const int robust[] = {
> + PTHREAD_MUTEX_STALLED,
> + PTHREAD_MUTEX_ROBUST
> + };
> +
> +
> + for (int t = 0; t < array_length (types); t++)
> + for (int r = 0; r < array_length (robust); r++)
> + {
> + pthread_mutexattr_t attr;
> +
> + xpthread_mutexattr_init (&attr);
> + xpthread_mutexattr_setprotocol (&attr, PTHREAD_PRIO_INHERIT);
> + xpthread_mutexattr_settype (&attr, types[t]);
> + xpthread_mutexattr_setrobust (&attr, robust[r]);
> +
> + pthread_mutex_t mtx;
> + xpthread_mutex_init (&mtx, &attr);
> +
> + struct timespec tmo = timespec_add (xclock_now
> (CLOCK_MONOTONIC),
> + make_timespec (0,
> 100000000)); +
> + TEST_COMPARE (pthread_mutex_clocklock (&mtx,
> CLOCK_MONOTONIC, &tmo),
> + EINVAL);
> +
> + xpthread_mutex_destroy (&mtx);
> + }
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
> diff --git a/sysdeps/pthread/tst-mutex5.c
> b/sysdeps/pthread/tst-mutex5.c index 14490768c3..bfe1a79fa4 100644
> --- a/sysdeps/pthread/tst-mutex5.c
> +++ b/sysdeps/pthread/tst-mutex5.c
> @@ -112,7 +112,9 @@ static int do_test (void)
> {
> do_test_clock (CLOCK_USE_TIMEDLOCK, "timedlock");
> do_test_clock (CLOCK_REALTIME, "clocklock(realtime)");
> +#ifndef ENABLE_PI
> do_test_clock (CLOCK_MONOTONIC, "clocklock(monotonic)");
> +#endif
> return 0;
> }
>
> diff --git a/sysdeps/pthread/tst-mutex9.c
> b/sysdeps/pthread/tst-mutex9.c index 2d7927b7c2..bfc01f8c75 100644
> --- a/sysdeps/pthread/tst-mutex9.c
> +++ b/sysdeps/pthread/tst-mutex9.c
> @@ -133,7 +133,9 @@ do_test (void)
> {
> do_test_clock (CLOCK_USE_TIMEDLOCK);
> do_test_clock (CLOCK_REALTIME);
> +#ifndef ENABLE_PI
> do_test_clock (CLOCK_MONOTONIC);
> +#endif
> return 0;
> }
>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread
* [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (10 preceding siblings ...)
2020-11-23 19:52 ` [PATCH v2 12/13] nptl: Return EINVAL for pthread_mutex_clocklock/PI with CLOCK_MONOTONIC [BZ #26801] Adhemerval Zanella
@ 2020-11-23 19:52 ` Adhemerval Zanella
2020-11-24 21:48 ` Lukasz Majewski
2020-11-24 17:51 ` [PATCH 01/13] linux: Remove unused internal futex functions Lukasz Majewski
12 siblings, 1 reply; 40+ messages in thread
From: Adhemerval Zanella @ 2020-11-23 19:52 UTC (permalink / raw)
To: libc-alpha; +Cc: Mike Crowe, Michael Kerrisk
The align the GNU extension with the others one that accept specify
which clock to wait for (such as pthread_mutex_clocklock).
Check on x86_64-linux-gnu.
---
nptl/pthread_clockjoin.c | 4 ++
sysdeps/pthread/Makefile | 2 +-
sysdeps/pthread/tst-join15.c | 80 ++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 1 deletion(-)
create mode 100644 sysdeps/pthread/tst-join15.c
diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
index 0baba1e83d..3d54fe588f 100644
--- a/nptl/pthread_clockjoin.c
+++ b/nptl/pthread_clockjoin.c
@@ -17,12 +17,16 @@
<http://www.gnu.org/licenses/>. */
#include <time.h>
+#include <futex-internal.h>
#include "pthreadP.h"
int
__pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
clockid_t clockid, const struct __timespec64 *abstime)
{
+ if (!futex_abstimed_supported_clockid (clockid))
+ return EINVAL;
+
return __pthread_clockjoin_ex (threadid, thread_return,
clockid, abstime, true);
}
diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
index 45a15b0b1a..8f335c13b5 100644
--- a/sysdeps/pthread/Makefile
+++ b/sysdeps/pthread/Makefile
@@ -77,7 +77,7 @@ tests += tst-cnd-basic tst-mtx-trylock tst-cnd-broadcast \
tst-getpid3 \
tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \
tst-join8 tst-join9 tst-join10 tst-join11 tst-join12 tst-join13 \
- tst-join14 \
+ tst-join14 tst-join15 \
tst-key1 tst-key2 tst-key3 tst-key4 \
tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \
tst-locale1 tst-locale2 \
diff --git a/sysdeps/pthread/tst-join15.c b/sysdeps/pthread/tst-join15.c
new file mode 100644
index 0000000000..4ed767e733
--- /dev/null
+++ b/sysdeps/pthread/tst-join15.c
@@ -0,0 +1,80 @@
+/* Check pthread_clockjoin_np clock support.
+ Copyright (C) 2020 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+
+ 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/>. */
+
+#include <pthread.h>
+#include <sys/time.h>
+#include <unistd.h>
+#include <errno.h>
+
+#include <array_length.h>
+#include <support/check.h>
+#include <support/timespec.h>
+#include <support/xthread.h>
+
+static void *
+tf (void *arg)
+{
+ pause ();
+ return NULL;
+}
+
+
+static int
+do_test (void)
+{
+ const clockid_t clocks[] = {
+ CLOCK_REALTIME,
+ CLOCK_MONOTONIC,
+ CLOCK_PROCESS_CPUTIME_ID,
+ CLOCK_THREAD_CPUTIME_ID,
+ CLOCK_THREAD_CPUTIME_ID,
+ CLOCK_MONOTONIC_RAW,
+ CLOCK_REALTIME_COARSE,
+ CLOCK_MONOTONIC_COARSE,
+#ifdef CLOCK_BOOTTIME
+ CLOCK_BOOTTIME,
+#endif
+#ifdef CLOCK_REALTIME_ALARM
+ CLOCK_REALTIME_ALARM,
+#endif
+#ifdef CLOCK_BOOTTIME_ALARM
+ CLOCK_BOOTTIME_ALARM,
+#endif
+#ifdef CLOCK_TAI
+ CLOCK_TAI
+#endif
+ };
+
+ pthread_t thr = xpthread_create (NULL, tf, NULL);
+
+ for (int t = 0; t < array_length (clocks); t++)
+ {
+ /* A valid timeout so valid clock timeout. */
+ struct timespec tmo = timespec_add (xclock_now (clocks[t]),
+ make_timespec (0, 100000000));
+
+ int ret = clocks[t] == CLOCK_REALTIME || clocks[t] == CLOCK_MONOTONIC
+ ? ETIMEDOUT : EINVAL;
+
+ TEST_COMPARE (pthread_clockjoin_np (thr, NULL, clocks[t], &tmo), ret);
+ }
+
+ return 0;
+}
+
+#include <support/test-driver.c>
--
2.25.1
^ permalink raw reply [flat|nested] 40+ messages in thread* Re: [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np
2020-11-23 19:52 ` [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np Adhemerval Zanella
@ 2020-11-24 21:48 ` Lukasz Majewski
2020-11-24 22:58 ` Lukasz Majewski
0 siblings, 1 reply; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 21:48 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 5017 bytes --]
Hi Adhemerval,
> The align the GNU extension with the others one that accept specify
> which clock to wait for (such as pthread_mutex_clocklock).
>
> Check on x86_64-linux-gnu.
> ---
> nptl/pthread_clockjoin.c | 4 ++
> sysdeps/pthread/Makefile | 2 +-
> sysdeps/pthread/tst-join15.c | 80
> ++++++++++++++++++++++++++++++++++++ 3 files changed, 85
> insertions(+), 1 deletion(-) create mode 100644
> sysdeps/pthread/tst-join15.c
>
> diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> index 0baba1e83d..3d54fe588f 100644
> --- a/nptl/pthread_clockjoin.c
> +++ b/nptl/pthread_clockjoin.c
> @@ -17,12 +17,16 @@
> <http://www.gnu.org/licenses/>. */
>
> #include <time.h>
> +#include <futex-internal.h>
> #include "pthreadP.h"
>
> int
> __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> clockid_t clockid, const struct
> __timespec64 *abstime) {
> + if (!futex_abstimed_supported_clockid (clockid))
> + return EINVAL;
> +
> return __pthread_clockjoin_ex (threadid, thread_return,
> clockid, abstime, true);
> }
> diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> index 45a15b0b1a..8f335c13b5 100644
> --- a/sysdeps/pthread/Makefile
> +++ b/sysdeps/pthread/Makefile
> @@ -77,7 +77,7 @@ tests += tst-cnd-basic tst-mtx-trylock
> tst-cnd-broadcast \ tst-getpid3 \
> tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6
> tst-join7 \ tst-join8 tst-join9 tst-join10 tst-join11 tst-join12
> tst-join13 \
> - tst-join14 \
> + tst-join14 tst-join15 \
> tst-key1 tst-key2 tst-key3 tst-key4 \
> tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6
> \ tst-locale1 tst-locale2 \
> diff --git a/sysdeps/pthread/tst-join15.c
> b/sysdeps/pthread/tst-join15.c new file mode 100644
> index 0000000000..4ed767e733
> --- /dev/null
> +++ b/sysdeps/pthread/tst-join15.c
> @@ -0,0 +1,80 @@
> +/* Check pthread_clockjoin_np clock support.
> + Copyright (C) 2020 Free Software Foundation, Inc.
> + This file is part of the GNU C Library.
> +
> + 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/>. */
> +
> +#include <pthread.h>
> +#include <sys/time.h>
> +#include <unistd.h>
> +#include <errno.h>
> +
> +#include <array_length.h>
> +#include <support/check.h>
> +#include <support/timespec.h>
> +#include <support/xthread.h>
> +
> +static void *
> +tf (void *arg)
> +{
> + pause ();
> + return NULL;
> +}
> +
> +
> +static int
> +do_test (void)
> +{
> + const clockid_t clocks[] = {
> + CLOCK_REALTIME,
> + CLOCK_MONOTONIC,
> + CLOCK_PROCESS_CPUTIME_ID,
> + CLOCK_THREAD_CPUTIME_ID,
> + CLOCK_THREAD_CPUTIME_ID,
> + CLOCK_MONOTONIC_RAW,
> + CLOCK_REALTIME_COARSE,
> + CLOCK_MONOTONIC_COARSE,
> +#ifdef CLOCK_BOOTTIME
> + CLOCK_BOOTTIME,
> +#endif
> +#ifdef CLOCK_REALTIME_ALARM
> + CLOCK_REALTIME_ALARM,
> +#endif
> +#ifdef CLOCK_BOOTTIME_ALARM
> + CLOCK_BOOTTIME_ALARM,
> +#endif
> +#ifdef CLOCK_TAI
> + CLOCK_TAI
> +#endif
> + };
> +
> + pthread_t thr = xpthread_create (NULL, tf, NULL);
> +
> + for (int t = 0; t < array_length (clocks); t++)
> + {
> + /* A valid timeout so valid clock timeout. */
> + struct timespec tmo = timespec_add (xclock_now (clocks[t]),
> + make_timespec (0,
> 100000000)); +
> + int ret = clocks[t] == CLOCK_REALTIME || clocks[t] ==
> CLOCK_MONOTONIC
> + ? ETIMEDOUT : EINVAL;
> +
> + TEST_COMPARE (pthread_clockjoin_np (thr, NULL, clocks[t],
> &tmo), ret);
> + }
> +
> + return 0;
> +}
> +
> +#include <support/test-driver.c>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
When can we expect that those patches will be pulled? It seems like we
mostly remove dead (i.e. non Y2038 supporting) code in this series.
As aio_suspend depends on this patch series I would be great if we
would pull those sooner than latter :-).
Adhemerval, thanks again for refactoring the futex code.
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] 40+ messages in thread* Re: [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np
2020-11-24 21:48 ` Lukasz Majewski
@ 2020-11-24 22:58 ` Lukasz Majewski
0 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 22:58 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 5941 bytes --]
On Tue, 24 Nov 2020 22:48:59 +0100
Lukasz Majewski <lukma@denx.de> wrote:
> Hi Adhemerval,
>
> > The align the GNU extension with the others one that accept specify
> > which clock to wait for (such as pthread_mutex_clocklock).
> >
> > Check on x86_64-linux-gnu.
> > ---
> > nptl/pthread_clockjoin.c | 4 ++
> > sysdeps/pthread/Makefile | 2 +-
> > sysdeps/pthread/tst-join15.c | 80
> > ++++++++++++++++++++++++++++++++++++ 3 files changed, 85
> > insertions(+), 1 deletion(-) create mode 100644
> > sysdeps/pthread/tst-join15.c
> >
> > diff --git a/nptl/pthread_clockjoin.c b/nptl/pthread_clockjoin.c
> > index 0baba1e83d..3d54fe588f 100644
> > --- a/nptl/pthread_clockjoin.c
> > +++ b/nptl/pthread_clockjoin.c
> > @@ -17,12 +17,16 @@
> > <http://www.gnu.org/licenses/>. */
> >
> > #include <time.h>
> > +#include <futex-internal.h>
> > #include "pthreadP.h"
> >
> > int
> > __pthread_clockjoin_np64 (pthread_t threadid, void **thread_return,
> > clockid_t clockid, const struct
> > __timespec64 *abstime) {
> > + if (!futex_abstimed_supported_clockid (clockid))
> > + return EINVAL;
> > +
> > return __pthread_clockjoin_ex (threadid, thread_return,
> > clockid, abstime, true);
> > }
> > diff --git a/sysdeps/pthread/Makefile b/sysdeps/pthread/Makefile
> > index 45a15b0b1a..8f335c13b5 100644
> > --- a/sysdeps/pthread/Makefile
> > +++ b/sysdeps/pthread/Makefile
> > @@ -77,7 +77,7 @@ tests += tst-cnd-basic tst-mtx-trylock
> > tst-cnd-broadcast \ tst-getpid3 \
> > tst-join1 tst-join2 tst-join3 tst-join4 tst-join5
> > tst-join6 tst-join7 \ tst-join8 tst-join9 tst-join10 tst-join11
> > tst-join12 tst-join13 \
> > - tst-join14 \
> > + tst-join14 tst-join15 \
> > tst-key1 tst-key2 tst-key3 tst-key4 \
> > tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5
> > tst-kill6 \ tst-locale1 tst-locale2 \
> > diff --git a/sysdeps/pthread/tst-join15.c
> > b/sysdeps/pthread/tst-join15.c new file mode 100644
> > index 0000000000..4ed767e733
> > --- /dev/null
> > +++ b/sysdeps/pthread/tst-join15.c
> > @@ -0,0 +1,80 @@
> > +/* Check pthread_clockjoin_np clock support.
> > + Copyright (C) 2020 Free Software Foundation, Inc.
> > + This file is part of the GNU C Library.
> > +
> > + 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/>. */
> > +
> > +#include <pthread.h>
> > +#include <sys/time.h>
> > +#include <unistd.h>
> > +#include <errno.h>
> > +
> > +#include <array_length.h>
> > +#include <support/check.h>
> > +#include <support/timespec.h>
> > +#include <support/xthread.h>
> > +
> > +static void *
> > +tf (void *arg)
> > +{
> > + pause ();
> > + return NULL;
> > +}
> > +
> > +
> > +static int
> > +do_test (void)
> > +{
> > + const clockid_t clocks[] = {
> > + CLOCK_REALTIME,
> > + CLOCK_MONOTONIC,
> > + CLOCK_PROCESS_CPUTIME_ID,
> > + CLOCK_THREAD_CPUTIME_ID,
> > + CLOCK_THREAD_CPUTIME_ID,
> > + CLOCK_MONOTONIC_RAW,
> > + CLOCK_REALTIME_COARSE,
> > + CLOCK_MONOTONIC_COARSE,
> > +#ifdef CLOCK_BOOTTIME
> > + CLOCK_BOOTTIME,
> > +#endif
> > +#ifdef CLOCK_REALTIME_ALARM
> > + CLOCK_REALTIME_ALARM,
> > +#endif
> > +#ifdef CLOCK_BOOTTIME_ALARM
> > + CLOCK_BOOTTIME_ALARM,
> > +#endif
> > +#ifdef CLOCK_TAI
> > + CLOCK_TAI
> > +#endif
> > + };
> > +
> > + pthread_t thr = xpthread_create (NULL, tf, NULL);
> > +
> > + for (int t = 0; t < array_length (clocks); t++)
> > + {
> > + /* A valid timeout so valid clock timeout. */
> > + struct timespec tmo = timespec_add (xclock_now (clocks[t]),
> > + make_timespec (0,
> > 100000000)); +
> > + int ret = clocks[t] == CLOCK_REALTIME || clocks[t] ==
> > CLOCK_MONOTONIC
> > + ? ETIMEDOUT : EINVAL;
> > +
> > + TEST_COMPARE (pthread_clockjoin_np (thr, NULL, clocks[t],
> > &tmo), ret);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +#include <support/test-driver.c>
>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
>
>
> When can we expect that those patches will be pulled? It seems like we
> mostly remove dead (i.e. non Y2038 supporting) code in this series.
>
> As aio_suspend depends on this patch series I would be great if we
> would pull those sooner than latter :-).
I've built tested following branch:
https://github.com/lmajewski/y2038_glibc/commits/y2038_edge-futex-rework
on x86-64-x32, x86, x86-64, arm 32 bits and pthread_* related tests
were OK, so
Tested-by: Lukasz Majewski <lukma@denx.de>
>
> Adhemerval, thanks again for refactoring the futex code.
>
> 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
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] 40+ messages in thread
* Re: [PATCH 01/13] linux: Remove unused internal futex functions
2020-11-23 19:52 [PATCH 01/13] linux: Remove unused internal futex functions Adhemerval Zanella
` (11 preceding siblings ...)
2020-11-23 19:52 ` [PATCH 13/13] linux: Return EINVAL for invalid clock for pthread_clockjoin_np Adhemerval Zanella
@ 2020-11-24 17:51 ` Lukasz Majewski
12 siblings, 0 replies; 40+ messages in thread
From: Lukasz Majewski @ 2020-11-24 17:51 UTC (permalink / raw)
To: Adhemerval Zanella via Libc-alpha; +Cc: Adhemerval Zanella, Michael Kerrisk
[-- Attachment #1: Type: text/plain, Size: 5991 bytes --]
Hi Adhemerval,
> The __futex_abstimed_wait usage was remove with 3102e28bd11 and the
> __futex_abstimed_wait_cancelable by 323592fdc92 and b8d3e8fbaac.
> The futex_lock_pi can be replaced by a futex_lock_pi64.
>
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
> nptl/pthread_mutex_lock.c | 3 +-
> sysdeps/nptl/futex-internal.h | 98
> ----------------------------------- 2 files changed, 1 insertion(+),
> 100 deletions(-)
>
> diff --git a/nptl/pthread_mutex_lock.c b/nptl/pthread_mutex_lock.c
> index fac774e608..0439002454 100644
> --- a/nptl/pthread_mutex_lock.c
> +++ b/nptl/pthread_mutex_lock.c
> @@ -413,8 +413,7 @@ __pthread_mutex_lock_full (pthread_mutex_t *mutex)
> int private = (robust
> ? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
> : PTHREAD_MUTEX_PSHARED (mutex));
> - int e = futex_lock_pi ((unsigned int *)
> &mutex->__data.__lock,
> - NULL, private);
> + int e = futex_lock_pi64 (&mutex->__data.__lock, NULL,
> private); if (e == ESRCH || e == EDEADLK)
> {
> assert (e != EDEADLK
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index c27d0cdac8..21e3b74be6 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -275,76 +275,6 @@ futex_abstimed_supported_clockid (clockid_t
> clockid) return lll_futex_supported_clockid (clockid);
> }
>
> -/* Like futex_reltimed_wait, but the provided timeout (ABSTIME) is an
> - absolute point in time; a call will time out after this point in
> time. */ -static __always_inline int
> -futex_abstimed_wait (unsigned int* futex_word, unsigned int expected,
> - clockid_t clockid,
> - const struct timespec* abstime, int private)
> -{
> - /* Work around the fact that the kernel rejects negative timeout
> values
> - despite them being valid. */
> - if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
> - return ETIMEDOUT;
> - int err = lll_futex_clock_wait_bitset (futex_word, expected,
> - clockid, abstime,
> - private);
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - case -ETIMEDOUT:
> - return -err;
> -
> - case -EFAULT: /* Must have been caused by a glibc or application
> bug. */
> - case -EINVAL: /* Either due to wrong alignment, unsupported
> - clockid 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_abstimed_wait_cancelable (unsigned int* futex_word,
> - unsigned int expected,
> - clockid_t clockid,
> - const struct timespec* abstime, int
> private) -{
> - /* Work around the fact that the kernel rejects negative timeout
> values
> - despite them being valid. */
> - if (__glibc_unlikely ((abstime != NULL) && (abstime->tv_sec < 0)))
> - return ETIMEDOUT;
> - int oldtype;
> - oldtype = __pthread_enable_asynccancel ();
> - int err = lll_futex_clock_wait_bitset (futex_word, expected,
> - clockid, abstime,
> - private);
> - __pthread_disable_asynccancel (oldtype);
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - case -ETIMEDOUT:
> - 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 ();
> - }
> -}
> -
> /* Atomically wrt other futex operations on the same futex, this
> unblocks the specified number of processes, or all processes blocked
> on this futex if there are fewer than the specified number.
> Semantically, this is @@ -410,34 +340,6 @@ futex_wake (unsigned int*
> futex_word, int processes_to_wake, int private) futex.
> - ETIMEDOUT if the ABSTIME expires.
> */
> -static __always_inline int
> -futex_lock_pi (unsigned int *futex_word, const struct timespec
> *abstime,
> - int private)
> -{
> - int err = lll_futex_timed_lock_pi (futex_word, abstime, private);
> - switch (err)
> - {
> - case 0:
> - case -EAGAIN:
> - case -EINTR:
> - case -ETIMEDOUT:
> - case -ESRCH:
> - case -EDEADLK:
> - case -EINVAL: /* This indicates either state corruption or that
> the kernel
> - found a waiter on futex address which is
> waiting via
> - FUTEX_WAIT or FUTEX_WAIT_BITSET. This is
> reported on
> - some futex_lock_pi usage
> (pthread_mutex_timedlock for
> - instance). */
> - return -err;
> -
> - case -EFAULT: /* 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 ();
> - }
> -}
> -
> static __always_inline int
> futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime,
> int private)
It is great that we can consolidate the thread code and remove one not
needed.
Reviewed-by: Lukasz Majewski <lukma@denx.de>
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] 40+ messages in thread