Re: [PATCH 01/11] Add the low level infrastructure for pthreads lock elision with TSX

["Carlos O'Donell" <carlos at redhat dot com>]

On 06/25/2013 01:09 AM, Andi Kleen wrote:
> 
> Here's a patch adding the requested comments to elision-conf
> 
> -Andi
> 
> 
> [PATCH] fixup! Add the low level infrastructure for pthreads lock elision with TSX
> 
> diff --git a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> index 39ea383..045f98e 100644
> --- a/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> +++ b/nptl/sysdeps/unix/sysv/linux/x86/elision-conf.c
> @@ -23,6 +23,9 @@
>  #include <elision-conf.h>
>  #include <glibc-var.h>
>  
> +/* Reasonable initial tuning values, may be revised in the future.
> +   This is a conservative initial value.  */

OK.

> +
>  struct elision_config __elision_aconf =
>    {
>      .skip_lock_busy = 3,
> @@ -110,19 +113,23 @@ elision_aconf_setup (const char *s)
>      }

Can we have something a little more detailed for __elision_aconf, either
here or in the definition?

e.g.

Torvald wrote this up:

+struct elision_config __elision_aconf =
+  {
+    /* How often to not attempt to use elision if a transaction aborted
+       because the lock is already acquired.  Expressed in number of lock
+       acquisition attempts.  */
+    .skip_lock_busy = 3,
+    /* How often to not attempt to use elision if a transaction aborted due
+       to reasons other than other threads' memory accesses. Expressed in
+       number of lock acquisition attempts.  */
+    .skip_lock_internal_abort = 3,
+    /* How often we retry using elision if there is chance for the transaction
+       to finish execution (e.g., it wasn't aborted due to the lock being
+       already acquired.  */
+    .retry_try_xbegin = 3,
+    /* Same as SKIP_LOCK_INTERNAL_ABORT but for trylock.  */
+    .skip_trylock_internal_abort = 3,
+  };



>  }
>  
> -/* Elided rwlock toggle.  */
> +/* Elided rwlock toggle, set when elision is available and is 
> +   enabled for rwlocks.  */
>  

OK.

>  int __rwlock_rtm_enabled attribute_hidden;
>  
> -/* Retries for elided rwlocks.  */
> +/* Retries for elided rwlocks on read. Conservative initial value.  */
>  
>  int __rwlock_rtm_read_retries attribute_hidden = 3;

OK.

> -/* Global elision check switch.  */
> +/* Set when the CPU supports elision. When false elision is never attempted.  */
>  
>  int __elision_available attribute_hidden;

OK.

> -/* Force elision for all new locks.  */
> +/* Force elision for all new locks. This is used to decide whether existing
> +   DEFAULT locks should be automatically upgraded to elision in
> +   pthread_mutex_lock(). Disabled for suid programs. Only used when elision
> +   is available.  */
>  
>  int __pthread_force_elision attribute_hidden;
  
Is it really for all new locks? AFAICT this isn't used for rwlocks where we
use __rwlock_rtm_enabled.

Should the variable be renamed to __mutex_rtm_enabled? It probably better
represents the current usage.

Cheers,
Carlos.