On Fri, Nov 25, 2011 at 10:33:28AM +1030, Alan Modra wrote:
> This fixes PR51249, a failure caused by insufficient care in waking
> threads on sem_post. It's quite a tricky business to get right, but I
> believe this rewrite is now correct. I've also converted over lock.c,
> mutex.h and mutex.c to use the new atomic builtins. This means no
> target should need target-specific versions of mutex.h. mutex-lock.h
> came about because at one stage I was trying out semaphore and lock
> implementations that used a lock/count int and an nwaiters int to
> track number of threads waiting. That turned out to be a really bad
> idea. You need barriers all over the place to ensure you don't see an
> inconsistent lock/semaphore state, and extra barriers are costly.
> It's much better to pay the price of an extra futex_wake system call
> when transitioning from contended to uncontended.
> Anyway, I left the lock abstraction as a good thing. I also changed
> the mutex implementation to use -1 to mean "locked and (possibly) some
> thread waiting on the lock" rather than using 2. The very minor
> benefit is that some targets may use one less instruction testing < 0
> compared to testing > 1. It's also just that little bit more like the
> semaphore implementation.
>
> Bootstrapped and regression tested powerpc64-linux. I do still see
> the occasional testsuite failures on power7 (see pr51298), but the
> semaphore failures are gone.
Thanks for working on it.
My preference would be to avoid the abstraction changes though, both
because it is additional clutter in the changeset and because omp_lock
and nested lock are part of public ABIs, so if struct is layed out
differently on some weird architecture, it would be an ABI change.
So, if you could keep gomp_mutex_t, omp_lock_t and gomp_sem_t as integers,
it would be appreciated.
Furthermore, I'd prefer if the patch could be split into smaller parts,
e.g. for bisecting purposes. One patch would do the mutex changes
to use new atomics, remove extra mutex.h headers and start using 0/1/-1
instead of 0/1/2. And another patch would rewrite the semaphores.
Jakub
