| Message ID | 55DC9C24.5090401@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 08/25/2015 12:47 PM, Adhemerval Zanella wrote: > I do not know which is better, since it will tie the ELIDE_LOCK implementation > with current internal pthread definitions. I prefer the idea of passing an inline function to ELIDE_LOCK and have the inline funciton do what it needs to compute is_lock_free and then return the result. This leaves ELIDE_LOCK simpler. However, I'm not really for one way or the other, they are roughly all the same. The inline function is just a cleaner interface and puts the evaluation of is_lock_free all in one place to be inlined by the compiler. I think the end result of Torvald's comments is that when we switch to C11 atomics, we'll need to reference __lock with atomic_load rleaxed, so we might as well start today. c.
On Tue, 2015-08-25 at 13:09 -0400, Carlos O'Donell wrote: > On 08/25/2015 12:47 PM, Adhemerval Zanella wrote: > > I do not know which is better, since it will tie the ELIDE_LOCK implementation > > with current internal pthread definitions. > > I prefer the idea of passing an inline function to ELIDE_LOCK > and have the inline funciton do what it needs to compute is_lock_free > and then return the result. This leaves ELIDE_LOCK simpler. I agree.
diff --git a/nptl/pthread_rwlock_wrlock.c b/nptl/pthread_rwlock_wrlock.c index 60fa909..0161876 100644 --- a/nptl/pthread_rwlock_wrlock.c +++ b/nptl/pthread_rwlock_wrlock.c @@ -98,9 +98,9 @@ __pthread_rwlock_wrlock (pthread_rwlock_t *rwlock) LIBC_PROBE (wrlock_entry, 1, rwlock); if (ELIDE_LOCK (rwlock->__data.__rwelision, - rwlock->__data.__lock == 0 - && rwlock->__data.__writer == 0 - && rwlock->__data.__nr_readers == 0)) + rwlock->__data.__lock, + rwlock->__data.__writer, + rwlock->__data.__nr_readers)) diff --git a/sysdeps/generic/elide.h b/sysdeps/generic/elide.h index 80a3a03..64d9cce 100644 --- a/sysdeps/generic/elide.h +++ b/sysdeps/generic/elide.h @@ -15,11 +15,12 @@ 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/>. */ + #ifndef ELIDE_H #define ELIDE_H 1 -#define ELIDE_LOCK(adapt_count, is_lock_free) 0 -#define ELIDE_TRYLOCK(adapt_count, is_lock_free, write) 0 -#define ELIDE_UNLOCK(is_lock_free) 0 +#define ELIDE_LOCK(adapt_count, lock, writer, readers) 0 +#define ELIDE_TRYLOCK(adapt_count, lock, writer, readers, write) 0 +#define ELIDE_UNLOCK(writer, readers) 0 #endif diff --git a/sysdeps/powerpc/nptl/elide.h b/sysdeps/powerpc/nptl/elide.h index 389f5a5..b3cc50a 100644 --- a/sysdeps/powerpc/nptl/elide.h +++ b/sysdeps/powerpc/nptl/elide.h @@ -27,7 +27,7 @@ ADAPT_COUNT is a pointer to per-lock state variable. */ static inline bool -__elide_lock (uint8_t *adapt_count, int is_lock_free) +__elide_lock (uint8_t *adapt_count, int *lock, int *writer, unsigned int *readers) { if (*adapt_count > 0) { @@ -39,7 +39,10 @@ __elide_lock (uint8_t *adapt_count, int is_lock_free) { if (__builtin_tbegin (0)) { - if (is_lock_free) + /* The compiler barrier is required because some GCC version might + reorder the lock read before the transaction init builtin. */ + asm volatile("" ::: "memory"); + if ((*lock == 0) && (*writer == 0) && (*readers == 0)) return true; /* Lock was busy. */ __builtin_tabort (_ABORT_LOCK_BUSY); @@ -66,30 +69,31 @@ __elide_lock (uint8_t *adapt_count, int is_lock_free) return false; } I do not know which is better, since it will tie the ELIDE_LOCK implementation with current internal pthread definitions.