public inbox for libc-alpha@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] nptl: Consolidate pthread_{timed,try}join{_np}
@ 2017-12-19 13:40 Adhemerval Zanella
  2017-12-20  5:44 ` Carlos O'Donell
  0 siblings, 1 reply; 3+ messages in thread
From: Adhemerval Zanella @ 2017-12-19 13:40 UTC (permalink / raw)
  To: libc-alpha

This patch consolidates the pthread_join and gnu extensions to avoid
code duplication.  The function pthread_join, pthread_tryjoin_np, and
pthread_timedjoin_np are now based on pthread_timedjoin_ex.

It also fixes some inconsistencies on ESRCH, EINVAL, EDEADLK handling
(where each implementation differs from each other) and also on
clenup handler (which now always use a CAS).

Checked on i686-linux-gnu and x86_64-linux-gnu.

	* nptl/pthreadP.h (__pthread_timedjoin_np): Define.
	* nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_np.
	* nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise.
	* nptl/pthread_timedjoin.c (cleanup): Use CAS on argument setting.
	(pthread_timedjoin_np): Define internal symbol and common code from
	pthread_join.
	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid):
	Remove superflous checks.
	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid):
	Likewise.
---
 ChangeLog                                     | 13 ++++
 nptl/pthreadP.h                               |  3 +
 nptl/pthread_join.c                           | 94 +--------------------------
 nptl/pthread_timedjoin.c                      | 80 +++++++++++++++--------
 nptl/pthread_tryjoin.c                        | 43 ++----------
 sysdeps/unix/sysv/linux/i386/lowlevellock.h   |  7 +-
 sysdeps/unix/sysv/linux/x86_64/lowlevellock.h |  7 +-
 7 files changed, 75 insertions(+), 172 deletions(-)

diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 713000e..2646b8f 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
 extern int __pthread_enable_asynccancel (void) attribute_hidden;
 extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
 extern void __pthread_testcancel (void);
+extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *,
+				   bool);
 
 #if IS_IN (libpthread)
 hidden_proto (__pthread_mutex_init)
@@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate)
 hidden_proto (__pthread_testcancel)
 hidden_proto (__pthread_mutexattr_init)
 hidden_proto (__pthread_mutexattr_settype)
+hidden_proto (__pthread_timedjoin_ex)
 #endif
 
 extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond);
diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
index afc8c37..47379a6 100644
--- a/nptl/pthread_join.c
+++ b/nptl/pthread_join.c
@@ -16,103 +16,11 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-
-#include <atomic.h>
 #include "pthreadP.h"
 
-#include <stap-probe.h>
-
-
-static void
-cleanup (void *arg)
-{
-  /* If we already changed the waiter ID, reset it.  The call cannot
-     fail for any reason but the thread not having done that yet so
-     there is no reason for a loop.  */
-  (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL,
-					       THREAD_SELF);
-}
-
-
 int
 __pthread_join (pthread_t threadid, void **thread_return)
 {
-  struct pthread *pd = (struct pthread *) threadid;
-
-  /* Make sure the descriptor is valid.  */
-  if (INVALID_NOT_TERMINATED_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* Is the thread joinable?.  */
-  if (IS_DETACHED (pd))
-    /* We cannot wait for the thread.  */
-    return EINVAL;
-
-  struct pthread *self = THREAD_SELF;
-  int result = 0;
-
-  LIBC_PROBE (pthread_join, 1, threadid);
-
-  /* During the wait we change to asynchronous cancellation.  If we
-     are canceled the thread we are waiting for must be marked as
-     un-wait-ed for again.  */
-  pthread_cleanup_push (cleanup, &pd->joinid);
-
-  /* Switch to asynchronous cancellation.  */
-  int oldtype = CANCEL_ASYNC ();
-
-  if ((pd == self
-       || (self->joinid == pd
-	   && (pd->cancelhandling
-	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
-		  | TERMINATED_BITMASK)) == 0))
-      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
-    /* This is a deadlock situation.  The threads are waiting for each
-       other to finish.  Note that this is a "may" error.  To be 100%
-       sure we catch this error we would have to lock the data
-       structures but it is not necessary.  In the unlikely case that
-       two threads are really caught in this situation they will
-       deadlock.  It is the programmer's problem to figure this
-       out.  */
-    result = EDEADLK;
-  /* Wait for the thread to finish.  If it is already locked something
-     is wrong.  There can only be one waiter.  */
-  else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
-								   self,
-								   NULL), 0))
-    /* There is already somebody waiting for the thread.  */
-    result = EINVAL;
-  else
-    /* Wait for the child.  */
-    lll_wait_tid (pd->tid);
-
-
-  /* Restore cancellation mode.  */
-  CANCEL_RESET (oldtype);
-
-  /* Remove the handler.  */
-  pthread_cleanup_pop (0);
-
-
-  if (__glibc_likely (result == 0))
-    {
-      /* We mark the thread as terminated and as joined.  */
-      pd->tid = -1;
-
-      /* Store the return value if the caller is interested.  */
-      if (thread_return != NULL)
-	*thread_return = pd->result;
-
-
-      /* Free the TCB.  */
-      __free_tcb (pd);
-    }
-
-  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
-
-  return result;
+  return __pthread_timedjoin_ex (threadid, thread_return, NULL, true);
 }
 weak_alias (__pthread_join, pthread_join)
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index 567c171..6b894f2 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -21,21 +21,25 @@
 #include <atomic.h>
 #include "pthreadP.h"
 
+#include <stap-probe.h>
+
 
 static void
 cleanup (void *arg)
 {
-  *(void **) arg = NULL;
+  /* If we already changed the waiter ID, reset it.  The call cannot
+     fail for any reason but the thread not having done that yet so
+     there is no reason for a loop.  */
+  struct pthread *self = THREAD_SELF;
+  atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
 }
 
 
 int
-pthread_timedjoin_np (pthread_t threadid, void **thread_return,
-		      const struct timespec *abstime)
+__pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
+			const struct timespec *abstime, bool block)
 {
-  struct pthread *self;
   struct pthread *pd = (struct pthread *) threadid;
-  int result;
 
   /* Make sure the descriptor is valid.  */
   if (INVALID_NOT_TERMINATED_TD_P (pd))
@@ -47,8 +51,17 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return,
     /* We cannot wait for the thread.  */
     return EINVAL;
 
-  self = THREAD_SELF;
-  if (pd == self || self->joinid == pd)
+  struct pthread *self = THREAD_SELF;
+  int result = 0;
+
+  LIBC_PROBE (pthread_join, 1, threadid);
+
+  if ((pd == self
+       || (self->joinid == pd
+	   && (pd->cancelhandling
+	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
+		  | TERMINATED_BITMASK)) == 0))
+      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
     /* This is a deadlock situation.  The threads are waiting for each
        other to finish.  Note that this is a "may" error.  To be 100%
        sure we catch this error we would have to lock the data
@@ -60,45 +73,56 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return,
 
   /* Wait for the thread to finish.  If it is already locked something
      is wrong.  There can only be one waiter.  */
-  if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
-							      self, NULL), 0))
+  else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
+								   &self,
+								   NULL)))
     /* There is already somebody waiting for the thread.  */
     return EINVAL;
 
+  if (block)
+    {
+      /* During the wait we change to asynchronous cancellation.  If we
+	 are cancelled the thread we are waiting for must be marked as
+	 un-wait-ed for again.  */
+      pthread_cleanup_push (cleanup, &pd->joinid);
 
-  /* During the wait we change to asynchronous cancellation.  If we
-     are cancelled the thread we are waiting for must be marked as
-     un-wait-ed for again.  */
-  pthread_cleanup_push (cleanup, &pd->joinid);
-
-  /* Switch to asynchronous cancellation.  */
-  int oldtype = CANCEL_ASYNC ();
-
-
-  /* Wait for the child.  */
-  result = lll_timedwait_tid (pd->tid, abstime);
-
+      int oldtype = CANCEL_ASYNC ();
 
-  /* Restore cancellation mode.  */
-  CANCEL_RESET (oldtype);
+      if (abstime != NULL)
+	result = lll_timedwait_tid (pd->tid, abstime);
+      else
+	lll_wait_tid (pd->tid);
 
-  /* Remove the handler.  */
-  pthread_cleanup_pop (0);
+      CANCEL_RESET (oldtype);
 
+      pthread_cleanup_pop (0);
+    }
 
-  /* We might have timed out.  */
-  if (result == 0)
+  if (__glibc_likely (result == 0))
     {
+      /* We mark the thread as terminated and as joined.  */
+      pd->tid = -1;
+
       /* Store the return value if the caller is interested.  */
       if (thread_return != NULL)
 	*thread_return = pd->result;
 
-
       /* Free the TCB.  */
       __free_tcb (pd);
     }
   else
     pd->joinid = NULL;
 
+  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
+
   return result;
 }
+hidden_def (__pthread_timedjoin_ex)
+
+int
+__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
+			const struct timespec *abstime)
+{
+  return __pthread_timedjoin_ex (threadid, thread_return, abstime, true);
+}
+weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
index 6a3b62e..e7c7595 100644
--- a/nptl/pthread_tryjoin.c
+++ b/nptl/pthread_tryjoin.c
@@ -26,47 +26,12 @@
 int
 pthread_tryjoin_np (pthread_t threadid, void **thread_return)
 {
-  struct pthread *self;
-  struct pthread *pd = (struct pthread *) threadid;
-
-  /* Make sure the descriptor is valid.  */
-  if (DEBUGGING_P && __find_in_stack_list (pd) == NULL)
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* Is the thread joinable?.  */
-  if (IS_DETACHED (pd))
-    /* We cannot wait for the thread.  */
-    return EINVAL;
-
-  self = THREAD_SELF;
-  if (pd == self || self->joinid == pd)
-    /* This is a deadlock situation.  The threads are waiting for each
-       other to finish.  Note that this is a "may" error.  To be 100%
-       sure we catch this error we would have to lock the data
-       structures but it is not necessary.  In the unlikely case that
-       two threads are really caught in this situation they will
-       deadlock.  It is the programmer's problem to figure this
-       out.  */
-    return EDEADLK;
-
   /* Return right away if the thread hasn't terminated yet.  */
+  struct pthread *pd = (struct pthread *) threadid;
   if (pd->tid != 0)
     return EBUSY;
 
-  /* Wait for the thread to finish.  If it is already locked something
-     is wrong.  There can only be one waiter.  */
-  if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL))
-    /* There is already somebody waiting for the thread.  */
-    return EINVAL;
-
-  /* Store the return value if the caller is interested.  */
-  if (thread_return != NULL)
-    *thread_return = pd->result;
-
-
-  /* Free the TCB.  */
-  __free_tcb (pd);
-
-  return 0;
+  /* If pd->tid != 0 then lll_wait_tid will not block on futex
+     operation.  */
+  return __pthread_timedjoin_ex (threadid, thread_return, NULL, false);
 }
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index 197bb1f..098c64b 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)
   ({									      \
     int __result = 0;							      \
     if ((tid) != 0)							      \
-      {									      \
-	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
-	  __result = EINVAL;						      \
-	else								      \
-	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
-      }									      \
+      __result = __lll_timedwait_tid (&(tid), (abstime));		      \
     __result; })
 
 
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index cbf6597..0b56a2e 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *)
   ({									      \
     int __result = 0;							      \
     if ((tid) != 0)							      \
-      {									      \
-	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
-	  __result = EINVAL;						      \
-	else								      \
-	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
-      }									      \
+      __result = __lll_timedwait_tid (&(tid), (abstime));		      \
     __result; })
 
 extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
-- 
2.7.4

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nptl: Consolidate pthread_{timed,try}join{_np}
  2017-12-19 13:40 [PATCH] nptl: Consolidate pthread_{timed,try}join{_np} Adhemerval Zanella
@ 2017-12-20  5:44 ` Carlos O'Donell
  2017-12-20 13:41   ` Adhemerval Zanella
  0 siblings, 1 reply; 3+ messages in thread
From: Carlos O'Donell @ 2017-12-20  5:44 UTC (permalink / raw)
  To: Adhemerval Zanella, libc-alpha

On 12/19/2017 05:40 AM, Adhemerval Zanella wrote:
> This patch consolidates the pthread_join and gnu extensions to avoid
> code duplication.  The function pthread_join, pthread_tryjoin_np, and
> pthread_timedjoin_np are now based on pthread_timedjoin_ex.
> 
> It also fixes some inconsistencies on ESRCH, EINVAL, EDEADLK handling
> (where each implementation differs from each other) and also on
> clenup handler (which now always use a CAS).

High level:

Refactoring common code like this is a good idea.

Design:

Moving everything into __pthread_timedjoin_ex and using a bool for
blocking seems like a good refactoring, anything less would duplicate
code again.

I like the removal of the EINVAL checks in the lowlevellock.h code
which harmonizes this more with all the arches.

Implementation:

We have started using pthread_cond_common.c, pthread_rwlock_common.c,
and I'd like to see us use pthread_join_common.c for the common file
with all others including it. This might sound like bike-shedding, but
it helps one quickly determine where the common code lies. New developers
will thank us for the logical choices like this.

> Checked on i686-linux-gnu and x86_64-linux-gnu.
> 
> 	* nptl/pthreadP.h (__pthread_timedjoin_np): Define.
> 	* nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_np.
> 	* nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise.
> 	* nptl/pthread_timedjoin.c (cleanup): Use CAS on argument setting.
> 	(pthread_timedjoin_np): Define internal symbol and common code from
> 	pthread_join.
> 	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid):
> 	Remove superflous checks.
> 	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid):
> 	Likewise.

OK with rework of pthread_join_common.c.

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

> ---
>  ChangeLog                                     | 13 ++++
>  nptl/pthreadP.h                               |  3 +
>  nptl/pthread_join.c                           | 94 +--------------------------
>  nptl/pthread_timedjoin.c                      | 80 +++++++++++++++--------
>  nptl/pthread_tryjoin.c                        | 43 ++----------
>  sysdeps/unix/sysv/linux/i386/lowlevellock.h   |  7 +-
>  sysdeps/unix/sysv/linux/x86_64/lowlevellock.h |  7 +-
>  7 files changed, 75 insertions(+), 172 deletions(-)
> 
> diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
> index 713000e..2646b8f 100644
> --- a/nptl/pthreadP.h
> +++ b/nptl/pthreadP.h
> @@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
>  extern int __pthread_enable_asynccancel (void) attribute_hidden;
>  extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
>  extern void __pthread_testcancel (void);
> +extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *,
> +				   bool);

OK.

>  
>  #if IS_IN (libpthread)
>  hidden_proto (__pthread_mutex_init)
> @@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate)
>  hidden_proto (__pthread_testcancel)
>  hidden_proto (__pthread_mutexattr_init)
>  hidden_proto (__pthread_mutexattr_settype)
> +hidden_proto (__pthread_timedjoin_ex)

OK.

>  #endif
>  
>  extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond);
> diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
> index afc8c37..47379a6 100644
> --- a/nptl/pthread_join.c
> +++ b/nptl/pthread_join.c
> @@ -16,103 +16,11 @@
>     License along with the GNU C Library; if not, see
>     <http://www.gnu.org/licenses/>.  */
>  
> -#include <errno.h>
> -#include <stdlib.h>
> -
> -#include <atomic.h>

OK.

>  #include "pthreadP.h"
>  
> -#include <stap-probe.h>
> -
> -
> -static void
> -cleanup (void *arg)
> -{
> -  /* If we already changed the waiter ID, reset it.  The call cannot
> -     fail for any reason but the thread not having done that yet so
> -     there is no reason for a loop.  */
> -  (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL,
> -					       THREAD_SELF);
> -}
> -
> -
>  int
>  __pthread_join (pthread_t threadid, void **thread_return)
>  {
> -  struct pthread *pd = (struct pthread *) threadid;
> -
> -  /* Make sure the descriptor is valid.  */
> -  if (INVALID_NOT_TERMINATED_TD_P (pd))
> -    /* Not a valid thread handle.  */
> -    return ESRCH;
> -
> -  /* Is the thread joinable?.  */
> -  if (IS_DETACHED (pd))
> -    /* We cannot wait for the thread.  */
> -    return EINVAL;
> -
> -  struct pthread *self = THREAD_SELF;
> -  int result = 0;
> -
> -  LIBC_PROBE (pthread_join, 1, threadid);
> -
> -  /* During the wait we change to asynchronous cancellation.  If we
> -     are canceled the thread we are waiting for must be marked as
> -     un-wait-ed for again.  */
> -  pthread_cleanup_push (cleanup, &pd->joinid);
> -
> -  /* Switch to asynchronous cancellation.  */
> -  int oldtype = CANCEL_ASYNC ();
> -
> -  if ((pd == self
> -       || (self->joinid == pd
> -	   && (pd->cancelhandling
> -	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> -		  | TERMINATED_BITMASK)) == 0))
> -      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
> -    /* This is a deadlock situation.  The threads are waiting for each
> -       other to finish.  Note that this is a "may" error.  To be 100%
> -       sure we catch this error we would have to lock the data
> -       structures but it is not necessary.  In the unlikely case that
> -       two threads are really caught in this situation they will
> -       deadlock.  It is the programmer's problem to figure this
> -       out.  */
> -    result = EDEADLK;
> -  /* Wait for the thread to finish.  If it is already locked something
> -     is wrong.  There can only be one waiter.  */
> -  else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
> -								   self,
> -								   NULL), 0))
> -    /* There is already somebody waiting for the thread.  */
> -    result = EINVAL;
> -  else
> -    /* Wait for the child.  */
> -    lll_wait_tid (pd->tid);
> -
> -
> -  /* Restore cancellation mode.  */
> -  CANCEL_RESET (oldtype);
> -
> -  /* Remove the handler.  */
> -  pthread_cleanup_pop (0);
> -
> -
> -  if (__glibc_likely (result == 0))
> -    {
> -      /* We mark the thread as terminated and as joined.  */
> -      pd->tid = -1;
> -
> -      /* Store the return value if the caller is interested.  */
> -      if (thread_return != NULL)
> -	*thread_return = pd->result;
> -
> -
> -      /* Free the TCB.  */
> -      __free_tcb (pd);
> -    }
> -
> -  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> -
> -  return result;
> +  return __pthread_timedjoin_ex (threadid, thread_return, NULL, true);

OK.

>  }
>  weak_alias (__pthread_join, pthread_join)
> diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
> index 567c171..6b894f2 100644
> --- a/nptl/pthread_timedjoin.c
> +++ b/nptl/pthread_timedjoin.c
> @@ -21,21 +21,25 @@
>  #include <atomic.h>
>  #include "pthreadP.h"
>  
> +#include <stap-probe.h>
> +
>  
>  static void
>  cleanup (void *arg)
>  {
> -  *(void **) arg = NULL;
> +  /* If we already changed the waiter ID, reset it.  The call cannot
> +     fail for any reason but the thread not having done that yet so
> +     there is no reason for a loop.  */
> +  struct pthread *self = THREAD_SELF;
> +  atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
>  }

OK.

>  
>  
>  int
> -pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> -		      const struct timespec *abstime)
> +__pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
> +			const struct timespec *abstime, bool block)
>  {
> -  struct pthread *self;
>    struct pthread *pd = (struct pthread *) threadid;
> -  int result;
>  
>    /* Make sure the descriptor is valid.  */
>    if (INVALID_NOT_TERMINATED_TD_P (pd))
> @@ -47,8 +51,17 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return,
>      /* We cannot wait for the thread.  */
>      return EINVAL;
>  
> -  self = THREAD_SELF;
> -  if (pd == self || self->joinid == pd)
> +  struct pthread *self = THREAD_SELF;
> +  int result = 0;
> +
> +  LIBC_PROBE (pthread_join, 1, threadid);

OK.

> +
> +  if ((pd == self
> +       || (self->joinid == pd
> +	   && (pd->cancelhandling
> +	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
> +		  | TERMINATED_BITMASK)) == 0))
> +      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
>      /* This is a deadlock situation.  The threads are waiting for each
>         other to finish.  Note that this is a "may" error.  To be 100%
>         sure we catch this error we would have to lock the data
> @@ -60,45 +73,56 @@ pthread_timedjoin_np (pthread_t threadid, void **thread_return,
>  
>    /* Wait for the thread to finish.  If it is already locked something
>       is wrong.  There can only be one waiter.  */
> -  if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
> -							      self, NULL), 0))
> +  else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
> +								   &self,
> +								   NULL)))
>      /* There is already somebody waiting for the thread.  */
>      return EINVAL;
>  
> +  if (block)
> +    {
> +      /* During the wait we change to asynchronous cancellation.  If we
> +	 are cancelled the thread we are waiting for must be marked as
> +	 un-wait-ed for again.  */
> +      pthread_cleanup_push (cleanup, &pd->joinid);
>  
> -  /* During the wait we change to asynchronous cancellation.  If we
> -     are cancelled the thread we are waiting for must be marked as
> -     un-wait-ed for again.  */
> -  pthread_cleanup_push (cleanup, &pd->joinid);
> -
> -  /* Switch to asynchronous cancellation.  */
> -  int oldtype = CANCEL_ASYNC ();
> -
> -
> -  /* Wait for the child.  */
> -  result = lll_timedwait_tid (pd->tid, abstime);
> -
> +      int oldtype = CANCEL_ASYNC ();

OK.

>  
> -  /* Restore cancellation mode.  */
> -  CANCEL_RESET (oldtype);
> +      if (abstime != NULL)
> +	result = lll_timedwait_tid (pd->tid, abstime);
> +      else
> +	lll_wait_tid (pd->tid);
>  
> -  /* Remove the handler.  */
> -  pthread_cleanup_pop (0);
> +      CANCEL_RESET (oldtype);
>  
> +      pthread_cleanup_pop (0);
> +    }
>  
> -  /* We might have timed out.  */
> -  if (result == 0)
> +  if (__glibc_likely (result == 0))
>      {
> +      /* We mark the thread as terminated and as joined.  */
> +      pd->tid = -1;

OK.

> +
>        /* Store the return value if the caller is interested.  */
>        if (thread_return != NULL)
>  	*thread_return = pd->result;
>  
> -
>        /* Free the TCB.  */
>        __free_tcb (pd);
>      }
>    else
>      pd->joinid = NULL;
>  
> +  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
> +
>    return result;
>  }
> +hidden_def (__pthread_timedjoin_ex)
> +
> +int
> +__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
> +			const struct timespec *abstime)
> +{
> +  return __pthread_timedjoin_ex (threadid, thread_return, abstime, true);
> +}

OK.

> +weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
> diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
> index 6a3b62e..e7c7595 100644
> --- a/nptl/pthread_tryjoin.c
> +++ b/nptl/pthread_tryjoin.c
> @@ -26,47 +26,12 @@
>  int
>  pthread_tryjoin_np (pthread_t threadid, void **thread_return)
>  {
> -  struct pthread *self;
> -  struct pthread *pd = (struct pthread *) threadid;
> -
> -  /* Make sure the descriptor is valid.  */
> -  if (DEBUGGING_P && __find_in_stack_list (pd) == NULL)
> -    /* Not a valid thread handle.  */
> -    return ESRCH;
> -
> -  /* Is the thread joinable?.  */
> -  if (IS_DETACHED (pd))
> -    /* We cannot wait for the thread.  */
> -    return EINVAL;
> -
> -  self = THREAD_SELF;
> -  if (pd == self || self->joinid == pd)
> -    /* This is a deadlock situation.  The threads are waiting for each
> -       other to finish.  Note that this is a "may" error.  To be 100%
> -       sure we catch this error we would have to lock the data
> -       structures but it is not necessary.  In the unlikely case that
> -       two threads are really caught in this situation they will
> -       deadlock.  It is the programmer's problem to figure this
> -       out.  */
> -    return EDEADLK;
> -
>    /* Return right away if the thread hasn't terminated yet.  */
> +  struct pthread *pd = (struct pthread *) threadid;
>    if (pd->tid != 0)
>      return EBUSY;
>  

OK.

> -  /* Wait for the thread to finish.  If it is already locked something
> -     is wrong.  There can only be one waiter.  */
> -  if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL))
> -    /* There is already somebody waiting for the thread.  */
> -    return EINVAL;
> -
> -  /* Store the return value if the caller is interested.  */
> -  if (thread_return != NULL)
> -    *thread_return = pd->result;
> -
> -
> -  /* Free the TCB.  */
> -  __free_tcb (pd);
> -
> -  return 0;
> +  /* If pd->tid != 0 then lll_wait_tid will not block on futex
> +     operation.  */
> +  return __pthread_timedjoin_ex (threadid, thread_return, NULL, false);

OK.

>  }
> diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
> index 197bb1f..098c64b 100644
> --- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
> +++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
> @@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)
>    ({									      \
>      int __result = 0;							      \
>      if ((tid) != 0)							      \
> -      {									      \
> -	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
> -	  __result = EINVAL;						      \
> -	else								      \
> -	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
> -      }									      \
> +      __result = __lll_timedwait_tid (&(tid), (abstime));		      \

OK. Don't handle EINVAL in any special way and harmonize in the common code.

>      __result; })
>  
>  
> diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
> index cbf6597..0b56a2e 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
> +++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
> @@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *)
>    ({									      \
>      int __result = 0;							      \
>      if ((tid) != 0)							      \
> -      {									      \
> -	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
> -	  __result = EINVAL;						      \
> -	else								      \
> -	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
> -      }									      \
> +      __result = __lll_timedwait_tid (&(tid), (abstime));		      \

OK.

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


-- 
Cheers,
Carlos.

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] nptl: Consolidate pthread_{timed,try}join{_np}
  2017-12-20  5:44 ` Carlos O'Donell
@ 2017-12-20 13:41   ` Adhemerval Zanella
  0 siblings, 0 replies; 3+ messages in thread
From: Adhemerval Zanella @ 2017-12-20 13:41 UTC (permalink / raw)
  To: Carlos O'Donell, libc-alpha



On 20/12/2017 03:44, Carlos O'Donell wrote:
> On 12/19/2017 05:40 AM, Adhemerval Zanella wrote:
>> This patch consolidates the pthread_join and gnu extensions to avoid
>> code duplication.  The function pthread_join, pthread_tryjoin_np, and
>> pthread_timedjoin_np are now based on pthread_timedjoin_ex.
>>
>> It also fixes some inconsistencies on ESRCH, EINVAL, EDEADLK handling
>> (where each implementation differs from each other) and also on
>> clenup handler (which now always use a CAS).
> 
> High level:
> 
> Refactoring common code like this is a good idea.
> 
> Design:
> 
> Moving everything into __pthread_timedjoin_ex and using a bool for
> blocking seems like a good refactoring, anything less would duplicate
> code again.
> 
> I like the removal of the EINVAL checks in the lowlevellock.h code
> which harmonizes this more with all the arches.
> 
> Implementation:
> 
> We have started using pthread_cond_common.c, pthread_rwlock_common.c,
> and I'd like to see us use pthread_join_common.c for the common file
> with all others including it. This might sound like bike-shedding, but
> it helps one quickly determine where the common code lies. New developers
> will thank us for the logical choices like this.
> 
>> Checked on i686-linux-gnu and x86_64-linux-gnu.
>>
>> 	* nptl/pthreadP.h (__pthread_timedjoin_np): Define.
>> 	* nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_np.
>> 	* nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise.
>> 	* nptl/pthread_timedjoin.c (cleanup): Use CAS on argument setting.
>> 	(pthread_timedjoin_np): Define internal symbol and common code from
>> 	pthread_join.
>> 	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid):
>> 	Remove superflous checks.
>> 	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid):
>> 	Likewise.
> 
> OK with rework of pthread_join_common.c.

Thanks for the reviewed, I pushed the following patch (with the
pthread_join_common.c change you requested):

diff --git a/ChangeLog b/ChangeLog
index ad2c9db..35d1d10 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,18 @@
+2017-12-20  Adhemerval Zanella  <adhemerval.zanella@linaro.org>
+
+	* nptl/Makefile (libpthread-routines): Add pthread_join_common.
+	* nptl/pthreadP.h (__pthread_timedjoin_ex): New prototype.
+	* nptl/pthread_join_common.c: New file: common function used on
+	pthread_join, pthread_timedjoin_np, pthread_tryjoin_np.
+	* nptl/pthread_join.c (pthread_join): Use __pthread_timedjoin_ex.
+	* nptl/pthread_tryjoin.c (pthread_tryjoin): Likewise.
+	* nptl/pthread_timedjoin.c (pthread_timedjoin_np): Likewise.
+	(cleanup): Move definition to pthread_join_common.c.
+	* sysdeps/unix/sysv/linux/i386/lowlevellock.h (__lll_timedwait_tid):
+	Remove superflous checks.
+	* sysdeps/unix/sysv/linux/x86_64/lowlevellock.h (__lll_timedwait_tid):
+	Likewise.
+
 2017-12-20  Szabolcs Nagy  <szabolcs.nagy@arm.com>
 
 	* sysdeps/aarch64/libm-test-ulps: Update.
diff --git a/nptl/Makefile b/nptl/Makefile
index ab8cc98..cf2ba81 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -49,6 +49,7 @@ pthread-compat-wrappers = \
 libpthread-routines = nptl-init vars events version pt-interp \
 		      pthread_create pthread_exit pthread_detach \
 		      pthread_join pthread_tryjoin pthread_timedjoin \
+		      pthread_join_common \
 		      compat-pthread_self pthread_equal pthread_yield \
 		      pthread_getconcurrency pthread_setconcurrency \
 		      pthread_getschedparam pthread_setschedparam \
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 713000e..2646b8f 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -506,6 +506,8 @@ extern int __pthread_setcanceltype (int type, int *oldtype);
 extern int __pthread_enable_asynccancel (void) attribute_hidden;
 extern void __pthread_disable_asynccancel (int oldtype) attribute_hidden;
 extern void __pthread_testcancel (void);
+extern int __pthread_timedjoin_ex (pthread_t, void **, const struct timespec *,
+				   bool);
 
 #if IS_IN (libpthread)
 hidden_proto (__pthread_mutex_init)
@@ -524,6 +526,7 @@ hidden_proto (__pthread_setcancelstate)
 hidden_proto (__pthread_testcancel)
 hidden_proto (__pthread_mutexattr_init)
 hidden_proto (__pthread_mutexattr_settype)
+hidden_proto (__pthread_timedjoin_ex)
 #endif
 
 extern int __pthread_cond_broadcast_2_0 (pthread_cond_2_0_t *cond);
diff --git a/nptl/pthread_join.c b/nptl/pthread_join.c
index afc8c37..47379a6 100644
--- a/nptl/pthread_join.c
+++ b/nptl/pthread_join.c
@@ -16,103 +16,11 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-
-#include <atomic.h>
 #include "pthreadP.h"
 
-#include <stap-probe.h>
-
-
-static void
-cleanup (void *arg)
-{
-  /* If we already changed the waiter ID, reset it.  The call cannot
-     fail for any reason but the thread not having done that yet so
-     there is no reason for a loop.  */
-  (void) atomic_compare_and_exchange_bool_acq ((struct pthread **) arg, NULL,
-					       THREAD_SELF);
-}
-
-
 int
 __pthread_join (pthread_t threadid, void **thread_return)
 {
-  struct pthread *pd = (struct pthread *) threadid;
-
-  /* Make sure the descriptor is valid.  */
-  if (INVALID_NOT_TERMINATED_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* Is the thread joinable?.  */
-  if (IS_DETACHED (pd))
-    /* We cannot wait for the thread.  */
-    return EINVAL;
-
-  struct pthread *self = THREAD_SELF;
-  int result = 0;
-
-  LIBC_PROBE (pthread_join, 1, threadid);
-
-  /* During the wait we change to asynchronous cancellation.  If we
-     are canceled the thread we are waiting for must be marked as
-     un-wait-ed for again.  */
-  pthread_cleanup_push (cleanup, &pd->joinid);
-
-  /* Switch to asynchronous cancellation.  */
-  int oldtype = CANCEL_ASYNC ();
-
-  if ((pd == self
-       || (self->joinid == pd
-	   && (pd->cancelhandling
-	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
-		  | TERMINATED_BITMASK)) == 0))
-      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
-    /* This is a deadlock situation.  The threads are waiting for each
-       other to finish.  Note that this is a "may" error.  To be 100%
-       sure we catch this error we would have to lock the data
-       structures but it is not necessary.  In the unlikely case that
-       two threads are really caught in this situation they will
-       deadlock.  It is the programmer's problem to figure this
-       out.  */
-    result = EDEADLK;
-  /* Wait for the thread to finish.  If it is already locked something
-     is wrong.  There can only be one waiter.  */
-  else if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
-								   self,
-								   NULL), 0))
-    /* There is already somebody waiting for the thread.  */
-    result = EINVAL;
-  else
-    /* Wait for the child.  */
-    lll_wait_tid (pd->tid);
-
-
-  /* Restore cancellation mode.  */
-  CANCEL_RESET (oldtype);
-
-  /* Remove the handler.  */
-  pthread_cleanup_pop (0);
-
-
-  if (__glibc_likely (result == 0))
-    {
-      /* We mark the thread as terminated and as joined.  */
-      pd->tid = -1;
-
-      /* Store the return value if the caller is interested.  */
-      if (thread_return != NULL)
-	*thread_return = pd->result;
-
-
-      /* Free the TCB.  */
-      __free_tcb (pd);
-    }
-
-  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
-
-  return result;
+  return __pthread_timedjoin_ex (threadid, thread_return, NULL, true);
 }
 weak_alias (__pthread_join, pthread_join)
diff --git a/nptl/pthread_join_common.c b/nptl/pthread_join_common.c
new file mode 100644
index 0000000..59c5445
--- /dev/null
+++ b/nptl/pthread_join_common.c
@@ -0,0 +1,115 @@
+/* Common definition for pthread_{timed,try}join{_np}.
+   Copyright (C) 2017 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/>.  */
+
+#include "pthreadP.h"
+#include <atomic.h>
+#include <stap-probe.h>
+
+static void
+cleanup (void *arg)
+{
+  /* If we already changed the waiter ID, reset it.  The call cannot
+     fail for any reason but the thread not having done that yet so
+     there is no reason for a loop.  */
+  struct pthread *self = THREAD_SELF;
+  atomic_compare_exchange_weak_acquire (&arg, &self, NULL);
+}
+
+int
+__pthread_timedjoin_ex (pthread_t threadid, void **thread_return,
+			const struct timespec *abstime, bool block)
+{
+  struct pthread *pd = (struct pthread *) threadid;
+
+  /* Make sure the descriptor is valid.  */
+  if (INVALID_NOT_TERMINATED_TD_P (pd))
+    /* Not a valid thread handle.  */
+    return ESRCH;
+
+  /* Is the thread joinable?.  */
+  if (IS_DETACHED (pd))
+    /* We cannot wait for the thread.  */
+    return EINVAL;
+
+  struct pthread *self = THREAD_SELF;
+  int result = 0;
+
+  LIBC_PROBE (pthread_join, 1, threadid);
+
+  if ((pd == self
+       || (self->joinid == pd
+	   && (pd->cancelhandling
+	       & (CANCELING_BITMASK | CANCELED_BITMASK | EXITING_BITMASK
+		  | TERMINATED_BITMASK)) == 0))
+      && !CANCEL_ENABLED_AND_CANCELED (self->cancelhandling))
+    /* This is a deadlock situation.  The threads are waiting for each
+       other to finish.  Note that this is a "may" error.  To be 100%
+       sure we catch this error we would have to lock the data
+       structures but it is not necessary.  In the unlikely case that
+       two threads are really caught in this situation they will
+       deadlock.  It is the programmer's problem to figure this
+       out.  */
+    return EDEADLK;
+
+  /* Wait for the thread to finish.  If it is already locked something
+     is wrong.  There can only be one waiter.  */
+  else if (__glibc_unlikely (atomic_compare_exchange_weak_acquire (&pd->joinid,
+								   &self,
+								   NULL)))
+    /* There is already somebody waiting for the thread.  */
+    return EINVAL;
+
+  if (block)
+    {
+      /* During the wait we change to asynchronous cancellation.  If we
+	 are cancelled the thread we are waiting for must be marked as
+	 un-wait-ed for again.  */
+      pthread_cleanup_push (cleanup, &pd->joinid);
+
+      int oldtype = CANCEL_ASYNC ();
+
+      if (abstime != NULL)
+	result = lll_timedwait_tid (pd->tid, abstime);
+      else
+	lll_wait_tid (pd->tid);
+
+      CANCEL_RESET (oldtype);
+
+      pthread_cleanup_pop (0);
+    }
+
+  if (__glibc_likely (result == 0))
+    {
+      /* We mark the thread as terminated and as joined.  */
+      pd->tid = -1;
+
+      /* Store the return value if the caller is interested.  */
+      if (thread_return != NULL)
+	*thread_return = pd->result;
+
+      /* Free the TCB.  */
+      __free_tcb (pd);
+    }
+  else
+    pd->joinid = NULL;
+
+  LIBC_PROBE (pthread_join_ret, 3, threadid, result, pd->result);
+
+  return result;
+}
+hidden_def (__pthread_timedjoin_ex)
diff --git a/nptl/pthread_timedjoin.c b/nptl/pthread_timedjoin.c
index 567c171..0f74a98 100644
--- a/nptl/pthread_timedjoin.c
+++ b/nptl/pthread_timedjoin.c
@@ -16,89 +16,12 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-#include <atomic.h>
 #include "pthreadP.h"
 
-
-static void
-cleanup (void *arg)
-{
-  *(void **) arg = NULL;
-}
-
-
 int
-pthread_timedjoin_np (pthread_t threadid, void **thread_return,
-		      const struct timespec *abstime)
+__pthread_timedjoin_np (pthread_t threadid, void **thread_return,
+			const struct timespec *abstime)
 {
-  struct pthread *self;
-  struct pthread *pd = (struct pthread *) threadid;
-  int result;
-
-  /* Make sure the descriptor is valid.  */
-  if (INVALID_NOT_TERMINATED_TD_P (pd))
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* Is the thread joinable?.  */
-  if (IS_DETACHED (pd))
-    /* We cannot wait for the thread.  */
-    return EINVAL;
-
-  self = THREAD_SELF;
-  if (pd == self || self->joinid == pd)
-    /* This is a deadlock situation.  The threads are waiting for each
-       other to finish.  Note that this is a "may" error.  To be 100%
-       sure we catch this error we would have to lock the data
-       structures but it is not necessary.  In the unlikely case that
-       two threads are really caught in this situation they will
-       deadlock.  It is the programmer's problem to figure this
-       out.  */
-    return EDEADLK;
-
-  /* Wait for the thread to finish.  If it is already locked something
-     is wrong.  There can only be one waiter.  */
-  if (__builtin_expect (atomic_compare_and_exchange_bool_acq (&pd->joinid,
-							      self, NULL), 0))
-    /* There is already somebody waiting for the thread.  */
-    return EINVAL;
-
-
-  /* During the wait we change to asynchronous cancellation.  If we
-     are cancelled the thread we are waiting for must be marked as
-     un-wait-ed for again.  */
-  pthread_cleanup_push (cleanup, &pd->joinid);
-
-  /* Switch to asynchronous cancellation.  */
-  int oldtype = CANCEL_ASYNC ();
-
-
-  /* Wait for the child.  */
-  result = lll_timedwait_tid (pd->tid, abstime);
-
-
-  /* Restore cancellation mode.  */
-  CANCEL_RESET (oldtype);
-
-  /* Remove the handler.  */
-  pthread_cleanup_pop (0);
-
-
-  /* We might have timed out.  */
-  if (result == 0)
-    {
-      /* Store the return value if the caller is interested.  */
-      if (thread_return != NULL)
-	*thread_return = pd->result;
-
-
-      /* Free the TCB.  */
-      __free_tcb (pd);
-    }
-  else
-    pd->joinid = NULL;
-
-  return result;
+  return __pthread_timedjoin_ex (threadid, thread_return, abstime, true);
 }
+weak_alias (__pthread_timedjoin_np, pthread_timedjoin_np)
diff --git a/nptl/pthread_tryjoin.c b/nptl/pthread_tryjoin.c
index 6a3b62e..b34df54 100644
--- a/nptl/pthread_tryjoin.c
+++ b/nptl/pthread_tryjoin.c
@@ -16,57 +16,17 @@
    License along with the GNU C Library; if not, see
    <http://www.gnu.org/licenses/>.  */
 
-#include <errno.h>
-#include <stdlib.h>
-
-#include <atomic.h>
 #include "pthreadP.h"
 
-
 int
 pthread_tryjoin_np (pthread_t threadid, void **thread_return)
 {
-  struct pthread *self;
-  struct pthread *pd = (struct pthread *) threadid;
-
-  /* Make sure the descriptor is valid.  */
-  if (DEBUGGING_P && __find_in_stack_list (pd) == NULL)
-    /* Not a valid thread handle.  */
-    return ESRCH;
-
-  /* Is the thread joinable?.  */
-  if (IS_DETACHED (pd))
-    /* We cannot wait for the thread.  */
-    return EINVAL;
-
-  self = THREAD_SELF;
-  if (pd == self || self->joinid == pd)
-    /* This is a deadlock situation.  The threads are waiting for each
-       other to finish.  Note that this is a "may" error.  To be 100%
-       sure we catch this error we would have to lock the data
-       structures but it is not necessary.  In the unlikely case that
-       two threads are really caught in this situation they will
-       deadlock.  It is the programmer's problem to figure this
-       out.  */
-    return EDEADLK;
-
   /* Return right away if the thread hasn't terminated yet.  */
+  struct pthread *pd = (struct pthread *) threadid;
   if (pd->tid != 0)
     return EBUSY;
 
-  /* Wait for the thread to finish.  If it is already locked something
-     is wrong.  There can only be one waiter.  */
-  if (atomic_compare_and_exchange_bool_acq (&pd->joinid, self, NULL))
-    /* There is already somebody waiting for the thread.  */
-    return EINVAL;
-
-  /* Store the return value if the caller is interested.  */
-  if (thread_return != NULL)
-    *thread_return = pd->result;
-
-
-  /* Free the TCB.  */
-  __free_tcb (pd);
-
-  return 0;
+  /* If pd->tid != 0 then lll_wait_tid will not block on futex
+     operation.  */
+  return __pthread_timedjoin_ex (threadid, thread_return, NULL, false);
 }
diff --git a/sysdeps/unix/sysv/linux/i386/lowlevellock.h b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
index 197bb1f..098c64b 100644
--- a/sysdeps/unix/sysv/linux/i386/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/i386/lowlevellock.h
@@ -237,12 +237,7 @@ extern int __lll_timedwait_tid (int *tid, const struct timespec *abstime)
   ({									      \
     int __result = 0;							      \
     if ((tid) != 0)							      \
-      {									      \
-	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
-	  __result = EINVAL;						      \
-	else								      \
-	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
-      }									      \
+      __result = __lll_timedwait_tid (&(tid), (abstime));		      \
     __result; })
 
 
diff --git a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
index cbf6597..0b56a2e 100644
--- a/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
+++ b/sysdeps/unix/sysv/linux/x86_64/lowlevellock.h
@@ -249,12 +249,7 @@ extern int __lll_timedwait_tid (int *, const struct timespec *)
   ({									      \
     int __result = 0;							      \
     if ((tid) != 0)							      \
-      {									      \
-	if ((abstime)->tv_nsec < 0 || (abstime)->tv_nsec >= 1000000000)	      \
-	  __result = EINVAL;						      \
-	else								      \
-	  __result = __lll_timedwait_tid (&(tid), (abstime));		      \
-      }									      \
+      __result = __lll_timedwait_tid (&(tid), (abstime));		      \
     __result; })
 
 extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
-- 
2.7.4

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-12-20 13:41 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-12-19 13:40 [PATCH] nptl: Consolidate pthread_{timed,try}join{_np} Adhemerval Zanella
2017-12-20  5:44 ` Carlos O'Donell
2017-12-20 13:41   ` Adhemerval Zanella

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).