Re: [PATCH 3/3] Add adaptive elision to rwlocks v2

["H.J. Lu" <hjl dot tools at gmail dot com>]

On Tue, May 6, 2014 at 2:36 PM, Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com>
>
> This patch relies on the C version of the rwlocks posted earlier.
> With C rwlocks it is very straight forward to do adaptive elision
> using TSX. It is based on the infrastructure added earlier
> for mutexes, but uses its own elision macros. The macros
> are fairly general purpose and could be used for other
> elision purposes too.
>
> This version is much cleaner than the earlier assembler based
> version, and in particular implements adaptation which makes
> it safer.
>
> I changed the behavior slightly to not require any changes
> in the test suite and fully conform to all expected
> behaviors (generally at the cost of not eliding in
> various situations). In particular this means the timedlock
> variants are not elided.  Nested trylock aborts.
>
> v2: Address Roland's feedback. Add ACCESS_ONCE usage.
>     I didn't use the nested function inlines, it
>     seemed too fragile. Also it still uses macros
>     to be generic.
>
> nptl/:
>
> 2014-05-06  Andi Kleen  <ak@linux.intel.com>
>
>         * pthread_rwlock_rdlock.c: Include elide.h.
>         (pthread_rwlock_rdlock): Add elision.
>         * pthread_rwlock_wrlock.c: Include elide.h.
>         (pthread_rwlock_wrlock): Add elision.
>         * pthread_rwlock_trywrlock.c: Include elide.h.
>         (pthread_rwlock_trywrlock): Add elision.
>         * pthread_rwlock_tryrdlock.c: Include elide.h.
>         (pthread_rwlock_tryrdlock): Add elision.
>         * pthread_rwlock_unlock.c: Include elide.h.
>         (pthread_rwlock_tryrdlock): Add elision unlock.
>         * sysdeps/pthread/pthread.h:
>         (__PTHREAD_RWLOCK_ELISION_EXTRA): Handle new define
>         (PTHREAD_RWLOCK_INITIALIZER,
>         PTHREAD_RWLOCK_WRITER_NONRECURSIVE_INITIALIZER_NP):
>         Handle new elision field.
>         * sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h:
>         (pthread_rwlock_t): Change __pad1 to __rwelision.
>         (__PTHREAD_RWLOCK_ELISION_EXTRA): Add.
>         * sysdeps/unix/sysv/linux/x86/elide.h:
>         New file. Add generic elision macros.
>         * sysdeps/unix/sysv/linux/x86/elision-conf.c:
>         (elision_init): Set try_xbegin to zero when no RTM.

> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> index b4329f6..bb1329c 100644
> --- a/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> +++ b/nptl/sysdeps/unix/sysv/linux/x86/bits/pthreadtypes.h
> @@ -184,11 +184,13 @@ typedef union
>      unsigned int __nr_writers_queued;
>      int __writer;
>      int __shared;
> -    unsigned long int __pad1;
> +    signed char __rwelision;
> +    unsigned char __pad1[7];
>      unsigned long int __pad2;


This is wrong for x32.  We need something like

diff --git a/sysdeps/x86/nptl/bits/pthreadtypes.h
b/sysdeps/x86/nptl/bits/pthreadtypes.h
index b04c32b..bfb9034 100644
--- a/sysdeps/x86/nptl/bits/pthreadtypes.h
+++ b/sysdeps/x86/nptl/bits/pthreadtypes.h
@@ -185,12 +185,17 @@ typedef union
     int __writer;
     int __shared;
     signed char __rwelision;
+#  ifdef __ILP32__
+    unsigned char __pad1[3];
+#   define __PTHREAD_RWLOCK_ELISION_EXTRA 0, {0, 0, 0 }
+#  else
     unsigned char __pad1[7];
+#   define __PTHREAD_RWLOCK_ELISION_EXTRA 0, {0, 0, 0, 0, 0, 0, 0 }
+#  endif
     unsigned long int __pad2;
     /* FLAGS must stay at this position in the structure to maintain
        binary compatibility.  */
     unsigned int __flags;
-# define __PTHREAD_RWLOCK_ELISION_EXTRA 0, {0, 0, 0, 0, 0, 0, 0 }
 # define __PTHREAD_RWLOCK_INT_FLAGS_SHARED  1
   } __data;
 # else


H.J.