diff mbox series

nptl: Reinstate pthread_timedjoin_np as a cancellation point (BZ#24215)

Message ID 20190212201819.23399-1-adhemerval.zanella@linaro.org
State Accepted
Commit eb76e5b465a4b7b569cde4b4f57d1fcb4695c1c6
Headers show
Series nptl: Reinstate pthread_timedjoin_np as a cancellation point (BZ#24215) | expand

Commit Message

Adhemerval Zanella Feb. 12, 2019, 8:18 p.m. UTC
Patch ce7eb0e90315 ("nptl: Cleanup cancellation macros") changed the
join sequence for internal common __pthread_timedjoin_ex to use the
new macro lll_wait_tid.  The idea was this macro would issue the
cancellable futex operation depending whether the timeout is used or
not.  However if a timeout is used, __lll_timedwait_tid is called and
it is not a cancellable entrypoint.

This patch fixes it by simplifying the code in various ways:

  - Instead of adding the cancellation handling on __lll_timedwait_tid,
    it moves the generic implementation to pthread_join_common.c (called
    now timedwait_tid with some fixes to use the correct type for pid).

  - The llvm_wait_tid macro is removed, along with its replication on
    x86_64, i686, and sparc arch-specific lowlevellock.h.

  - sparc32 __lll_timedwait_tid is also removed, since the code is similar
    to generic one.

  - x86_64 and i386 provides arch-specific __lll_timedwait_tid which is
    also removed since they are similar in functionality to generic C code
    and there is no indication it is better than compiler generated code.

New tests, tst-join8 and tst-join9, are provided to check if
pthread_timedjoin_np acts as a cancellation point.

Checked on x86_64-linux-gnu, i686-linux-gnu, sparcv9-linux-gnu, and
aarch64-linux-gnu.

	[BZ #2421]
	* nptl/Makefile (lpthread-routines): Remove lll_timedwait_tid.
	(tests): Add tst-join8 tst-join9.
	* nptl/lll_timedwait_tid.c: Remove file.
	* sysdeps/sparc/sparc32/lll_timedwait_tid.c: Likewise.
	* sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c: Likewise.
	* sysdeps/sysv/linux/x86_64/lll_timedwait_tid.c: Likewise.
	* nptl/pthread_join_common.c (timedwait_tid): New function.
	(__pthread_timedjoin_ex): Act as cancellation entrypoint is block
	is set.
	* nptl/tst-join5.c (thread_join): New function.
	(tf1, tf2, do_test): Use libsupport and add pthread_timedjoin_np
	check.
	* nptl/tst-join8.c: New file.
	* nptl/tst-join9.c: Likewise.
	* sysdeps/nptl/lowlevellock-futex.h (lll_futex_wait_cancel,
	lll_futex_timed_wait_cancel): Add generic macros.
	* sysdeps/nptl/lowlevellock.h (__lll_timedwait_tid, lll_wait_tid):
	Remove definitions.
	* sysdeps/unix/sysv/linux/i386/lowlevellock.h: Likewise.
	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h: Likewise.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Likewise.
	* sysdeps/sparc/sparc32/lowlevellock.c (__lll_timedwait_tid):
	Remove function.
	* sysdeps/unix/sysv/linux/i386/lowlevellock.S (__lll_timedwait_tid):
	Likewise.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: Likewise.
	* sysdeps/unix/sysv/linux/lowlevellock-futex.h
	(lll_futex_timed_wait_cancel): New macro.
---
 nptl/Makefile                                 |  3 +-
 nptl/lll_timedwait_tid.c                      | 70 --------------
 nptl/pthread_join_common.c                    | 57 +++++++++++-
 nptl/tst-join5.c                              | 92 +++++++------------
 nptl/tst-join8.c                              | 20 ++++
 nptl/tst-join9.c                              | 21 +++++
 sysdeps/nptl/lowlevellock-futex.h             |  7 ++
 sysdeps/nptl/lowlevellock.h                   | 26 ------
 sysdeps/sparc/sparc32/lll_timedwait_tid.c     |  1 -
 sysdeps/sparc/sparc32/lowlevellock.c          | 40 --------
 .../unix/sysv/linux/i386/lll_timedwait_tid.c  |  1 -
 sysdeps/unix/sysv/linux/i386/lowlevellock.S   | 66 -------------
 sysdeps/unix/sysv/linux/i386/lowlevellock.h   | 25 -----
 sysdeps/unix/sysv/linux/lowlevellock-futex.h  |  7 ++
 sysdeps/unix/sysv/linux/sparc/lowlevellock.h  | 25 -----
 .../sysv/linux/x86_64/lll_timedwait_tid.c     |  1 -
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.S | 83 -----------------
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 25 -----
 18 files changed, 147 insertions(+), 423 deletions(-)
 delete mode 100644 nptl/lll_timedwait_tid.c
 create mode 100644 nptl/tst-join8.c
 create mode 100644 nptl/tst-join9.c
 delete mode 100644 sysdeps/sparc/sparc32/lll_timedwait_tid.c
 delete mode 100644 sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c
 delete mode 100644 sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c

-- 
2.17.1

Comments

Carlos O'Donell Feb. 13, 2019, 2:09 p.m. UTC | #1
On 2/12/19 3:18 PM, Adhemerval Zanella wrote:
> Patch ce7eb0e90315 ("nptl: Cleanup cancellation macros") changed the

> join sequence for internal common __pthread_timedjoin_ex to use the

> new macro lll_wait_tid.  The idea was this macro would issue the

> cancellable futex operation depending whether the timeout is used or

> not.  However if a timeout is used, __lll_timedwait_tid is called and

> it is not a cancellable entrypoint.


Thanks for following up on this.

OK for master with additional comment below (see Fix:).

Once you commit I'll clean up the straggling comments that I fixed
before (pthread_tryjoin.c still has a wrong comment).

Reviewed-by: Carlos O'Donell <carlos@redhat.com>


> This patch fixes it by simplifying the code in various ways:

> 

>   - Instead of adding the cancellation handling on __lll_timedwait_tid,

>     it moves the generic implementation to pthread_join_common.c (called

>     now timedwait_tid with some fixes to use the correct type for pid).

> 

>   - The llvm_wait_tid macro is removed, along with its replication on

>     x86_64, i686, and sparc arch-specific lowlevellock.h.

> 

>   - sparc32 __lll_timedwait_tid is also removed, since the code is similar

>     to generic one.

> 

>   - x86_64 and i386 provides arch-specific __lll_timedwait_tid which is

>     also removed since they are similar in functionality to generic C code

>     and there is no indication it is better than compiler generated code.


Yay!


> New tests, tst-join8 and tst-join9, are provided to check if

> pthread_timedjoin_np acts as a cancellation point.


Double yay!

> Checked on x86_64-linux-gnu, i686-linux-gnu, sparcv9-linux-gnu, and

> aarch64-linux-gnu.

> 

> 	[BZ #2421]

> 	* nptl/Makefile (lpthread-routines): Remove lll_timedwait_tid.

> 	(tests): Add tst-join8 tst-join9.

> 	* nptl/lll_timedwait_tid.c: Remove file.

> 	* sysdeps/sparc/sparc32/lll_timedwait_tid.c: Likewise.

> 	* sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c: Likewise.

> 	* sysdeps/sysv/linux/x86_64/lll_timedwait_tid.c: Likewise.

> 	* nptl/pthread_join_common.c (timedwait_tid): New function.

> 	(__pthread_timedjoin_ex): Act as cancellation entrypoint is block

> 	is set.

> 	* nptl/tst-join5.c (thread_join): New function.

> 	(tf1, tf2, do_test): Use libsupport and add pthread_timedjoin_np

> 	check.

> 	* nptl/tst-join8.c: New file.

> 	* nptl/tst-join9.c: Likewise.

> 	* sysdeps/nptl/lowlevellock-futex.h (lll_futex_wait_cancel,

> 	lll_futex_timed_wait_cancel): Add generic macros.

> 	* sysdeps/nptl/lowlevellock.h (__lll_timedwait_tid, lll_wait_tid):

> 	Remove definitions.

> 	* sysdeps/unix/sysv/linux/i386/lowlevellock.h: Likewise.

> 	* sysdeps/unix/sysv/linux/sparc/lowlevellock.h: Likewise.

> 	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h: Likewise.

> 	* sysdeps/sparc/sparc32/lowlevellock.c (__lll_timedwait_tid):

> 	Remove function.

> 	* sysdeps/unix/sysv/linux/i386/lowlevellock.S (__lll_timedwait_tid):

> 	Likewise.

> 	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.S: Likewise.

> 	* sysdeps/unix/sysv/linux/lowlevellock-futex.h

> 	(lll_futex_timed_wait_cancel): New macro.

> ---

>  nptl/Makefile                                 |  3 +-

>  nptl/lll_timedwait_tid.c                      | 70 --------------

>  nptl/pthread_join_common.c                    | 57 +++++++++++-

>  nptl/tst-join5.c                              | 92 +++++++------------

>  nptl/tst-join8.c                              | 20 ++++

>  nptl/tst-join9.c                              | 21 +++++

>  sysdeps/nptl/lowlevellock-futex.h             |  7 ++

>  sysdeps/nptl/lowlevellock.h                   | 26 ------

>  sysdeps/sparc/sparc32/lll_timedwait_tid.c     |  1 -

>  sysdeps/sparc/sparc32/lowlevellock.c          | 40 --------

>  .../unix/sysv/linux/i386/lll_timedwait_tid.c  |  1 -

>  sysdeps/unix/sysv/linux/i386/lowlevellock.S   | 66 -------------

>  sysdeps/unix/sysv/linux/i386/lowlevellock.h   | 25 -----

>  sysdeps/unix/sysv/linux/lowlevellock-futex.h  |  7 ++

>  sysdeps/unix/sysv/linux/sparc/lowlevellock.h  | 25 -----

>  .../sysv/linux/x86_64/lll_timedwait_tid.c     |  1 -

>  sysdeps/unix/sysv/linux/x86_64/lowlevellock.S | 83 -----------------

>  sysdeps/unix/sysv/linux/x86_64/lowlevellock.h | 25 -----

>  18 files changed, 147 insertions(+), 423 deletions(-)

>  delete mode 100644 nptl/lll_timedwait_tid.c

>  create mode 100644 nptl/tst-join8.c

>  create mode 100644 nptl/tst-join9.c

>  delete mode 100644 sysdeps/sparc/sparc32/lll_timedwait_tid.c

>  delete mode 100644 sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c

>  delete mode 100644 sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c

> 

> diff --git a/nptl/Makefile b/nptl/Makefile

> index 0e316edfac..e79ca8f064 100644

> --- a/nptl/Makefile

> +++ b/nptl/Makefile

> @@ -120,7 +120,7 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \

>  		      pt-longjmp pt-cleanup\

>  		      cancellation \

>  		      lowlevellock \

> -		      lll_timedlock_wait lll_timedwait_tid \

> +		      lll_timedlock_wait \


OK.

>  		      pt-fork pt-vfork pt-fcntl \

>  		      $(pthread-compat-wrappers) \

>  		      pt-raise pt-system \

> @@ -271,6 +271,7 @@ tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \

>  	tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \

>  	tst-raise1 \

>  	tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \

> +	tst-join8 tst-join9 \


OK.

>  	tst-detach1 \

>  	tst-eintr1 tst-eintr2 tst-eintr3 tst-eintr4 tst-eintr5 \

>  	tst-tsd1 tst-tsd2 tst-tsd3 tst-tsd4 tst-tsd5 tst-tsd6 \

> diff --git a/nptl/lll_timedwait_tid.c b/nptl/lll_timedwait_tid.c

> deleted file mode 100644

> index 59ecbb4446..0000000000

> --- a/nptl/lll_timedwait_tid.c

> +++ /dev/null

> @@ -1,70 +0,0 @@

> -/* Timed waiting for thread death.  Generic futex-using version.

> -   Copyright (C) 2003-2019 Free Software Foundation, Inc.

> -   This file is part of the GNU C Library.

> -   Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003.

> -

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

> -   <http://www.gnu.org/licenses/>.  */

> -

> -#include <atomic.h>

> -#include <errno.h>

> -#include <lowlevellock.h>

> -#include <sys/time.h>

> -

> -

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

> -int

> -__lll_timedwait_tid (int *tidp, const struct timespec *abstime)

> -{

> -  int tid;

> -

> -  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)

> -    return EINVAL;

> -

> -  /* Repeat until thread terminated.  */

> -  while ((tid = *tidp) != 0)

> -    {

> -      struct timeval tv;

> -      struct timespec rt;

> -

> -      /* Get the current time.  */

> -      (void) __gettimeofday (&tv, NULL);

> -

> -      /* Compute relative timeout.  */

> -      rt.tv_sec = abstime->tv_sec - tv.tv_sec;

> -      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;

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

> -      */

> -      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)

> -        return ETIMEDOUT;

> -    }

> -

> -  return 0;

> -}


OK.

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c

> index ecb78ffba5..3f9cf1674b 100644

> --- a/nptl/pthread_join_common.c

> +++ b/nptl/pthread_join_common.c


OK. The comment I pointed out as wrong in pthread_join_common.c is now correct
thanks to this change.

> @@ -30,6 +30,52 @@ 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

> +timedwait_tid (pid_t *tidp, const struct timespec *abstime)

> +{

> +  pid_t tid;

> +

> +  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)

> +    return EINVAL;

> +

> +  /* Repeat until thread terminated.  */

> +  while ((tid = *tidp) != 0)

> +    {

> +      struct timeval tv;

> +      struct timespec rt;

> +

> +      /* Get the current time.  */

> +      __gettimeofday (&tv, NULL);

> +

> +      /* Compute relative timeout.  */

> +      rt.tv_sec = abstime->tv_sec - tv.tv_sec;

> +      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;

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

> +      if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)


OK, uses the *cancel variant.

Fix: Needs a comment explaining *why* we use the cancel variant.

> +	  == -ETIMEDOUT)

> +        return ETIMEDOUT;

> +    }

> +

> +  return 0;

> +}


OK.

> +

>  int

>  __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,

>  			const struct timespec *abstime, bool block)

> @@ -81,7 +127,16 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,

>  	 un-wait-ed for again.  */

>        pthread_cleanup_push (cleanup, &pd->joinid);

>  

> -      result = lll_wait_tid (pd->tid, abstime);

> +      if (abstime != NULL)

> +	result = timedwait_tid (&pd->tid, abstime);


OK.

> +      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);


OK.

Fix: Needs a comment explaining why we need cancellation.


> +	}

>  

>        pthread_cleanup_pop (0);

>      }

> diff --git a/nptl/tst-join5.c b/nptl/tst-join5.c

> index f60a024baa..39f74dce09 100644

> --- a/nptl/tst-join5.c

> +++ b/nptl/tst-join5.c

> @@ -23,6 +23,8 @@

>  #include <time.h>

>  #include <unistd.h>

>  

> +#include <support/check.h>

> +#include <support/xthread.h>


OK.

>  

>  static void

>  wait_code (void)

> @@ -37,22 +39,31 @@ wait_code (void)

>  static pthread_barrier_t b;

>  #endif

>  

> +static int

> +thread_join (pthread_t thread, void **retval)

> +{

> +#ifdef USE_PTHREAD_TIMEDJOIN_NP

> +  struct timespec tv;

> +  TEST_COMPARE (clock_gettime (CLOCK_REALTIME, &tv), 0);

> +  /* Arbitrary large timeout to make it act as pthread_join.  */


OK.

> +  tv.tv_sec += 1000;

> +  return pthread_timedjoin_np ((pthread_t) thread, retval, &tv);

> +#else

> +  return pthread_join ((pthread_t) thread, retval);

> +#endif


OK.

> +}

> +

>  

>  static void *

>  tf1 (void *arg)

>  {

>  #ifdef WAIT_IN_CHILD

> -  int e = pthread_barrier_wait (&b);

> -  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)

> -    {

> -      printf ("%s: barrier_wait failed\n", __func__);

> -      exit (1);

> -    }

> +  xpthread_barrier_wait (&b);


OK.

>  

>    wait_code ();

>  #endif

>  

> -  pthread_join ((pthread_t) arg, NULL);

> +  thread_join ((pthread_t) arg, NULL);


OK.

>  

>    exit (42);

>  }

> @@ -62,16 +73,12 @@ static void *

>  tf2 (void *arg)

>  {

>  #ifdef WAIT_IN_CHILD

> -  int e = pthread_barrier_wait (&b);

> -  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)

> -    {

> -      printf ("%s: barrier_wait failed\n", __func__);

> -      exit (1);

> -    }

> +  xpthread_barrier_wait (&b);


OK.

>  

>    wait_code ();

>  #endif

> -  pthread_join ((pthread_t) arg, NULL);

> +

> +  thread_join ((pthread_t) arg, NULL);


OK.

>  

>    exit (43);

>  }

> @@ -81,16 +88,12 @@ static int

>  do_test (void)

>  {

>  #ifdef WAIT_IN_CHILD

> -  if (pthread_barrier_init (&b, NULL, 2) != 0)

> -    {

> -      puts ("barrier_init failed");

> -      return 1;

> -    }

> +  xpthread_barrier_init (&b, NULL, 2);


OK.

>  #endif

>  

>    pthread_t th;

>  

> -  int err = pthread_join (pthread_self (), NULL);

> +  int err = thread_join (pthread_self (), NULL);


OK.

>    if (err == 0)

>      {

>        puts ("1st circular join succeeded");

> @@ -102,33 +105,20 @@ do_test (void)

>        return 1;

>      }

>  

> -  if (pthread_create (&th, NULL, tf1, (void *) pthread_self ()) != 0)

> -    {

> -      puts ("1st create failed");

> -      return 1;

> -    }

> +  th = xpthread_create (NULL, tf1, (void *) pthread_self ());


OK.

>  

>  #ifndef WAIT_IN_CHILD

>    wait_code ();

>  #endif

>  

> -  if (pthread_cancel (th) != 0)

> -    {

> -      puts ("cannot cancel 1st thread");

> -      return 1;

> -    }

> +  xpthread_cancel (th);


OK.

>  

>  #ifdef WAIT_IN_CHILD

> -  int e = pthread_barrier_wait (&b);

> -  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)

> -    {

> -      printf ("%s: barrier_wait failed\n", __func__);

> -      return 1;

> -    }

> +  xpthread_barrier_wait (&b);


OK.

>  #endif

>  

>    void *r;

> -  err = pthread_join (th, &r);

> +  err = thread_join (th, &r);


OK.

>    if (err != 0)

>      {

>        printf ("cannot join 1st thread: %d\n", err);

> @@ -140,7 +130,7 @@ do_test (void)

>        return 1;

>      }

>  

> -  err = pthread_join (pthread_self (), NULL);

> +  err = thread_join (pthread_self (), NULL);


OK.

>    if (err == 0)

>      {

>        puts ("2nd circular join succeeded");

> @@ -152,32 +142,19 @@ do_test (void)

>        return 1;

>      }

>  

> -  if (pthread_create (&th, NULL, tf2, (void *) pthread_self ()) != 0)

> -    {

> -      puts ("2nd create failed");

> -      return 1;

> -    }

> +  th = xpthread_create (NULL, tf2, (void *) pthread_self ());


OK.

>  

>  #ifndef WAIT_IN_CHILD

>    wait_code ();

>  #endif

>  

> -  if (pthread_cancel (th) != 0)

> -    {

> -      puts ("cannot cancel 2nd thread");

> -      return 1;

> -    }

> +  xpthread_cancel (th);


OK.

>  

>  #ifdef WAIT_IN_CHILD

> -  e = pthread_barrier_wait (&b);

> -  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)

> -    {

> -      printf ("%s: barrier_wait failed\n", __func__);

> -      return 1;

> -    }

> +  xpthread_barrier_wait (&b);


OK.

>  #endif

>  

> -  if (pthread_join (th, &r) != 0)

> +  if (thread_join (th, &r) != 0)


OK.

>      {

>        puts ("cannot join 2nd thread");

>        return 1;

> @@ -188,7 +165,7 @@ do_test (void)

>        return 1;

>      }

>  

> -  err = pthread_join (pthread_self (), NULL);

> +  err = thread_join (pthread_self (), NULL);


OK.

>    if (err == 0)

>      {

>        puts ("3rd circular join succeeded");

> @@ -203,5 +180,4 @@ do_test (void)

>    return 0;

>  }

>  

> -#define TEST_FUNCTION do_test ()

> -#include "../test-skeleton.c"

> +#include <support/test-driver.c>


OK.

> diff --git a/nptl/tst-join8.c b/nptl/tst-join8.c

> new file mode 100644

> index 0000000000..f3241fae1c

> --- /dev/null

> +++ b/nptl/tst-join8.c

> @@ -0,0 +1,20 @@

> +/* Check if pthread_timedjoin_np is a cancellation entrypoint.


OK.

> +   Copyright (C) 2019 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

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#define USE_PTHREAD_TIMEDJOIN_NP 1

> +#include <nptl/tst-join5.c>


OK.

> diff --git a/nptl/tst-join9.c b/nptl/tst-join9.c

> new file mode 100644

> index 0000000000..7a47f582f1

> --- /dev/null

> +++ b/nptl/tst-join9.c

> @@ -0,0 +1,21 @@

> +/* Check if pthread_timedjoin_np is a cancellation entrypoint.

> +   Copyright (C) 2019 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

> +   <http://www.gnu.org/licenses/>.  */

> +

> +#define USE_PTHREAD_TIMEDJOIN_NP 1

> +#define WAIT_IN_CHILD 1

> +#include <nptl/tst-join5.c>


OK.

> diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h

> index 50ceba71ff..bb8effe288 100644

> --- a/sysdeps/nptl/lowlevellock-futex.h

> +++ b/sysdeps/nptl/lowlevellock-futex.h

> @@ -82,5 +82,12 @@

>                                   val, private)                          \

>    -ENOSYS

>  

> +/* Like lll_futex_wait, but acting as a cancellable entrypoint.  */

> +#define lll_futex_wait_cancel(futexp, val, private) \

> +  -ENOSYS

> +

> +/* Like lll_futex_timed_wait, but acting as a cancellable entrypoint.  */

> +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \

> +  -ENOSYS


OK.

>  

>  #endif  /* lowlevellock-futex.h */

> diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h

> index 36bbc472e7..6f017afdd5 100644

> --- a/sysdeps/nptl/lowlevellock.h

> +++ b/sysdeps/nptl/lowlevellock.h

> @@ -175,30 +175,4 @@ extern int __lll_timedlock_wait (int *futex, const struct timespec *,

>  #define LLL_LOCK_INITIALIZER		(0)

>  #define LLL_LOCK_INITIALIZER_LOCKED	(1)

>  

> -extern int __lll_timedwait_tid (int *, const struct timespec *)

> -     attribute_hidden;

> -

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

> -   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout

> -   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.

> -   The futex operation are issues with cancellable versions.  */

> -#define lll_wait_tid(tid, abstime)					\

> -  ({									\

> -    int __res = 0;							\

> -    __typeof (tid) __tid;						\

> -    if (abstime != NULL)						\

> -      __res = __lll_timedwait_tid (&(tid), (abstime));			\

> -    else								\

> -      /* 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 (&(tid))) != 0)		\

> -        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\

> -    __res;								\

> -  })

> -

> -


OK. Unused.

>  #endif	/* lowlevellock.h */

> diff --git a/sysdeps/sparc/sparc32/lll_timedwait_tid.c b/sysdeps/sparc/sparc32/lll_timedwait_tid.c

> deleted file mode 100644

> index 511608ead9..0000000000

> --- a/sysdeps/sparc/sparc32/lll_timedwait_tid.c

> +++ /dev/null

> @@ -1 +0,0 @@

> -/* __lll_timedwait_tid is in lowlevellock.c.  */

> diff --git a/sysdeps/sparc/sparc32/lowlevellock.c b/sysdeps/sparc/sparc32/lowlevellock.c

> index a6b2961bd3..1a0b7bbe00 100644

> --- a/sysdeps/sparc/sparc32/lowlevellock.c

> +++ b/sysdeps/sparc/sparc32/lowlevellock.c

> @@ -88,44 +88,4 @@ __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private)

>  

>    return 0;

>  }

> -

> -

> -int

> -__lll_timedwait_tid (int *tidp, const struct timespec *abstime)

> -{

> -  int tid;

> -

> -  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)

> -    return EINVAL;

> -

> -  /* Repeat until thread terminated.  */

> -  while ((tid = *tidp) != 0)

> -    {

> -      struct timeval tv;

> -      struct timespec rt;

> -

> -      /* Get the current time.  */

> -      (void) __gettimeofday (&tv, NULL);

> -

> -      /* Compute relative timeout.  */

> -      rt.tv_sec = abstime->tv_sec - tv.tv_sec;

> -      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;

> -      if (rt.tv_nsec < 0)

> -	{

> -	  rt.tv_nsec += 1000000000;

> -	  --rt.tv_sec;

> -	}

> -

> -      /* Already timed out?  */

> -      if (rt.tv_sec < 0)

> -	return ETIMEDOUT;

> -

> -      /* Wait until thread terminates.  The kernel so far does not use

> -	 the private futex operations for this.  */

> -      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)

> -	return ETIMEDOUT;

> -    }

> -

> -  return 0;

> -}


OK. Removed.

>  #endif

> diff --git a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c

> deleted file mode 100644

> index 43900c6294..0000000000

> --- a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c

> +++ /dev/null

> @@ -1 +0,0 @@

> -/* __lll_timedwait_tid is in lowlevellock.S.  */

> diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.S b/sysdeps/unix/sysv/linux/i386/lowlevellock.S

> index b5f5b5b11c..83191a3899 100644

> --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.S

> +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.S

> @@ -365,70 +365,4 @@ __lll_unlock_wake:

>  	ret

>  	cfi_endproc

>  	.size	__lll_unlock_wake,.-__lll_unlock_wake

> -

> -	.globl	__lll_timedwait_tid

> -	.type	__lll_timedwait_tid,@function

> -	.hidden	__lll_timedwait_tid

> -	.align	16

> -__lll_timedwait_tid:

> -	pushl	%edi

> -	pushl	%esi

> -	pushl	%ebx

> -	pushl	%ebp

> -

> -	movl	%eax, %ebp

> -	movl	%edx, %edi

> -	subl	$8, %esp

> -

> -	/* Get current time.  */

> -2:	movl	%esp, %ebx

> -	xorl	%ecx, %ecx

> -	movl	$__NR_gettimeofday, %eax

> -	ENTER_KERNEL

> -

> -	/* Compute relative timeout.  */

> -	movl	4(%esp), %eax

> -	movl	$1000, %edx

> -	mul	%edx		/* Milli seconds to nano seconds.  */

> -	movl	(%edi), %ecx

> -	movl	4(%edi), %edx

> -	subl	(%esp), %ecx

> -	subl	%eax, %edx

> -	jns	5f

> -	addl	$1000000000, %edx

> -	subl	$1, %ecx

> -5:	testl	%ecx, %ecx

> -	js	6f		/* Time is already up.  */

> -

> -	movl	%ecx, (%esp)	/* Store relative timeout.  */

> -	movl	%edx, 4(%esp)

> -

> -	movl	(%ebp), %edx

> -	testl	%edx, %edx

> -	jz	4f

> -

> -	movl	%esp, %esi

> -	/* XXX The kernel so far uses global futex for the wakeup at

> -	   all times.  */

> -	xorl	%ecx, %ecx	/* movl $FUTEX_WAIT, %ecx */

> -	movl	%ebp, %ebx

> -	movl	$SYS_futex, %eax

> -	ENTER_KERNEL

> -

> -	cmpl	$0, (%ebx)

> -	jne	1f

> -4:	xorl	%eax, %eax

> -

> -3:	addl	$8, %esp

> -	popl	%ebp

> -	popl	%ebx

> -	popl	%esi

> -	popl	%edi

> -	ret

> -

> -1:	cmpl	$-ETIMEDOUT, %eax

> -	jne	2b

> -6:	movl	$ETIMEDOUT, %eax

> -	jmp	3b

> -	.size	__lll_timedwait_tid,.-__lll_timedwait_tid



OK. ZOMG thank you for deleting assembly.

>  #endif

> diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h

> index aa8f4bfdf6..94dccc4ce7 100644

> --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h

> +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h

> @@ -219,31 +219,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count,

>  #define lll_islocked(futex) \

>    (futex != LLL_LOCK_INITIALIZER)

>  

> -extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)

> -     __attribute__ ((regparm (2))) attribute_hidden;

> -

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

> -   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout

> -   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.

> -   The futex operation are issues with cancellable versions.  */

> -#define lll_wait_tid(tid, abstime)					\

> -  ({									\

> -    int __res = 0;							\

> -    __typeof (tid) __tid;						\

> -    if (abstime != NULL)						\

> -      __res = __lll_timedwait_tid (&(tid), (abstime));			\

> -    else								\

> -      /* 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 (&(tid))) != 0)		\

> -        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\

> -    __res;								\

> -  })

> -


OK. Thank you for deleting duplicate code.

>  extern int __lll_lock_elision (int *futex, short *adapt_count, int private)

>    attribute_hidden;

>  

> diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h

> index 8ca3417aaa..6f060b1739 100644

> --- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h

> +++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h

> @@ -135,6 +135,13 @@

>      __err;							       \

>    })

>  

> +#define lll_futex_timed_wait_cancel(futexp, val, timeout, private)	   \

> +  ({									   \

> +    int __oldtype = CANCEL_ASYNC ();				       	   \

> +    long int __err = lll_futex_timed_wait (futexp, val, timeout, private); \

> +    CANCEL_RESET (__oldtype);						   \

> +    __err;								   \

> +  })


OK. Acceptable. Thank you for the generic version.

>  

>  #endif  /* !__ASSEMBLER__  */

>  

> diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h

> index 5aed572d43..c778f4259b 100644

> --- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h

> +++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h

> @@ -108,29 +108,4 @@ __lll_timedlock (int *futex, const struct timespec *abstime, int private)

>  #define LLL_LOCK_INITIALIZER		(0)

>  #define LLL_LOCK_INITIALIZER_LOCKED	(1)

>  

> -extern int __lll_timedwait_tid (int *, const struct timespec *)

> -     attribute_hidden;

> -

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

> -   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout

> -   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.

> -   The futex operation are issues with cancellable versions.  */

> -#define lll_wait_tid(tid, abstime)					\

> -  ({									\

> -    int __res = 0;							\

> -    __typeof (tid) __tid;						\

> -    if (abstime != NULL)						\

> -      __res = __lll_timedwait_tid (&(tid), (abstime));			\

> -    else								\

> -      /* 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 (&(tid))) != 0)		\

> -        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\

> -    __res;								\

> -  })

> -


OK. Deleted unused.

>  #endif	/* lowlevellock.h */

> diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c

> deleted file mode 100644

> index 43900c6294..0000000000

> --- a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c

> +++ /dev/null

> @@ -1 +0,0 @@

> -/* __lll_timedwait_tid is in lowlevellock.S.  */

> diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S

> index f9abb4cb1f..166dbcfd3b 100644

> --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S

> +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S

> @@ -345,87 +345,4 @@ __lll_unlock_wake:

>  	retq

>  	cfi_endproc

>  	.size	__lll_unlock_wake,.-__lll_unlock_wake

> -

> -	.globl	__lll_timedwait_tid

> -	.type	__lll_timedwait_tid,@function

> -	.hidden	__lll_timedwait_tid

> -	.align	16

> -__lll_timedwait_tid:

> -	cfi_startproc

> -	pushq	%r12

> -	cfi_adjust_cfa_offset(8)

> -	pushq	%r13

> -	cfi_adjust_cfa_offset(8)

> -	cfi_offset(%r12, -16)

> -	cfi_offset(%r13, -24)

> -

> -	movq	%rdi, %r12

> -	movq	%rsi, %r13

> -

> -	/* Align stack to 16 bytes when calling __gettimeofday.  */

> -	subq	$24, %rsp

> -	cfi_adjust_cfa_offset(24)

> -

> -	/* Get current time.  */

> -2:	movq	%rsp, %rdi

> -	xorl	%esi, %esi

> -	/* This call works because we directly jump to a system call entry

> -	   which preserves all the registers.  */

> -	call	JUMPTARGET(__gettimeofday)

> -

> -	/* Compute relative timeout.  */

> -	movq	8(%rsp), %rax

> -	movl	$1000, %edi

> -	mul	%rdi		/* Milli seconds to nano seconds.  */

> -	movq	(%r13), %rdi

> -	movq	8(%r13), %rsi

> -	subq	(%rsp), %rdi

> -	subq	%rax, %rsi

> -	jns	5f

> -	addq	$1000000000, %rsi

> -	decq	%rdi

> -5:	testq	%rdi, %rdi

> -	js	6f		/* Time is already up.  */

> -

> -	movq	%rdi, (%rsp)	/* Store relative timeout.  */

> -	movq	%rsi, 8(%rsp)

> -

> -	movl	(%r12), %edx

> -	testl	%edx, %edx

> -	jz	4f

> -

> -	movq	%rsp, %r10

> -	/* XXX The kernel so far uses global futex for the wakeup at

> -	   all times.  */

> -#if FUTEX_WAIT == 0

> -	xorl	%esi, %esi

> -#else

> -	movl	$FUTEX_WAIT, %esi

> -#endif

> -	movq	%r12, %rdi

> -	movl	$SYS_futex, %eax

> -	syscall

> -

> -	cmpl	$0, (%rdi)

> -	jne	1f

> -4:	xorl	%eax, %eax

> -

> -8:	addq	$24, %rsp

> -	cfi_adjust_cfa_offset(-24)

> -	popq	%r13

> -	cfi_adjust_cfa_offset(-8)

> -	cfi_restore(%r13)

> -	popq	%r12

> -	cfi_adjust_cfa_offset(-8)

> -	cfi_restore(%r12)

> -	retq

> -

> -	cfi_adjust_cfa_offset(32)

> -1:	cmpq	$-ETIMEDOUT, %rax

> -	jne	2b

> -

> -6:	movl	$ETIMEDOUT, %eax

> -	jmp	8b

> -	cfi_endproc

> -	.size	__lll_timedwait_tid,.-__lll_timedwait_tid


OK. Amazing. More assembly deleted.

>  #endif

> diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h

> index 47d4eabd61..8cbc1caa5b 100644

> --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h

> +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h

> @@ -222,31 +222,6 @@ extern int __lll_timedlock_elision (int *futex, short *adapt_count,

>  #define lll_islocked(futex) \

>    (futex != LLL_LOCK_INITIALIZER)

>  

> -extern int __lll_timedwait_tid (int *, const struct timespec *)

> -     attribute_hidden;

> -

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

> -   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout

> -   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.

> -   The futex operation are issues with cancellable versions.  */

> -#define lll_wait_tid(tid, abstime)					\

> -  ({									\

> -    int __res = 0;							\

> -    __typeof (tid) __tid;						\

> -    if (abstime != NULL)						\

> -      __res = __lll_timedwait_tid (&(tid), (abstime));			\

> -    else								\

> -      /* 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 (&(tid))) != 0)		\

> -        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\

> -    __res;								\

> -  })

> -


OK. Unused.

>  extern int __lll_lock_elision (int *futex, short *adapt_count, int private)

>    attribute_hidden;

>  

> 



-- 
Cheers,
Carlos.
Adhemerval Zanella Feb. 14, 2019, 5:02 p.m. UTC | #2
On 13/02/2019 12:09, Carlos O'Donell wrote:
> On 2/12/19 3:18 PM, Adhemerval Zanella wrote:

>> Patch ce7eb0e90315 ("nptl: Cleanup cancellation macros") changed the

>> join sequence for internal common __pthread_timedjoin_ex to use the

>> new macro lll_wait_tid.  The idea was this macro would issue the

>> cancellable futex operation depending whether the timeout is used or

>> not.  However if a timeout is used, __lll_timedwait_tid is called and

>> it is not a cancellable entrypoint.

> 

> Thanks for following up on this.

> 

> OK for master with additional comment below (see Fix:).

> 

> Once you commit I'll clean up the straggling comments that I fixed

> before (pthread_tryjoin.c still has a wrong comment).


Thanks for the review, I added the change locally:

diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index 3f9cf1674b..f719613268 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -120,6 +120,10 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
     /* There is already somebody waiting for the thread.  */
     return EINVAL;
 
+  /* BLOCK waits either indefinitely or based on an absolute time.  POSIX also
+     states a cancellation points shall occur for pthread_join, and we use the
+     same rationale for posix_timedjoin_np.  Both timedwait_tid and the futex
+     call use the cancellable variant.  */
   if (block)
     {
       /* During the wait we change to asynchronous cancellation.  If we
Andreas Schwab Feb. 14, 2019, 5:18 p.m. UTC | #3
On Feb 14 2019, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:

> diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c

> index 3f9cf1674b..f719613268 100644

> --- a/nptl/pthread_join_common.c

> +++ b/nptl/pthread_join_common.c

> @@ -120,6 +120,10 @@ __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,

>      /* There is already somebody waiting for the thread.  */

>      return EINVAL;

>  

> +  /* BLOCK waits either indefinitely or based on an absolute time.  POSIX also

> +     states a cancellation points shall occur for pthread_join, and we use the


s/points/point/

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."
diff mbox series

Patch

diff --git a/nptl/Makefile b/nptl/Makefile
index 0e316edfac..e79ca8f064 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -120,7 +120,7 @@  libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \
 		      pt-longjmp pt-cleanup\
 		      cancellation \
 		      lowlevellock \
-		      lll_timedlock_wait lll_timedwait_tid \
+		      lll_timedlock_wait \
 		      pt-fork pt-vfork pt-fcntl \
 		      $(pthread-compat-wrappers) \
 		      pt-raise pt-system \
@@ -271,6 +271,7 @@  tests = tst-attr1 tst-attr2 tst-attr3 tst-default-attr \
 	tst-kill1 tst-kill2 tst-kill3 tst-kill4 tst-kill5 tst-kill6 \
 	tst-raise1 \
 	tst-join1 tst-join2 tst-join3 tst-join4 tst-join5 tst-join6 tst-join7 \
+	tst-join8 tst-join9 \
 	tst-detach1 \
 	tst-eintr1 tst-eintr2 tst-eintr3 tst-eintr4 tst-eintr5 \
 	tst-tsd1 tst-tsd2 tst-tsd3 tst-tsd4 tst-tsd5 tst-tsd6 \
diff --git a/nptl/lll_timedwait_tid.c b/nptl/lll_timedwait_tid.c
deleted file mode 100644
index 59ecbb4446..0000000000
--- a/nptl/lll_timedwait_tid.c
+++ /dev/null
@@ -1,70 +0,0 @@ 
-/* Timed waiting for thread death.  Generic futex-using version.
-   Copyright (C) 2003-2019 Free Software Foundation, Inc.
-   This file is part of the GNU C Library.
-   Contributed by Paul Mackerras <paulus@au.ibm.com>, 2003.
-
-   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
-   <http://www.gnu.org/licenses/>.  */
-
-#include <atomic.h>
-#include <errno.h>
-#include <lowlevellock.h>
-#include <sys/time.h>
-
-
-/* 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.  */
-int
-__lll_timedwait_tid (int *tidp, const struct timespec *abstime)
-{
-  int tid;
-
-  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
-    return EINVAL;
-
-  /* Repeat until thread terminated.  */
-  while ((tid = *tidp) != 0)
-    {
-      struct timeval tv;
-      struct timespec rt;
-
-      /* Get the current time.  */
-      (void) __gettimeofday (&tv, NULL);
-
-      /* Compute relative timeout.  */
-      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
-      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
-      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.
-      */
-      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)
-        return ETIMEDOUT;
-    }
-
-  return 0;
-}
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
index ecb78ffba5..3f9cf1674b 100644
--- a/nptl/pthread_join_common.c
+++ b/nptl/pthread_join_common.c
@@ -30,6 +30,52 @@  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
+timedwait_tid (pid_t *tidp, const struct timespec *abstime)
+{
+  pid_t tid;
+
+  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
+    return EINVAL;
+
+  /* Repeat until thread terminated.  */
+  while ((tid = *tidp) != 0)
+    {
+      struct timeval tv;
+      struct timespec rt;
+
+      /* Get the current time.  */
+      __gettimeofday (&tv, NULL);
+
+      /* Compute relative timeout.  */
+      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
+      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
+      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.  */
+      if (lll_futex_timed_wait_cancel (tidp, tid, &rt, LLL_SHARED)
+	  == -ETIMEDOUT)
+        return ETIMEDOUT;
+    }
+
+  return 0;
+}
+
 int
 __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
 			const struct timespec *abstime, bool block)
@@ -81,7 +127,16 @@  __pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
 	 un-wait-ed for again.  */
       pthread_cleanup_push (cleanup, &pd->joinid);
 
-      result = lll_wait_tid (pd->tid, abstime);
+      if (abstime != NULL)
+	result = timedwait_tid (&pd->tid, 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);
+	}
 
       pthread_cleanup_pop (0);
     }
diff --git a/nptl/tst-join5.c b/nptl/tst-join5.c
index f60a024baa..39f74dce09 100644
--- a/nptl/tst-join5.c
+++ b/nptl/tst-join5.c
@@ -23,6 +23,8 @@ 
 #include <time.h>
 #include <unistd.h>
 
+#include <support/check.h>
+#include <support/xthread.h>
 
 static void
 wait_code (void)
@@ -37,22 +39,31 @@  wait_code (void)
 static pthread_barrier_t b;
 #endif
 
+static int
+thread_join (pthread_t thread, void **retval)
+{
+#ifdef USE_PTHREAD_TIMEDJOIN_NP
+  struct timespec tv;
+  TEST_COMPARE (clock_gettime (CLOCK_REALTIME, &tv), 0);
+  /* Arbitrary large timeout to make it act as pthread_join.  */
+  tv.tv_sec += 1000;
+  return pthread_timedjoin_np ((pthread_t) thread, retval, &tv);
+#else
+  return pthread_join ((pthread_t) thread, retval);
+#endif
+}
+
 
 static void *
 tf1 (void *arg)
 {
 #ifdef WAIT_IN_CHILD
-  int e = pthread_barrier_wait (&b);
-  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
-    {
-      printf ("%s: barrier_wait failed\n", __func__);
-      exit (1);
-    }
+  xpthread_barrier_wait (&b);
 
   wait_code ();
 #endif
 
-  pthread_join ((pthread_t) arg, NULL);
+  thread_join ((pthread_t) arg, NULL);
 
   exit (42);
 }
@@ -62,16 +73,12 @@  static void *
 tf2 (void *arg)
 {
 #ifdef WAIT_IN_CHILD
-  int e = pthread_barrier_wait (&b);
-  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
-    {
-      printf ("%s: barrier_wait failed\n", __func__);
-      exit (1);
-    }
+  xpthread_barrier_wait (&b);
 
   wait_code ();
 #endif
-  pthread_join ((pthread_t) arg, NULL);
+
+  thread_join ((pthread_t) arg, NULL);
 
   exit (43);
 }
@@ -81,16 +88,12 @@  static int
 do_test (void)
 {
 #ifdef WAIT_IN_CHILD
-  if (pthread_barrier_init (&b, NULL, 2) != 0)
-    {
-      puts ("barrier_init failed");
-      return 1;
-    }
+  xpthread_barrier_init (&b, NULL, 2);
 #endif
 
   pthread_t th;
 
-  int err = pthread_join (pthread_self (), NULL);
+  int err = thread_join (pthread_self (), NULL);
   if (err == 0)
     {
       puts ("1st circular join succeeded");
@@ -102,33 +105,20 @@  do_test (void)
       return 1;
     }
 
-  if (pthread_create (&th, NULL, tf1, (void *) pthread_self ()) != 0)
-    {
-      puts ("1st create failed");
-      return 1;
-    }
+  th = xpthread_create (NULL, tf1, (void *) pthread_self ());
 
 #ifndef WAIT_IN_CHILD
   wait_code ();
 #endif
 
-  if (pthread_cancel (th) != 0)
-    {
-      puts ("cannot cancel 1st thread");
-      return 1;
-    }
+  xpthread_cancel (th);
 
 #ifdef WAIT_IN_CHILD
-  int e = pthread_barrier_wait (&b);
-  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
-    {
-      printf ("%s: barrier_wait failed\n", __func__);
-      return 1;
-    }
+  xpthread_barrier_wait (&b);
 #endif
 
   void *r;
-  err = pthread_join (th, &r);
+  err = thread_join (th, &r);
   if (err != 0)
     {
       printf ("cannot join 1st thread: %d\n", err);
@@ -140,7 +130,7 @@  do_test (void)
       return 1;
     }
 
-  err = pthread_join (pthread_self (), NULL);
+  err = thread_join (pthread_self (), NULL);
   if (err == 0)
     {
       puts ("2nd circular join succeeded");
@@ -152,32 +142,19 @@  do_test (void)
       return 1;
     }
 
-  if (pthread_create (&th, NULL, tf2, (void *) pthread_self ()) != 0)
-    {
-      puts ("2nd create failed");
-      return 1;
-    }
+  th = xpthread_create (NULL, tf2, (void *) pthread_self ());
 
 #ifndef WAIT_IN_CHILD
   wait_code ();
 #endif
 
-  if (pthread_cancel (th) != 0)
-    {
-      puts ("cannot cancel 2nd thread");
-      return 1;
-    }
+  xpthread_cancel (th);
 
 #ifdef WAIT_IN_CHILD
-  e = pthread_barrier_wait (&b);
-  if (e != 0 && e != PTHREAD_BARRIER_SERIAL_THREAD)
-    {
-      printf ("%s: barrier_wait failed\n", __func__);
-      return 1;
-    }
+  xpthread_barrier_wait (&b);
 #endif
 
-  if (pthread_join (th, &r) != 0)
+  if (thread_join (th, &r) != 0)
     {
       puts ("cannot join 2nd thread");
       return 1;
@@ -188,7 +165,7 @@  do_test (void)
       return 1;
     }
 
-  err = pthread_join (pthread_self (), NULL);
+  err = thread_join (pthread_self (), NULL);
   if (err == 0)
     {
       puts ("3rd circular join succeeded");
@@ -203,5 +180,4 @@  do_test (void)
   return 0;
 }
 
-#define TEST_FUNCTION do_test ()
-#include "../test-skeleton.c"
+#include <support/test-driver.c>
diff --git a/nptl/tst-join8.c b/nptl/tst-join8.c
new file mode 100644
index 0000000000..f3241fae1c
--- /dev/null
+++ b/nptl/tst-join8.c
@@ -0,0 +1,20 @@ 
+/* Check if pthread_timedjoin_np is a cancellation entrypoint.
+   Copyright (C) 2019 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
+   <http://www.gnu.org/licenses/>.  */
+
+#define USE_PTHREAD_TIMEDJOIN_NP 1
+#include <nptl/tst-join5.c>
diff --git a/nptl/tst-join9.c b/nptl/tst-join9.c
new file mode 100644
index 0000000000..7a47f582f1
--- /dev/null
+++ b/nptl/tst-join9.c
@@ -0,0 +1,21 @@ 
+/* Check if pthread_timedjoin_np is a cancellation entrypoint.
+   Copyright (C) 2019 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
+   <http://www.gnu.org/licenses/>.  */
+
+#define USE_PTHREAD_TIMEDJOIN_NP 1
+#define WAIT_IN_CHILD 1
+#include <nptl/tst-join5.c>
diff --git a/sysdeps/nptl/lowlevellock-futex.h b/sysdeps/nptl/lowlevellock-futex.h
index 50ceba71ff..bb8effe288 100644
--- a/sysdeps/nptl/lowlevellock-futex.h
+++ b/sysdeps/nptl/lowlevellock-futex.h
@@ -82,5 +82,12 @@ 
                                  val, private)                          \
   -ENOSYS
 
+/* Like lll_futex_wait, but acting as a cancellable entrypoint.  */
+#define lll_futex_wait_cancel(futexp, val, private) \
+  -ENOSYS
+
+/* Like lll_futex_timed_wait, but acting as a cancellable entrypoint.  */
+#define lll_futex_timed_wait_cancel(futexp, val, timeout, private) \
+  -ENOSYS
 
 #endif  /* lowlevellock-futex.h */
diff --git a/sysdeps/nptl/lowlevellock.h b/sysdeps/nptl/lowlevellock.h
index 36bbc472e7..6f017afdd5 100644
--- a/sysdeps/nptl/lowlevellock.h
+++ b/sysdeps/nptl/lowlevellock.h
@@ -175,30 +175,4 @@  extern int __lll_timedlock_wait (int *futex, const struct timespec *,
 #define LLL_LOCK_INITIALIZER		(0)
 #define LLL_LOCK_INITIALIZER_LOCKED	(1)
 
-extern int __lll_timedwait_tid (int *, const struct timespec *)
-     attribute_hidden;
-
-/* 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.
-   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout
-   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.
-   The futex operation are issues with cancellable versions.  */
-#define lll_wait_tid(tid, abstime)					\
-  ({									\
-    int __res = 0;							\
-    __typeof (tid) __tid;						\
-    if (abstime != NULL)						\
-      __res = __lll_timedwait_tid (&(tid), (abstime));			\
-    else								\
-      /* 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 (&(tid))) != 0)		\
-        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\
-    __res;								\
-  })
-
-
 #endif	/* lowlevellock.h */
diff --git a/sysdeps/sparc/sparc32/lll_timedwait_tid.c b/sysdeps/sparc/sparc32/lll_timedwait_tid.c
deleted file mode 100644
index 511608ead9..0000000000
--- a/sysdeps/sparc/sparc32/lll_timedwait_tid.c
+++ /dev/null
@@ -1 +0,0 @@ 
-/* __lll_timedwait_tid is in lowlevellock.c.  */
diff --git a/sysdeps/sparc/sparc32/lowlevellock.c b/sysdeps/sparc/sparc32/lowlevellock.c
index a6b2961bd3..1a0b7bbe00 100644
--- a/sysdeps/sparc/sparc32/lowlevellock.c
+++ b/sysdeps/sparc/sparc32/lowlevellock.c
@@ -88,44 +88,4 @@  __lll_timedlock_wait (int *futex, const struct timespec *abstime, int private)
 
   return 0;
 }
-
-
-int
-__lll_timedwait_tid (int *tidp, const struct timespec *abstime)
-{
-  int tid;
-
-  if (abstime->tv_nsec < 0 || abstime->tv_nsec >= 1000000000)
-    return EINVAL;
-
-  /* Repeat until thread terminated.  */
-  while ((tid = *tidp) != 0)
-    {
-      struct timeval tv;
-      struct timespec rt;
-
-      /* Get the current time.  */
-      (void) __gettimeofday (&tv, NULL);
-
-      /* Compute relative timeout.  */
-      rt.tv_sec = abstime->tv_sec - tv.tv_sec;
-      rt.tv_nsec = abstime->tv_nsec - tv.tv_usec * 1000;
-      if (rt.tv_nsec < 0)
-	{
-	  rt.tv_nsec += 1000000000;
-	  --rt.tv_sec;
-	}
-
-      /* Already timed out?  */
-      if (rt.tv_sec < 0)
-	return ETIMEDOUT;
-
-      /* Wait until thread terminates.  The kernel so far does not use
-	 the private futex operations for this.  */
-      if (lll_futex_timed_wait (tidp, tid, &rt, LLL_SHARED) == -ETIMEDOUT)
-	return ETIMEDOUT;
-    }
-
-  return 0;
-}
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c
deleted file mode 100644
index 43900c6294..0000000000
--- a/sysdeps/unix/sysv/linux/i386/lll_timedwait_tid.c
+++ /dev/null
@@ -1 +0,0 @@ 
-/* __lll_timedwait_tid is in lowlevellock.S.  */
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.S b/sysdeps/unix/sysv/linux/i386/lowlevellock.S
index b5f5b5b11c..83191a3899 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.S
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.S
@@ -365,70 +365,4 @@  __lll_unlock_wake:
 	ret
 	cfi_endproc
 	.size	__lll_unlock_wake,.-__lll_unlock_wake
-
-	.globl	__lll_timedwait_tid
-	.type	__lll_timedwait_tid,@function
-	.hidden	__lll_timedwait_tid
-	.align	16
-__lll_timedwait_tid:
-	pushl	%edi
-	pushl	%esi
-	pushl	%ebx
-	pushl	%ebp
-
-	movl	%eax, %ebp
-	movl	%edx, %edi
-	subl	$8, %esp
-
-	/* Get current time.  */
-2:	movl	%esp, %ebx
-	xorl	%ecx, %ecx
-	movl	$__NR_gettimeofday, %eax
-	ENTER_KERNEL
-
-	/* Compute relative timeout.  */
-	movl	4(%esp), %eax
-	movl	$1000, %edx
-	mul	%edx		/* Milli seconds to nano seconds.  */
-	movl	(%edi), %ecx
-	movl	4(%edi), %edx
-	subl	(%esp), %ecx
-	subl	%eax, %edx
-	jns	5f
-	addl	$1000000000, %edx
-	subl	$1, %ecx
-5:	testl	%ecx, %ecx
-	js	6f		/* Time is already up.  */
-
-	movl	%ecx, (%esp)	/* Store relative timeout.  */
-	movl	%edx, 4(%esp)
-
-	movl	(%ebp), %edx
-	testl	%edx, %edx
-	jz	4f
-
-	movl	%esp, %esi
-	/* XXX The kernel so far uses global futex for the wakeup at
-	   all times.  */
-	xorl	%ecx, %ecx	/* movl $FUTEX_WAIT, %ecx */
-	movl	%ebp, %ebx
-	movl	$SYS_futex, %eax
-	ENTER_KERNEL
-
-	cmpl	$0, (%ebx)
-	jne	1f
-4:	xorl	%eax, %eax
-
-3:	addl	$8, %esp
-	popl	%ebp
-	popl	%ebx
-	popl	%esi
-	popl	%edi
-	ret
-
-1:	cmpl	$-ETIMEDOUT, %eax
-	jne	2b
-6:	movl	$ETIMEDOUT, %eax
-	jmp	3b
-	.size	__lll_timedwait_tid,.-__lll_timedwait_tid
 #endif
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index aa8f4bfdf6..94dccc4ce7 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -219,31 +219,6 @@  extern int __lll_timedlock_elision (int *futex, short *adapt_count,
 #define lll_islocked(futex) \
   (futex != LLL_LOCK_INITIALIZER)
 
-extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)
-     __attribute__ ((regparm (2))) attribute_hidden;
-
-/* 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.
-   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout
-   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.
-   The futex operation are issues with cancellable versions.  */
-#define lll_wait_tid(tid, abstime)					\
-  ({									\
-    int __res = 0;							\
-    __typeof (tid) __tid;						\
-    if (abstime != NULL)						\
-      __res = __lll_timedwait_tid (&(tid), (abstime));			\
-    else								\
-      /* 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 (&(tid))) != 0)		\
-        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\
-    __res;								\
-  })
-
 extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
   attribute_hidden;
 
diff --git a/sysdeps/unix/sysv/linux/lowlevellock-futex.h b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
index 8ca3417aaa..6f060b1739 100644
--- a/sysdeps/unix/sysv/linux/lowlevellock-futex.h
+++ b/sysdeps/unix/sysv/linux/lowlevellock-futex.h
@@ -135,6 +135,13 @@ 
     __err;							       \
   })
 
+#define lll_futex_timed_wait_cancel(futexp, val, timeout, private)	   \
+  ({									   \
+    int __oldtype = CANCEL_ASYNC ();				       	   \
+    long int __err = lll_futex_timed_wait (futexp, val, timeout, private); \
+    CANCEL_RESET (__oldtype);						   \
+    __err;								   \
+  })
 
 #endif  /* !__ASSEMBLER__  */
 
diff --git a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
index 5aed572d43..c778f4259b 100644
--- a/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/sparc/lowlevellock.h
@@ -108,29 +108,4 @@  __lll_timedlock (int *futex, const struct timespec *abstime, int private)
 #define LLL_LOCK_INITIALIZER		(0)
 #define LLL_LOCK_INITIALIZER_LOCKED	(1)
 
-extern int __lll_timedwait_tid (int *, const struct timespec *)
-     attribute_hidden;
-
-/* 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.
-   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout
-   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.
-   The futex operation are issues with cancellable versions.  */
-#define lll_wait_tid(tid, abstime)					\
-  ({									\
-    int __res = 0;							\
-    __typeof (tid) __tid;						\
-    if (abstime != NULL)						\
-      __res = __lll_timedwait_tid (&(tid), (abstime));			\
-    else								\
-      /* 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 (&(tid))) != 0)		\
-        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\
-    __res;								\
-  })
-
 #endif	/* lowlevellock.h */
diff --git a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c b/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c
deleted file mode 100644
index 43900c6294..0000000000
--- a/sysdeps/unix/sysv/linux/x86_64/lll_timedwait_tid.c
+++ /dev/null
@@ -1 +0,0 @@ 
-/* __lll_timedwait_tid is in lowlevellock.S.  */
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
index f9abb4cb1f..166dbcfd3b 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.S
@@ -345,87 +345,4 @@  __lll_unlock_wake:
 	retq
 	cfi_endproc
 	.size	__lll_unlock_wake,.-__lll_unlock_wake
-
-	.globl	__lll_timedwait_tid
-	.type	__lll_timedwait_tid,@function
-	.hidden	__lll_timedwait_tid
-	.align	16
-__lll_timedwait_tid:
-	cfi_startproc
-	pushq	%r12
-	cfi_adjust_cfa_offset(8)
-	pushq	%r13
-	cfi_adjust_cfa_offset(8)
-	cfi_offset(%r12, -16)
-	cfi_offset(%r13, -24)
-
-	movq	%rdi, %r12
-	movq	%rsi, %r13
-
-	/* Align stack to 16 bytes when calling __gettimeofday.  */
-	subq	$24, %rsp
-	cfi_adjust_cfa_offset(24)
-
-	/* Get current time.  */
-2:	movq	%rsp, %rdi
-	xorl	%esi, %esi
-	/* This call works because we directly jump to a system call entry
-	   which preserves all the registers.  */
-	call	JUMPTARGET(__gettimeofday)
-
-	/* Compute relative timeout.  */
-	movq	8(%rsp), %rax
-	movl	$1000, %edi
-	mul	%rdi		/* Milli seconds to nano seconds.  */
-	movq	(%r13), %rdi
-	movq	8(%r13), %rsi
-	subq	(%rsp), %rdi
-	subq	%rax, %rsi
-	jns	5f
-	addq	$1000000000, %rsi
-	decq	%rdi
-5:	testq	%rdi, %rdi
-	js	6f		/* Time is already up.  */
-
-	movq	%rdi, (%rsp)	/* Store relative timeout.  */
-	movq	%rsi, 8(%rsp)
-
-	movl	(%r12), %edx
-	testl	%edx, %edx
-	jz	4f
-
-	movq	%rsp, %r10
-	/* XXX The kernel so far uses global futex for the wakeup at
-	   all times.  */
-#if FUTEX_WAIT == 0
-	xorl	%esi, %esi
-#else
-	movl	$FUTEX_WAIT, %esi
-#endif
-	movq	%r12, %rdi
-	movl	$SYS_futex, %eax
-	syscall
-
-	cmpl	$0, (%rdi)
-	jne	1f
-4:	xorl	%eax, %eax
-
-8:	addq	$24, %rsp
-	cfi_adjust_cfa_offset(-24)
-	popq	%r13
-	cfi_adjust_cfa_offset(-8)
-	cfi_restore(%r13)
-	popq	%r12
-	cfi_adjust_cfa_offset(-8)
-	cfi_restore(%r12)
-	retq
-
-	cfi_adjust_cfa_offset(32)
-1:	cmpq	$-ETIMEDOUT, %rax
-	jne	2b
-
-6:	movl	$ETIMEDOUT, %eax
-	jmp	8b
-	cfi_endproc
-	.size	__lll_timedwait_tid,.-__lll_timedwait_tid
 #endif
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index 47d4eabd61..8cbc1caa5b 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -222,31 +222,6 @@  extern int __lll_timedlock_elision (int *futex, short *adapt_count,
 #define lll_islocked(futex) \
   (futex != LLL_LOCK_INITIALIZER)
 
-extern int __lll_timedwait_tid (int *, const struct timespec *)
-     attribute_hidden;
-
-/* 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.
-   If ABSTIME is not NULL, is used a timeout for futex call.  If the timeout
-   occurs then return ETIMEOUT, if ABSTIME is invalid, return EINVAL.
-   The futex operation are issues with cancellable versions.  */
-#define lll_wait_tid(tid, abstime)					\
-  ({									\
-    int __res = 0;							\
-    __typeof (tid) __tid;						\
-    if (abstime != NULL)						\
-      __res = __lll_timedwait_tid (&(tid), (abstime));			\
-    else								\
-      /* 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 (&(tid))) != 0)		\
-        lll_futex_wait_cancel (&(tid), __tid, LLL_SHARED);		\
-    __res;								\
-  })
-
 extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
   attribute_hidden;