[PATCH] Elision, both for s390 and x86_64, should be enabled via --enable-lock-elision=yes.

[PATCH] Elision, both for s390 and x86_64, should be enabled via --enable-lock-elision=yes.

[Carlos O'Donell]

Andreas,

All lock elision enablement should be controlled by the
--enable-lock-elision configure option.

I have already written to Andi about this, and he agreed
that it's OK for x86_64, and is what we agreed upon at
the global level for glibc.

Would you agree that this is also OK for s390?

The goal is to be able to enable or disable lock elision
for all architectures with a single configure option.

I understand that this is slightly different from what
we do for other features that we simply detect and use.

We do it this way for elision to give distributions the
flexibility because this feature is quite new and has potential
side-effects that perhaps might only be best enabled in a newer
version of the distribution.

It also gives users the ability to disable it if it's causing
problems for applications that are important to their use
cases, but that don't work well with elision.

In the long term I expect that runtime tunnables will be useful
in enabling or disabling elision on a per-application basis.

OK to commit for s390?

2014-09-29  Carlos O'Donell  <carlos@redhat.com>

	* configure.ac: --enable-lock-elision enables or disables
	all elision for locks, not just mutexes.
	* configure: Regenerate.
	* sysdeps/unix/sysv/linux/s390/elision-conf.c (elision_init)
	[!ENABLE_LOCK_ELISION]: Set __pthread_force_elision to zero.
	* sysdeps/unix/sysv/linux/x86/elision-conf.c (elision_init)
	[!ENABLE_LOCK_ELISION]: Set __pthread_force_elision,
	__elision_available, and __elision_aconf.retry_try_xbegin to zero.

diff --git a/configure b/configure
index 89566c5..9a9cf08 100755
--- a/configure
+++ b/configure
@@ -1413,7 +1413,7 @@ Optional Features:
                           initialize __stack_chk_guard canary with a random
                           number at program start
   --enable-lock-elision=yes/no
-                          Enable lock elision for pthread mutexes by default
+                          Enable hardware lock elision by default
   --enable-add-ons[=DIRS...]
                           configure and build add-ons in DIR1,DIR2,... search
                           for add-ons if no parameter given
diff --git a/configure.ac b/configure.ac
index 82d0896..d113772 100644
--- a/configure.ac
+++ b/configure.ac
@@ -169,7 +169,7 @@ fi
 
 AC_ARG_ENABLE([lock-elision],
 	      AC_HELP_STRING([--enable-lock-elision[=yes/no]],
-			     [Enable lock elision for pthread mutexes by default]),
+			     [Enable hardware lock elision by default]),
 	      [enable_lock_elision=$enableval],
 	      [enable_lock_elision=no])
 AC_SUBST(enable_lock_elision)
diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c b/sysdeps/unix/sysv/linux/s390/elision-conf.c
index 69c0483..852433e 100644
--- a/sysdeps/unix/sysv/linux/s390/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c
@@ -60,11 +60,16 @@ elision_init (int argc __attribute__ ((unused)),
 	      char **argv  __attribute__ ((unused)),
 	      char **environ)
 {
+#ifdef ENABLE_LOCK_ELISION
   /* Set when the CPU and the kernel supports transactional execution.
      When false elision is never attempted.  */
   int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
 
   __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
+#else
+  /* This configuration has elision disabled.  */
+  __pthread_force_elision = 0;
+#endif
 }
 
 #ifdef SHARED
diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c
index 28e48d9..abc42b7 100644
--- a/sysdeps/unix/sysv/linux/x86/elision-conf.c
+++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c
@@ -62,12 +62,17 @@ elision_init (int argc __attribute__ ((unused)),
 	      char **argv  __attribute__ ((unused)),
 	      char **environ)
 {
-  __elision_available = HAS_RTM;
 #ifdef ENABLE_LOCK_ELISION
+  __elision_available = HAS_RTM;
   __pthread_force_elision = __libc_enable_secure ? 0 : __elision_available;
-#endif
   if (!HAS_RTM)
-    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
+    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks.  */
+#else
+  /* This configuration has elision disabled.  */
+  __elision_available = 0;
+  __pthread_force_elision = 0;
+  __elision_aconf.retry_try_xbegin = 0;
+#endif
 }
 
 #ifdef SHARED
---

Re: [PATCH] Elision, both for s390 and x86_64, should be enabled via --enable-lock-elision=yes.

[Andreas Schwab]

"Carlos O'Donell" <carlos@redhat.com> writes:

> diff --git a/configure b/configure
> index 89566c5..9a9cf08 100755
> --- a/configure
> +++ b/configure
> @@ -1413,7 +1413,7 @@ Optional Features:
>                            initialize __stack_chk_guard canary with a random
>                            number at program start
>    --enable-lock-elision=yes/no
> -                          Enable lock elision for pthread mutexes by default
> +                          Enable hardware lock elision by default
>    --enable-add-ons[=DIRS...]
>                            configure and build add-ons in DIR1,DIR2,... search
>                            for add-ons if no parameter given
> diff --git a/configure.ac b/configure.ac
> index 82d0896..d113772 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -169,7 +169,7 @@ fi
>  
>  AC_ARG_ENABLE([lock-elision],
>  	      AC_HELP_STRING([--enable-lock-elision[=yes/no]],

While you are at it, please remove [=yes/no].  This is common to all
--enable- options, and as you see above this is underquoted, so that the
brackets are eaten by m4.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [PATCH] Elision, both for s390 and x86_64, should be enabled via --enable-lock-elision=yes.

[Carlos O'Donell]

On 09/29/2014 06:43 PM, Andreas Schwab wrote:
> "Carlos O'Donell" <carlos@redhat.com> writes:
> 
>> diff --git a/configure b/configure
>> index 89566c5..9a9cf08 100755
>> --- a/configure
>> +++ b/configure
>> @@ -1413,7 +1413,7 @@ Optional Features:
>>                            initialize __stack_chk_guard canary with a random
>>                            number at program start
>>    --enable-lock-elision=yes/no
>> -                          Enable lock elision for pthread mutexes by default
>> +                          Enable hardware lock elision by default
>>    --enable-add-ons[=DIRS...]
>>                            configure and build add-ons in DIR1,DIR2,... search
>>                            for add-ons if no parameter given
>> diff --git a/configure.ac b/configure.ac
>> index 82d0896..d113772 100644
>> --- a/configure.ac
>> +++ b/configure.ac
>> @@ -169,7 +169,7 @@ fi
>>  
>>  AC_ARG_ENABLE([lock-elision],
>>  	      AC_HELP_STRING([--enable-lock-elision[=yes/no]],
> 
> While you are at it, please remove [=yes/no].  This is common to all
> --enable- options, and as you see above this is underquoted, so that the
> brackets are eaten by m4.

Done. Thanks for the review.

Cheers,
Carlos.

Re: [PATCH] Elision, both for s390 and x86_64, should be enabled via --enable-lock-elision=yes.

[Andreas Krebbel]

On 09/29/2014 09:55 PM, Carlos O'Donell wrote:
> Andreas,
> 
> All lock elision enablement should be controlled by the
> --enable-lock-elision configure option.
> 
> I have already written to Andi about this, and he agreed
> that it's OK for x86_64, and is what we agreed upon at
> the global level for glibc.
> 
> Would you agree that this is also OK for s390?
> 
> The goal is to be able to enable or disable lock elision
> for all architectures with a single configure option.
> 
> I understand that this is slightly different from what
> we do for other features that we simply detect and use.
> 
> We do it this way for elision to give distributions the
> flexibility because this feature is quite new and has potential
> side-effects that perhaps might only be best enabled in a newer
> version of the distribution.
> 
> It also gives users the ability to disable it if it's causing
> problems for applications that are important to their use
> cases, but that don't work well with elision.
> 
> In the long term I expect that runtime tunnables will be useful
> in enabling or disabling elision on a per-application basis.
> 
> OK to commit for s390?

For s390 the change is not required.  For Glibc builds without lock elision we do not even want the
variable check in the pthread code.  So we do not even define DO_ELISION if ENABLE_LOCK_ELISION
isn't there (see force-elision.h). Perhaps it makes sense to do the same for x86?

Bye,

-Andreas-

> 
> 2014-09-29  Carlos O'Donell  <carlos@redhat.com>
> 
> 	* configure.ac: --enable-lock-elision enables or disables
> 	all elision for locks, not just mutexes.
> 	* configure: Regenerate.
> 	* sysdeps/unix/sysv/linux/s390/elision-conf.c (elision_init)
> 	[!ENABLE_LOCK_ELISION]: Set __pthread_force_elision to zero.
> 	* sysdeps/unix/sysv/linux/x86/elision-conf.c (elision_init)
> 	[!ENABLE_LOCK_ELISION]: Set __pthread_force_elision,
> 	__elision_available, and __elision_aconf.retry_try_xbegin to zero.
> 
> diff --git a/configure b/configure
> index 89566c5..9a9cf08 100755
> --- a/configure
> +++ b/configure
> @@ -1413,7 +1413,7 @@ Optional Features:
>                            initialize __stack_chk_guard canary with a random
>                            number at program start
>    --enable-lock-elision=yes/no
> -                          Enable lock elision for pthread mutexes by default
> +                          Enable hardware lock elision by default
>    --enable-add-ons[=DIRS...]
>                            configure and build add-ons in DIR1,DIR2,... search
>                            for add-ons if no parameter given
> diff --git a/configure.ac b/configure.ac
> index 82d0896..d113772 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -169,7 +169,7 @@ fi
>  
>  AC_ARG_ENABLE([lock-elision],
>  	      AC_HELP_STRING([--enable-lock-elision[=yes/no]],
> -			     [Enable lock elision for pthread mutexes by default]),
> +			     [Enable hardware lock elision by default]),
>  	      [enable_lock_elision=$enableval],
>  	      [enable_lock_elision=no])
>  AC_SUBST(enable_lock_elision)
> diff --git a/sysdeps/unix/sysv/linux/s390/elision-conf.c b/sysdeps/unix/sysv/linux/s390/elision-conf.c
> index 69c0483..852433e 100644
> --- a/sysdeps/unix/sysv/linux/s390/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/s390/elision-conf.c
> @@ -60,11 +60,16 @@ elision_init (int argc __attribute__ ((unused)),
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> +#ifdef ENABLE_LOCK_ELISION
>    /* Set when the CPU and the kernel supports transactional execution.
>       When false elision is never attempted.  */
>    int elision_available = (GLRO (dl_hwcap) & HWCAP_S390_TE) ? 1 : 0;
>  
>    __pthread_force_elision = __libc_enable_secure ? 0 : elision_available;
> +#else
> +  /* This configuration has elision disabled.  */
> +  __pthread_force_elision = 0;
> +#endif
>  }
>  
>  #ifdef SHARED
> diff --git a/sysdeps/unix/sysv/linux/x86/elision-conf.c b/sysdeps/unix/sysv/linux/x86/elision-conf.c
> index 28e48d9..abc42b7 100644
> --- a/sysdeps/unix/sysv/linux/x86/elision-conf.c
> +++ b/sysdeps/unix/sysv/linux/x86/elision-conf.c
> @@ -62,12 +62,17 @@ elision_init (int argc __attribute__ ((unused)),
>  	      char **argv  __attribute__ ((unused)),
>  	      char **environ)
>  {
> -  __elision_available = HAS_RTM;
>  #ifdef ENABLE_LOCK_ELISION
> +  __elision_available = HAS_RTM;
>    __pthread_force_elision = __libc_enable_secure ? 0 : __elision_available;
> -#endif
>    if (!HAS_RTM)
> -    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks */
> +    __elision_aconf.retry_try_xbegin = 0; /* Disable elision on rwlocks.  */
> +#else
> +  /* This configuration has elision disabled.  */
> +  __elision_available = 0;
> +  __pthread_force_elision = 0;
> +  __elision_aconf.retry_try_xbegin = 0;
> +#endif
>  }
>  
>  #ifdef SHARED
> ---
>