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

[Stefan Liebler]

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