[03/13] nptl: Remove clockwait_tid

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

Commit Message

Fangrui Song via Libc-alpha Nov. 23, 2020, 7:52 p.m.
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(-)

-- 
2.25.1

Comments

Lukasz Majewski Nov. 24, 2020, 6:13 p.m. | #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
Fangrui Song via Libc-alpha Dec. 14, 2020, 12:16 p.m. | #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
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Andreas Schwab Dec. 14, 2020, 12:47 p.m. | #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.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."
Fangrui Song via Libc-alpha Dec. 14, 2020, 12:52 p.m. | #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);
  [...]
Fangrui Song via Libc-alpha Dec. 14, 2020, 1:11 p.m. | #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
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill
Fangrui Song via Libc-alpha Dec. 14, 2020, 2:02 p.m. | #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
-- 
Red Hat GmbH, https://de.redhat.com/ , Registered seat: Grasbrunn,
Commercial register: Amtsgericht Muenchen, HRB 153243,
Managing Directors: Charles Cachera, Brian Klemm, Laurie Krebs, Michael O'Neill

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.  */