[PATCH v2 1/2] pthread: add missing memory barrier

[PATCH v2 1/2] pthread: add missing memory barrier

[Bobby Bingham]

POSIX requires pthread_join to synchronize memory on success.  The
futex wait inside __timedwait_cp cannot handle this because it's not
called in all cases.  Also, in the case of a spurious wake, tid can
become zero between the wake and when the joining thread checks it.
---
 src/thread/pthread_join.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
index 966b4ab..694d377 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -13,6 +13,7 @@ int __pthread_join(pthread_t t, void **res)
 	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
 	while ((tmp = t->tid)) __timedwait_cp(&t->tid, tmp, 0, 0, 0);
 	__pthread_setcancelstate(cs, 0);
+	a_barrier();
 	if (res) *res = t->result;
 	if (t->map_base) __munmap(t->map_base, t->map_size);
 	return 0;
-- 
2.6.0

[PATCH v2 2/2] pthread: implement try/timed join variants

[Bobby Bingham]

---
 include/pthread.h         |  2 ++
 src/thread/pthread_join.c | 29 ++++++++++++++++++++++-------
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/include/pthread.h b/include/pthread.h
index af70b73..3d2e0c4 100644
--- a/include/pthread.h
+++ b/include/pthread.h
@@ -214,6 +214,8 @@ struct cpu_set_t;
 int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *);
 int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *);
 int pthread_getattr_np(pthread_t, pthread_attr_t *);
+int pthread_tryjoin_np(pthread_t, void **);
+int pthread_timedjoin_np(pthread_t, void **, const struct timespec *);
 #endif
 
 #ifdef __cplusplus
diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
index 694d377..b041ff8 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -5,18 +5,33 @@ int __munmap(void *, size_t);
 void __pthread_testcancel(void);
 int __pthread_setcancelstate(int, int *);
 
-int __pthread_join(pthread_t t, void **res)
+int __pthread_tryjoin_np(pthread_t t, void **res)
 {
-	int tmp, cs;
-	__pthread_testcancel();
-	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
-	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
-	while ((tmp = t->tid)) __timedwait_cp(&t->tid, tmp, 0, 0, 0);
-	__pthread_setcancelstate(cs, 0);
+	if (t->tid) return EBUSY;
 	a_barrier();
 	if (res) *res = t->result;
 	if (t->map_base) __munmap(t->map_base, t->map_size);
 	return 0;
 }
 
+int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec *at)
+{
+	int tmp, cs, r = 0;
+	__pthread_testcancel();
+	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
+	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
+	while ((tmp = t->tid) && r != ETIMEDOUT && r != EINVAL)
+		r = __timedwait_cp(&t->tid, tmp, CLOCK_REALTIME, at, 0);
+	__pthread_setcancelstate(cs, 0);
+	if (r == ETIMEDOUT || r == EINVAL) return r;
+	return __pthread_tryjoin_np(t, res);
+}
+
+int __pthread_join(pthread_t t, void **res)
+{
+	return __pthread_timedjoin_np(t, res, 0);
+}
+
+weak_alias(__pthread_tryjoin_np, pthread_tryjoin_np);
+weak_alias(__pthread_timedjoin_np, pthread_timedjoin_np);
 weak_alias(__pthread_join, pthread_join);
-- 
2.6.0

Re: [PATCH v2 2/2] pthread: implement try/timed join variants

[Rich Felker]

On Sun, Oct 04, 2015 at 02:55:30PM -0500, Bobby Bingham wrote:
> ---
>  include/pthread.h         |  2 ++
>  src/thread/pthread_join.c | 29 ++++++++++++++++++++++-------
>  2 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/include/pthread.h b/include/pthread.h
> index af70b73..3d2e0c4 100644
> --- a/include/pthread.h
> +++ b/include/pthread.h
> @@ -214,6 +214,8 @@ struct cpu_set_t;
>  int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *);
>  int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *);
>  int pthread_getattr_np(pthread_t, pthread_attr_t *);
> +int pthread_tryjoin_np(pthread_t, void **);
> +int pthread_timedjoin_np(pthread_t, void **, const struct timespec *);
>  #endif
>  
>  #ifdef __cplusplus
> diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
> index 694d377..b041ff8 100644
> --- a/src/thread/pthread_join.c
> +++ b/src/thread/pthread_join.c
> @@ -5,18 +5,33 @@ int __munmap(void *, size_t);
>  void __pthread_testcancel(void);
>  int __pthread_setcancelstate(int, int *);
>  
> -int __pthread_join(pthread_t t, void **res)
> +int __pthread_tryjoin_np(pthread_t t, void **res)
>  {
> -	int tmp, cs;
> -	__pthread_testcancel();
> -	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> -	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
> -	while ((tmp = t->tid)) __timedwait_cp(&t->tid, tmp, 0, 0, 0);
> -	__pthread_setcancelstate(cs, 0);
> +	if (t->tid) return EBUSY;
>  	a_barrier();
>  	if (res) *res = t->result;
>  	if (t->map_base) __munmap(t->map_base, t->map_size);
>  	return 0;
>  }

This can spuriously fail when lack of memory synchronization causes
the caller not to see that the target thread has exited. However this
condition does not seem observable; calling pthread_tryjoin_np from
multiple threads where the calls could race is certainly UB, and
otherwise there should be no way to distinguish between the state of
just-before-exiting and already-exited. So I _think_ this is okay.

> +int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec *at)
> +{
> +	int tmp, cs, r = 0;
> +	__pthread_testcancel();
> +	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> +	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
> +	while ((tmp = t->tid) && r != ETIMEDOUT && r != EINVAL)
> +		r = __timedwait_cp(&t->tid, tmp, CLOCK_REALTIME, at, 0);
> +	__pthread_setcancelstate(cs, 0);
> +	if (r == ETIMEDOUT || r == EINVAL) return r;
> +	return __pthread_tryjoin_np(t, res);
> +}
> +
> +int __pthread_join(pthread_t t, void **res)
> +{
> +	return __pthread_timedjoin_np(t, res, 0);
> +}
> +
> +weak_alias(__pthread_tryjoin_np, pthread_tryjoin_np);
> +weak_alias(__pthread_timedjoin_np, pthread_timedjoin_np);
>  weak_alias(__pthread_join, pthread_join);

This all looks okay to me. I'll probably commit the patch as-is, as
long as nobody thinks the reasoning on the tryjoin issue above is
wrong.

Rich

Re: [PATCH v2 2/2] pthread: implement try/timed join variants

[Rich Felker]

On Sun, Oct 11, 2015 at 10:30:20PM -0400, Rich Felker wrote:
> > +int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec *at)
> > +{
> > +	int tmp, cs, r = 0;
> > +	__pthread_testcancel();
> > +	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
> > +	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
> > +	while ((tmp = t->tid) && r != ETIMEDOUT && r != EINVAL)
> > +		r = __timedwait_cp(&t->tid, tmp, CLOCK_REALTIME, at, 0);
> > +	__pthread_setcancelstate(cs, 0);
> > +	if (r == ETIMEDOUT || r == EINVAL) return r;
> > +	return __pthread_tryjoin_np(t, res);
> > +}
> > +
> > +int __pthread_join(pthread_t t, void **res)
> > +{
> > +	return __pthread_timedjoin_np(t, res, 0);
> > +}
> > +
> > +weak_alias(__pthread_tryjoin_np, pthread_tryjoin_np);
> > +weak_alias(__pthread_timedjoin_np, pthread_timedjoin_np);
> >  weak_alias(__pthread_join, pthread_join);
> 
> This all looks okay to me. I'll probably commit the patch as-is, as
> long as nobody thinks the reasoning on the tryjoin issue above is
> wrong.

I went ahead and committed patch 1 since it's an obvious bug fix. The
second patch still looks ok to me but I'm going to hold off until
after release because I don't want to rush addition of a new
nonstandard extension without good review.

Rich

[PATCH v3] pthread: implement try/timed join variants

[Bobby Bingham]

---

In the interest of unstalling this, here's a new, less invasive version.

In the previous patch, timejoin waited for the thread to exit, and tail
called to tryjoin which cleaned up after the thread and retrieved the
thread result.  The regular join was just a timedjoin with an infinite
timeout.

Now, timedjoin has all the logic for waiting, cleaning up after the
thread, and retrieving the result.  Regular join is still just a
timedjoin with an infinite timeout.  However, tryjoin now just checks if
the thread has exited and either returns EBUSY or calls join, depending.

 include/pthread.h         |  2 ++
 src/thread/pthread_join.c | 20 +++++++++++++++++---
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/include/pthread.h b/include/pthread.h
index af70b73..3d2e0c4 100644
--- a/include/pthread.h
+++ b/include/pthread.h
@@ -214,6 +214,8 @@ struct cpu_set_t;
 int pthread_getaffinity_np(pthread_t, size_t, struct cpu_set_t *);
 int pthread_setaffinity_np(pthread_t, size_t, const struct cpu_set_t *);
 int pthread_getattr_np(pthread_t, pthread_attr_t *);
+int pthread_tryjoin_np(pthread_t, void **);
+int pthread_timedjoin_np(pthread_t, void **, const struct timespec *);
 #endif
 
 #ifdef __cplusplus
diff --git a/src/thread/pthread_join.c b/src/thread/pthread_join.c
index 694d377..5211148 100644
--- a/src/thread/pthread_join.c
+++ b/src/thread/pthread_join.c
@@ -5,18 +5,32 @@ int __munmap(void *, size_t);
 void __pthread_testcancel(void);
 int __pthread_setcancelstate(int, int *);
 
-int __pthread_join(pthread_t t, void **res)
+int __pthread_timedjoin_np(pthread_t t, void **res, const struct timespec *at)
 {
-	int tmp, cs;
+	int tmp, cs, r = 0;
 	__pthread_testcancel();
 	__pthread_setcancelstate(PTHREAD_CANCEL_DISABLE, &cs);
 	if (cs == PTHREAD_CANCEL_ENABLE) __pthread_setcancelstate(cs, 0);
-	while ((tmp = t->tid)) __timedwait_cp(&t->tid, tmp, 0, 0, 0);
+	while ((tmp = t->tid) && r != ETIMEDOUT && r != EINVAL)
+		r = __timedwait_cp(&t->tid, tmp, CLOCK_REALTIME, at, 0);
 	__pthread_setcancelstate(cs, 0);
+	if (r == ETIMEDOUT || r == EINVAL) return r;
 	a_barrier();
 	if (res) *res = t->result;
 	if (t->map_base) __munmap(t->map_base, t->map_size);
 	return 0;
 }
 
+int __pthread_join(pthread_t t, void **res)
+{
+	return __pthread_timedjoin_np(t, res, 0);
+}
+
+int __pthread_tryjoin_np(pthread_t t, void **res)
+{
+	return t->tid ? EBUSY : __pthread_join(t, res);
+}
+
+weak_alias(__pthread_tryjoin_np, pthread_tryjoin_np);
+weak_alias(__pthread_timedjoin_np, pthread_timedjoin_np);
 weak_alias(__pthread_join, pthread_join);
-- 
2.8.1