On Wed, Mar 16, 2016 at 02:37:40PM +0200, Paul Irofti wrote:
> > What do you think?
>
> I think this diff is even safer: it checks for threads entering and
> exiting the barrier while holding the mutex, and if both values are 0 it
> proceeds to destroy the barrier.
Not 100% sure but I think a situation could arise where sofar == 0 and
the mutex is unlocked, so I agree. This needs a major bump.
>
> I also renamed sofar to in so that the relationship between threads is
> more obvious.
>
>
> Index: rthread.h
> ===================================================================
> RCS file: /cvs/src/lib/librthread/rthread.h,v
> retrieving revision 1.55
> diff -u -p -r1.55 rthread.h
> --- rthread.h 27 Jan 2016 08:40:05 -0000 1.55
> +++ rthread.h 16 Mar 2016 12:36:39 -0000
> @@ -141,7 +141,8 @@ struct pthread_barrier {
> pthread_mutex_t mutex;
> pthread_cond_t cond;
> int threshold;
> - int sofar;
> + int in;
> + int out;
> int generation;
> };
>
> Index: rthread_barrier.c
> ===================================================================
> RCS file: /cvs/src/lib/librthread/rthread_barrier.c,v
> retrieving revision 1.2
> diff -u -p -r1.2 rthread_barrier.c
> --- rthread_barrier.c 23 Apr 2012 08:30:33 -0000 1.2
> +++ rthread_barrier.c 16 Mar 2016 12:36:39 -0000
> @@ -74,12 +74,18 @@ pthread_barrier_destroy(pthread_barrier_
> if (barrier == NULL || *barrier == NULL)
> return (EINVAL);
>
> + if (pthread_mutex_trylock(&(*barrier)->mutex))
> + return (EBUSY);
Please don't hardcode EBUSY instead of passing the return code of
trylock.
The program should have a chance to terminate with an error instead of
spinning on an unrecoverable situation (eg when the mutex is already
destroyed).
Nothing prevents mutex_trylock from succeeding as long as there's no
segfault or a junk that causes an error, but we can't do anything about
that.
> +
> b = *barrier;
b can be NULL here because the compiler should (must?) reload *barrier
after a function call that could've done god knows what.
Was this your intention? You could add another NULL check but that's
more duct tape on a "problem" that can't be solved without another lock
or CAS for the barrier itself.
I'm for moving this line outside the critical region. b should stay
valid during the entire function, even if the memory it points to goes
away. Anything else is way to tricky and relies on the compiler not
knowing what mutex_trylock does.
>
> - if (b->sofar > 0)
> + if (b->out > 0 || b->in > 0) {
> + pthread_mutex_unlock(&b->mutex);
> return (EBUSY);
> + }
>
> *barrier = NULL;
> + pthread_mutex_unlock(&b->mutex);
> pthread_mutex_destroy(&b->mutex);
> pthread_cond_destroy(&b->cond);
> free(b);
> @@ -103,9 +109,10 @@ pthread_barrier_wait(pthread_barrier_t *
> if ((rc = pthread_mutex_lock(&b->mutex)))
> goto cancel;
>
> - _rthread_debug(6, "sofar: %d, threshold: %d\n", b->sofar, b->threshold);
> - if (++b->sofar == b->threshold) {
> - b->sofar = 0;
> + _rthread_debug(6, "in: %d, threshold: %d\n", b->in, b->threshold);
> + if (++b->in == b->threshold) {
> + b->out = b->in - 1; /* mark thread exit */
> + b->in = 0;
> b->generation++;
> if ((rc = pthread_cond_broadcast(&b->cond)))
> goto err;
> @@ -118,6 +125,7 @@ pthread_barrier_wait(pthread_barrier_t *
> if ((rc = pthread_cond_wait(&b->cond, &b->mutex)))
> goto err;
> } while (gen == b->generation);
> + b->out--; /* mark thread exit */
> }
>
> err:
>
While we're at it, I noticed all the goto err and cancel paths have
their return code overwritten.
