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

[Torvald Riegel <triegel at redhat dot com>]

On Fri, 2016-12-02 at 16:38 +0100, Stefan Liebler wrote:
> 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?

I changed this to:
  /* adapt_count can be accessed concurrently; these accesses can be
both
     inside of transactions (if critical sections are nested and the
outer
     critical section uses lock elision) and outside of transactions.
Thus,
     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.  */

> 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.

Err, yes.  I removed that part of the comment.  I must have been
thinking about the powerpc code...

I committed the patch with these changes applied.