powerpc: More elision improvements

  __lll_trylock_elision sets the adapt_count variable too
aggressively, and incorrectly on persistent aborts.  Taking
a cue from s390, adapt_count is only updated if the lock
is locked, or a persistent failure occurs.

In addition, the abort codes have been renumbered and
refactored for clarity.  As it stands, glibc only cares
if the abort is persistent or not.

All aborts are now persistent, excepting a busy lock.  This
includes changing _ABORT_NESTED_TRYLOCK into a persistent
abort.

2015-10-28  Paul E. Murphy  <murphyp@linux.vnet.ibm.com>

	* sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
	(__lll_trylock_elision): Fix setting of adapt_count.
	* sysdeps/unix/sysv/linux/powerpc/htm.h
	(_ABORT_PERSISTENT): Define to clarify persistent aborts.
	(_ABORT_NESTED_TRYLOCK): Renumber, and make persistent.
	(_ABORT_SYSCALL): Renumber, and clarify definition.
	(_ABORT_LOCK_BUSY): Renumber, make non-persistent.
---
 sysdeps/unix/sysv/linux/powerpc/elision-trylock.c | 11 ++++++-----
 sysdeps/unix/sysv/linux/powerpc/htm.h             | 12 +++++++-----
 2 files changed, 13 insertions(+), 10 deletions(-)
  

On 28-10-2015 20:34, Paul E. Murphy wrote:
> __lll_trylock_elision sets the adapt_count variable too
> aggressively, and incorrectly on persistent aborts.  Taking
> a cue from s390, adapt_count is only updated if the lock
> is locked, or a persistent failure occurs.
> 
> In addition, the abort codes have been renumbered and
> refactored for clarity.  As it stands, glibc only cares
> if the abort is persistent or not.
> 
> All aborts are now persistent, excepting a busy lock.  This
> includes changing _ABORT_NESTED_TRYLOCK into a persistent
> abort.
> 
> 2015-10-28  Paul E. Murphy  <murphyp@linux.vnet.ibm.com>
> 
> 	* sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
> 	(__lll_trylock_elision): Fix setting of adapt_count.
> 	* sysdeps/unix/sysv/linux/powerpc/htm.h
> 	(_ABORT_PERSISTENT): Define to clarify persistent aborts.
> 	(_ABORT_NESTED_TRYLOCK): Renumber, and make persistent.
> 	(_ABORT_SYSCALL): Renumber, and clarify definition.
> 	(_ABORT_LOCK_BUSY): Renumber, make non-persistent.
> ---
>  sysdeps/unix/sysv/linux/powerpc/elision-trylock.c | 11 ++++++-----
>  sysdeps/unix/sysv/linux/powerpc/htm.h             | 12 +++++++-----
>  2 files changed, 13 insertions(+), 10 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c b/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
> index 440939c..6f61eba 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
> +++ b/sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
> @@ -44,8 +44,12 @@ __lll_trylock_elision (int *futex, short *adapt_count)
>        if (*futex == 0)
>  	return 0;
>  
> -      /* Lock was busy.  Fall back to normal locking.  */
> -      __builtin_tabort (_ABORT_LOCK_BUSY);
> +      /* Lock was busy.  This is never a nested transaction.
> +         End it, and set the adapt count.  */
> +      __builtin_tend (0);
> +
> +      if (aconf.skip_lock_busy > 0)
> +	*adapt_count = aconf.skip_lock_busy;
>      }
>    else
>      {
> @@ -57,9 +61,6 @@ __lll_trylock_elision (int *futex, short *adapt_count)
>  	  if (aconf.skip_trylock_internal_abort > 0)
>  	    *adapt_count = aconf.skip_trylock_internal_abort;
>  	}
> -
> -	if (aconf.skip_lock_busy > 0)
> -	  *adapt_count = aconf.skip_lock_busy;
>      }
>  
>  use_lock:
> diff --git a/sysdeps/unix/sysv/linux/powerpc/htm.h b/sysdeps/unix/sysv/linux/powerpc/htm.h
> index 57d5cd6..5127b4b 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/htm.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/htm.h
> @@ -129,10 +129,12 @@
>  
>  #endif /* __ASSEMBLER__ */
>  
> -/* Definitions used for TEXASR Failure code (bits 0:6), they need to be even
> -   because tabort. always sets the first bit.  */
> -#define _ABORT_LOCK_BUSY       0x3f   /* Lock already used.  */
> -#define _ABORT_NESTED_TRYLOCK  0x3e   /* Write operation in trylock.  */
> -#define _ABORT_SYSCALL         0x3d   /* Syscall issued.  */
> +/* Definitions used for TEXASR Failure code (bits 0:7).  If the failure
> +   should be persistent, the abort code must be odd.  0xd0 through 0xff
> +   are reserved for the kernel and potential hypervisor.  */
> +#define _ABORT_PERSISTENT      0x01   /* An unspecified persistent abort.  */
> +#define _ABORT_LOCK_BUSY       0x34   /* Busy lock, not persistent.  */
> +#define _ABORT_NESTED_TRYLOCK  (0x32 | _ABORT_PERSISTENT)
> +#define _ABORT_SYSCALL         (0x30 | _ABORT_PERSISTENT)
>  
>  #endif
> 

LGTM.
  

LGTM.
  

"Paul E. Murphy" <murphyp@linux.vnet.ibm.com> writes:

> __lll_trylock_elision sets the adapt_count variable too
> aggressively, and incorrectly on persistent aborts.  Taking
> a cue from s390, adapt_count is only updated if the lock
> is locked, or a persistent failure occurs.
>
> In addition, the abort codes have been renumbered and
> refactored for clarity.  As it stands, glibc only cares
> if the abort is persistent or not.
>
> All aborts are now persistent, excepting a busy lock.  This
> includes changing _ABORT_NESTED_TRYLOCK into a persistent
> abort.
>
> 2015-10-28  Paul E. Murphy  <murphyp@linux.vnet.ibm.com>
>
> 	* sysdeps/unix/sysv/linux/powerpc/elision-trylock.c
> 	(__lll_trylock_elision): Fix setting of adapt_count.
> 	* sysdeps/unix/sysv/linux/powerpc/htm.h
> 	(_ABORT_PERSISTENT): Define to clarify persistent aborts.
> 	(_ABORT_NESTED_TRYLOCK): Renumber, and make persistent.
> 	(_ABORT_SYSCALL): Renumber, and clarify definition.
> 	(_ABORT_LOCK_BUSY): Renumber, make non-persistent.

LGTM too.

Pushed as 86b49398.
  

On Wed, 2015-10-28 at 17:34 -0500, Paul E. Murphy wrote:
> diff --git a/sysdeps/unix/sysv/linux/powerpc/htm.h b/sysdeps/unix/sysv/linux/powerpc/htm.h
> index 57d5cd6..5127b4b 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/htm.h
> +++ b/sysdeps/unix/sysv/linux/powerpc/htm.h
> @@ -129,10 +129,12 @@
>  
>  #endif /* __ASSEMBLER__ */
>  
> -/* Definitions used for TEXASR Failure code (bits 0:6), they need to be even
> -   because tabort. always sets the first bit.  */
> -#define _ABORT_LOCK_BUSY       0x3f   /* Lock already used.  */
> -#define _ABORT_NESTED_TRYLOCK  0x3e   /* Write operation in trylock.  */
> -#define _ABORT_SYSCALL         0x3d   /* Syscall issued.  */
> +/* Definitions used for TEXASR Failure code (bits 0:7).  If the failure
> +   should be persistent, the abort code must be odd.  0xd0 through 0xff
> +   are reserved for the kernel and potential hypervisor.  */
> +#define _ABORT_PERSISTENT      0x01   /* An unspecified persistent abort.  */
> +#define _ABORT_LOCK_BUSY       0x34   /* Busy lock, not persistent.  */
> +#define _ABORT_NESTED_TRYLOCK  (0x32 | _ABORT_PERSISTENT)
> +#define _ABORT_SYSCALL         (0x30 | _ABORT_PERSISTENT)
>  
>  #endif

I think the abort code should be well documented, and they are
effectively ABI in some cases.  Whether something is persistent or not
should be ABI because it requires any (unrelated) outermost
transactional code to do the right thing.  For other abort codes, it
would not be strictly necessary to make them part of the ABI, but
probably helpful for performance;  we'll have to try to pick retry
strategies that work across projects (eg, gcc, libitm, libstdc++, ...).

Thus, are these documented anywhere, or do you plan to add such
documentation?  (Making glibc the canonical source would be fine too for
me, as long as everyone is aware of that.)

(This is not a powerpc-specific problem, and I've asked Andi Kleen the
same.  It's also not a reason to not commit this patch because I assume
that there aren't yet any users out there in the wild that depend on the
previous failure codes).