* [PATCH RFC 1/3] nptl: Introduce futex_lock_pi2()
2021-06-21 11:16 [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Kurt Kanzenbach
@ 2021-06-21 11:16 ` Kurt Kanzenbach
2021-06-21 11:16 ` [PATCH RFC 2/3] nptl: Use futex_lock_pi2() Kurt Kanzenbach
` (2 subsequent siblings)
3 siblings, 0 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-21 11:16 UTC (permalink / raw)
To: libc-alpha
Cc: Adhemerval Zanella, Florian Weimer, Carlos O'Donell,
Thomas Gleixner, Sebastian Andrzej Siewior, Kurt Kanzenbach
This variant of futex_lock() has support for selectable clocks and priority
inheritance. The underlying FUTEX_LOCK_PI2 operation has been recently
introduced into the Linux kernel.
It can be used for implementing pthread_mutex_clocklock(MONOTONIC)/PI.
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
sysdeps/nptl/futex-internal.h | 77 +++++++++++++++++++++++++++++++
sysdeps/nptl/lowlevellock-futex.h | 1 +
2 files changed, 78 insertions(+)
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 969ab2bf4bf8..60bdae84405c 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -295,6 +295,83 @@ futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime,
}
}
+/* The operation checks the value of the futex, if the value is 0, then
+ it is atomically set to the caller's thread ID. If the futex value is
+ nonzero, it is atomically sets the FUTEX_WAITERS bit, which signals wrt
+ other futex owner that it cannot unlock the futex in user space by
+ atomically by setting its value to 0.
+
+ If more than one wait operations is issued, the enqueueing of the waiters
+ are done in descending priority order.
+
+ The ABSTIME arguments provides an absolute timeout (measured against the
+ CLOCK_REALTIME or CLOCK_MONOTONIC clock). If TIMEOUT is NULL, the operation
+ will block indefinitely.
+
+ Returns:
+
+ - 0 if woken by a PI unlock operation or spuriously.
+ - EAGAIN if the futex owner thread ID is about to exit, but has not yet
+ handled the state cleanup.
+ - EDEADLK if the futex is already locked by the caller.
+ - ESRCH if the thread ID int he futex does not exist.
+ - EINVAL is the state is corrupted or if there is a waiter on the
+ futex or if the clockid is invalid.
+ - ETIMEDOUT if the ABSTIME expires.
+ - ENOSYS if FUTEX_LOCK_PI2 is not available.
+*/
+static __always_inline int
+futex_lock_pi2_64 (int *futex_word, clockid_t clockid,
+ const struct __timespec64 *abstime,
+ int private)
+{
+ unsigned int clockbit;
+
+ if (! lll_futex_supported_clockid (clockid))
+ return EINVAL;
+
+ clockbit = (clockid == CLOCK_REALTIME) ? FUTEX_CLOCK_REALTIME : 0;
+ int op = __lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private);
+
+ int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op,
+ 0, abstime);
+#ifndef __ASSUME_TIME64_SYSCALLS
+ if (err == -ENOSYS)
+ {
+ if (abstime != NULL && ! in_time_t_range (abstime->tv_sec))
+ return EOVERFLOW;
+
+ struct timespec ts32;
+ if (abstime != NULL)
+ ts32 = valid_timespec64_to_timespec (*abstime);
+
+ err = INTERNAL_SYSCALL_CALL (futex, futex_word, op, 0,
+ abstime != NULL ? &ts32 : NULL);
+ }
+#endif
+ switch (err)
+ {
+ case 0:
+ case -EAGAIN:
+ case -EINTR:
+ case -ETIMEDOUT:
+ case -ESRCH:
+ case -EDEADLK:
+ case -ENOSYS: /* Futex lock pi 2 is not available on this kernel. */
+ 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. */
+ /* No other errors are documented at this time. */
+ default:
+ futex_fatal_error ();
+ }
+}
+
/* Wakes the top priority waiter that called a futex_lock_pi operation on
the futex.
diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h
index 66ebfe50f4c1..abda179e0de2 100644
--- a/sysdeps/nptl/lowlevellock-futex.h
+++ b/sysdeps/nptl/lowlevellock-futex.h
@@ -38,6 +38,7 @@
#define FUTEX_WAKE_BITSET 10
#define FUTEX_WAIT_REQUEUE_PI 11
#define FUTEX_CMP_REQUEUE_PI 12
+#define FUTEX_LOCK_PI2 13
#define FUTEX_PRIVATE_FLAG 128
#define FUTEX_CLOCK_REALTIME 256
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 2/3] nptl: Use futex_lock_pi2()
2021-06-21 11:16 [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Kurt Kanzenbach
2021-06-21 11:16 ` [PATCH RFC 1/3] nptl: Introduce futex_lock_pi2() Kurt Kanzenbach
@ 2021-06-21 11:16 ` Kurt Kanzenbach
2021-06-21 11:16 ` [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests Kurt Kanzenbach
2021-06-21 21:32 ` [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Adhemerval Zanella
3 siblings, 0 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-21 11:16 UTC (permalink / raw)
To: libc-alpha
Cc: Adhemerval Zanella, Florian Weimer, Carlos O'Donell,
Thomas Gleixner, Sebastian Andrzej Siewior, Kurt Kanzenbach
Use futex_lock_pi2() by default, because it supports selectable clocks.
On older kernels futex_lock_pi2() will return ENOSYS. Fallback to
futex_lock_pi() and don't support CLOCK_MONOTONIC then.
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
nptl/pthread_mutex_timedlock.c | 24 ++++++++++++++++++------
sysdeps/nptl/futex-internal.h | 17 +++++++++++++++++
2 files changed, 35 insertions(+), 6 deletions(-)
diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 5afd6222d61e..e7e001d2c17a 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -300,11 +300,15 @@ __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))
+ /* 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.
+
+ However, newer Linux kernels support FUTEX_LOCK_PI2 with selectable
+ clock support. */
+ if (__glibc_unlikely (! futex_lockpi2_supported () &&
+ clockid != CLOCK_REALTIME))
return EINVAL;
int kind, robust;
@@ -370,7 +374,15 @@ __pthread_mutex_clocklock_common (pthread_mutex_t *mutex,
int private = (robust
? PTHREAD_ROBUST_MUTEX_PSHARED (mutex)
: PTHREAD_MUTEX_PSHARED (mutex));
- int e = futex_lock_pi64 (&mutex->__data.__lock, abstime, private);
+ int e = futex_lock_pi2_64 (&mutex->__data.__lock, clockid, abstime,
+ private);
+
+ /* Use futex futex_lock_pi2() by default. This doesn't work for
+ older kernels. So fallback to futex_lock_pi() then. The clockid
+ is checked above already. */
+ if (__glibc_unlikely (e == ENOSYS))
+ e = futex_lock_pi64 (&mutex->__data.__lock, abstime, private);
+
if (e == ETIMEDOUT)
return ETIMEDOUT;
else if (e == ESRCH || e == EDEADLK)
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 60bdae84405c..b5f101b484c2 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -372,6 +372,23 @@ futex_lock_pi2_64 (int *futex_word, clockid_t clockid,
}
}
+/* Checks whether FUTEX_LOCK_PI2 operation is available on the running Linux
+ kernel. FUTEX_LOCK_PI2 has been introduced on newer kernel versions.
+
+ Returns true if FUTEX_LOCK_PI2 is available, false otherwise. */
+static __always_inline bool
+futex_lockpi2_supported (void)
+{
+ int err = INTERNAL_SYSCALL_CALL (futex_time64, NULL, FUTEX_LOCK_PI2, 0, NULL);
+
+#ifndef __ASSUME_TIME64_SYSCALLS
+ if (err == -ENOSYS)
+ err = INTERNAL_SYSCALL_CALL (futex, NULL, FUTEX_LOCK_PI2, 0, NULL);
+#endif
+
+ return err != -ENOSYS;
+}
+
/* Wakes the top priority waiter that called a futex_lock_pi operation on
the futex.
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests
2021-06-21 11:16 [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Kurt Kanzenbach
2021-06-21 11:16 ` [PATCH RFC 1/3] nptl: Introduce futex_lock_pi2() Kurt Kanzenbach
2021-06-21 11:16 ` [PATCH RFC 2/3] nptl: Use futex_lock_pi2() Kurt Kanzenbach
@ 2021-06-21 11:16 ` Kurt Kanzenbach
2021-06-21 20:07 ` Joseph Myers
2021-06-21 21:32 ` [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Adhemerval Zanella
3 siblings, 1 reply; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-21 11:16 UTC (permalink / raw)
To: libc-alpha
Cc: Adhemerval Zanella, Florian Weimer, Carlos O'Donell,
Thomas Gleixner, Sebastian Andrzej Siewior, Kurt Kanzenbach
Include pthread_mutex_clocklock(MONOTONIC)/PI in the testcases if FUTEX_LOCK_PI2
is available. Add the check at compile time to keep the test implementation
simple.
Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
nptl/tst-mutexpi10.c | 8 ++++++++
sysdeps/pthread/tst-mutex5.c | 3 ++-
sysdeps/pthread/tst-mutex9.c | 3 ++-
sysdeps/unix/sysv/linux/kernel-features.h | 8 ++++++++
4 files changed, 20 insertions(+), 2 deletions(-)
diff --git a/nptl/tst-mutexpi10.c b/nptl/tst-mutexpi10.c
index da781d0d7a93..19644b6f06a9 100644
--- a/nptl/tst-mutexpi10.c
+++ b/nptl/tst-mutexpi10.c
@@ -25,6 +25,8 @@
#include <support/xthread.h>
#include <support/timespec.h>
+#include <kernel-features.h>
+
static int
do_test (void)
{
@@ -56,8 +58,14 @@ do_test (void)
struct timespec tmo = timespec_add (xclock_now (CLOCK_MONOTONIC),
make_timespec (0, 100000000));
+#if __ASSUME_FUTEX_LOCK_PI2
+ TEST_COMPARE (pthread_mutex_clocklock (&mtx, CLOCK_MONOTONIC, &tmo),
+ 0);
+ xpthread_mutex_unlock (&mtx);
+#else
TEST_COMPARE (pthread_mutex_clocklock (&mtx, CLOCK_MONOTONIC, &tmo),
EINVAL);
+#endif
xpthread_mutex_destroy (&mtx);
}
diff --git a/sysdeps/pthread/tst-mutex5.c b/sysdeps/pthread/tst-mutex5.c
index 7dc5331cfc08..82235ae479b0 100644
--- a/sysdeps/pthread/tst-mutex5.c
+++ b/sysdeps/pthread/tst-mutex5.c
@@ -26,6 +26,7 @@
#include <config.h>
#include <support/check.h>
#include <support/timespec.h>
+#include <kernel-features.h>
#ifdef ENABLE_PP
#include "tst-tpp.h"
@@ -122,7 +123,7 @@ static int do_test (void)
do_test_clock (CLOCK_USE_TIMEDLOCK, "timedlock");
do_test_clock (CLOCK_REALTIME, "clocklock(realtime)");
-#ifndef ENABLE_PI
+#if ! defined(ENABLE_PI) || __ASSUME_FUTEX_LOCK_PI2
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 58c3a1aec263..e43efb80bada 100644
--- a/sysdeps/pthread/tst-mutex9.c
+++ b/sysdeps/pthread/tst-mutex9.c
@@ -29,6 +29,7 @@
#include <support/check.h>
#include <support/timespec.h>
#include <support/xunistd.h>
+#include <kernel-features.h>
#ifdef ENABLE_PP
#include "tst-tpp.h"
@@ -144,7 +145,7 @@ do_test (void)
do_test_clock (CLOCK_USE_TIMEDLOCK);
do_test_clock (CLOCK_REALTIME);
-#ifndef ENABLE_PI
+#if ! defined(ENABLE_PI) || __ASSUME_FUTEX_LOCK_PI2
do_test_clock (CLOCK_MONOTONIC);
#endif
return 0;
diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 1680b10ca1b6..eb5c4d5a04bd 100644
--- a/sysdeps/unix/sysv/linux/kernel-features.h
+++ b/sysdeps/unix/sysv/linux/kernel-features.h
@@ -218,4 +218,12 @@
# define __ASSUME_FACCESSAT2 0
#endif
+/* The FUTEX_LOCK_PI2 operation was introduced across all architectures in Linux
+ 5.14. */
+#if __LINUX_KERNEL_VERSION >= 0x051400
+# define __ASSUME_FUTEX_LOCK_PI2 1
+#else
+# define __ASSUME_FUTEX_LOCK_PI2 0
+#endif
+
#endif /* kernel-features.h */
--
2.30.2
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests
2021-06-21 11:16 ` [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests Kurt Kanzenbach
@ 2021-06-21 20:07 ` Joseph Myers
2021-06-22 8:58 ` Kurt Kanzenbach
0 siblings, 1 reply; 12+ messages in thread
From: Joseph Myers @ 2021-06-21 20:07 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: libc-alpha, Florian Weimer, Sebastian Andrzej Siewior, Thomas Gleixner
On Mon, 21 Jun 2021, Kurt Kanzenbach via Libc-alpha wrote:
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 1680b10ca1b6..eb5c4d5a04bd 100644
> --- a/sysdeps/unix/sysv/linux/kernel-features.h
> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
> @@ -218,4 +218,12 @@
> # define __ASSUME_FACCESSAT2 0
> #endif
>
> +/* The FUTEX_LOCK_PI2 operation was introduced across all architectures in Linux
> + 5.14. */
> +#if __LINUX_KERNEL_VERSION >= 0x051400
0x051400 is 5.20.0, not 5.14.0.
I'd expect this change to appear in an earlier patch, with
__ASSUME_FUTEX_LOCK_PI2 conditionals being used to disable fallback
support for kernels found at runtime not to support the new feature.
--
Joseph S. Myers
joseph@codesourcery.com
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests
2021-06-21 20:07 ` Joseph Myers
@ 2021-06-22 8:58 ` Kurt Kanzenbach
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-22 8:58 UTC (permalink / raw)
To: Joseph Myers
Cc: libc-alpha, Florian Weimer, Sebastian Andrzej Siewior, Thomas Gleixner
[-- Attachment #1: Type: text/plain, Size: 878 bytes --]
On Mon Jun 21 2021, Joseph Myers wrote:
> On Mon, 21 Jun 2021, Kurt Kanzenbach via Libc-alpha wrote:
>
>> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
>> index 1680b10ca1b6..eb5c4d5a04bd 100644
>> --- a/sysdeps/unix/sysv/linux/kernel-features.h
>> +++ b/sysdeps/unix/sysv/linux/kernel-features.h
>> @@ -218,4 +218,12 @@
>> # define __ASSUME_FACCESSAT2 0
>> #endif
>>
>> +/* The FUTEX_LOCK_PI2 operation was introduced across all architectures in Linux
>> + 5.14. */
>> +#if __LINUX_KERNEL_VERSION >= 0x051400
>
> 0x051400 is 5.20.0, not 5.14.0.
OK.
>
> I'd expect this change to appear in an earlier patch, with
> __ASSUME_FUTEX_LOCK_PI2 conditionals being used to disable fallback
> support for kernels found at runtime not to support the new feature.
Yes, understood.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-21 11:16 [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Kurt Kanzenbach
` (2 preceding siblings ...)
2021-06-21 11:16 ` [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests Kurt Kanzenbach
@ 2021-06-21 21:32 ` Adhemerval Zanella
2021-06-22 7:26 ` Kurt Kanzenbach
3 siblings, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2021-06-21 21:32 UTC (permalink / raw)
To: Kurt Kanzenbach, libc-alpha
Cc: Florian Weimer, Carlos O'Donell, Thomas Gleixner,
Sebastian Andrzej Siewior
On 21/06/2021 08:16, Kurt Kanzenbach wrote:
> Hi,
>
> Linux real time application often make use of mutexes with priority inheritance
> enabled due to problems such as unbounded priority inversion. In addition, some
> applications make use of timeouts e.g., by utilizing pthread_mutex_clocklock().
>
> However, the combination of priority inheritance enabled mutexes with timeouts
> based on CLOCK_MONOTONIC doesn't work. That is because the underlying Linux
> kernel futex interface doesn't support it, yet. Using CLOCK_REALTIME is
> possible, but it is subject to adjustments (NTP, PTP, ...). Therefore, Thomas
> proposed to add a new futex operation called FUTEX_LOCK_PI2 with support for
> selectable clocks [1]. That patch series is not merged, yet.
>
> Here is a proof of concept patch set adding FUTEX_LOCK_PI2 in order to support
> pthread_mutex_clocklock(MONOTONIC)/PI. The idea is to use futex_lock_pi2() by
> default, and fallback to futex_lock_pi() in case the kernel is too old. I think,
> you get the idea.
>
> There's also a bugreport for glibc with a test case:
>
> https://sourceware.org/bugzilla/show_bug.cgi?id=26801
>
> Thoughts?
>
> Thanks,
> Kurt
Currently we check for PI mutex support on pthread_mutex_init which basically
check for futex_cmpxchg_enabled within kernel (so it fails only on a handful
configurations).
For FUTEX_LOCK_PI2 I think we will need to rework it, since we are moving
the futex PI failure from pthread_mutex_init to pthread_mutex_{timed}lock.
It means that we will need to remove the prio_inherit_missing() test on
pthread_mutex_init and make the pthread_mutex_{timed}lock fail instead
(not sure if we should use ENOTSUP or keep with current EINVAL).
The proposed futex_lockpi2_supported() incurs in an extra syscall on every
mutex slow path, we should avoid it. I would like also to avoid the CRIU
issue as well, so I think it would be better to avoid any caching (as done
by prio_inherit_missing()), and optimize the FUTEX_LOCK_PI2 to be used only
when the timeout for clock different than CLOCK_REALTIME is required.
So instead it would be better to move the logic solely on futex_lock_pi()
(I am assuming FUTEX_LOCK_PI2 would be only added for futex_time64):
static __always_inline int
futex_lock_pi2_64 (int *futex_word, const struct __timespec64 *abtime,
int private)
{
# if __ASSUME_FUTEX_LOCK_PI2
return INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
__lll_private_flag (FUTEX_LOCK_PI2, private), 0,
abstime);
# else
if (abstime != NULL && clockid != CLOCK_REALTIME)
return INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
__lll_private_flag (FUTEX_LOCK_PI2, private), 0,
abstime);
else
{
int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
__lll_private_flag (FUTEX_LOCK_PI, private), 0,
abstime);
if (err == -ENOSYS)
err = -EOVERFLOW;
}
# endif /* __ASSUME_FUTEX_LOCK_PI2 */
}
static __always_inline int
futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime,
int private)
{
int err;
#ifdef __ASSUME_TIME64_SYSCALLS
err = futex_lock_pi2_64 (futex_word, abstime, private);
#else /* __ASSUME_TIME64_SYSCALLS */
bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec)
if (need_time64)
{
err = futex_lock_pi2_64 (futex_word, abstime, private);
}
else
{
struct timespec ts32;
if (abstime != NULL)
ts32 = valid_timespec64_to_timespec (*abstime);
err = INTERNAL_SYSCALL_CALL (futex, futex_word, __lll_private_flag
(FUTEX_LOCK_PI, private), 0,
abstime != NULL ? &ts32 : NULL);
}
#endif /* __ASSUME_TIME64_SYSCALLS */
[...]
}
It would make the changes on pthread_mutex code minimal, it would be only to
remove the extra check for clockid and adjust the comment.
Also, as Joseph has noted the __ASSUME_FUTEX_LOCK_PI2 should be the first
patch. It also does not make sense to add the __ASSUME_FUTEX_LOCK_PI2 on
tests, they need to be kernel agnostic so you will need to handle a possible
EINVAL/ENOSUP failure instead.
>
> [1] - https://lkml.kernel.org/lkml/20210422194417.866740847@linutronix.de/
>
> Kurt Kanzenbach (3):
> nptl: Introduce futex_lock_pi2()
> nptl: Use futex_lock_pi2()
> nptl: Include CLOCK_MONOTONIC in mutex tests
>
> nptl/pthread_mutex_timedlock.c | 24 ++++--
> nptl/tst-mutexpi10.c | 8 ++
> sysdeps/nptl/futex-internal.h | 94 +++++++++++++++++++++++
> sysdeps/nptl/lowlevellock-futex.h | 1 +
> sysdeps/pthread/tst-mutex5.c | 3 +-
> sysdeps/pthread/tst-mutex9.c | 3 +-
> sysdeps/unix/sysv/linux/kernel-features.h | 8 ++
> 7 files changed, 133 insertions(+), 8 deletions(-)
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-21 21:32 ` [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2 Adhemerval Zanella
@ 2021-06-22 7:26 ` Kurt Kanzenbach
2021-06-22 7:29 ` Florian Weimer
2021-06-22 12:30 ` Adhemerval Zanella
0 siblings, 2 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-22 7:26 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
Cc: Florian Weimer, Carlos O'Donell, Thomas Gleixner,
Sebastian Andrzej Siewior
[-- Attachment #1: Type: text/plain, Size: 4610 bytes --]
On Mon Jun 21 2021, Adhemerval Zanella wrote:
> Currently we check for PI mutex support on pthread_mutex_init which basically
> check for futex_cmpxchg_enabled within kernel (so it fails only on a handful
> configurations).
>
> For FUTEX_LOCK_PI2 I think we will need to rework it, since we are moving
> the futex PI failure from pthread_mutex_init to pthread_mutex_{timed}lock.
> It means that we will need to remove the prio_inherit_missing() test on
> pthread_mutex_init and make the pthread_mutex_{timed}lock fail instead
> (not sure if we should use ENOTSUP or keep with current EINVAL).
>
> The proposed futex_lockpi2_supported() incurs in an extra syscall on every
> mutex slow path, we should avoid it.
Yes, sure.
> I would like also to avoid the CRIU issue as well, so I think it would
> be better to avoid any caching (as done by prio_inherit_missing()),
> and optimize the FUTEX_LOCK_PI2 to be used only when the timeout for
> clock different than CLOCK_REALTIME is required.
OK.
>
> So instead it would be better to move the logic solely on futex_lock_pi()
> (I am assuming FUTEX_LOCK_PI2 would be only added for futex_time64):
The kernel also adds FUTEX_LOCK_PI2 for the old system call interface.
>
> static __always_inline int
> futex_lock_pi2_64 (int *futex_word, const struct __timespec64 *abtime,
> int private)
> {
> # if __ASSUME_FUTEX_LOCK_PI2
> return INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
> __lll_private_flag (FUTEX_LOCK_PI2, private), 0,
> abstime);
> # else
> if (abstime != NULL && clockid != CLOCK_REALTIME)
> return INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
> __lll_private_flag (FUTEX_LOCK_PI2, private), 0,
> abstime);
At this point __ASSUME_FUTEX_LOCK_PI2 is false meaning the kernel does
not have FUTEX_LOCK_PI2 support. But, it calls FUTEX_LOCK_PI2 for
clockid monotonic. This will result in ENOSYS unless it's an old kernel
which is patched. Is that intended?
> else
> {
> int err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word,
> __lll_private_flag (FUTEX_LOCK_PI, private), 0,
> abstime);
> if (err == -ENOSYS)
> err = -EOVERFLOW;
> }
> # endif /* __ASSUME_FUTEX_LOCK_PI2 */
> }
>
> static __always_inline int
> futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime,
> int private)
> {
> int err;
> #ifdef __ASSUME_TIME64_SYSCALLS
> err = futex_lock_pi2_64 (futex_word, abstime, private);
Makes sense.
> #else /* __ASSUME_TIME64_SYSCALLS */
> bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec)
> if (need_time64)
> {
> err = futex_lock_pi2_64 (futex_word, abstime, private);
> }
> else
> {
> struct timespec ts32;
> if (abstime != NULL)
> ts32 = valid_timespec64_to_timespec (*abstime);
>
> err = INTERNAL_SYSCALL_CALL (futex, futex_word, __lll_private_flag
> (FUTEX_LOCK_PI, private), 0,
> abstime != NULL ? &ts32 : NULL);
> }
> #endif /* __ASSUME_TIME64_SYSCALLS */
> [...]
> }
>
> It would make the changes on pthread_mutex code minimal, it would be only to
> remove the extra check for clockid and adjust the comment.
Well, that's an interesting point. I think the current check has to
stay, because there are two locking paths. Only the slow path calls
futex_lock_pi_64() which may result in ENOSYS for clock monotonic. But,
the fast path which doesn't call futex_lock_pi64() would succeed if the
check is removed.
Maybe something like this:
sysdeps/nptl/futex-internal.h:
|static __always_inline bool
|futex_lockpi2_supported (void)
|{
| return __ASSUME_FUTEX_LOCK_PI2;
|}
nptl/pthread_mutex_timedlock.c:
| if (__glibc_unlikely (! futex_lockpi2_supported () &&
| clockid != CLOCK_REALTIME))
| return EINVAL;
Or did I get something wrong?
>
> Also, as Joseph has noted the __ASSUME_FUTEX_LOCK_PI2 should be the first
> patch.
OK.
> It also does not make sense to add the __ASSUME_FUTEX_LOCK_PI2 on
> tests, they need to be kernel agnostic so you will need to handle a
> possible EINVAL/ENOSUP failure instead.
Agreed.
Thanks for your feedback. I'll rework it accordingly.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-22 7:26 ` Kurt Kanzenbach
@ 2021-06-22 7:29 ` Florian Weimer
2021-06-22 8:55 ` Kurt Kanzenbach
2021-06-22 12:30 ` Adhemerval Zanella
1 sibling, 1 reply; 12+ messages in thread
From: Florian Weimer @ 2021-06-22 7:29 UTC (permalink / raw)
To: Kurt Kanzenbach
Cc: Adhemerval Zanella, libc-alpha, Carlos O'Donell,
Thomas Gleixner, Sebastian Andrzej Siewior
* Kurt Kanzenbach:
> At this point __ASSUME_FUTEX_LOCK_PI2 is false meaning the kernel does
> not have FUTEX_LOCK_PI2 support. But, it calls FUTEX_LOCK_PI2 for
> clockid monotonic. This will result in ENOSYS unless it's an old kernel
> which is patched. Is that intended?
That's not quite how the __ASSUME_* macros work. If not defined, it
just means that glibc won't assume that the kernel feature is there. It
can still use it (the run-time kernel might have it after all), but
glibc has to check for the feature and has to compile in some sort of
fallback code.
Thanks,
Florian
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-22 7:29 ` Florian Weimer
@ 2021-06-22 8:55 ` Kurt Kanzenbach
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-22 8:55 UTC (permalink / raw)
To: Florian Weimer
Cc: Adhemerval Zanella, libc-alpha, Carlos O'Donell,
Thomas Gleixner, Sebastian Andrzej Siewior
[-- Attachment #1: Type: text/plain, Size: 689 bytes --]
On Tue Jun 22 2021, Florian Weimer wrote:
> * Kurt Kanzenbach:
>
>> At this point __ASSUME_FUTEX_LOCK_PI2 is false meaning the kernel does
>> not have FUTEX_LOCK_PI2 support. But, it calls FUTEX_LOCK_PI2 for
>> clockid monotonic. This will result in ENOSYS unless it's an old kernel
>> which is patched. Is that intended?
>
> That's not quite how the __ASSUME_* macros work. If not defined, it
> just means that glibc won't assume that the kernel feature is there. It
> can still use it (the run-time kernel might have it after all), but
> glibc has to check for the feature and has to compile in some sort of
> fallback code.
OK, makes sense. Thanks for the explanation.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-22 7:26 ` Kurt Kanzenbach
2021-06-22 7:29 ` Florian Weimer
@ 2021-06-22 12:30 ` Adhemerval Zanella
2021-06-22 14:25 ` Kurt Kanzenbach
1 sibling, 1 reply; 12+ messages in thread
From: Adhemerval Zanella @ 2021-06-22 12:30 UTC (permalink / raw)
To: Kurt Kanzenbach, libc-alpha
Cc: Florian Weimer, Carlos O'Donell, Thomas Gleixner,
Sebastian Andrzej Siewior
On 22/06/2021 04:26, Kurt Kanzenbach wrote:
>> #else /* __ASSUME_TIME64_SYSCALLS */
>> bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec)
>> if (need_time64)
>> {
>> err = futex_lock_pi2_64 (futex_word, abstime, private);
>> }
>> else
>> {
>> struct timespec ts32;
>> if (abstime != NULL)
>> ts32 = valid_timespec64_to_timespec (*abstime);
>>
>> err = INTERNAL_SYSCALL_CALL (futex, futex_word, __lll_private_flag
>> (FUTEX_LOCK_PI, private), 0,
>> abstime != NULL ? &ts32 : NULL);
>> }
>> #endif /* __ASSUME_TIME64_SYSCALLS */
>> [...]
>> }
>>
>> It would make the changes on pthread_mutex code minimal, it would be only to
>> remove the extra check for clockid and adjust the comment.
>
> Well, that's an interesting point. I think the current check has to
> stay, because there are two locking paths. Only the slow path calls
> futex_lock_pi_64() which may result in ENOSYS for clock monotonic. But,
> the fast path which doesn't call futex_lock_pi64() would succeed if the
> check is removed.
>
> Maybe something like this:
>
> sysdeps/nptl/futex-internal.h:
> |static __always_inline bool
> |futex_lockpi2_supported (void)
> |{
> | return __ASSUME_FUTEX_LOCK_PI2;
> |}
>
> nptl/pthread_mutex_timedlock.c:
> | if (__glibc_unlikely (! futex_lockpi2_supported () &&
> | clockid != CLOCK_REALTIME))
> | return EINVAL;
>
> Or did I get something wrong?
Besides what Florian has explained about __ASSUME_* macros, another issue we
have static initialization for pthread_mutex. So the syscall probe only
works for dynamic initialization (where caller issue a pthread_mutex_init).
I view this as an inconsistent behavior and I don't have a straightforward
solution that won't result in a performance penalization for common cases
(it would require to probe for FUTEX_LOCK_PI2 *and* FUTEX_LOCK_PI).
Instead I think we should move the possible error on the slow path and let
the kernel advertise it any missing support.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2
2021-06-22 12:30 ` Adhemerval Zanella
@ 2021-06-22 14:25 ` Kurt Kanzenbach
0 siblings, 0 replies; 12+ messages in thread
From: Kurt Kanzenbach @ 2021-06-22 14:25 UTC (permalink / raw)
To: Adhemerval Zanella, libc-alpha
Cc: Florian Weimer, Carlos O'Donell, Thomas Gleixner,
Sebastian Andrzej Siewior
[-- Attachment #1: Type: text/plain, Size: 843 bytes --]
On Tue Jun 22 2021, Adhemerval Zanella wrote:
> Besides what Florian has explained about __ASSUME_* macros, another issue we
> have static initialization for pthread_mutex. So the syscall probe only
> works for dynamic initialization (where caller issue a pthread_mutex_init).
>
> I view this as an inconsistent behavior and I don't have a straightforward
> solution that won't result in a performance penalization for common cases
> (it would require to probe for FUTEX_LOCK_PI2 *and* FUTEX_LOCK_PI).
>
> Instead I think we should move the possible error on the slow path and let
> the kernel advertise it any missing support.
OK, good. That makes the timedlock implementation straight forward. I
implemented it and started testing now with different Linux kernels. A
new version of patch set should be ready next week or so.
Thanks,
Kurt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]
^ permalink raw reply [flat|nested] 12+ messages in thread