Re: [PATCH] S/390: Port of lock elision to System/z

[Andreas Krebbel <krebbel at linux dot vnet dot ibm dot com>]

On 04/30/2014 04:28 PM, Stefan Liebler wrote:
> Hi,
> 
> like mentioned in the discussion before, the clobbering of 
> FPRs/cc/memory is not needed anymore. In an older patch-version, the 
> transaction was either started by the builtin_tbegin or an asm("tbegin").
> In the latter case, the clobbering of FPRs was needed.
> There was also a non-transactional store, which is not needed anymore.
> The objdumps of this patch-version with and without the clobbering does 
> not differ.
> 
> The file nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_unlock.c is not 
> added anymore, because it only includes the nptl/pthread_mutex_unlock.c.
> 
> Ok?
> 
> Bye
> 
> ---
> 2014-04-30  Dominik Vogt  <vogt@linux.vnet.ibm.com>
>              Stefan Liebler  <stli@linux.vnet.ibm.com>
> 
> 	* config.make.in (enable-lock-elision): New Makefile variable.
> 	* configure.ac: Likewise.
> 	* configure: Regenerate.
> 	* sysdeps/s390/configure.ac:
> 	Add check for gcc transactions support.
> 	* sysdeps/s390/configure: Regenerate.
> 	* nptl/sysdeps/unix/sysv/linux/s390/Makefile: New file.
> 	Build elision files if enabled.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-conf.c: New file.
> 	Add lock elision support for s390.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-conf.h: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-lock.c: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-timed.c: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-trylock.c: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/elision-unlock.c: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/force-elision.h: Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_cond_lock.c:
> 	Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_lock.c:
> 	Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_timedlock.c:
> 	Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/pthread_mutex_trylock.c:
> 	Likewise.
> 	* nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h:
> 	(__lll_timedlock_elision, __lll_lock_elision)
> 	(__lll_unlock_elision, __lll_trylock_elision)
> 	(lll_timedlock_elision, lll_lock_elision)
> 	(lll_unlock_elision, lll_trylock_elision): Add.
> 	* nptl/sysdeps/unix/sysv/linux/s390/bits/pthreadtypes.h
> 	(pthread_mutex_t): Add lock elision support for s390.
> ---
> 

Just some (very) minor nitpicking:

nptl/sysdeps/unix/sysv/linux/s390/lowlevellock.h:

+#ifdef ENABLE_LOCK_ELISION
+extern int __lll_lock_elision (int *futex, short *adapt_count, int private)
+  attribute_hidden;
+
+extern int __lll_unlock_elision(int *lock, int private)
+  attribute_hidden;
+
+extern int __lll_trylock_elision(int *lock, short *adapt_count)
+  attribute_hidden;

What's the reason for calling the first argument futex once and lock
in the other cases?

nptl/sysdeps/unix/sysv/linux/s390/elision-conf.c:

+{
+  /* Set when the CPU supports elision. When false elision is never
  attempted.  */
... when the CPU and the kernel support transactional execution. ...

+  int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;


nptl/sysdeps/unix/sysv/linux/s390/elision-lock.c:

+int
+__lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
+{
+  if (*adapt_count <= 0)
+    {
...
+    {
+  else
+    {
+      /* Lost updates are possible, but harmless.  Due to races this might lead
+	 to *adapt_count becoming less than zero.  */
+      (*adapt_count)--;
+    }
+
+  use_lock:
+  return LLL_LOCK ((*futex), private);
+}

Perhaps something like this to reduce indentation of the whole function?!

+int
+__lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int private)
+{
+  if (*adapt_count > 0)
+    {
+      /* Lost updates are possible, but harmless.  Due to races this might lead
+	 to *adapt_count becoming less than zero.  */
+      (*adapt_count)--;
+      goto use_lock;
+    }
...
+
+  use_lock:
+  return LLL_LOCK ((*futex), private);
+}

+      /* Same logic as above, but for for a number of tepmorary failures in a
+	 row.  */

... temporary ...

nptl/sysdeps/unix/sysv/linux/s390/elision-trylock.c:

+  /* Implement POSIX semantics by forbiding nesting elided trylocks.
... forbidding ...

+      __builtin_tabort (_HTM_FIRST_USER_ABORT_CODE + 1);

Here you want to set the lowest order bit in order to enforce a
persistent abort.  While the code works fine since
_HTM_FIRST_USER_ABORT_CODE is even (_HTM_FIRST_USER_ABORT_CODE | 1)
would appear more natural to me.  The same appears in elision-lock.c.

Bye,

-Andreas-