Re: [PATCH] Use C11-like atomics instead of plain memory accesses in x86 lock elision.

[Stefan Liebler <stli at linux dot vnet dot ibm dot com>]

Hi Torvald,

On 12/02/2016 10:56 AM, Torvald Riegel wrote:
> This uses atomic operations to access lock elision metadata that is
> accessed concurrently (ie, adapt_count fields).  The size of the data is
> less than a word but accessed only with atomic loads and stores;
> therefore, we add support for shorter-size atomic load and stores too.
>
> Once committed, I will add a note to the Concurrency page on the wiki.
> The reason for just enabling shorter-size atomic loads and stores is
> that so far, we have no need for shorter-size atomic read-modify-write
> operations, and it would be harder to enable these on certain archs than
> just loads and stores.
>
> Other architectures that use lock elision should apply similar changes.
>
> Tested on x86_64-linux.
> diff --git a/sysdeps/unix/sysv/linux/x86/elision-lock.c 
b/sysdeps/unix/sysv/linux/x86/elision-lock.c
> index 5e66960..384c48e 100644
> --- a/sysdeps/unix/sysv/linux/x86/elision-lock.c
> +++ b/sysdeps/unix/sysv/linux/x86/elision-lock.c
> @@ -44,7 +44,11 @@
>  int
>  __lll_lock_elision (int *futex, short *adapt_count, EXTRAARG int 
private)
>  {
> -  if (*adapt_count <= 0)
> +  /* adapt_count is accessed inside and outside of transactions 
concurrently,
> +     so we need to use atomic accesses to avoid data races. 
However, the
> +     value of adapt_count is just a hint, so relaxed MO accesses are
> +     sufficient.  */
>

Can you extend the comment about the access of adapt_count "inside" of a 
transaction?
If the reader thinks about one call of pthread_mutex_lock, the 
adapt_count is not accessed inside a transaction:
/* mut_a->elision (=adapt_count) is loaded before starting the 
transaction or acquiring the lock.  If the transaction is aborted, it is 
accessed without an acquired lock.   */
pthread_mutex_lock(mut_a)
pthread_mutex_unlock(mut_a)

Only if you have a nested transaction, adapt_count is accessed within a 
transaction:
/* mut_a->elision (=adapt_count) is accessed before starting the 
transaction or aquiring the lock.  */
pthread_mutex_lock(mut_a)

/* mut_b->elision (=adapt_count) is accessed within the transaction 
started while locking mut_a.  */
pthread_mutex_lock(mut_b)

pthread_mutex_unlock(mut_b)
pthread_mutex_unlock(mut_a)

On 12/02/2016 10:56 AM, Torvald Riegel wrote:
> @@ -70,15 +74,23 @@ __lll_lock_elision (int *futex, short 
*adapt_count, EXTRAARG int private)
>  			&& _XABORT_CODE (status) == _ABORT_LOCK_BUSY)
>  	        {
>  		  /* Right now we skip here.  Better would be to wait a bit
> -		     and retry.  This likely needs some spinning.  */
> -		  if (*adapt_count != aconf.skip_lock_busy)
> -		    *adapt_count = aconf.skip_lock_busy;
> +		     and retry.  This likely needs some spinning.
> +		     While the transaction already ensures atomicity, we use
> +		     atomic accesses here too just for consistency, and to
> +		     make a potential future transition to C11 atomic data
> +		     types easier.
>
Here we are not within a transaction as _xbegin has not returned 
_XBEGIN_STARTED or the transaction started successfully but was aborted 
because the lock was busy.

I'll post a similar patch for s390.

Bye.
Stefan