[PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

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

[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(-)

-- 
2.30.2

[PATCH RFC 1/3] nptl: Introduce futex_lock_pi2()

[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

[PATCH RFC 2/3] nptl: Use futex_lock_pi2()

[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

[PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests

[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

Re: [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests

[Joseph Myers]

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

Re: [PATCH RFC 3/3] nptl: Include CLOCK_MONOTONIC in mutex tests

[Kurt Kanzenbach]

[-- 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 --]

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Adhemerval Zanella]

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

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- 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 --]

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Florian Weimer]

* 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

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- 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 --]

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Adhemerval Zanella]

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.

Re: [PATCH RFC 0/3] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- 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 --]