diff mbox series

[03/13] nptl: Remove clockwait_tid

Message ID 20201123195256.3336217-3-adhemerval.zanella@linaro.org
State Committed
Headers show
Series [01/13] linux: Remove unused internal futex functions | expand

Commit Message

Adhemerval Zanella Nov. 23, 2020, 7:52 p.m. UTC
It can be replaced with a __futex_abstimed_wait_cancelable64 call,
with the advantage that there is no need to further clock adjustments
to create a absolute timeout.  It allows to remove the now ununsed
futex_timed_wait_cancel64 internal function.

Checked on x86_64-linux-gnu and i686-linux-gnu.
---
 nptl/pthread_join_common.c    | 70 ++++++-----------------------------
 sysdeps/nptl/futex-internal.h | 49 ------------------------
 2 files changed, 12 insertions(+), 107 deletions(-)

Comments

Lukasz Majewski Nov. 24, 2020, 6:13 p.m. UTC | #1
On Mon, 23 Nov 2020 16:52:46 -0300
Adhemerval Zanella via Libc-alpha <libc-alpha@sourceware.org> wrote:

> It can be replaced with a __futex_abstimed_wait_cancelable64 call,
> with the advantage that there is no need to further clock adjustments
> to create a absolute timeout.  It allows to remove the now ununsed
> futex_timed_wait_cancel64 internal function.
> 
> Checked on x86_64-linux-gnu and i686-linux-gnu.
> ---
>  nptl/pthread_join_common.c    | 70
> ++++++----------------------------- sysdeps/nptl/futex-internal.h |
> 49 ------------------------ 2 files changed, 12 insertions(+), 107
> deletions(-)
> 
> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
> index 67d8e2b780..8a95c58ff3 100644
> --- a/nptl/pthread_join_common.c
> +++ b/nptl/pthread_join_common.c
> @@ -32,55 +32,6 @@ cleanup (void *arg)
>    atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
>  }
>  
> -/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via
> futex
> -   wake-up when the clone terminates.  The memory location contains
> the
> -   thread ID while the clone is running and is reset to zero by the
> kernel
> -   afterwards.  The kernel up to version 3.16.3 does not use the
> private futex
> -   operations for futex wake-up when the clone terminates.  */
> -static int
> -clockwait_tid (pid_t *tidp, clockid_t clockid,
> -               const struct __timespec64 *abstime)
> -{
> -  pid_t tid;
> -  int ret;
> -
> -  if (! valid_nanoseconds (abstime->tv_nsec))
> -    return EINVAL;
> -
> -  /* Repeat until thread terminated.  */
> -  while ((tid = *tidp) != 0)
> -    {
> -      struct __timespec64 rt;
> -
> -      /* Get the current time. This can only fail if clockid is
> -         invalid. */
> -      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
> -        return EINVAL;
> -
> -      /* Compute relative timeout.  */
> -      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
> -      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
> -      if (rt.tv_nsec < 0)
> -        {
> -          rt.tv_nsec += 1000000000;
> -          --rt.tv_sec;
> -        }
> -
> -      /* Already timed out?  */
> -      if (rt.tv_sec < 0)
> -        return ETIMEDOUT;
> -
> -      /* If *tidp == tid, wait until thread terminates or the wait
> times out.
> -         The kernel up to version 3.16.3 does not use the private
> futex
> -         operations for futex wake-up when the clone terminates.  */
> -      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
> -      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
> -        return -ret;
> -    }
> -
> -  return 0;
> -}
> -
>  int
>  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
>                          clockid_t clockid,
> @@ -137,15 +88,18 @@ __pthread_clockjoin_ex (pthread_t threadid, void
> **thread_return, un-wait-ed for again.  */
>        pthread_cleanup_push (cleanup, &pd->joinid);
>  
> -      if (abstime != NULL)
> -	result = clockwait_tid (&pd->tid, clockid, abstime);
> -      else
> -	{
> -	  pid_t tid;
> -	  /* We need acquire MO here so that we synchronize with the
> -	     kernel's store to 0 when the clone terminates. (see
> above)  */
> -	  while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> -	    lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
> +     /* We need acquire MO here so that we synchronize with the
> +        kernel's store to 0 when the clone terminates. (see above)
> */
> +      pid_t tid;
> +      while ((tid = atomic_load_acquire (&pd->tid)) != 0)
> +        {
> +	  int ret = __futex_abstimed_wait_cancelable64 (
> +	    (unsigned int *) &pd->tid, tid, clockid, abstime,
> LLL_SHARED);
> +	  if (ret == ETIMEDOUT || ret == EOVERFLOW)
> +	    {
> +	      result = ret;
> +	      break;
> +	    }
>  	}
>  
>        pthread_cleanup_pop (0);
> diff --git a/sysdeps/nptl/futex-internal.h
> b/sysdeps/nptl/futex-internal.h index 96d1318091..d5f13d15fb 100644
> --- a/sysdeps/nptl/futex-internal.h
> +++ b/sysdeps/nptl/futex-internal.h
> @@ -390,55 +390,6 @@ futex_unlock_pi (unsigned int *futex_word, int
> private) }
>  }
>  
> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> -                           const struct __timespec64 *timeout, int
> private) -{
> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> -                                     __lll_private_flag (FUTEX_WAIT,
> private),
> -                                     tid, timeout);
> -#ifndef __ASSUME_TIME64_SYSCALLS
> -  if (err == -ENOSYS)
> -    {
> -      if (in_time_t_range (timeout->tv_sec))
> -        {
> -          struct timespec ts32 = valid_timespec64_to_timespec
> (*timeout); -
> -          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
> -                                         __lll_private_flag
> (FUTEX_WAIT,
> -
> private),
> -                                         tid, &ts32);
> -        }
> -      else
> -        err = -EOVERFLOW;
> -    }
> -#endif
> -  switch (err)
> -    {
> -    case 0:
> -    case -EAGAIN:
> -    case -EINTR:
> -    case -ETIMEDOUT:
> -    case -EDEADLK:
> -    case -ENOSYS:
> -    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t
> type, but
> -                         underlying kernel does not support 64 bit
> time_t futex
> -                         syscalls.  */
> -    case -EPERM:  /*  The caller is not allowed to attach itself to
> the futex.
> -		      Used to check if PI futexes are supported by
> the
> -		      kernel.  */
> -      return -err;
> -
> -    case -EINVAL: /* Either due to wrong alignment or due to the
> timeout not
> -		     being normalized.  Must have been caused by a
> glibc or
> -		     application bug.  */
> -    case -EFAULT: /* Must have been caused by a glibc or application
> bug.  */
> -    /* No other errors are documented at this time.  */
> -    default:
> -      futex_fatal_error ();
> -    }
> -}
> -
>  /* The futex_abstimed_wait_cancelable64 has been moved to a separate
> file to avoid problems with exhausting available registers on some
> architectures
>     - e.g. on m68k architecture.  */

Reviewed-by: Lukasz Majewski <lukma@denx.de>


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de
Florian Weimer Dec. 14, 2020, 12:16 p.m. UTC | #2
* Adhemerval Zanella via Libc-alpha:

> -static __always_inline int
> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
> -                           const struct __timespec64 *timeout, int private)
> -{
> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
> -                                     __lll_private_flag (FUTEX_WAIT, private),
> -                                     tid, timeout);

This uses FUTEX_WAIT.  But the replacement,
__futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.  I do not think
this is correct because the kernel will use FUTEX_WAKE internally for
the pd->tid wakeup relied upon by pthread_join.

This seems to cause pthread_join regressions on some kernel versions.

We need to audit all callers of __futex_abstimed_wait64 if they are
actually compatible with FUTEX_WAIT_BITSET.

Thanks,
Florian
Andreas Schwab Dec. 14, 2020, 12:47 p.m. UTC | #3
On Dez 14 2020, Florian Weimer via Libc-alpha wrote:

> This uses FUTEX_WAIT.  But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.

FUTEX_WAIT is exactly equivalent to
FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.

Andreas.
Adhemerval Zanella Dec. 14, 2020, 12:52 p.m. UTC | #4
On 14/12/2020 09:16, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> -static __always_inline int
>> -futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
>> -                           const struct __timespec64 *timeout, int private)
>> -{
>> -  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
>> -                                     __lll_private_flag (FUTEX_WAIT, private),
>> -                                     tid, timeout);
> 
> This uses FUTEX_WAIT.  But the replacement,
> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.  I do not think
> this is correct because the kernel will use FUTEX_WAKE internally for
> the pd->tid wakeup relied upon by pthread_join.
> 
> This seems to cause pthread_join regressions on some kernel versions.
> 
> We need to audit all callers of __futex_abstimed_wait64 if they are
> actually compatible with FUTEX_WAIT_BITSET.

I don't think it should matter, as Andreas has put FUTEX_WAIT is exactly
as FUTEX_WAIT_BITSET plus FUTEX_BITSET_MATCH_ANY.  On a recent kernel:


  SYSCALL_DEFINE6(futex, u32 __user *, uaddr, int, op, u32, val,
                  struct __kernel_timespec __user *, utime, u32 __user *, uaddr2,
                  u32, val3)
  {
  [...]
    return do_futex(uaddr, op, val, tp, uaddr2, val2, val3);
  }

  long do_futex(u32 __user *uaddr, int op, u32 val, ktime_t *timeout,
                u32 __user *uaddr2, u32 val2, u32 val3)
  {
  [...]
          case FUTEX_WAIT:
                  val3 = FUTEX_BITSET_MATCH_ANY;
                  fallthrough;
          case FUTEX_WAIT_BITSET:
                  return futex_wait(uaddr, flags, val, timeout, val3);
  [...]
Florian Weimer Dec. 14, 2020, 1:11 p.m. UTC | #5
* Andreas Schwab:

> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>
>> This uses FUTEX_WAIT.  But the replacement,
>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>
> FUTEX_WAIT is exactly equivalent to
> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.

Hmm.  I will continue debugging.  I encounter a missed wakeup in
pthread_join after making some changes that only should affect timing
(intl/tst-gettext6 is among the failures).

Reverting this patch here seems to help.

Thanks,
Florian
Florian Weimer Dec. 14, 2020, 2:02 p.m. UTC | #6
* Florian Weimer:

> * Andreas Schwab:
>
>> On Dez 14 2020, Florian Weimer via Libc-alpha wrote:
>>
>>> This uses FUTEX_WAIT.  But the replacement,
>>> __futex_abstimed_wait_common64, uses FUTEX_WAIT_BITSET.
>>
>> FUTEX_WAIT is exactly equivalent to
>> FUTEX_WAIT_BITSET/FUTEX_BITSET_MATCH_ANY.
>
> Hmm.  I will continue debugging.  I encounter a missed wakeup in
> pthread_join after making some changes that only should affect timing
> (intl/tst-gettext6 is among the failures).
>
> Reverting this patch here seems to help.

It's an accident, due to my changes and:

  /* In libc.so or ld.so all futexes are private.  */

in lowlevellock-futex.h.  So the patch posted here should be okay.

Thanks,
Florian
diff mbox series

Patch

diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 67d8e2b780..8a95c58ff3 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -32,55 +32,6 @@  cleanup (void *arg)
   atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
 }
 
-/* The kernel notifies a process which uses CLONE_CHILD_CLEARTID via futex
-   wake-up when the clone terminates.  The memory location contains the
-   thread ID while the clone is running and is reset to zero by the kernel
-   afterwards.  The kernel up to version 3.16.3 does not use the private futex
-   operations for futex wake-up when the clone terminates.  */
-static int
-clockwait_tid (pid_t *tidp, clockid_t clockid,
-               const struct __timespec64 *abstime)
-{
-  pid_t tid;
-  int ret;
-
-  if (! valid_nanoseconds (abstime->tv_nsec))
-    return EINVAL;
-
-  /* Repeat until thread terminated.  */
-  while ((tid = *tidp) != 0)
-    {
-      struct __timespec64 rt;
-
-      /* Get the current time. This can only fail if clockid is
-         invalid. */
-      if (__glibc_unlikely (__clock_gettime64 (clockid, &rt)))
-        return EINVAL;
-
-      /* Compute relative timeout.  */
-      rt.tv_sec = abstime->tv_sec - rt.tv_sec;
-      rt.tv_nsec = abstime->tv_nsec - rt.tv_nsec;
-      if (rt.tv_nsec < 0)
-        {
-          rt.tv_nsec += 1000000000;
-          --rt.tv_sec;
-        }
-
-      /* Already timed out?  */
-      if (rt.tv_sec < 0)
-        return ETIMEDOUT;
-
-      /* If *tidp == tid, wait until thread terminates or the wait times out.
-         The kernel up to version 3.16.3 does not use the private futex
-         operations for futex wake-up when the clone terminates.  */
-      ret = futex_timed_wait_cancel64 (tidp, tid, &rt, LLL_SHARED);
-      if (ret == -ETIMEDOUT || ret == -EOVERFLOW)
-        return -ret;
-    }
-
-  return 0;
-}
-
 int
 __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
                         clockid_t clockid,
@@ -137,15 +88,18 @@  __pthread_clockjoin_ex (pthread_t threadid, void **thread_return,
 	 un-wait-ed for again.  */
       pthread_cleanup_push (cleanup, &pd->joinid);
 
-      if (abstime != NULL)
-	result = clockwait_tid (&pd->tid, clockid, abstime);
-      else
-	{
-	  pid_t tid;
-	  /* We need acquire MO here so that we synchronize with the
-	     kernel's store to 0 when the clone terminates. (see above)  */
-	  while ((tid = atomic_load_acquire (&pd->tid)) != 0)
-	    lll_futex_wait_cancel (&pd->tid, tid, LLL_SHARED);
+     /* We need acquire MO here so that we synchronize with the
+        kernel's store to 0 when the clone terminates. (see above)  */
+      pid_t tid;
+      while ((tid = atomic_load_acquire (&pd->tid)) != 0)
+        {
+	  int ret = __futex_abstimed_wait_cancelable64 (
+	    (unsigned int *) &pd->tid, tid, clockid, abstime, LLL_SHARED);
+	  if (ret == ETIMEDOUT || ret == EOVERFLOW)
+	    {
+	      result = ret;
+	      break;
+	    }
 	}
 
       pthread_cleanup_pop (0);
diff --git a/sysdeps/nptl/futex-internal.h b/sysdeps/nptl/futex-internal.h
index 96d1318091..d5f13d15fb 100644
--- a/sysdeps/nptl/futex-internal.h
+++ b/sysdeps/nptl/futex-internal.h
@@ -390,55 +390,6 @@  futex_unlock_pi (unsigned int *futex_word, int private)
     }
 }
 
-static __always_inline int
-futex_timed_wait_cancel64 (pid_t *tidp,  pid_t tid,
-                           const struct __timespec64 *timeout, int private)
-{
-  int err = INTERNAL_SYSCALL_CANCEL (futex_time64, tidp,
-                                     __lll_private_flag (FUTEX_WAIT, private),
-                                     tid, timeout);
-#ifndef __ASSUME_TIME64_SYSCALLS
-  if (err == -ENOSYS)
-    {
-      if (in_time_t_range (timeout->tv_sec))
-        {
-          struct timespec ts32 = valid_timespec64_to_timespec (*timeout);
-
-          err = INTERNAL_SYSCALL_CANCEL (futex, tidp,
-                                         __lll_private_flag (FUTEX_WAIT,
-                                                             private),
-                                         tid, &ts32);
-        }
-      else
-        err = -EOVERFLOW;
-    }
-#endif
-  switch (err)
-    {
-    case 0:
-    case -EAGAIN:
-    case -EINTR:
-    case -ETIMEDOUT:
-    case -EDEADLK:
-    case -ENOSYS:
-    case -EOVERFLOW:  /* Passed absolute timeout uses 64 bit time_t type, but
-                         underlying kernel does not support 64 bit time_t futex
-                         syscalls.  */
-    case -EPERM:  /*  The caller is not allowed to attach itself to the futex.
-		      Used to check if PI futexes are supported by the
-		      kernel.  */
-      return -err;
-
-    case -EINVAL: /* Either due to wrong alignment or due to the timeout not
-		     being normalized.  Must have been caused by a glibc or
-		     application bug.  */
-    case -EFAULT: /* Must have been caused by a glibc or application bug.  */
-    /* No other errors are documented at this time.  */
-    default:
-      futex_fatal_error ();
-    }
-}
-
 /* The futex_abstimed_wait_cancelable64 has been moved to a separate file
    to avoid problems with exhausting available registers on some architectures
    - e.g. on m68k architecture.  */