[PATCH v1 0/6] nptl: Introduce and use FUTEX_LOCK_PI2

[PATCH v1 0/6] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

Linux real time applications 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 didn't support it, yet. Using CLOCK_REALTIME is possible,
but it is subject to adjustments (NTP, PTP, ...). Therefore, Thomas proposed and
introduced a new futex operation called FUTEX_LOCK_PI2 with support for
selectable clocks [1].

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.

There's also a bugreport for glibc with a test case:

 https://sourceware.org/bugzilla/show_bug.cgi?id=26801

Changes since RFC:

 * FUTEX_LOCK_PI2 merged in upstream Linux kernel (Thomas Gleixner, Peter Zijlstra)
 * Make of __ASSUME_FUTEX_LOCK_PI2 in the implementation (Adhemerval Zanella)
 * Get rid of probing for FUTEX_LOCK_PI2 in timedlock (Adhemerval Zanella)
 * Adjust the test cases (Adhemerval Zanella)
 * Use correct Linux kernel version (Joseph Myers)

Previous versions:

 * https://sourceware.org/pipermail/libc-alpha/2021-June/127798.html

Thanks,
Kurt

[1] - https://lkml.kernel.org/lkml/20210422194417.866740847@linutronix.de/

Kurt Kanzenbach (6):
  Linux: Add FUTEX_LOCK_PI2
  nptl: Introduce futex_lock_pi2()
  nptl: Use futex_lock_pi2()
  nptl: Remove mutex test 10
  nptl: mutex-test5: Include CLOCK_MONOTONIC for PI
  nptl: mutex-test9: Include CLOCK_MONOTONIC for PI

 nptl/Makefile                             |   2 +-
 nptl/pthread_mutex_timedlock.c            |  10 +-
 nptl/tst-mutexpi10.c                      |  68 -----------
 sysdeps/nptl/futex-internal.h             | 131 ++++++++++++++++++++++
 sysdeps/nptl/lowlevellock-futex.h         |   1 +
 sysdeps/pthread/tst-mutex5.c              |  20 +++-
 sysdeps/pthread/tst-mutex9.c              |   9 +-
 sysdeps/unix/sysv/linux/kernel-features.h |   8 ++
 8 files changed, 163 insertions(+), 86 deletions(-)
 delete mode 100644 nptl/tst-mutexpi10.c

-- 
2.30.2

[PATCH v1 1/6] Linux: Add FUTEX_LOCK_PI2

[Kurt Kanzenbach]

Linux v5.14.0 introduced a new futex operation called FUTEX_LOCK_PI2.

This kernel feature can be used to implement
pthread_mutex_clocklock(MONOTONIC)/PI.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 sysdeps/unix/sysv/linux/kernel-features.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
index 1680b10ca1b6..af4b2b304715 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 >= 0x050e00
+# define __ASSUME_FUTEX_LOCK_PI2 1
+#else
+# define __ASSUME_FUTEX_LOCK_PI2 0
+#endif
+
 #endif /* kernel-features.h */
-- 
2.30.2

Re: [PATCH v1 1/6] Linux: Add FUTEX_LOCK_PI2

[Adhemerval Zanella]

On 25/06/2021 05:10, Kurt Kanzenbach wrote:
> Linux v5.14.0 introduced a new futex operation called FUTEX_LOCK_PI2.
> 
> This kernel feature can be used to implement
> pthread_mutex_clocklock(MONOTONIC)/PI.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

The __LINUX_KERNEL_VERSION looks fine now.

You will need to check with Carlos O'Donell if this is suitable for 2.34.

Reviewed-by: Adhemerval Zanella  <adhemerval.zanella@linaro.org>

> ---
>  sysdeps/unix/sysv/linux/kernel-features.h | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/sysdeps/unix/sysv/linux/kernel-features.h b/sysdeps/unix/sysv/linux/kernel-features.h
> index 1680b10ca1b6..af4b2b304715 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 >= 0x050e00
> +# define __ASSUME_FUTEX_LOCK_PI2 1
> +#else
> +# define __ASSUME_FUTEX_LOCK_PI2 0
> +#endif
> +
>  #endif /* kernel-features.h */
> 

[PATCH v1 2/6] 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.

The code works like this:

 * If kernel support is assumed, then use FUTEX_LOCK_PI2
 * If not, distuingish between clockid:
   * For realtime use FUTEX_LOCK_PI
   * For monotonic try to use FUTEX_LOCK_PI2 which might not be available

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 sysdeps/nptl/futex-internal.h     | 131 ++++++++++++++++++++++++++++++
 sysdeps/nptl/lowlevellock-futex.h |   1 +
 2 files changed, 132 insertions(+)

diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 79a366604d9e..38c969831276 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -303,6 +303,137 @@ 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.
+*/
+static __always_inline int
+futex_lock_pi2_64 (int *futex_word, clockid_t clockid,
+                   const struct __timespec64 *abstime, int private)
+{
+  unsigned int clockbit;
+  int err;
+
+  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);
+
+  /* FUTEX_LOCK_PI2 is a new futex operation. It supports selectable clocks
+     whereas the old FUTEX_LOCK_PI does only support CLOCK_REALTIME.
+
+     Therefore, the code works like this:
+
+     - If kernel support is available, then use FUTEX_LOCK_PI2
+     - If not, distuingish between clockid: For realtime use FUTEX_LOCK_PI and
+       for monotonic try to use FUTEX_LOCK_PI2 which might not be available.  */
+
+#if __ASSUME_FUTEX_LOCK_PI2
+
+# ifdef __ASSUME_TIME64_SYSCALLS
+  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
+# else
+
+  bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec);
+  if (need_time64)
+    {
+      err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
+      if (err == -ENOSYS)
+	err = -EOVERFLOW;
+    }
+  else
+    {
+      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	 /* __ASSUME_TIME64_SYSCALLS */
+
+#else
+
+  /* For CLOCK_MONOTONIC the only option is to use FUTEX_LOCK_PI2 */
+  if (abstime != NULL && clockid != CLOCK_REALTIME)
+    {
+# ifdef __ASSUME_TIME64_SYSCALLS
+      err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
+# else
+      bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec);
+      if (need_time64)
+	{
+	  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0,
+				       abstime);
+	}
+      else
+	{
+	  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	 /* __ASSUME_TIME64_SYSCALLS */
+
+      /* FUTEX_LOCK_PI2 is not available on this kernel */
+      if (err == -ENOSYS)
+	return EINVAL;
+    }
+  else
+    {
+      /* Otherwise use CLOCK_REALTIME and FUTEX_LOCK_PI */
+      return futex_lock_pi64 (futex_word, abstime, private);
+    }
+#endif 	/* __ASSUME_FUTEX_LOCK_PI2  */
+
+  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 ();
+    }
+}
+
 /* 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

Re: [PATCH v1 2/6] nptl: Introduce futex_lock_pi2()

[Adhemerval Zanella]

On 25/06/2021 05:11, Kurt Kanzenbach wrote:
> 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.
> 
> The code works like this:
> 
>  * If kernel support is assumed, then use FUTEX_LOCK_PI2
>  * If not, distuingish between clockid:

s/distuingish/distinguish

>    * For realtime use FUTEX_LOCK_PI
>    * For monotonic try to use FUTEX_LOCK_PI2 which might not be available
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>


> ---
>  sysdeps/nptl/futex-internal.h     | 131 ++++++++++++++++++++++++++++++
>  sysdeps/nptl/lowlevellock-futex.h |   1 +
>  2 files changed, 132 insertions(+)
> 
> diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
> index 79a366604d9e..38c969831276 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -303,6 +303,137 @@ 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.
> +*/
> +static __always_inline int
> +futex_lock_pi2_64 (int *futex_word, clockid_t clockid,
> +                   const struct __timespec64 *abstime, int private)
> +{

This routine has become quite complex and I think it should move it to the
sysdeps/nptl/futex-internal.c, since it is called in two places now
(pthread_mutex_lock.c and pthread_mutex_timedlock.c).  I think also we
should move futex_lock_pi2() futex-internal.c, so if any other implementation
that might want PI-futex will use the proper implementation.

> +  unsigned int clockbit;
> +  int err;
> +
> +  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);
> +
> +  /* FUTEX_LOCK_PI2 is a new futex operation. It supports selectable clocks
> +     whereas the old FUTEX_LOCK_PI does only support CLOCK_REALTIME.
> +
> +     Therefore, the code works like this:
> +
> +     - If kernel support is available, then use FUTEX_LOCK_PI2
> +     - If not, distuingish between clockid: For realtime use FUTEX_LOCK_PI and
> +       for monotonic try to use FUTEX_LOCK_PI2 which might not be available.  */
> +
> +#if __ASSUME_FUTEX_LOCK_PI2
> +
> +# ifdef __ASSUME_TIME64_SYSCALLS
> +  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
> +# else
> +
> +  bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec);
> +  if (need_time64)
> +    {
> +      err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
> +      if (err == -ENOSYS)
> +	err = -EOVERFLOW;
> +    }
> +  else
> +    {
> +      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	 /* __ASSUME_TIME64_SYSCALLS */
> +

I think we can assume that if FUTEX_LOCK_PI2 is support, 64-bit time_t syscalls
are also supported. 

> +#else
> +
> +  /* For CLOCK_MONOTONIC the only option is to use FUTEX_LOCK_PI2 */
> +  if (abstime != NULL && clockid != CLOCK_REALTIME)
> +    {
> +# ifdef __ASSUME_TIME64_SYSCALLS
> +      err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0, abstime);
> +# else
> +      bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec);
> +      if (need_time64)
> +	{
> +	  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op, 0,
> +				       abstime);
> +	}
> +      else
> +	{
> +	  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);

We will hit here only if the timeout if not null, so there is no need to
check it again.

> +	}
> +
> +
> +      /* FUTEX_LOCK_PI2 is not available on this kernel */
> +      if (err == -ENOSYS)
> +	return EINVAL;
> +    }
> +  else
> +    {
> +      /* Otherwise use CLOCK_REALTIME and FUTEX_LOCK_PI */
> +      return futex_lock_pi64 (futex_word, abstime, private);
> +    }
> +#endif 	/* __ASSUME_FUTEX_LOCK_PI2  */
> +

I think we can simplify to only one futex pi operation to something like:

int
futex_lock_pi64 (int *futex_word, const struct __timespec64 *abstime,
                 int private)
{
  int op_pi2 = _lll_private_flag (FUTEX_LOCK_PI2 | clockbit, private);
#if __ASSUME_FUTEX_LOCK_PI2
  /* Assume __ASSUME_TIME64_SYSCALLS.  */
  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi2, 0, abstime);
#else
  int op_pi1 = _lll_private_flag (FUTEX_LOCK_PI | clockbit, private),
  
  /* For CLOCK_MONOTONIC the only option is to use FUTEX_LOCK_PI2.  */
  int op_pi = abstime != NULL && clockid != CLOCK_REALTIME ? op_pi2 : op_pi1;
  
# ifdef __ASSUME_TIME64_SYSCALLS
  err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime);
# else
  bool need_time64 = abstime != NULL && !in_time_t_range (abstime->tv_sec);
  if (need_time64)
    err = INTERNAL_SYSCALL_CALL (futex_time64, futex_word, op_pi, 0, abstime);
  else
    {
      struct timespec ts32, *pts32 = NULL;
      if (abstime != NULL)
      	{
      	  ts32 = valid_timespec64_to_timespec (*abstime);
      	  pts32 = &ts32;
      	}
      err = INTERNAL_SYSCALL_CALL (futex, futex_word, op_pi, 0, &ts32);
    }
# endif	 /* __ASSUME_TIME64_SYSCALLS */
   /* FUTEX_LOCK_PI2 is not available on this kernel */
   if (err == -ENOSYS)
     err =  EINVAL;
#endif /* __ASSUME_FUTEX_LOCK_PI2  */

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


I think there is no much gain in adding another PI futex internal
function, so we can just use the same name.


> +  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 ();
> +    }
> +}
> +
>  /* 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
>  
> 

Ok.

[PATCH v1 3/6] nptl: Use futex_lock_pi2()

[Kurt Kanzenbach]

Currently it is not possible to specify CLOCK_MONOTONIC for timeouts for PI
mutexes. The FUTEX_LOCK_PI2 operation can be used to implement that.

Therefore, use it by default. In case FUTEX_LOCK_PI2 is not available on the
running kernel, then EINVAL is returned as of now.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 nptl/pthread_mutex_timedlock.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
index 5afd6222d61e..95892a32a6af 100644
--- a/nptl/pthread_mutex_timedlock.c
+++ b/nptl/pthread_mutex_timedlock.c
@@ -300,13 +300,6 @@ __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
@@ -370,7 +363,8 @@ __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);
 	    if (e == ETIMEDOUT)
 	      return ETIMEDOUT;
 	    else if (e == ESRCH || e == EDEADLK)
-- 
2.30.2

Re: [PATCH v1 3/6] nptl: Use futex_lock_pi2()

[Adhemerval Zanella]

On 25/06/2021 05:11, Kurt Kanzenbach wrote:
> Currently it is not possible to specify CLOCK_MONOTONIC for timeouts for PI
> mutexes. The FUTEX_LOCK_PI2 operation can be used to implement that.
> 
> Therefore, use it by default. In case FUTEX_LOCK_PI2 is not available on the
> running kernel, then EINVAL is returned as of now.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
> ---
>  nptl/pthread_mutex_timedlock.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/nptl/pthread_mutex_timedlock.c b/nptl/pthread_mutex_timedlock.c
> index 5afd6222d61e..95892a32a6af 100644
> --- a/nptl/pthread_mutex_timedlock.c
> +++ b/nptl/pthread_mutex_timedlock.c
> @@ -300,13 +300,6 @@ __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
> @@ -370,7 +363,8 @@ __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);
>  	    if (e == ETIMEDOUT)
>  	      return ETIMEDOUT;
>  	    else if (e == ESRCH || e == EDEADLK)
> 

I think we should change futex_lock_pi64() to use FUTEX_LOCK_PI2 and only
exports it instead of both futex_lock_pi64() and futex_lock_pi2_64.  It
can simplify the implementation and provide a complete internal futex
functionality (since by using two different PI-futex operation caller
should be aware that only futex_lock_pi2_64 supports FUTEX_LOCK_PI2).

[PATCH v1 4/6] nptl: Remove mutex test 10

[Kurt Kanzenbach]

That test checks whether pthread_mutex_clocklock(MONOTONIC)/PI returns EINVAL.

However, support for that has been implemented. Therefore, remove the test.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 nptl/Makefile        |  2 +-
 nptl/tst-mutexpi10.c | 68 --------------------------------------------
 2 files changed, 1 insertion(+), 69 deletions(-)
 delete mode 100644 nptl/tst-mutexpi10.c

diff --git a/nptl/Makefile b/nptl/Makefile
index c64e4af2f680..5ab25c524fac 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -269,7 +269,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-mutexpi10 \
+	tst-mutexpi9 \
 	tst-cond22 tst-cond26 \
 	tst-robustpi1 tst-robustpi2 tst-robustpi3 tst-robustpi4 tst-robustpi5 \
 	tst-robustpi6 tst-robustpi7 tst-robustpi9 \
diff --git a/nptl/tst-mutexpi10.c b/nptl/tst-mutexpi10.c
deleted file mode 100644
index da781d0d7a93..000000000000
--- a/nptl/tst-mutexpi10.c
+++ /dev/null
@@ -1,68 +0,0 @@
-/* Check if pthread_mutex_clocklock with PRIO_INHERIT fails with clock
-   different than CLOCK_REALTIME.
-   Copyright (C) 2020-2021 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>
-- 
2.30.2

Re: [PATCH v1 4/6] nptl: Remove mutex test 10

[Adhemerval Zanella]

On 25/06/2021 05:11, Kurt Kanzenbach wrote:
> That test checks whether pthread_mutex_clocklock(MONOTONIC)/PI returns EINVAL.
> 
> However, support for that has been implemented. Therefore, remove the test.
> 
> Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>

I think the patch should be kept, but change to FAIL_UNSUPPORTED if the *first*
call return EINVAL and trigger a real failure if first type success but
subsequent types returns EINVAL.

> ---
>  nptl/Makefile        |  2 +-
>  nptl/tst-mutexpi10.c | 68 --------------------------------------------
>  2 files changed, 1 insertion(+), 69 deletions(-)
>  delete mode 100644 nptl/tst-mutexpi10.c
> 
> diff --git a/nptl/Makefile b/nptl/Makefile
> index c64e4af2f680..5ab25c524fac 100644
> --- a/nptl/Makefile
> +++ b/nptl/Makefile
> @@ -269,7 +269,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-mutexpi10 \
> +	tst-mutexpi9 \
>  	tst-cond22 tst-cond26 \
>  	tst-robustpi1 tst-robustpi2 tst-robustpi3 tst-robustpi4 tst-robustpi5 \
>  	tst-robustpi6 tst-robustpi7 tst-robustpi9 \
> diff --git a/nptl/tst-mutexpi10.c b/nptl/tst-mutexpi10.c
> deleted file mode 100644
> index da781d0d7a93..000000000000
> --- a/nptl/tst-mutexpi10.c
> +++ /dev/null
> @@ -1,68 +0,0 @@
> -/* Check if pthread_mutex_clocklock with PRIO_INHERIT fails with clock
> -   different than CLOCK_REALTIME.
> -   Copyright (C) 2020-2021 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>
> 

[PATCH v1 5/6] nptl: mutex-test5: Include CLOCK_MONOTONIC for PI

[Kurt Kanzenbach]

pthread_mutex_clocklock(MONOTONIC)/PI is now supported.

Adjust the test accordingly.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 sysdeps/pthread/tst-mutex5.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/sysdeps/pthread/tst-mutex5.c b/sysdeps/pthread/tst-mutex5.c
index 7dc5331cfc08..6ad50f845601 100644
--- a/sysdeps/pthread/tst-mutex5.c
+++ b/sysdeps/pthread/tst-mutex5.c
@@ -77,11 +77,21 @@ do_test_clock (clockid_t clockid, const char *fnname)
                                              make_timespec (2, 0));
 
   if (clockid == CLOCK_USE_TIMEDLOCK)
-    TEST_COMPARE (pthread_mutex_timedlock (&m, &ts_timeout), ETIMEDOUT);
+    {
+      err = pthread_mutex_timedlock (&m, &ts_timeout);
+      TEST_COMPARE (err, ETIMEDOUT);
+    }
   else
-    TEST_COMPARE (pthread_mutex_clocklock (&m, clockid, &ts_timeout),
-		  ETIMEDOUT);
-  TEST_TIMESPEC_BEFORE_NOW (ts_timeout, clockid_for_get);
+    {
+      err = pthread_mutex_clocklock (&m, clockid, &ts_timeout);
+
+      /* In case of CLOCK_MONOTONIC the error might be EINVAL if CLOCK_MONOTONIC
+         is not supported. */
+      TEST_VERIFY (err == ETIMEDOUT ||
+		   (clockid == CLOCK_MONOTONIC && err == EINVAL));
+    }
+  if (err == ETIMEDOUT)
+    TEST_TIMESPEC_BEFORE_NOW (ts_timeout, clockid_for_get);
 
   /* The following makes the ts value invalid.  */
   ts_timeout.tv_nsec += 1000000000;
@@ -122,9 +132,7 @@ 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;
 }
 
-- 
2.30.2

[PATCH v1 6/6] nptl: mutex-test9: Include CLOCK_MONOTONIC for PI

[Kurt Kanzenbach]

pthread_mutex_clocklock(MONOTONIC)/PI is now supported.

Adjust the test accordingly.

Signed-off-by: Kurt Kanzenbach <kurt@linutronix.de>
---
 sysdeps/pthread/tst-mutex9.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/sysdeps/pthread/tst-mutex9.c b/sysdeps/pthread/tst-mutex9.c
index 58c3a1aec263..d53eda841551 100644
--- a/sysdeps/pthread/tst-mutex9.c
+++ b/sysdeps/pthread/tst-mutex9.c
@@ -114,7 +114,12 @@ do_test_clock (clockid_t clockid)
       if (clockid == CLOCK_USE_TIMEDLOCK)
         TEST_COMPARE (pthread_mutex_timedlock (m, &ts), ETIMEDOUT);
       else
-        TEST_COMPARE (pthread_mutex_clocklock (m, clockid, &ts), ETIMEDOUT);
+	{
+	  int err = pthread_mutex_clocklock (m, clockid, &ts);
+
+	  TEST_VERIFY (err == ETIMEDOUT ||
+		       (clockid == CLOCK_MONOTONIC && err == EINVAL));
+	}
 
       alarm (1);
 
@@ -144,9 +149,7 @@ 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.30.2

Re: [PATCH v1 0/6] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 1585 bytes --]

On Fri Jun 25 2021, Kurt Kanzenbach wrote:
> Linux real time applications 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 didn't support it, yet. Using CLOCK_REALTIME is possible,
> but it is subject to adjustments (NTP, PTP, ...). Therefore, Thomas proposed and
> introduced a new futex operation called FUTEX_LOCK_PI2 with support for
> selectable clocks [1].
>
> 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.
>
> There's also a bugreport for glibc with a test case:
>
>  https://sourceware.org/bugzilla/show_bug.cgi?id=26801
>
> Changes since RFC:
>
>  * FUTEX_LOCK_PI2 merged in upstream Linux kernel (Thomas Gleixner, Peter Zijlstra)
>  * Make of __ASSUME_FUTEX_LOCK_PI2 in the implementation (Adhemerval Zanella)
>  * Get rid of probing for FUTEX_LOCK_PI2 in timedlock (Adhemerval Zanella)
>  * Adjust the test cases (Adhemerval Zanella)
>  * Use correct Linux kernel version (Joseph Myers)
>
> Previous versions:
>
>  * https://sourceware.org/pipermail/libc-alpha/2021-June/127798.html

Ping. Any comments on this version?

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]

Re: [PATCH v1 0/6] nptl: Introduce and use FUTEX_LOCK_PI2

[Adhemerval Zanella]

On 25/06/2021 05:10, Kurt Kanzenbach wrote:
> Linux real time applications 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 didn't support it, yet. Using CLOCK_REALTIME is possible,
> but it is subject to adjustments (NTP, PTP, ...). Therefore, Thomas proposed and
> introduced a new futex operation called FUTEX_LOCK_PI2 with support for
> selectable clocks [1].
> 
> 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.
> 
> There's also a bugreport for glibc with a test case:
> 
>  https://sourceware.org/bugzilla/show_bug.cgi?id=26801
> 
> Changes since RFC:
> 
>  * FUTEX_LOCK_PI2 merged in upstream Linux kernel (Thomas Gleixner, Peter Zijlstra)
>  * Make of __ASSUME_FUTEX_LOCK_PI2 in the implementation (Adhemerval Zanella)
>  * Get rid of probing for FUTEX_LOCK_PI2 in timedlock (Adhemerval Zanella)
>  * Adjust the test cases (Adhemerval Zanella)
>  * Use correct Linux kernel version (Joseph Myers)
> 
> Previous versions:
> 
>  * https://sourceware.org/pipermail/libc-alpha/2021-June/127798.html
> 
> Thanks,
> Kurt
> 
> [1] - https://lkml.kernel.org/lkml/20210422194417.866740847@linutronix.de/

Patch looks ok in general, I think I have only minor comments.  I see that
FUTEX_LOCK_PI2 is already on Linus tree, should it should be safe to assume
it will on v5.14 (although is not yet released).

We are in the slush freeze for the 2.34 release, so you would like to check
with Carlos O'Donell if this set is suitable to inclusion.  It does not
change any ABI, but I will need to some time to boot a kernel to actually
test it.

Re: [PATCH v1 0/6] nptl: Introduce and use FUTEX_LOCK_PI2

[Kurt Kanzenbach]

[-- Attachment #1: Type: text/plain, Size: 743 bytes --]

On Fri Jul 09 2021, Adhemerval Zanella wrote:
> Patch looks ok in general, I think I have only minor comments.  I see that
> FUTEX_LOCK_PI2 is already on Linus tree, should it should be safe to assume
> it will on v5.14 (although is not yet released).
>
> We are in the slush freeze for the 2.34 release, so you would like to check
> with Carlos O'Donell if this set is suitable to inclusion.  It does not
> change any ABI, but I will need to some time to boot a kernel to actually
> test it.

There is no hurry. I just wanted to make sure that the implementation is
moving into the right direction. I'll integrate your review comments and
post v2 when Linux v5.14.0 is actually released. That should make it
easier for testing.

Thanks,
Kurt

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 861 bytes --]